[Intel-gfx] [PATCH] drm/i915: Do NOT skip the first 4k of stolen memory for pre-allocated buffers

[Daniel Vetter]

On Fri, Mar 30, 2018 at 02:27:15PM +0200, Hans de Goede wrote:
> Before this commit the WaSkipStolenMemoryFirstPage workaround code was
> skipping the first 4k by passing 4096 as start of the address range passed
> to drm_mm_init(). This means that calling drm_mm_reserve_node() to try and
> reserve the firmware framebuffer so that we can inherit it would always
> fail, as the firmware framebuffer starts at address 0.
> 
> Commit d43537610470 ("drm/i915: skip the first 4k of stolen memory on
> everything >= gen8") says in its commit message: "This is confirmed to fix
> Skylake screen flickering issues (probably caused by the fact that we
> initialized a ring in the first page of stolen, but I didn't 100% confirm
> this theory)."
> 
> Which suggests that it is safe to use the first page for a linear
> framebuffer as the firmware is doing.
> 
> This commit always passes 0 as start to drm_mm_init() and works around
> WaSkipStolenMemoryFirstPage in i915_gem_stolen_insert_node_in_range()
> by insuring the start address passed by to drm_mm_insert_node_in_range()
> is always 4k or more. All entry points to i915_gem_stolen.c go through
> i915_gem_stolen_insert_node_in_range(), so that any newly allocated
> objects such as ring-buffers will not be allocated in the first 4k.
> 
> The one exception is i915_gem_object_create_stolen_for_preallocated()
> which directly calls drm_mm_reserve_node() which now will be able to
> use the first 4k.
> 
> This fixes the i915 driver no longer being able to inherit the firmware
> framebuffer on gen8+, which fixes the video output changing from the
> vendor logo to a black screen as soon as the i915 driver is loaded
> (on systems without fbcon).
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>

I think this is worth a shot. The only explanation I can think of why the
GOP could get away with this and still follow the w/a is if it doesn't
have a 1:1 mapping between GGTT and stolen. Atm we hardcode that
assumption in intel_alloc_initial_plane_obj by passing the base_aligned as
both the stolen_offset and the gtt_offset (but it's only the gtt_offset
really). And since we're not re-writing the ptes it's not noticeable.

I think to decide whether this is the right approach we should
double-check whether that 1:1 assumption really holds true: Either read
back the ggtt ptes and check their addresses (but iirc on some platforms
their write-only, readback doesn't work), or we also rewrite the ptes
again for preallocated stuff, like when binding a normal object into the
gtt. If either of these approaches confirms that those affected gen8+
machines still use the 1:1 mapping, then I'm happy to put my r-b on this
patch. If not, well then we at least know what to fix: We need to read out
the real stolen_offset, instead of making assumptions.

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index af915d041281..ad949cc30928 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -51,6 +51,10 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>  		return -ENODEV;
>  
> +	/* WaSkipStolenMemoryFirstPage:bdw+ */
> +	if (INTEL_GEN(dev_priv) >= 8 && start < 4096)
> +		start = 4096;
> +
>  	mutex_lock(&dev_priv->mm.stolen_lock);
>  	ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node,
>  					  size, alignment, 0,
> @@ -343,7 +347,6 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  {
>  	resource_size_t reserved_base, stolen_top;
>  	resource_size_t reserved_total, reserved_size;
> -	resource_size_t stolen_usable_start;
>  
>  	mutex_init(&dev_priv->mm.stolen_lock);
>  
> @@ -435,17 +438,11 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  			 (u64)resource_size(&dev_priv->dsm) >> 10,
>  			 ((u64)resource_size(&dev_priv->dsm) - reserved_total) >> 10);
>  
> -	stolen_usable_start = 0;
> -	/* WaSkipStolenMemoryFirstPage:bdw+ */
> -	if (INTEL_GEN(dev_priv) >= 8)
> -		stolen_usable_start = 4096;
> -
>  	dev_priv->stolen_usable_size =
> -		resource_size(&dev_priv->dsm) - reserved_total - stolen_usable_start;
> +		resource_size(&dev_priv->dsm) - reserved_total;
>  
>  	/* Basic memrange allocator for stolen space. */
> -	drm_mm_init(&dev_priv->mm.stolen, stolen_usable_start,
> -		    dev_priv->stolen_usable_size);
> +	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->stolen_usable_size);
>  
>  	return 0;
>  }
> -- 
> 2.17.0.rc2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch