[Intel-gfx] [PATCH] drm/i915/gen9: Probe power well 1 status on dc status query

[Patrik Jakobsson]

On Wed, Jan 27, 2016 at 08:18:57PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 26, 2016 at 05:45:31PM +0200, Mika Kuoppala wrote:
> > There has been cases where we read DC_STATE and get something that we
> > did not write there. As DMC owns power well 1, this could be that DMC
> > snoops DC_STATE accesses and needs to wake up power well 1 up to serve
> > the access. But the waking up power well 1 takes time and we might end up
> > reading during unfinished well wakeup.
> > 
> > When we want the dc status, wake up DMC and make sure that DMC has
> > properly woken up well 1 before we read for the final value.

I was thinking about PW0 and not PW1 as in this patch. DC_STATE_EN is on PW0 so
this will not work (unless it by accident wakes the DMC). Also, I was under the
assumption we had control over PW0 but that doesn't seem to be the case.

> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=93768
> > Suggested-by: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> > Cc: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 86 ++++++++++++++++++++++++++-------
> >  1 file changed, 69 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index bbca527184d0..83c24e73cb88 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -418,15 +418,62 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  	BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |	\
> >  	BIT(POWER_DOMAIN_INIT))
> >  
> > +
> > +static bool __gen9_power_well_enabled(struct drm_i915_private *dev_priv,
> > +				      const enum skl_disp_power_wells pw,
> > +				      const bool req)
> > +{
> > +	uint32_t mask = SKL_POWER_WELL_STATE(pw);
> > +
> > +	if (req)
> > +		mask |= SKL_POWER_WELL_REQ(pw);
> > +
> > +	return (I915_READ(HSW_PWR_WELL_DRIVER) & mask) == mask;
> > +}
> > +
> > +static bool gen9_power_well_enabled(struct drm_i915_private *dev_priv,
> > +				    const enum skl_disp_power_wells pw)
> > +{
> > +	return __gen9_power_well_enabled(dev_priv, pw, true);
> > +}
> > +
> > +static bool gen9_power_well_enabled_noreq(struct drm_i915_private *dev_priv,
> > +					  const enum skl_disp_power_wells pw)
> > +{
> > +	return __gen9_power_well_enabled(dev_priv, pw, false);
> > +}
> > +
> > +static bool gen9_pw1_enabled(struct drm_i915_private *dev_priv)
> > +{
> > +	/* DMC owns the pw1. Don't check for request */
> > +	return gen9_power_well_enabled_noreq(dev_priv, SKL_DISP_PW_1);
> > +}
> > +
> > +static u32 gen9_get_dc_state(struct drm_i915_private *dev_priv)
> > +{
> > +	if (gen9_pw1_enabled(dev_priv))
> > +		return I915_READ(DC_STATE_EN);
> > +
> > +	/* DMC should snoop this and wakeup */
> > +	I915_READ(DC_STATE_EN);

> 
> Not sure it does. I don't think I've never seen a list of trapped
> registers anywhere.

In order to do what it's supposed to it needs to trap all accesses on PW1 and
possibly PW0. The PCU needs to be able to trap at least PW0. The PCU must trap
PW0 before the DMC since the DMC assumes that PW0 is powered when it's trap
routine for DC6 exit starts (PW0 restore).

> 
> Assuming it does, it would quite surprising that it would release
> the trap before it actually woke up the hardware. I guess it might be
> possible if the registers don't actually live inside the power well
> (they'd have to live in power well 0 then I suppose). But that would
> seem to open up races all over the place where we have to touch a
> register also touched by the DMC, so it would seem like a fairly major
> problem to me.

It does trap and hold until it has restored power but powering on can fail and
if that happens it sends an interrupt and flips a few magic bits depending on the
failure. Not sure if there is a way for us to detect that or if it's only
intended for the PCU. Also not sure if we can recover from that with full DC
state functionality.

> 
> If it doesn't trap this, well, then we should just be able to access
> any actually trapped register, and that should kick the DMC out of
> DC5/6.
> 
> That DC_STATE_EN seems to change on its own is still baffling me. One
> idea I had is that the pcu might do the save/restore for DC_STATE_EN,
> and then we might race with it somehow. I suppose that could happen if
> the pcu does some kind of speculative save of that register, and then
> we might end up changing it between the time it got saved and the time
> power well 0 gets turned off. Otherwise AFAIK the power well 0 should
> remain enabled as long as the machine stays out of deep pc states,
> which means the cpu being active should be enough. Or maybe it's the
> DMC that saves it when it indicates DC state readyness to the pcu, but
> if there's no trap we might end up changing the value after that before
> power well 0 gets turned off.

The DMC does the save and restore of all PW0 registers (including DC_STATE_EN)
but doesn't perform the actual PW0 power toggling. PW0 save occurs when the PCU
pokes the DMC to enter DC6. PCU pokes can be masked by the DMC while a
transition occurs so it is not interrupted.

I'm thinking that since we get all 1's when reading DC_STATE_EN with PW0
unpowered we're ORing those bits into our write and consequently mess up the
poke mask bit and other important stuff we shouldn't touch. The state of the DMC
is basically broken at that point and the PCU pokes to exit DC6 never gets
through.

> 
> I'm not sure how we'd go about fixing that. Anything I come up with
> seems to have some races in it. But of course it's really just guesswork
> without knowing what the the dmc and pcu are doing. We'd definitely
> need some way to make sure that any saved stale DC_STATE_EN value doesn't
> get to live past the point where we change it.
> 
> One still quite racy looking idea would be to kick the DMC on both sides
> of the DC_STATE_EN write to lessen the chance that it would end up
> saving the stale value just before we update it. But since the DMC
> doesn't do much in the way of delayed power off, I'm not sure that would
> really help. And even with a delayed power off, it'd remain racy.
> 

The race would be between the read and write of DC_STATE_EN. What we can do is
to check if any of the bits that indicate that the DMC is busy in DC_STATE_EN
is set when reading. That way we're at least not writing bogus or disturbing it
while it's busy if it's trap is not holding for some reason. But still not
perfect... 

-Patrik

---
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027