On Tue, Sep 29, 2015 at 03:01:57PM +0200, Daniel Vetter wrote: > On Tue, Sep 29, 2015 at 2:35 PM, Patrik Jakobsson > <patrik.jakobsson@xxxxxxxxxxxxxxx> wrote: > > On Tue, Sep 29, 2015 at 11:01:35AM +0200, Daniel Vetter wrote: > >> On Tue, Sep 29, 2015 at 11:08:25AM +0530, Animesh Manna wrote: > >> > > >> > > >> > On 09/28/2015 12:51 PM, Daniel Vetter wrote: > >> > >On Wed, Aug 26, 2015 at 01:36:07AM +0530, Animesh Manna wrote: > >> > >>Mmio register access after dc6/dc5 entry is not allowed when > >> > >>DC6 power states are enabled according to bspec (bspec-id 0527), > >> > >>so enabling dc6 as the last call in suspend flow. > >> > >We unconditionaly grab a power well reference for everything in our > >> > >suspend/resume functions, which means dc6/5 should be prevented for long > >> > >enough. > >> > > > >> > >Also since dc6/5 are part of the power well framework you can't just > >> > >enable/disable them directly, instead you need to get/put the > >> > >corresponding power wells. > >> > > > >> > >Finally this patch doesn't just do that, but also frobs around a lot in > >> > >set power well code itself, and I have no idea what it does there and why. > >> > >It does smell a bit like you're just breaking dc6 for runtime pm though, > >> > >which wouldn't be good. > >> > > > >> > >Anyway I decided to just merge this since this patch series has been > >> > >floating around since forever, but then the patch didn't apply cleanly and > >> > >so dropped it. > >> > > >> > I mentioned in my commit message that we have a h/w workaround. > >> > "Mmio register access after dc6/dc5 entry is not allowed when > >> > DC6 power states are enabled" > >> > > >> > This patch is mandatory to solve the pc10 entry issue for skylake. > >> > Planning to send again after rebase on top of tree. > > > > Hi Animesh and Daniel > > > > I'm trying to shed some light on this. It seems rather confusing atm. Animesh, > > please correct me if I'm wrong below. > > > >> The problem isn't that the patch didn't apply cleanly, but that it seems > >> to have fundamental issues: > >> - We already grab power well references around suspend/resume code, see > >> the call to intel_power_domains_init_hw and the calls to > >> intel_display_set_init_power. That code is supposed to ensure that while > >> suspend/resume is going on _all_ power wells are on (and hence dc6/5 are > >> forbidden). It's a bit a big hammer, but we already have the code to do > >> exactly what you're trying to do here. It could be though that for skl > >> it's in the wrong order (but then the commit message must state very > >> clearly what's the exact depency, the gpu is assembled from a pile of > >> various different blocks which are all mostly independent). > > > > DC6 can only be enabled when _all_ power wells are disabled. Hence this patch > > moves DC6 enabled/disabled to the runtime suspend/resume callbacks. Previously > > we only disabled DC6 when PW2 was enabled. > > > > The DC state table says: > > - Everything off: DC6 enabled > > - PW0 enabled: DC5 enabled but DC6 is disabled > > - PW1 enabled: DC5 and DC6 disabled > > - PW2 enabled: DC5 and DC6 disabled > > Ok, so what the patch does (and that is at least self-consistent) is > to move the DC6 enable/disable from PW2 to the top-level domain. That > doesn't quite match what you're describing here since we still have > PW1 managed by the driver, but that's short-circuited now. Hmm. Why are we going back and forth on this all the time? Was there some problem with the plan [1] that Imre and I hatched? [1] http://lists.freedesktop.org/archives/intel-gfx/2015-September/076612.html > > >> - Your patch directly calls skl_enable/disable_dc6 instead of going > >> through the power well abstraction. Breaking these abstraction needs a > >> really good reason, adequate assurance that the refcounts are ok and > >> nothing breaks (using WARN_ON) and an explanation why the refcount > >> interface for display power management doesn't work. > > > > This is indeed a hack but since all we need is the guarantee that a power well > > is in fact enabled we can see the operation as: enable pw + disable dc state. > > This is currently hacked into skl_set_power_well(). It's not nice but it takes > > away the complexity of modeling DC states as power wells. > > > > The alternative would be to turn DC5 and DC6 into power wells and add proper > > dependencies. What I've discovered is that it's quite a tight fit. > > Nah, if the goal is to move just DC6 the patch seems fine. I was just > really confused that the commit message talked about both dc6 _and_ > dc5 being impossible to do, and then only moving half of the story > around. And I was also a bit confused with runtime pm vs. system > suspend (again). > > So if we can confirm that this is really only about DC6 and not also > about dc5 like the commit message claims, then I can clean up the > commit message and apply the patch. > > >> - You also do changes to the power well code itself, which looks like it > >> might break dc6 for runtime pm (by only going into dc5 at most). That > >> needs to be a separate patch or have a much better explanation. > > > > If I understood correctly, that is the entire point of this patch. Don't allow > > DC6 unless all power wells are disabled. My question though is why we don't > > disable DC5 for PW1 as well? > > Afaiui dmc manages pw1 for us and we shouldn't ever touch it from the > driver side. I'm not entirely sure about that though since the current > fix is a hack. If we really shouldn't touch pw1 on skl then we should > remove that power domain from our skl code entirely. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
