Closed Bug 1160102 Opened 9 years ago Closed 9 years ago

Fix the wrong vsync contorl when screen on/off

Categories

(Core :: Graphics, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- unaffected
firefox39 --- unaffected
firefox40 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

Details

Attachments

(2 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1155797 +++

In https://bugzilla.mozilla.org/show_bug.cgi?id=1155797#c22 , it still call the hwc vsync control even we use software timer. There will have problem if both sw and hwc are turned on together.
Attachment #8599811 - Flags: review?(mwu) → review?(bugmail.mozilla)
Comment on attachment 8599811 [details] [diff] [review]
use VsyncDisplay interface to turn on/off vsync. v1

Review of attachment 8599811 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsWindow.cpp
@@ +137,5 @@
> +        VsyncSource::Display &display = gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay();
> +        if (enabled) {
> +            display.EnableVsync();
> +        } else {
> +            display.DisableVsync();

Please rebase on top of bug 1157874, this code is different now. Also EnableVsync()/DisableVsync() need to be called on the main thread, and it doesn't look like this runs on the main thread.
Attachment #8599811 - Flags: review?(bugmail.mozilla) → review-
rebase to current m-c.
Attachment #8599811 - Attachment is obsolete: true
Attachment #8600839 - Flags: review?(bugmail.mozilla)
Comment on attachment 8600839 [details] [diff] [review]
use VsyncDisplay interface to turn on/off vsync. v2

Review of attachment 8600839 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsWindow.cpp
@@ +1115,5 @@
>  }
>  
>  void
>  nsScreenManagerGonk::DisplayEnabled(bool aEnabled)
>  {

All hal function already runs at main thread.
Should we care about the thread model here?

https://dxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#378
unlike attachment 8600839 [details] [diff] [review], use runnable method here.
Attachment #8600851 - Flags: review?(bugmail.mozilla)
Comment on attachment 8600839 [details] [diff] [review]
use VsyncDisplay interface to turn on/off vsync. v2

Review of attachment 8600839 [details] [diff] [review]:
-----------------------------------------------------------------

This is ok since you have the MOZ_ASSERT to make sure it is on the main thread.

::: widget/gonk/nsWindow.cpp
@@ +1131,1 @@
>      NS_DispatchToMainThread(aEnabled ? mScreenOnEvent : mScreenOffEvent);

Why is this code doing a DispatchToMainThread if this function already runs on the main thread? Is it just to delay processing until later? If so that should probably be commented.
Attachment #8600839 - Flags: review?(bugmail.mozilla) → review+
Attachment #8600851 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Comment on attachment 8600839 [details] [diff] [review]
> use VsyncDisplay interface to turn on/off vsync. v2 
> ::: widget/gonk/nsWindow.cpp
> @@ +1131,1 @@
> >      NS_DispatchToMainThread(aEnabled ? mScreenOnEvent : mScreenOffEvent);
> 
> Why is this code doing a DispatchToMainThread if this function already runs
> on the main thread? Is it just to delay processing until later? If so that
> should probably be commented.

Hi Michael,

The [1] comes from [2]. I think we call PHal function on main thread. Should we need to post the runnable to main thread here?
Like:
NS_DispatchToMainThread(aEnabled ? mScreenOnEvent : mScreenOffEvent);  

[1]
https://hg.mozilla.org/mozilla-central/annotate/dc5f85980a82/widget/gonk/nsWindow.cpp#l1120
[2]
https://hg.mozilla.org/mozilla-central/rev/40a51c5dca9e#l6.77
Flags: needinfo?(mwu)
Check GonkDisplayICS - it fires a screen on/off event from a separate thread.
Flags: needinfo?(mwu)
Attachment #8600839 - Attachment is obsolete: true
Comment on attachment 8600851 [details] [diff] [review]
use VsyncDisplay interface to turn on/off vsync. v3

Thanks to :mwu, SetEnabled() will call at non-main-thread in ICS[1].

[1]
https://hg.mozilla.org/mozilla-central/annotate/34828fed1639/widget/gonk/libdisplay/GonkDisplayICS.cpp#l164
Attachment #8600851 - Flags: review?(bugmail.mozilla)
Attachment #8600851 - Flags: review?(bugmail.mozilla) → review+
Please land the attachment 8600851 [details] [diff] [review] to m-c.

try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc3ed95b6994
I saw the similar failed for m1 m11 m13 m20 test.
Status: NEW → ASSIGNED
Comment on attachment 8600851 [details] [diff] [review]
use VsyncDisplay interface to turn on/off vsync. v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 1147753. We get flickering on n5-l if we turn on hw vsync.

User impact if declined:
If we init vsync hw timer failed, we will turn on both hw and sw timer after screen turn off and on. Then, compositor will compose more than once and cpu will always be busy.

Testing completed:
Manual test at flame-kk and nexus5-l. It seems work well.

Risk to taking this patch (and alternatives if risky):
low
String or UUID changes made by this patch:
none
Attachment #8600851 - Flags: approval-mozilla-b2g37?
https://hg.mozilla.org/mozilla-central/rev/8cb1e6f993a7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #13)
> Comment on attachment 8600851 [details] [diff] [review]
> use VsyncDisplay interface to turn on/off vsync. v3
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #):
> Bug 1147753. We get flickering on n5-l if we turn on hw vsync.
Given vsync is off in 2.2 I am not sure if we need this , unless there is a plan to enable it. Can you confirm? 
> 
> User impact if declined:
> If we init vsync hw timer failed, we will turn on both hw and sw timer after
> screen turn off and on. Then, compositor will compose more than once and cpu
> will always be busy.
> 
> Testing completed:
> Manual test at flame-kk and nexus5-l. It seems work well.
> 
> Risk to taking this patch (and alternatives if risky):
> low
> String or UUID changes made by this patch:
> none
Flags: needinfo?(hshih)
(In reply to bhavana bajaj [:bajaj] from comment #15)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #13)
> > Comment on attachment 8600851 [details] [diff] [review]
> > use VsyncDisplay interface to turn on/off vsync. v3
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #):
> > Bug 1147753. We get flickering on n5-l if we turn on hw vsync.
> Given vsync is off in 2.2 I am not sure if we need this , unless there is a
> plan to enable it. Can you confirm? 

Vsync is enabled on 2.2, just not on L devices. Other devices may hit this same problem if they are on kit-kat and encounter the same issue as the Nexus 5. This seems to be the case from https://bugzilla.mozilla.org/show_bug.cgi?id=1155797#c55.
(In reply to bhavana bajaj [:bajaj] from comment #15)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #13)
> > Comment on attachment 8600851 [details] [diff] [review]
> > use VsyncDisplay interface to turn on/off vsync. v3
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #):
> > Bug 1147753. We get flickering on n5-l if we turn on hw vsync.
> Given vsync is off in 2.2 I am not sure if we need this , unless there is a
> plan to enable it. Can you confirm? 
> > 

We already enable vsync on kk device in 2.2. If the device has the problem to init vsync, we will hit the problem as [1]. Vsync works well on Flame-kk, but "nexus5-kk" hits [1].

[1]
https://bugzilla.mozilla.org/show_bug.cgi?id=1155797#c22
Flags: needinfo?(hshih) → needinfo?(bbajaj)
Flags: needinfo?(bajaj.bhavana)
Attachment #8600851 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Needs rebasing for b2g37 uplift.
v2.2 doesn't have bug 1157874. Create a vsync control runnable object to handle the main thread task.
Attachment #8604485 - Flags: review?(bugmail.mozilla)
Comment on attachment 8604485 [details] [diff] [review]
use VsyncDisplay interface to turn on/off vsync for v2.2. v1

Review of attachment 8604485 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsWindow.cpp
@@ +147,5 @@
> +        MOZ_ASSERT(NS_IsMainThread());
> +
> +        VsyncSource::Display &display =
> +            gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay();
> +        if(aEnabled) {

nit: space between if and (
Attachment #8604485 - Flags: review?(bugmail.mozilla) → review+
Attachment #8604485 - Attachment is obsolete: true
Flags: needinfo?(hshih)
Please land attachment 8604692 [details] [diff] [review] to gecko 2.2.
carry r+ and approval from comment 20 and attachment 8600851 [details] [diff] [review].
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.