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

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

[Pierre-Eric Pelloux-Prayer]

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))
+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)
 {
 	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);
-- 
2.43.0

[PATCH v1 2/2] drm/amdgpu: remove gart_window_lock usage from gmc v12

[Pierre-Eric Pelloux-Prayer]

This lock was part of the SDMA workaround originally implemented in
gmc_v10_0_flush_gpu_tlb (a70cb2176f7ef6f moved it to
amdgpu_gmc_flush_gpu_tlb).

This means this lock is useless and be safely dropped.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
index 76d3c40735b0..454fd1104c6d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
@@ -312,9 +312,7 @@ static void gmc_v12_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
 		return;
 	}
 
-	mutex_lock(&adev->mman.gtt_window_lock);
 	gmc_v12_0_flush_vm_hub(adev, vmid, vmhub, 0);
-	mutex_unlock(&adev->mman.gtt_window_lock);
 	return;
 }
 
-- 
2.43.0

Re: [PATCH v1 2/2] drm/amdgpu: remove gart_window_lock usage from gmc v12

[Christian König]

On 16.09.25 09:08, Pierre-Eric Pelloux-Prayer wrote:
> This lock was part of the SDMA workaround originally implemented in
> gmc_v10_0_flush_gpu_tlb (a70cb2176f7ef6f moved it to
> amdgpu_gmc_flush_gpu_tlb).
> 
> This means this lock is useless and be safely dropped.
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
> index 76d3c40735b0..454fd1104c6d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
> @@ -312,9 +312,7 @@ static void gmc_v12_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>  		return;
>  	}
>  
> -	mutex_lock(&adev->mman.gtt_window_lock);
>  	gmc_v12_0_flush_vm_hub(adev, vmid, vmhub, 0);
> -	mutex_unlock(&adev->mman.gtt_window_lock);
>  	return;
>  }
>  

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

[Christian König]

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

But I think for this case here it is also not a must have to have that.

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

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

[Pierre-Eric Pelloux-Prayer]

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


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

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

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

[Christian König]

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

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

[Pierre-Eric Pelloux-Prayer]

Le 16/09/2025 à 12:52, Christian König a écrit :
> 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?

clang supports it:

https://clang.llvm.org/docs/AttributeReference.html#id10

And both syntaxes are already used in the drm subtree by i915.

Pierre-Eric

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

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

[Christian König]

On 16.09.25 13:58, Pierre-Eric Pelloux-Prayer wrote:
> 
> 
> Le 16/09/2025 à 12:52, Christian König a écrit :
>> 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?
> 
> clang supports it:
> 
> https://clang.llvm.org/docs/AttributeReference.html#id10
> 
> And both syntaxes are already used in the drm subtree by i915.

Ok in that case Reviewed-by: Christian König <christian.koenig@amd.com>.

Regards,
Christian.

> 
> Pierre-Eric
> 
>>
>>>
>>>
>>>>
>>>> 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);
>>