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

From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Thomas Zimmermann <tzimmermann@suse.de>,
	airlied@linux.ie, daniel@ffwll.ch, kraxel@redhat.com,
	zackr@vmware.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify
Date: Thu, 11 Feb 2021 16:05:13 +0100	[thread overview]
Message-ID: <acc54d35-38b1-52b0-1ef1-75a8a4f6469a@gmail.com> (raw)
In-Reply-To: <365e99a6-24c4-33b5-ae9c-15904b056b89@suse.de>

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,
>>
>

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