Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 25, 2013 at 02:04:03PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2013/1/25 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Fri, Jan 25, 2013 at 01:40:08PM -0200, Paulo Zanoni wrote:
> >> Hi
> >>
> >> 2013/1/21 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> >> > On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
> >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >>
> >> >> If the power well is disabled, we should not try to read its
> >> >> registers, otherwise we'll get "unclaimed register" messages.
> >> >>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
> >> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> >> index a7fb7e1..921b020 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> >> @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> >> >>       if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> >> >>               state = true;
> >> >>
> >> >> -     reg = PIPECONF(cpu_transcoder);
> >> >> -     val = I915_READ(reg);
> >> >> -     cur_state = !!(val & PIPECONF_ENABLE);
> >> >> +     if (cpu_transcoder == TRANSCODER_EDP ||
> >> >> +         (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
> >> >
> >> > Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
> >> > enabled, while the driver has it disabled. But KVMR might have already
> >> > disabled the well, and it might get disabled just after this check,
> >> > and then you would hit the unclaimed register issue again.
> >>
> >> By reading your text it seems you think that if bit 31 is 0 then bit
> >> 30 will necessarily be 0. This is not true. If any entity (including
> >> KVMr) needs the power well enabled (and is requesting so), then bit 30
> >> of HSW_PWR_WELL_DRIVER will be 1, no matter the status of bit 31.
> >
> > Did you verify that on real HW?
> 
> Yes.
> 
> >
> > It makes little sense when you consider that the spec says not
> > to toggle the request bit when there's a state transition currently
> > happening. If all the status bits read as the same thing, then
> > there's no way to know when a transition is actually happening,
> > and hence the text in the spec is complete nonsense.
> >
> 
> I agree... I'm investigating this problem. That's why there is still
> no new version of patch 3/10 here :)
> 
> > The only sensible interpretation of that text I could think of is
> > that you're not supposed to abort a state transition you yourself
> > caused. That would be achieved by polling the status bit after
> > toggling the request bit, assuming that the status bit is just a
> > delayed version of request bit.
> 
> If we're enabling then it's fine: we requested it to enable, then at
> some point it will be enabled, then we can start using the registers.
> We can always avoid touching the registers when the well is still
> being enabled.

I think you can only assume this if the status bit goes to 0 immediately
when the well starts powering down. And it won't go back to 1 until the
well is fully powered again. Well, I guess that's the only design that
would make sense anyway. But even then you can't tell the fully powered
down case apart from the currently going down case.

> The only situation where the "restriction" doesn't make
> sense is on the "disable" case.
> 
> OTOH, if we requested it to be disabled, the fact that we can't
> predict whether it's really going to be disabled or not shouldn't
> really matter that much since we're not supposed to touch the
> "possibly disabled" registers, and if anybody needs to touch those
> registers they will have to request the well to be enabled first...
> This still doesn't explain the "Restriction", but maybe that
> restriction is just for the "enable" case.
> 
> >
> >> Another thing to mention is that no entity can force the power well to
> >> be "off", it can only force to be "on".
> >>
> >> Still, your text does contain an example of a problem with the current
> >> code. Let me explicitly write it:
> >> - Our driver doesn't need it to be enabled, so bit 31 of
> >> HSW_PWR_WELL_DRIVER is 0
> >> - KVMr needs it enabled, so HSW_PWR_WELL_DRIVER bit 30 is 1
> >> - We do the I915_READ above
> >> - After the read, KVMr says it doesn't need the power well to be
> >> enabled anymore, so it gets disabled because no one else is requesting
> >> it
> >> - Then we touch an unclaimed register.
> >>
> >> I think the only policy to avoid these race conditions should be "if I
> >> don't need the power well to be enabled, then I won't touch registers
> >> that require it to be enabled, no matter what the real state of the
> >> power well is".This should prevent any kinds of problems, because if I
> >> need the power well to be enabled, then I requested it to be enabled
> >> and it is enabled because I asked so. I'll send a second version of
> >> the patch with this.
> >>
> >> >
> >> >> +             reg = PIPECONF(cpu_transcoder);
> >> >> +             val = I915_READ(reg);
> >> >> +             cur_state = !!(val & PIPECONF_ENABLE);
> >> >> +     } else {
> >> >> +             cur_state = false;
> >> >> +     }
> >> >> +
> >> >>       WARN(cur_state != state,
> >> >>            "pipe %c assertion failure (expected %s, current %s)\n",
> >> >>            pipe_name(pipe), state_string(state), state_string(cur_state));
> >> >> --
> >> >> 1.7.10.4
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC