[Intel-gfx] [PATCH] drm/i915: VGA also requires the power well

[Paulo Zanoni]

2013/6/6 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> On Thu, Jun 06, 2013 at 11:35:15AM -0300, Paulo Zanoni wrote:
>> 2013/6/6 Ville Syrjälä <ville.syrjala at linux.intel.com>:
>> > On Wed, Jun 05, 2013 at 06:05:51PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >>
>> >> So add a power domain and check for it before we try to read
>> >> VGA_CONTROL.
>> >>
>> >> This fixes unclaimed register messages that happen on suspend/resume.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_drv.h      | 1 +
>> >>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>> >>  drivers/gpu/drm/i915/intel_pm.c      | 1 +
>> >>  3 files changed, 5 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> >> index 46b1f70..d51ce13 100644
>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> @@ -89,6 +89,7 @@ enum port {
>> >>  #define port_name(p) ((p) + 'A')
>> >>
>> >>  enum intel_display_power_domain {
>> >> +     POWER_DOMAIN_VGA,
>> >>       POWER_DOMAIN_PIPE_A,
>> >>       POWER_DOMAIN_PIPE_B,
>> >>       POWER_DOMAIN_PIPE_C,
>> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> index 4c8fcec..3719d99 100644
>> >> --- a/drivers/gpu/drm/i915/intel_display.c
>> >> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> @@ -9950,6 +9950,9 @@ void i915_redisable_vga(struct drm_device *dev)
>> >>       struct drm_i915_private *dev_priv = dev->dev_private;
>> >>       u32 vga_reg = i915_vgacntrl_reg(dev);
>> >>
>> >> +     if (!intel_display_power_enabled(dev, POWER_DOMAIN_VGA))
>> >> +             return;
>> >> +
>> >
>> > So it looks like you're essentially making intel_redisable_vga() a nop
>> > for HSW.
>>
>> It's not a nop for HSW, it's only a nop if the power well is disabled,
>> which means VGA is disabled, so it's a nop if VGA is disabled. But if
>> you look at the current function it's also a nop if VGA is disabled.
>> So we're keeping the same behavior, but checking the power well before
>> checking vga_reg.
>>
>> VGA mode requires the power well to be enabled, we can be sure that if
>> the power well is disabled, then VGA is disabled, so you don't need to
>> do the check for VGA_DISP_DISABLE.
>
> Rigth, but intel_display_power_enabled() only checks the driver power
> well register. If BIOS can leave VGA enabled, then I guess it could've
> left the power well on too. So I'm thinking we should check for that
> rather than the what the driver requested.

Oh, right. I guess you found one case where our "if we don't want the
power well enabled then we don' touch its registers" policy is not
enough.

It's kinda hard to deal with this "something might just re-enable VGA
while you're distracted" case. There are many possible racing
conditions, even more if you add the power well.

And this case + the case on the PSR patches are examples that our
"power domains" abstraction doesn't seem to be a fit-all approach.



>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni