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

[Matthew Brost]

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?

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
> > >