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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Sep 23, 2013 at 01:48:42PM -0300, Paulo Zanoni wrote:
> 2013/9/20  <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.
> >
> > v2: Add intel_display_power_put() for the num_pipes==0 case
> 
> Even though the patch was applied, the issue didn't go away. It looks
> like the conflict-merging went wrong. Look:
> http://cgit.freedesktop.org/~danvet/drm-intel/commit/?h=drm-intel-next-queued&id=971b0597b3a13576e1af1e0a90379aa308f498e5
> 
> The new _put function is not after i915_disable_vga_mem().


Oops, so much for trusting the tools too much ;-) Should be fixed now,
thanks for catching this.
-Daniel

> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e5c7b10..956d143 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1326,13 +1326,18 @@ 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. */
> >         /* FIXME: do pre/post-mode set stuff in core KMS code */
> >         dev->vblank_disable_allowed = 1;
> > -       if (INTEL_INFO(dev)->num_pipes == 0)
> > +       if (INTEL_INFO(dev)->num_pipes == 0) {
> > +               intel_display_power_put(dev, POWER_DOMAIN_VGA);
> >                 return 0;
> > +       }
> >
> >         ret = intel_fbdev_init(dev);
> >         if (ret)
> > @@ -1358,6 +1363,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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch