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

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

On 7/11/2023 5:59 PM, Tvrtko Ursulin wrote:
>
> On 10/07/2023 10:00, Nirmoy Das wrote:
>> Hi Tvrkto,
>>
>> On 7/6/2023 3:43 PM, Tvrtko Ursulin wrote:
>>>
>>> On 06/07/2023 14:35, Nirmoy Das wrote:
>>>>
>>>> On 7/6/2023 3:32 PM, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 30/06/2023 18:01, Nirmoy Das wrote:
>>>>>> Use smem on MTL due to a HW bug in MTL that prevents
>>>>>> reading from stolen memory using LMEM BAR.
>>>>>
>>>>> Does anything remain in stolen or could the memory region just not 
>>>>> be created?
>>>>
>>>>
>>>> GSC requires DSM which can't use smem for another bug.
>>>
>>> Okay, thanks.
>>>
>>> As a related comment, these if-if-if object creation ladders were 
>>> always a bit ugly and some years ago I was suggesting we create a 
>>> helper with some "intent/usage" flags. Which could then dtrt ie. 
>>> create the right object for that intent/usage and platform. I 
>>> *think* I possibly even had a RFC... need to try and find it.
>>
>>
>> Did you find it :) Would be nice to have a better way to detect and 
>> apply memory region as per platfrom/usecase.
>
> Nope. Basically the idea boiled down to figuring out if it is possible 
> to express the "requirements" via intent flags. Like do we need CPU 
> access, is it mostly GPU, can it be volatile etc. And then combine the 
> intent with the platform to figure out what kind of object to create. 


Yes, that would be nice.

> But it was many years ago and I am not sure if the idea would still 
> apply so easily, without looking at the all call sites. Could easily 
> end up complicated so I cannot dare to say it is worth spending time 
> looking at this.

I will record this in a Jira and hopefully get back to this when I have 
less urgent work.


Thanks,

Nirmoy

>
> Regards,
>
> Tvrtko
>
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>>
>>>>>> 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);
>>>>>>       } 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))
>>>>>>           obj = i915_gem_object_create_internal(i915, PAGE_SIZE);
>>>>>>       if (IS_ERR(obj))
>>>>>>           return PTR_ERR(obj);