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

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 7/5/2023 12:02 PM, Andrzej Hajda wrote:
>
>
> 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@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   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,


I will resend with those suggestion.


Thanks,

Nirmoy

> please go your way if you wish:
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>
> Regards
> Andrzej
>
>>           obj = i915_gem_object_create_internal(i915, PAGE_SIZE);
>>       if (IS_ERR(obj))
>>           return PTR_ERR(obj);
>