Re: [Intel-gfx] [PATCH v3 09/12] drm/ttm: Document and optimize ttm_bo_pipeline_gutting()

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Tue, 2021-05-25 at 12:00 +0100, Matthew Auld wrote:
> On Fri, 21 May 2021 at 16:33, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
> > 
> > If the bo is idle when calling ttm_bo_pipeline_gutting(), we
> > unnecessarily
> > create a ghost object and push it out to delayed destroy.
> > Fix this by adding a path for idle, and document the function.
> > 
> > Also avoid having the bo end up in a bad state vulnerable to user-
> > space
> > triggered kernel BUGs if the call to ttm_tt_create() fails.
> > 
> > Finally reuse ttm_bo_pipeline_gutting() in ttm_bo_evict().
> > 
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >  drivers/gpu/drm/ttm/ttm_bo.c      | 20 ++++++------
> >  drivers/gpu/drm/ttm/ttm_bo_util.c | 52
> > ++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/ttm/ttm_tt.c      |  5 +++
> >  include/drm/ttm/ttm_tt.h          | 10 ++++++
> >  4 files changed, 73 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> > b/drivers/gpu/drm/ttm/ttm_bo.c
> > index ca1b098b6a56..a8fa3375b8aa 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -501,10 +501,15 @@ static int ttm_bo_evict(struct
> > ttm_buffer_object *bo,
> >         bdev->funcs->evict_flags(bo, &placement);
> > 
> >         if (!placement.num_placement &&
> > !placement.num_busy_placement) {
> > -               ttm_bo_wait(bo, false, false);
> > +               ret = ttm_bo_wait(bo, true, false);
> > +               if (ret)
> > +                       return ret;
> > 
> > -               ttm_bo_cleanup_memtype_use(bo);
> > -               return ttm_tt_create(bo, false);
> > +               /*
> > +                * Since we've already synced, this frees backing
> > store
> > +                * immediately.
> > +                */
> > +               return ttm_bo_pipeline_gutting(bo);
> >         }
> > 
> >         ret = ttm_bo_mem_space(bo, &placement, &evict_mem, ctx);
> > @@ -974,13 +979,8 @@ int ttm_bo_validate(struct ttm_buffer_object
> > *bo,
> >         /*
> >          * Remove the backing store if no placement is given.
> >          */
> > -       if (!placement->num_placement && !placement-
> > >num_busy_placement) {
> > -               ret = ttm_bo_pipeline_gutting(bo);
> > -               if (ret)
> > -                       return ret;
> > -
> > -               return ttm_tt_create(bo, false);
> > -       }
> > +       if (!placement->num_placement && !placement-
> > >num_busy_placement)
> > +               return ttm_bo_pipeline_gutting(bo);
> > 
> >         /*
> >          * Check whether we need to move buffer.
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > index 4a7d3d672f9a..7fa9b3a852eb 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > @@ -585,26 +585,70 @@ int ttm_bo_move_accel_cleanup(struct
> > ttm_buffer_object *bo,
> >  }
> >  EXPORT_SYMBOL(ttm_bo_move_accel_cleanup);
> > 
> > +/**
> > + * ttm_bo_pipeline_gutting - purge the contents of a bo
> > + * @bo: The buffer object
> > + *
> > + * Purge the contents of a bo, async if the bo is not idle.
> > + * After a successful call, the bo is left unpopulated in
> > + * system placement. The function may wait uninterruptible
> > + * for idle on OOM.
> > + *
> > + * Return: 0 if successful, negative error code on failure.
> > + */
> >  int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
> >  {
> >         static const struct ttm_place sys_mem = { .mem_type =
> > TTM_PL_SYSTEM };
> >         struct ttm_buffer_object *ghost;
> > +       struct ttm_tt *ttm;
> >         int ret;
> > 
> > -       ret = ttm_buffer_object_transfer(bo, &ghost);
> > +       /* If already idle, no need for ghost object dance. */
> > +       ret = ttm_bo_wait(bo, false, true);
> > +       if (ret != -EBUSY) {
> > +               if (!bo->ttm) {
> > +                       ret = ttm_tt_create(bo, true);
> 
> Why do we now unconditionally add clearing? Below also.

Here we've dropped the bo content and we add but do not populate a page
vector. Now if someone resurrects this object we obtain new pages and
those must be cleared, at least that's the intention.

/Thomas



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx