Re: [PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 11.02.21 um 15:52 schrieb Thomas Zimmermann:
> Hi
>
> Am 11.02.21 um 14:16 schrieb Christian König:
> > Swapping bo->mem was completely unecessary. Cleanup the function which
> > is just a leftover from a TTM cleanup.
>
> Yes this was introduced in a recent cleanup effort. Can you explain 
> what the code intends to do? It seems as if it tries to "re-unmap the 
> BO" if the move_memcpy fails.
>
> If the move_memcpy fails now, it seems like we can live without 
> reverting that call to drm_gem_vram_bo_driver_move_notify(). (?)

I think so, but I'm not 100% sure either.

The swap() -> notify -> swap() was just how TTM did it and that was 
moved into the drivers.

I'm now just trying to remove all the hard write accesses to bo->mem 
from drivers and stumbled over this here.

Thanks for the comments,
Christian.

> Best regards
> Thomas
>
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > ---
> >   drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++--------------
> >   1 file changed, 4 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
> > b/drivers/gpu/drm/drm_gem_vram_helper.c
> > index a0992f0b8afd..0c2233ee6029 100644
> > --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> > @@ -564,9 +564,7 @@ static void 
> > drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
> >       *pl = gbo->placement;
> >   }
> >   -static void drm_gem_vram_bo_driver_move_notify(struct 
> > drm_gem_vram_object *gbo,
> > -                           bool evict,
> > -                           struct ttm_resource *new_mem)
> > +static void drm_gem_vram_bo_driver_move_notify(struct 
> > drm_gem_vram_object *gbo)
> >   {
> >       struct ttm_buffer_object *bo = &gbo->bo;
> >       struct drm_device *dev = bo->base.dev;
> > @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct 
> > drm_gem_vram_object *gbo,
> >                          struct ttm_operation_ctx *ctx,
> >                          struct ttm_resource *new_mem)
> >   {
> > -    int ret;
> > -
> > -    drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
> > -    ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
> > -    if (ret) {
> > -        swap(*new_mem, gbo->bo.mem);
> > -        drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);
> > -        swap(*new_mem, gbo->bo.mem);
> > -    }
> > -    return ret;
> > +    drm_gem_vram_bo_driver_move_notify(gbo);
> > +    return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
> >   }
> >     /*
> > @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct 
> > ttm_buffer_object *bo)
> >         gbo = drm_gem_vram_of_bo(bo);
> >   -    drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
> > +    drm_gem_vram_bo_driver_move_notify(gbo);
> >   }
> >     static int bo_driver_move(struct ttm_buffer_object *bo,

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel