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

[Christian König]

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 at intel.com>
>>> Cc: Christian König <christian.koenig at 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 at amd.com>.
>
> Should we go ahead and land this? (minus patch 3 since that is already 
> fixed by vmware folks).

Yeah, sure go ahead.

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