[Intel-gfx] [PATCH 5/5] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[Paulo Zanoni]

2013/9/12  <ville.syrjala at linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> VGA registers live inside the power well on HSW, so in order to write
> the VGA MSR register we need the power well to be on.
>
> We really must write to the register to properly clear the
> VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
> the power well is down. It seems that the implicit zeroing done by
> the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
> whomever is actually responsible for the memory decode ranges.

Due to the paragraph above, don't we also need to fix
i915_redisable_vga() ? If not, please explain why on the commit
message.

Don't we also need to do something about i915_enable_vga_mem()?


>
> If we leave VGA memory decode enabled, and then turn off the power well,
> all VGA memory reads will return zeroes. But if we first disable VGA
> memory deocde and then turn off the power well, VGA memory reads
> return all ones, indicating that the access wasn't claimed by anyone.
> For the vga arbiter to function correctly the IGD must not claim the
> VGA memory accesses.
>
> Previously we were doing the VGA_MSR register access while the power well
> was excplicitly powered up during driver init. But ever since
>
>  commit 6e1b4fdad5157bb9e88777d525704aba24389bee
>  Author: Ville Syrjälä <ville.syrjala at linux.intel.com>
>  Date:   Thu Sep 5 20:40:52 2013 +0300
>
>     drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done
>
> we delay the VGA memory disable until fbcon has initialized, and so
> there's a possibility that the power well got turned off during the
> fbcon modeset. Also vgacon_save_screen() will need the power well to be
> on to be able to read the VGA memory.
>
> So immediately after enabling the power well during init grab a refence
> for VGA purposes, and after all the VGA handling is done, release it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e5c7b10..23f965d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
>         intel_init_power_well(dev);
>
> +       /* Keep VGA alive until i915_disable_vga_mem() */

I'd write more here. Maybe copy the paragraph in the commit message.
This seems like an important thing and people won't really get the
idea if they don't git-blame.


> +       intel_display_power_get(dev, POWER_DOMAIN_VGA);
> +
>         intel_modeset_gem_init(dev);
>
>         /* Always safe in the mode setting case. */
> @@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>          * vgacon_save_screen() works during the handover.
>          */
>         i915_disable_vga_mem(dev);
> +       intel_display_power_put(dev, POWER_DOMAIN_VGA);
>
>         /* Only enable hotplug handling once the fbdev is fully set up. */
>         dev_priv->enable_hotplug_processing = true;
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni