On Fri, Jan 25, 2013 at 02:04:03PM -0200, Paulo Zanoni wrote:
> Hi
>
> 2013/1/25 Ville Syrj?l? <ville.syrjala at 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 at linux.intel.com>:
> >> > On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
> >> >> From: Paulo Zanoni <paulo.r.zanoni at 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 at 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 at 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
