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@xxxxxxxxxxxxxxx> 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@xxxxxxx>
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > ---
> > 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
