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:35:59PM +0300, Imre Deak wrote:
> 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?

The combinatorial explosion in testing is why I don't want to support
everything. So no, I don't think CI should test with and without DMC.
That's also why I'm so much opposed to features merged into upstream, but
disabled by default.

Imo non-default cases should be best effort only, and we fix up if it's a
fireworks.
-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