Re: [Intel-gfx] [PATCH] drm/i915/display: Add smem fallback allocation for dpt

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 22.3.2022 12.45, Matthew Auld wrote:
> On Mon, 21 Mar 2022 at 18:36, Juha-Pekka Heikkila
> <juhapekka.heikkila@gmail.com> wrote:
>>
>> On 21.3.2022 14.29, Matthew Auld wrote:
>>> On Fri, 18 Mar 2022 at 09:22, Juha-Pekka Heikkila
>>> <juhapekka.heikkila@gmail.com> wrote:
>>>>
>>>> On 17.3.2022 13.55, Matthew Auld wrote:
>>>>> On Wed, 16 Mar 2022 at 22:23, Juha-Pekka Heikkila
>>>>> <juhapekka.heikkila@gmail.com> wrote:
>>>>>>
>>>>>> Add fallback smem allocation for dpt if stolen memory
>>>>>> allocation failed.
>>>>>>
>>>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/display/intel_dpt.c | 18 ++++++++++++++----
>>>>>>     1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c
>>>>>> index fb0e7e79e0cd..c8b66433d4db 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dpt.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
>>>>>> @@ -10,6 +10,7 @@
>>>>>>     #include "intel_display_types.h"
>>>>>>     #include "intel_dpt.h"
>>>>>>     #include "intel_fb.h"
>>>>>> +#include "gem/i915_gem_internal.h"
>>>>>
>>>>> Nit: these should be kept sorted
>>>>>
>>>>>>
>>>>>>     struct i915_dpt {
>>>>>>            struct i915_address_space vm;
>>>>>> @@ -128,6 +129,10 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>>>>>>            void __iomem *iomem;
>>>>>>            struct i915_gem_ww_ctx ww;
>>>>>>            int err;
>>>>>> +       u64 pin_flags = 0;
>>>>>> +
>>>>>> +       if (i915_gem_object_is_stolen(dpt->obj))
>>>>>> +               pin_flags |= PIN_MAPPABLE; /* for i915_vma_pin_iomap(stolen) */
>>>>>>
>>>>>>            wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>>>>>>            atomic_inc(&i915->gpu_error.pending_fb_pin);
>>>>>> @@ -138,7 +143,7 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>>>>>>                            continue;
>>>>>>
>>>>>>                    vma = i915_gem_object_ggtt_pin_ww(dpt->obj, &ww, NULL, 0, 4096,
>>>>>> -                                                 HAS_LMEM(i915) ? 0 : PIN_MAPPABLE);
>>>>>> +                                                 pin_flags);
>>>>>>                    if (IS_ERR(vma)) {
>>>>>>                            err = PTR_ERR(vma);
>>>>>>                            continue;
>>>>>> @@ -248,10 +253,15 @@ intel_dpt_create(struct intel_framebuffer *fb)
>>>>>>
>>>>>>            size = round_up(size * sizeof(gen8_pte_t), I915_GTT_PAGE_SIZE);
>>>>>>
>>>>>> -       if (HAS_LMEM(i915))
>>>>>> -               dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
>>>>>> -       else
>>>>>> +       dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
>>>>>> +       if (IS_ERR(dpt_obj) && i915_ggtt_has_aperture(to_gt(i915)->ggtt))
>>>>>>                    dpt_obj = i915_gem_object_create_stolen(i915, size);
>>>>>> +       if (IS_ERR(dpt_obj) && !HAS_LMEM(i915)) {
>>>>>> +               drm_dbg_kms(&i915->drm, "fb: [FB:%d] Allocating dpt from smem\n",
>>>>>> +                           fb->base.base.id);
>>>>>> +
>>>>>> +               dpt_obj = i915_gem_object_create_internal(i915, size);
>>>>>
>>>>> Looks like we are missing some prerequisite patch to be able to
>>>>> directly map such memory in vma_pin_iomap?
>>>>
>>>> For these functions I'm more like a consumer, I was following
>>>> suggestions from Chris on this. Is there something extra that should be
>>>> considered in this regard when use it like this?
>>>
>>> AFAICT this will trigger the WARN_ON() in vma_pin_iomap() if we
>>> fallback to create_internal(), since the object is now not lmem and is
>>> also not map_and_fenceable(i.e PIN_MAPPABLE).
>>
>> This shouldn't affect case when dpt allocation from lmem failed, it is
>> expected to go to "return ERR_CAST(dpt_obj);" below these comments. On
>> situation when allocating lmem and stolen failed on next "if" I added
>> !HAS_LMEM(i915) to handle situation with lmem. Though, when I was
>> originally trying this patch without limiting lmem case I remember with
>> dg2 I got black screen but I don't remember seeing WARN_ON() in logs.
>>
>>>
>>> The other issue is that we need some way of CPU mapping this type of
>>> object, like with calling i915_gem_object_pin_map() inside
>>> vma_pin_iomap(). It looks like there is an internal patch that tries
>>> to handle both issues, so I guess we need to also bring that patch
>>> upstream as a prerequisite to this?
>>
>> I have above in intel_dpt_pin(..) that "pin_flags |= PIN_MAPPABLE" when
>> handling stolen memory. I suspect patch you are referring to is this
>> same patch I wrote, here just adjusted for upstreaming. This patch was
>> earlier tried by Lucas and Manasi to be working with adlp and apparently
>> cases with virtual machine this make it possible to have tiled
>> framebuffers. Without this patch those special cases will get -e2big
>> when creating tiled fb and no stolen memory available.
> 
> When the GGTT pin eventually ends up returning some vma that is not
> within the ggtt->mappable_end, then we will start hitting the above
> issues, starting with the WARN_ON. If you use PIN_HIGH here for the
> non-stolen case, it should highlight the issue more reliably I think.
> 

You mean once there's no space left in stolen there would be WARN_ON()? 
This is case which was earlier tested by Lucas and Manasi on adlp to be 
working correctly, this was on top of drm-tip. Also on internal testing 
you can see platforms taking this path reliably with no errors.

I'm not sure why use PIN_HIGH for non stolen case, my exposure to gem 
related parts is limited hence I was following Chris's suggestion to put 
zero flag for i915_gem_object_ggtt_pin_ww(..) when not using stolen.

/Juha-pekka