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