[Intel-gfx] [DMC_BUGFIX_SKL_V2 3/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Ville Syrjälä]

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 at linux.intel.com> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC