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

[Chris Wilson]

On Fri, May 03, 2013 at 05:55:31PM +0200, Daniel Vetter wrote:
> On Fri, May 3, 2013 at 2:22 PM, Chris Wilson <chris at 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 at 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.

Not quite. It was added afterwards, I know for it is one of my mistakes
that I belatedly recognised as trying to workaround a bug in UXA. And we
then layered on further bugs to try and patch up the glaring holes then
introduced.

> 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.

I think the patch is quite ugly as we do not attempt to avoid the
unnecessary modeset step and so penalise working userspace code.  Now,
if you were first to make intel_modeset_affected_pipes() smarter, such
that the explicit fb set-base optimisation was no longer ever necessary,
things would be more interesting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre