Re: [PATCH v1 1/2] drm/amdgpu: make non-NULL out fence mandatory

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

On 16.09.25 11:46, Pierre-Eric Pelloux-Prayer wrote:
> 
> 
> Le 16/09/2025 à 11:25, Christian König a écrit :
>> On 16.09.25 09:08, Pierre-Eric Pelloux-Prayer wrote:
>>> amdgpu_ttm_copy_mem_to_mem has a single caller, make sure the out
>>> fence is non-NULL to simplify the code.
>>> Since none of the pointers should be NULL, we can enable
>>> __attribute__((nonnull))__.
>>>
>>> While at it make the function static since it's only used from
>>> amdgpuu_ttm.c.
>>>
>>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 17 ++++++++---------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  6 ------
>>>   2 files changed, 8 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 27ab4e754b2a..70b817b5578d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -284,12 +284,13 @@ static int amdgpu_ttm_map_buffer(struct ttm_buffer_object *bo,
>>>    * move and different for a BO to BO copy.
>>>    *
>>>    */
>>> -int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>> -                   const struct amdgpu_copy_mem *src,
>>> -                   const struct amdgpu_copy_mem *dst,
>>> -                   uint64_t size, bool tmz,
>>> -                   struct dma_resv *resv,
>>> -                   struct dma_fence **f)
>>> +__attribute__((nonnull))
>>
>> That looks fishy.
>>
>>> +static int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>> +                      const struct amdgpu_copy_mem *src,
>>> +                      const struct amdgpu_copy_mem *dst,
>>> +                      uint64_t size, bool tmz,
>>> +                      struct dma_resv *resv,
>>> +                      struct dma_fence **f)
>>
>> I'm not an expert for those, but looking at other examples that should be here and look something like:
>>
>> __attribute__((nonnull(7)))
> 
> Both syntax are valid. The GCC docs says:
> 
>    If no arg-index is given to the nonnull attribute, all pointer arguments are marked as non-null

Never seen that before. Is that gcc specifc or standardized?

> 
> 
>>
>> But I think for this case here it is also not a must have to have that.
> 
> I can remove it if you prefer, but it doesn't hurt to have the compiler validate usage of the functions.

Yeah it's clearly useful, but I'm worried that clang won't like it.

Christian.

> 
> Pierre-Eric
> 
> 
>>
>> Regards,
>> Christian.
>>
>>>   {
>>>       struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring;
>>>       struct amdgpu_res_cursor src_mm, dst_mm;
>>> @@ -363,9 +364,7 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>>       }
>>>   error:
>>>       mutex_unlock(&adev->mman.gtt_window_lock);
>>> -    if (f)
>>> -        *f = dma_fence_get(fence);
>>> -    dma_fence_put(fence);
>>> +    *f = fence;
>>>       return r;
>>>   }
>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> index bb17987f0447..07ae2853c77c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> @@ -170,12 +170,6 @@ int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
>>>                  struct dma_resv *resv,
>>>                  struct dma_fence **fence, bool direct_submit,
>>>                  bool vm_needs_flush, uint32_t copy_flags);
>>> -int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>> -                   const struct amdgpu_copy_mem *src,
>>> -                   const struct amdgpu_copy_mem *dst,
>>> -                   uint64_t size, bool tmz,
>>> -                   struct dma_resv *resv,
>>> -                   struct dma_fence **f);
>>>   int amdgpu_ttm_clear_buffer(struct amdgpu_bo *bo,
>>>                   struct dma_resv *resv,
>>>                   struct dma_fence **fence);