[Intel-gfx] [PATCH 1/2] drm/i915: check for power well in redisable VGA with generic fn

[Paulo Zanoni]

2013/10/11 Jesse Barnes <jbarnes at virtuousgeek.org>:
> On Fri, 11 Oct 2013 17:16:55 -0300
> Paulo Zanoni <przanoni at gmail.com> wrote:
>
>> 2013/10/11 Jesse Barnes <jbarnes at virtuousgeek.org>:
>> > Rather than using a HSW specific check.
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 53c4ea8..5452b34 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -10667,8 +10667,7 @@ void i915_redisable_vga(struct drm_device *dev)
>> >          * level, just check if the power well is enabled instead of trying to
>> >          * follow the "don't touch the power well if we don't need it" policy
>> >          * the rest of the driver uses. */
>> > -       if (HAS_POWER_WELL(dev) &&
>> > -           (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE_ENABLED) == 0)
>> > +       if (!intel_display_power_enabled(dev, POWER_DOMAIN_VGA))
>>
>> See the big comment. With your patch we're not only going to return in
>> case the power well is disabled, we're going to return in case it's
>> disabled _or_ we promised to not touch it (even if it's enabled). I
>> originally made the same mistake, then Ville spotted it on the code
>> review. This function should assume the BIOS is being an idiot and is
>> trying to mess with us.
>>
>> I admit I don't like it, suggestions to further improve this code are
>> welcome. But it seems no one really knows what's the best and safest
>> thing to do here :(
>
> Hm I'm not seeing it.  For a simple status check we don't depend on the
> power_well struct at all, we just look at the bits.

Are you suggesting we should just read vga_reg without checking if the
power well is enabled? Then we'll hit "unclaimed register" error
messages every time we do this and the power well is disabled.

>
> In this case do we need to explicitly ignore the
> HSW_PWR_WELL_ENABLE_REQUEST bit or something?

The thing about the power well is that it can be enabled by not only
graphics, but also KVMr and other things (there are 4 power well
registers: one for the BIOS, one for the driver, one for KVMr and one
for debug). The idea behind the hardware is: if anybody wants it, it
gets enabled. So in theory things like the KVMr could be trying to
enable the power well while we're not looking, and this creates a huge
amount of possible racing conditions. To avoid most of the racing
conditions, our driver has a policy of "if we don't need the power
well, we're never going to touch anything inside it", and we respect
this policy everywhere in the driver _except_ for the function you're
touching.

Why? If you do a "git blame" on this function and see the commits and
bugs related, it seems that in some machines the BIOS has this nice
idea of re-enabling VGA during some ACPI events. So we concluded that
this specific function should try to assume that maybe something else
(BIOS, KVMr, Debug) could have enabled the power well, then enabled
VGA. So if we just do the "intel_display_power_enabled" check we'll
_always_ do the early return on this function and we'll never really
check if something else really enabled power_well+VGA while the driver
was still keeping its promise of "never touching the power well while
we don't need it". I know, it's confusing and  sounds like a huge
mess. I'm open to suggestions on how to improve it :) You may want to
read this paragraph again, maybe 2 or 3 times.

>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni