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

[Daniel Vetter <daniel@ffwll.ch>]

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.

> > 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 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx