On Sat, Nov 9, 2019 at 12:03 AM Lucas Stach l.stach@pengutronix.de wrote:
Am Freitag, den 08.11.2019, 22:32 +0100 schrieb Arnd Bergmann:
struct timespec is being removed from the kernel because it often leads to code that is not y2038-safe.
In the etnaviv driver, monotonic timestamps are used, which do not suffer from overflow, but using ktime_t still leads to better code overall.
The conversion is straightforward for the most part, except for etnaviv_timeout_to_jiffies(), which needs to handle arguments larger than MAX_JIFFY_OFFSET on 32-bit architectures.
Signed-off-by: Arnd Bergmann arnd@arndb.de
@@ -368,7 +366,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, return -ENXIO;
if (args->flags & ETNA_WAIT_NONBLOCK)
timeout = NULL;
timeout = ktime_set(0, 0);This is a change in behavior, as far as I can see. After this change the called internal function is not able to differentiate between a NONBLOCK call and a blocking call with 0 timeout. The difference being that on a busy object the NONBLOCK call will return -EBUSY, while a blocking call will return -ETIMEDOUT.
Ah, good point. I created this patch a long time ago (cherry-picked it out of an older branch I had done), so I don't remember how I concluded this was a safe conversion, of if I missed that difference originally.
But then CLOCK_MONOTONIC starts at 0 and should not never wrap, right?
Yes, that is correct.
If that's the case then we should never encounter a genuine 0 timeout and this change would be okay.
That's quite likely, I'd say any program passing {0,0} as a timeout without ETNA_WAIT_NONBLOCK is already broken, but if we leave it like that, it would be best to describe the reasoning in the changelog.
Should I change the changelog, or change the patch to restore the current behavior instead?
I guess I could fold the change below into my patch to make it transparent to the application again.
Arnd
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 1250c5e06329..162cedfb7f72 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -354,6 +354,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, ktime_t timeout = ktime_set(args->timeout.tv_sec, args->timeout.tv_nsec); struct etnaviv_gpu *gpu; + int ret;
if (args->flags & ~(ETNA_WAIT_NONBLOCK)) return -EINVAL; @@ -365,8 +366,12 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, if (!gpu) return -ENXIO;
- if (args->flags & ETNA_WAIT_NONBLOCK) - timeout = ktime_set(0, 0); + if (args->flags & ETNA_WAIT_NONBLOCK) { + ret = etnaviv_gpu_wait_fence_interruptible(gpu, args->fence, + ktime_set(0, 0)); + + return (ret == -ETIMEDOUT) ? -EBUSY : ret; + }
return etnaviv_gpu_wait_fence_interruptible(gpu, args->fence, timeout); @@ -421,10 +426,13 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, if (!obj) return -ENOENT;
- if (args->flags & ETNA_WAIT_NONBLOCK) - timeout = ktime_set(0, 0); - - ret = etnaviv_gem_wait_bo(gpu, obj, timeout); + if (args->flags & ETNA_WAIT_NONBLOCK) { + ret = etnaviv_gem_wait_bo(gpu, obj, ktime_set(0, 0)); + if (ret == -ETIMEDOUT) + ret = -EBUSY; + } else { + ret = etnaviv_gem_wait_bo(gpu, obj, timeout); + }
drm_gem_object_put_unlocked(obj);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index e42b1c4d902c..fa6986c5a5fe 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1135,6 +1135,7 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, u32 id, ktime_t timeout) { struct dma_fence *fence; + unsigned long remaining; int ret;
/* @@ -1151,12 +1152,12 @@ int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu, if (!fence) return 0;
- if (!timeout) { - /* No timeout was requested: just test for completion */ - ret = dma_fence_is_signaled(fence) ? 0 : -EBUSY; + if (!timeout || + (remaining = etnaviv_timeout_to_jiffies(timeout)) == 0) { + /* No timeout was requested, or timeout is already expired, + * just test for completion */ + ret = dma_fence_is_signaled(fence) ? 0 : -ETIMEDOUT; } else { - unsigned long remaining = etnaviv_timeout_to_jiffies(timeout); - ret = dma_fence_wait_timeout(fence, true, remaining); if (ret == 0) ret = -ETIMEDOUT; @@ -1185,7 +1186,7 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu, long ret;
if (!timeout) - return !is_active(etnaviv_obj) ? 0 : -EBUSY; + return !is_active(etnaviv_obj) ? 0 : -ETIMEDOUT;
remaining = etnaviv_timeout_to_jiffies(timeout);