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

[Paulo Zanoni]

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.
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