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,
