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

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

On Thu, Sep 19, 2013 at 07:05:14PM -0300, Paulo Zanoni wrote:
> 2013/9/16  <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.
> >
> > 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() */
> > +       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);
> 
> There's a "return 0" between the get and the put. (I was already at
> patch 8 when I realized that!)

Ugh. That whole 0 pipes thing seems rather wonky. Does that thing even
have many of the display resources we touch before the return 0 there?
I guess it's not a real problem for HSW at this point since there aren't
HSWs w/o the display block yet. But I'll add a put call there just to
keep the resemblence of sanity for that case.

> 
> While reviewing your series I started thinking: shouldn't we just
> get/put POWER_DOMAIN_VGA directly inside the functions that touch the
> VGA code? This shouldn't really be an expensive thing, and it will
> make our code simpler and harder to break. While your series seems
> correct, there is code that depends on state left by other code, which
> IMHO increases the complexity of the already-too-complex init/resume
> paths. This is just a suggestion, not a requirement :)

We need VGA to stay alive (and hence the power well) the whole time until
i915_disable_vga_mem() is called. I didn't want to put the get/put calls
inside some random functions we call during init since then it becomes
much harder to see the relationship between the get/put calls.

> 
> >
> >         /* 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