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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 13, 2013 at 05:27:59PM -0300, Paulo Zanoni wrote:
> 2013/9/12  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@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()?

Yeah probably we should. For i915_enable_vga_mem() we could just power
the well back around the access if it's not already up.

The rediable_vga case is somewhat more problematic since we call it
during lid events. We'd always need to power the well back up just to
check if the BIOS was an idiot and re-enabled VGA. Maybe we can assume
that on HSW the BIOS isn't that stupid. For resume I guess we may want
to power it back up and do the redisable_vga thing. I'll take a closer
look at it next week.

> 
> 
> >
> > 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@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@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC