Re: [PATCH v2 4/4] drm/i915/gen9: Fix runtime PM refcounting in case DMC firmware isn't loaded

[Imre Deak <imre.deak@intel.com>]

On ke, 2016-04-20 at 15:16 +0200, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 04:13:25PM +0300, Imre Deak wrote:
> > On ke, 2016-04-20 at 15:02 +0200, Daniel Vetter wrote:
> > > On Mon, Apr 18, 2016 at 02:48:21PM +0300, Imre Deak wrote:
> > > > While we disable runtime PM and with that display power well support if
> > > > the DMC firmware isn't loaded, we still want to disable power wells
> > > > during system suspend and driver unload. So drop/reacquire the
> > > > corresponding power refcount during suspend/resume and driver unloading.
> > > > This also means we have to check if DMC is not loaded and skip enabling
> > > > DC states in the power well code.
> > > > 
> > > > v2:
> > > > - Reuse intel_csr_ucode_suspend() in intel_csr_ucode_fini() instead of
> > > >   opencoding the former. (Chris)
> > > > - Add docbook comment to the public resume and suspend functions.
> > > > 
> > > > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > 
> > > > ---
> > > > 
> > > > As this fixes the special case when firmware loading failed with the
> > > > result of possibly leaving power wells enabled during suspend/resume and
> > > > driver unloading it's not for -fixes or stable.
> > > 
> > > Do we even care about this to bother fixing it? We pretty much agreed that
> > > we need dmc to make power management work on skl, so if it doesn't work
> > > too well over suspend/resume (soix), meh.
> > 
> > It's possible that someone runs w/o the firmware, either because of not
> > having the latest version for a given kernel or on purpose disabling
> > DMC functionality (for example for debugging). We want to keep things
> > working in this case too. And it's also the right thing to do during
> > driver unload, we'd leak the RPM reference otherwise.
> 
> Ok, if this is needed anyway for driver unload I think it's ok. But
> otherwise keeping stuff working without DMC is imo a fringe use case we
> shouldn't bother about.

Well we could still not do the suspend/resume time put/get, but I'd
still argue for those see below.

> > > And your patch here does make the suspend/resume code even more fragile
> > > and filled with special cases, so not too sure we should apply this one.
> > 
> > It's the same case for all DMC platforms.
> 
> I more meant that there's not yet another little bit we need to do at
> exactly the right time in an already massive sequence of events. It
> certainly doesn't make suspend/resume simpler ;-)

Right agreed, and annoyed a lot by this fact. But we are already
talking about the exception case anyway where DMC isn't loaded so it's
not risking to regress the usual case. Also while the no DMC case is an
exception, it's still possible. And in that case, I'm not sure what
would happen if we left power wells on during system s/r. I suppose
BIOS would still do the right thing (and not hang the machine for
example), but I'm not sure. If we go with white listing the DMC version
then this becomes even more of an issue. Would it be possible to have
CI test both cases?

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx