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

[Ville Syrjälä]

On Thu, Dec 15, 2016 at 01:40:15PM +0000, Chris Wilson wrote:
> 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() :)

/me goes get some coffee...

> 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

-- 
Ville Syrjälä
Intel OTC