Split disabling of unused displays from parking used ones

Split disabling of unused displays from parking used ones

If a display event is triggered, but the display configuration doesn't change, then parking displays at (0,0) has a visual impact for the user. Since the framebuffer size might not change, but displays may have been disconnected (mirror mode), this patch allows us to always disable unused displays but only configure the used ones if the framebuffer size changed.

BUG=381482
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276321

[dnicoara, 2014-06-05 19:10:46]

PTAL

[Daniel Kurtz, 2014-06-06 08:29:44]

https://codereview.chromium.org/312353002/diff/60001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/native_display_delegate_x11.cc (right):

https://codereview.chromium.org/312353002/diff/60001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_delegate_x11.cc:589:
ConfigureCrtc(x11_output->crtc(), mode, output, 0, 0);
When plugging an external monitor, the framebuffer will grow larger than just
the internal monitor.  But the internal monitor will stay the same size and at
the same position (0,0).

In this case, however, I think we are still doing a ConfigureCrtc().  Will this
still cause the monitor to DPMS off then on?  Or, is this caught lower down and
turn into a no-op?

As a datapoint I just tried on my peach_pi, and I do see a noticeable black
flash of internal display when hotplugging an external monitor - but only about
1/2 the time.


Also, isn't it possible to have multiple outputs driven by the same crtc?  I
think this is "hardware mirror mode".  In that case, we would be configuring
them twice by this loop, right?

[Daniel Kurtz, 2014-06-06 08:53:52]

Are there any tests for this code?

Also, have you seen:
https://code.google.com/p/chrome-os-partner/issues/detail?id=26251#c37

and:
https://code.google.com/p/chrome-os-partner/issues/detail?id=26251#c51

Did you consider using option (2)?
 (1) grow Framebuffer to new_screen_size.SetToMax(current_screen_size)
 (2) resize & move all CRTCs
 (3) snap Framebuffer back to its new size

This may actually give a better overall user experience, since resizing the
Framebuffer may be a faster/easier operation than configuring CRTCs.

[dnicoara, 2014-06-06 14:25:30]

Having 2 calls to SetScreenSize would cause even more black flashes. I don't
think we'd want that.

I've looked through your posts and overall I agree that we're performing too
many setCrtc calls and we could do a better job at checking against the previous
state to see if we could optimize them out. However that requires more
refactoring of DisplayConfigurator and perhaps even in the way we call into
NativeDisplayDelegate to configure displays (This is outside the scope of this
CL).

Besides, this CL does simplify the logic a bit and does remove some unnecessary
calls to setCrtc :)

I'm happy to keep discussing this, as I have some ideas on how to simplify the
display configuration (but, perhaps we should continue it via email).

https://codereview.chromium.org/312353002/diff/60001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/native_display_delegate_x11.cc (right):

https://codereview.chromium.org/312353002/diff/60001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_delegate_x11.cc:589:
ConfigureCrtc(x11_output->crtc(), mode, output, 0, 0);
On 2014/06/06 08:29:43, Daniel Kurtz wrote:
> When plugging an external monitor, the framebuffer will grow larger than just
> the internal monitor.  But the internal monitor will stay the same size and at
> the same position (0,0).
> 
> In this case, however, I think we are still doing a ConfigureCrtc().  Will
this
> still cause the monitor to DPMS off then on?  Or, is this caught lower down
and
> turn into a no-op?
> 

My understanding from ynovikov@'s explanation is that in such case X11 optimizes
the call into a no-op. It works, at least, on the Pixel. But if there are
examples of devices where this doesn't work we can a check for size here later
on. (I'm trying to incrementally update the code, and since this is what was
done before I didn't want to modify it just yet.)

> As a datapoint I just tried on my peach_pi, and I do see a noticeable black
> flash of internal display when hotplugging an external monitor - but only
about
> 1/2 the time.
> 
> 

The black flash is likely from re-creating the framebuffer.

> Also, isn't it possible to have multiple outputs driven by the same crtc?  I
> think this is "hardware mirror mode".  In that case, we would be configuring
> them twice by this loop, right?

Currently GetDisplays() and InitDisplaySnapshot() map 1 unique crtc to 1 unique
output. This would be outside the scope of this CL.

[Daniel Kurtz, 2014-06-06 16:38:47]

lgtm

[dnicoara, 2014-06-10 13:48:45]

-marcheu@ +oshima@ for OWNERS

[oshima, 2014-06-10 18:49:20]

lgtm

[dnicoara, 2014-06-10 19:53:08]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-06-10 19:54:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/312353002/60001

[commit-bot: I haz the power, 2014-06-10 21:29:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[commit-bot: I haz the power, 2014-06-10 21:54:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 21:54:26]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[Daniel Kurtz, 2014-06-11 08:12:14]

The CQ bit was checked by djkurtz@chromium.org

[commit-bot: I haz the power, 2014-06-11 08:13:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/312353002/60001

[commit-bot: I haz the power, 2014-06-11 09:35:15]

Change committed as 276321