[Intel-gfx] [PATCH 2/2] drm/i915: fully apply WaSkipStolenMemoryFirstPage

[Chris Wilson]

On Wed, Dec 14, 2016 at 06:39:08PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 14, 2016 at 12:55:38PM -0200, Paulo Zanoni wrote:
> > Reserve the first page of stolen memory right after initializing the
> > mm allocator. This means that we won't inherit the FB in case the BIOS
> > decides to put it at the start of stolen. But the BIOS should not be
> > putting it at the start of stolen since it's going to get corrupted. I
> > suppose the bug here is that some pixels at the very top of the screen
> > will be corrupted, so it's not exactly easy to notice.
> > 
> > We have confirmation that the first page of stolen does actually get
> > corrupted, so I really think we should do this in order to avoid any
> > possible future headaches, even if that means losing BIOS framebuffer
> > inheritance. Let's not use the HW in a way it's not supposed to be
> > used.
> > 
> > v2: don't even put the first page on the mm (Chris).
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 27 ++++++++-------------------
> >  1 file changed, 8 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index b1c8897..7435540 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -54,12 +54,6 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
> >  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> >  		return -ENODEV;
> >  
> > -	/* See the comment at the drm_mm_init() call for more about this check.
> > -	 * WaSkipStolenMemoryFirstPage:bdw+ (incomplete)
> > -	 */
> > -	if (start < 4096 && INTEL_GEN(dev_priv) >= 8)
> > -		start = 4096;
> > -
> >  	mutex_lock(&dev_priv->mm.stolen_lock);
> >  	ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size,
> >  					  alignment, start, end,
> > @@ -410,7 +404,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
> >  {
> >  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> >  	unsigned long reserved_total, reserved_base = 0, reserved_size;
> > -	unsigned long stolen_top;
> > +	unsigned long stolen_start = 0, stolen_top;
> >  
> >  	mutex_init(&dev_priv->mm.stolen_lock);
> >  
> > @@ -491,18 +485,13 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
> >  
> >  	ggtt->stolen_usable_size = ggtt->stolen_size - reserved_total;
> >  
> > -	/*
> > -	 * Basic memrange allocator for stolen space.
> > -	 *
> > -	 * TODO: Notice that some platforms require us to not use the first page
> > -	 * of the stolen memory but their BIOSes may still put the framebuffer
> > -	 * on the first page. So we don't reserve this page for now because of
> > -	 * that. Our current solution is to just prevent new nodes from being
> > -	 * inserted on the first page - see the check we have at
> > -	 * i915_gem_stolen_insert_node_in_range(). We may want to fix the fbcon
> > -	 * problem later.
> > -	 */
> > -	drm_mm_init(&dev_priv->mm.stolen, 0, ggtt->stolen_usable_size);
> > +	/* WaSkipStolenMemoryFirstPage:bdw+ */
> > +	if (INTEL_GEN(dev_priv) >= 8)
> > +		stolen_start = 4096;
> > +
> > +	/* Basic memrange allocator for stolen space. */
> > +	drm_mm_init(&dev_priv->mm.stolen, stolen_start,
> > +		    ggtt->stolen_usable_size);
> 
> Don't you need to reduce the size by 4k as well?

Sigh. Yes. No more review for me.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre