Re: [PATCH 2/2] drm/i915/display: Do not use stolen on MTL

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

On 30.06.2023 19:01, Nirmoy Das wrote:
> Use smem on MTL due to a HW bug in MTL that prevents
> reading from stolen memory using LMEM BAR.
>
> Cc: Oak Zeng <oak.zeng@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/display/intel_fbdev.c   | 2 ++
>   drivers/gpu/drm/i915/display/intel_overlay.c | 7 ++++---
>   2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 1cc0ddc6a310..10e38d60f9ef 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -182,6 +182,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>   		obj = i915_gem_object_create_lmem(dev_priv, size,
>   						  I915_BO_ALLOC_CONTIGUOUS |
>   						  I915_BO_ALLOC_USER);
> +	} else if (IS_METEORLAKE(dev_priv)) { /* Wa_22018444074 */
> +		obj = i915_gem_object_create_shmem(dev_priv, size);

If you put the check inside following else clause, you will have only 
one place to call i915_gem_object_create_shmem.

>   	} else {
>   		/*
>   		 * If the FB is too big, just don't use it since fbdev is not very
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> index d6fe2bbabe55..05ae446c8a56 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -1348,12 +1348,13 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
>   static int get_registers(struct intel_overlay *overlay, bool use_phys)
>   {
>   	struct drm_i915_private *i915 = overlay->i915;
> -	struct drm_i915_gem_object *obj;
> +	struct drm_i915_gem_object *obj = NULL;
>   	struct i915_vma *vma;
>   	int err;
>   
> -	obj = i915_gem_object_create_stolen(i915, PAGE_SIZE);
> -	if (IS_ERR(obj))
> +	if (!IS_METEORLAKE(i915)) /* Wa_22018444074 */
> +		obj = i915_gem_object_create_stolen(i915, PAGE_SIZE);
> +	if (IS_ERR_OR_NULL(obj))

Initializing obj with ERR_PTR(-ENODEV) and using IS_ERR here will be 
aligned to previous stanza (intelfb_alloc).

Since my comments are close to  bikeshedding, please go your way if you 
wish:
Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>

Regards
Andrzej

>   		obj = i915_gem_object_create_internal(i915, PAGE_SIZE);
>   	if (IS_ERR(obj))
>   		return PTR_ERR(obj);