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@xxxxxxxxxxxxxxxxxx> > > > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > > > > > > > > > --- > > > > > > > > > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
