Re: [Intel-gfx] [PATCH 1/4] drm/gem-vram: handle NULL bo->resource in move callback

["Christian König" <christian.koenig@amd.com>]

Am 21.02.23 um 17:26 schrieb Matthew Auld:
> On 21/02/2023 16:17, Christian König wrote:
>> Am 21.02.23 um 17:13 schrieb Matthew Auld:
>>> On 10/02/2023 11:03, Christian König wrote:
>>>> Am 08.02.23 um 15:53 schrieb Matthew Auld:
>>>>> The ttm BO now initially has NULL bo->resource, and leaves the driver
>>>>> the handle that. However it looks like we forgot to handle that for
>>>>> ttm_bo_move_memcpy() users, like with vram-gem, since it just 
>>>>> silently
>>>>> returns zero. This seems to then trigger warnings like:
>>>>>
>>>>> WARNING: CPU: 0 PID: 1 at 
>>>>> drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?)
>>>>>
>>>>> Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM,
>>>>> otherwise do the multi-hop sequence to ensure can safely call into
>>>>> ttm_bo_move_memcpy(), since it might also need to clear the memory.
>>>>> This should give the same behaviour as before.
>>>>>
>>>>> While we are here let's also treat calling ttm_bo_move_memcpy() with
>>>>> NULL bo->resource as programmer error, where expectation is that 
>>>>> upper
>>>>> layers should now handle it.
>>>>>
>>>>> Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources 
>>>>> during BO creation")
>>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>
>>>> Oh, I wasn't aware that this broke at so many places. Especially 
>>>> radeon was tested earlier in the development of the patch set.
>>>>
>>>> Thanks for looking into that, the radeon patch has my rb and the 
>>>> rest of the series is Acked-by: Christian König 
>>>> <christian.koenig@amd.com>.
>>>
>>> Should we go ahead and land this? (minus patch 3 since that is 
>>> already fixed by vmware folks).
>>
>> Yeah, sure go ahead.
>
> I assume this has to go via some drm-misc type tree, for which I don't 
> currently have commit rights. Can you help with merging this?

Sure, going to push the series tomorrow.

Thanks,
Christian.

>
>>
>> Thanks,
>> Christian.
>>
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++++
>>>>>   drivers/gpu/drm/ttm/ttm_bo_util.c     |  4 ++--
>>>>>   2 files changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
>>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>> index d40b3edb52d0..0bea3df2a16d 100644
>>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>>>> @@ -916,6 +916,17 @@ static int bo_driver_move(struct 
>>>>> ttm_buffer_object *bo,
>>>>>   {
>>>>>       struct drm_gem_vram_object *gbo;
>>>>> +    if (!bo->resource) {
>>>>> +        if (new_mem->mem_type != TTM_PL_SYSTEM) {
>>>>> +            hop->mem_type = TTM_PL_SYSTEM;
>>>>> +            hop->flags = TTM_PL_FLAG_TEMPORARY;
>>>>> +            return -EMULTIHOP;
>>>>> +        }
>>>>> +
>>>>> +        ttm_bo_move_null(bo, new_mem);
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>>       gbo = drm_gem_vram_of_bo(bo);
>>>>>       return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem);
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
>>>>> b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>>> index d9d2b0903b22..fd9fd3d15101 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>>> @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct 
>>>>> ttm_buffer_object *bo,
>>>>>       bool clear;
>>>>>       int ret = 0;
>>>>> -    if (!src_mem)
>>>>> -        return 0;
>>>>> +    if (WARN_ON(!src_mem))
>>>>> +        return -EINVAL;
>>>>>       src_man = ttm_manager_type(bdev, src_mem->mem_type);
>>>>>       if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
>>>>
>>