2013/10/11 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: > On Fri, 11 Oct 2013 17:16:55 -0300 > Paulo Zanoni <przanoni@xxxxxxxxx> wrote: > >> 2013/10/11 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: >> > Rather than using a HSW specific check. >> > >> > Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> >> > --- >> > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
