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

On 8/10/26 17:32, Maíra Canal wrote:
> 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)`.

Yeah that is pretty much what I had in mind as well. That would be much better already.

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

Mhm, I'm not sure if that behavior is a good idea.

IIRC MAX_SCHEDULE_TIMEOUT-1 is only ~50 days on 32bit systems, but that is way lower than the potentially requested time here. I mean we basically changing a wait of more than 250 years into a 50 days wait...

Most likely not relevant in practice, but I would still say that returning an infinity wait for timeout_ns > S64_MAX is more defensive.

Regards,
Christian.

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