Re: [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 3 May 2013 17:55:31 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, May 3, 2013 at 2:22 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Fri, May 03, 2013 at 02:55:45PM +0300, Imre Deak wrote:
> >> On Fri, 2013-05-03 at 12:44 +0100, Chris Wilson wrote:
> >> > On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
> >> > > Currently the driver's assumed behavior for a modeset with an attached
> >> > > FB is that the corresponding connector will be switched to DPMS ON mode
> >> > > if it happened to be in DPMS OFF (or another power save mode). This
> >> > > wasn't enforced though if only the FB changed, everything else (format,
> >> > > connector etc.) remaining the same. In this case we only set the new FB
> >> > > base and left the connector in the old power save mode.
> >> > >
> >> > > Fix this by forcing a full modeset whenever there is an attached FB and
> >> > > any affected connector is in a power save mode.
> >> > >
> >> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> >
> >> > NAK. Papering over bugs, much?
> >>
> >> Ok. I posted this based on our IRC discussion where we seemed to agree
> >> that DPMS_ON is assumed already; just that it's not done consistently.
> >> That is if user space does a modeset now that ends up in a full modeset
> >> (lets say a new FB with different format) then the kernel will do a
> >> DPMS_ON. But if the new FB happens to be the same format then it won't.
> >> I thought this is inconsistent and should be fixed.
> >>
> >> Another way to make it consistent would be then to remove DPMS ON from
> >> the full modeset path..
> >
> > Right, we have mentioned in the past that the conflation between DPMS
> > and modesetting is a more recent bug that is going to cause us even more
> > trouble later. I am not sure how we can fix that as UXA already makes
> > many bad assumptions.
> 
> Afaict the conflagration of dpms on after a modeset has been even in
> the old crtc helpers. The only difference is that now we explicitly
> update the dpms state tracking to avoid double-encoder enables. That
> part allowed us to rip out tons of state tracking from tons of places.
> 
> So ever since kms happened, we have inconsistent setcrtc semantics.
> Stopping to force dpms on will horribly break existing userspace, so
> we can't do this. Making sure that the semantics are more consistent
> (especially if we further optimize modeset transitions for fastboot)
> seems like the lesser of two evils.
> 
> We'll get a 2nd chance to get this right the atomic modeset ioctl.
> Until then, I want this duct-taped together, and Imre's patch looks
> like a viable approach. Working beats ugly semantics here imo.

Agreed; there's definitely redundancy here, but turning the display on
at mode set time when it was clearly intended and implied by other
usages seems like following the policy of least surprise.

-- 
Jesse Barnes, Intel Open Source Technology Center