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

[Matthew Auld]

On Mon, 21 Mar 2022 at 18:36, Juha-Pekka Heikkila
<juhapekka.heikkila at 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 at 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 at gmail.com> wrote:
> >>>>
> >>>> Add fallback smem allocation for dpt if stolen memory
> >>>> allocation failed.
> >>>>
> >>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at 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.

>
> /Juha-Pekka
>
> >
> >>
> >>>
> >>>> +       }
> >>>>           if (IS_ERR(dpt_obj))
> >>>>                   return ERR_CAST(dpt_obj);
> >>>>
> >>>> --
> >>>> 2.28.0
> >>>>
> >>
>