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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 11.02.21 um 16:05 schrieb Christian König:
> 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.

We already have a vmap count of zero; so unmapping the BO pages is fine 
at any time. The next caller of vmap will simple instantiate a new mapping.

Let me give this patch a test run tomorrow, but it seems correct.

Best regards
Thomas

> 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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

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