[Intel-xe] [PATCH] drm/xe: Use Xe BO pin / unpin functions for FB pin

[Matthew Auld]

On Wed, 5 Apr 2023 at 15:28, Matthew Brost <matthew.brost at intel.com> wrote:
>
> On Wed, Apr 05, 2023 at 02:14:22PM +0000, Matthew Brost wrote:
> > On Wed, Apr 05, 2023 at 02:19:35PM +0100, Matthew Auld wrote:
> > > On Wed, 5 Apr 2023 at 05:19, Matthew Brost <matthew.brost at intel.com> wrote:
> > > >
> > > > Need to use XE BO pin / unpin for suspend / resume to work properly.
> > > > Part this fix too is too allow xe_bo_pin to be called more than once.
> > > >
> > > > Should fix:
> > > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/244
> > > >
> > > > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > >
> > > Change makes sense to me, but I think we also need to update xe_bo_unpin()?
> > >
> >
> > Yep, thought that coded correctly but was looking at xe_bo_unpin_external().
> >
> > > But I must be misunderstanding something here. AFAICT we can have
> > > various pinned VRAM objects, and xe_bo_pin() still adds them to
> > > pinned.kernel_bo_present, where during suspend we evict everything on
> > > that list to system memory. But as these are all still pinned, suspend
> > > is still going to fail with this patch, given the recent TTM changes.
> >
> > What reference recent TTM changes? This all worked at one point in time.
> >
>
> Oh, I see:
> git format-patch -1 f87c1f0b7b79b
>
> I would've NAK'd that one as this broke us. What you think is the best
> way to move forward on this one? I like the idea of using TTM to move
> the objects...
>
> I know we could just hack this and set pin_count=0 before calling
> validate aand restore it after? Horried hack though?
>
> Revert the above patch + post upstream?

Christian, can you share your thoughts here?

This is regarding the TTM change: f87c1f0b7b79 ("drm/ttm: prevent
moving of pinned BOs"). To quickly summarize, it looks like Xe was
relying on being able to move pinned BOs for suspend-resume, where the
driver has some special kernel-internal VRAM buffers that are
perma-pinned, but need to be moved to system memory during suspend and
then copied back when resuming. This was all handled by just doing
something like ttm_bo_validate(PL_SYSTEM), without needing any special
handling. But this ofc no longer works.

So wondering if it makes any sense to move the decision back into the
driver, for whether to treat moving pinned objects as an error or not,
given the above use case with suspend-resume in Xe?

>
> BTW, this patch is needed regardless with the fix in xe_bo_unpin().
>
> Matt
>
> > Matt
> >
> > > What am I missing?
> > >
> > > > ---
> > > >  drivers/gpu/drm/xe/display/xe_fb_pin.c | 4 ++--
> > > >  drivers/gpu/drm/xe/xe_bo.c             | 5 +++--
> > > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > > index 65c0bc28a3d1..c7c849be288f 100644
> > > > --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > > +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > > @@ -205,7 +205,7 @@ static struct i915_vma *__xe_pin_fb_vma(struct intel_framebuffer *fb,
> > > >
> > > >         ret = xe_bo_validate(bo, NULL, true);
> > > >         if (!ret)
> > > > -               ttm_bo_pin(&bo->ttm);
> > > > +               xe_bo_pin(bo);
> > > >         ttm_bo_unreserve(&bo->ttm);
> > > >         if (ret)
> > > >                 goto err;
> > > > @@ -222,7 +222,7 @@ static struct i915_vma *__xe_pin_fb_vma(struct intel_framebuffer *fb,
> > > >
> > > >  err_unpin:
> > > >         ttm_bo_reserve(&bo->ttm, false, false, NULL);
> > > > -       ttm_bo_unpin(&bo->ttm);
> > > > +       xe_bo_unpin(bo);
> > > >         ttm_bo_unreserve(&bo->ttm);
> > > >  err:
> > > >         kfree(vma);
> > > > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > > > index 3a482c61c3ec..25c696bd380c 100644
> > > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > > @@ -1278,8 +1278,8 @@ int xe_bo_pin(struct xe_bo *bo)
> > > >          */
> > > >         XE_BUG_ON(bo->ttm.base.import_attach);
> > > >
> > > > -       /* We only expect at most 1 pin */
> > > > -       XE_BUG_ON(xe_bo_is_pinned(bo));
> > > > +       if (xe_bo_is_pinned(bo))
> > > > +               goto pin;
> > > >
> > > >         err = xe_bo_validate(bo, NULL, false);
> > > >         if (err)
> > > > @@ -1308,6 +1308,7 @@ int xe_bo_pin(struct xe_bo *bo)
> > > >                 }
> > > >         }
> > > >
> > > > +pin:
> > > >         ttm_bo_pin(&bo->ttm);
> > > >
> > > >         /*
> > > > --
> > > > 2.34.1
> > > >