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

[Chris Wilson]

On Thu, Dec 15, 2016 at 03:26:59PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 15, 2016 at 11:23:55AM -0200, Paulo Zanoni wrote:
> > Don't even tell the mm allocator to handle the first page of stolen on
> > the affected platforms. 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.
> > 
> > Notice that now ggtt->stolen_usable_size won't reflect the ending
> > address of the stolen usable range anymore, so we have to fix the
> > places that rely on this. To simplify, we'll just use U64_MAX.
> > 
> > v2: don't even put the first page on the mm (Chris)
> > v3: drm_mm_init() takes size instead of end as argument (Ville)
> > v4: add a comment explaining the reserved ranges (Chris)
> >     use 0 for start and U64_MAX for end when possible (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_gtt.h    | 10 +++++++++-
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 36 ++++++++++++----------------------
> >  drivers/gpu/drm/i915/intel_fbc.c       |  2 +-
> >  3 files changed, 22 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index 8965bbb..0055b85 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -315,8 +315,16 @@ struct i915_ggtt {
> >  	struct i915_address_space base;
> >  	struct io_mapping mappable;	/* Mapping to our CPU mappable region */
> >  
> > +	/* Stolen memory is segmented in hardware with different portions
> > +	 * offlimits to certain functions.
> > +	 *
> > +	 * The drm_mm is initialised to the total accessible range, as found
> > +	 * from the PCI config. On Broadwell+, this is further restricted to
> > +	 * avoid the first page! The upper end of stolen memory is reserved for
> > +	 * hardware functions and similarly removed from the accessible range.
> > +	 */
> >  	size_t stolen_size;		/* Total size of stolen memory */
> > -	size_t stolen_usable_size;	/* Total size minus BIOS reserved */
> > +	size_t stolen_usable_size;	/* Total size minus reserved ranges */
> >  	size_t stolen_reserved_base;
> >  	size_t stolen_reserved_size;
> >  	u64 mappable_end;		/* End offset that we can CPU map */
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index b1c8897..cbbfc64 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,
> > @@ -73,11 +67,8 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
> >  				struct drm_mm_node *node, u64 size,
> >  				unsigned alignment)
> >  {
> > -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> > -
> >  	return i915_gem_stolen_insert_node_in_range(dev_priv, node, size,
> > -						    alignment, 0,
> > -						    ggtt->stolen_usable_size);
> > +						    alignment, 0, U64_MAX);
> 
> Just i915_gem_stolen_insert_node() then?

This is stolen_insert_node() :)
Could be turned into an inline later.

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre