Re: [PATCH 3/6] drm/amdgpu: Use drm_timeout_abs_to_jiffies()

Hi Christian,

On 10/08/26 10:34, Christian König wrote:
> On 8/9/26 21:23, Maíra Canal wrote:
>> amdgpu_gem_timeout() converts an absolute deadline in ns into jiffies,
>> which is what drm_timeout_abs_to_jiffies() already does for the other
>> drivers whose wait UAPI takes a deadline. Use the shared helper and keep
>> only the part that is specific to amdgpu.
>>
>> Two details change along this conversion: the helper rounds up rather than
>> truncating, so a deadline less than a tick away now waits for one jiffy
>> instead of returning 0. It also uses nsecs_to_jiffies64(), so the
>> conversion no longer truncates on 32-bit, where a large deadline could
>> previously be reduced to an arbitrary shorter one.
>>
>> Signed-off-by: Maíra Canal <[email protected]>
>>
>> ---
>>
>> As a note, this patch can be merged independently to the AMD tree
>> without any dependencies.
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 16 ++--------------
>>   1 file changed, 2 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 6a0699746fbc..84b509a484b0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -25,7 +25,6 @@
>>    *          Alex Deucher
>>    *          Jerome Glisse
>>    */
>> -#include <linux/ktime.h>
>>   #include <linux/module.h>
>>   #include <linux/overflow.h>
>>   #include <linux/pagemap.h>
>> @@ -40,6 +39,7 @@
>>   #include <drm/drm_gem_ttm_helper.h>
>>   #include <drm/ttm/ttm_tt.h>
>>   #include <drm/drm_syncobj.h>
>> +#include <drm/drm_utils.h>
>>   
>>   #include "amdgpu.h"
>>   #include "amdgpu_display.h"
>> @@ -622,23 +622,11 @@ int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>    */
>>   unsigned long amdgpu_gem_timeout(uint64_t timeout_ns)
> 
> Please completely nuke that function and replace it with calls to drm_timeout_abs_to_jiffies().
> 
>>   {
>> -	unsigned long timeout_jiffies;
>> -	ktime_t timeout;
>> -
>>   	/* clamp timeout if it's to large */
>>   	if (((int64_t)timeout_ns) < 0)
>>   		return MAX_SCHEDULE_TIMEOUT;
> 
> That check was actually never correct at all as far as I can see.
> 

I'm not sure about that... Taking a look at other UAPIs (like Panfrost),
I see that they use a s64 timeout_ns, which matches the ktime_t the
deadline is converted through. Contrary to that, AMD exposes a __u64 in
the UAPI and defines AMDGPU_TIMEOUT_INFINITE = 0xffffffffffffffffull in
libdrm. Therefore the check looks correct to me: it detects the values
above S64_MAX and maps them to an infinite wait.

Having said that, the comment is quite misleading. How about?

       /* Map anything that doesn't fit in a s64 to an infinite wait */

To make it even clearer, we could use `if (timeout_ns > S64_MAX)`.

> We need to make sure that when timeout_ns is larger than represent-able in long jiffies (especially on 32bit systems) then MAX_SCHEDULE_TIMEOUT is returned by drm_timeout_abs_to_jiffies().
> 

Actually, what drm_timeout_abs_to_jiffies() does is clamp the
nsecs_to_jiffies64() return to MAX_SCHEDULE_TIMEOUT - 1, so that a
finite deadline is never turned into an infinite wait. So it is safe on
32-bit, but it's not doing exactly what you described.

Best regards,
- Maíra

> And I hope that drm_timeout_abs_to_jiffies() does that correctly already, if not this seriously needs fixing anyway.
> 
> Regards,
> Christian.
> 
>>   
>> -	timeout = ktime_sub(ns_to_ktime(timeout_ns), ktime_get());
>> -	if (ktime_to_ns(timeout) < 0)
>> -		return 0;
>> -
>> -	timeout_jiffies = nsecs_to_jiffies(ktime_to_ns(timeout));
>> -	/*  clamp timeout to avoid unsigned-> signed overflow */
>> -	if (timeout_jiffies > MAX_SCHEDULE_TIMEOUT)
>> -		return MAX_SCHEDULE_TIMEOUT - 1;
>> -
>> -	return timeout_jiffies;
>> +	return drm_timeout_abs_to_jiffies(timeout_ns);
>>   }
>>   
>>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>>
>