| Message ID | 20210211131659.276275-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify | expand |
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(). (?) Best regards Thomas > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > 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, >
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@amd.com> >> --- >> 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, >> >
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@amd.com> >>> --- >>> 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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. > > Signed-off-by: Christian König <christian.koenig@amd.com> Appears to work. Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Tested-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > 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, >
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,
Swapping bo->mem was completely unecessary. Cleanup the function which is just a leftover from a TTM cleanup. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)