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

[Matthew Auld <matthew.auld@intel.com>]

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?

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