From: Arnd Bergmann <arnd@arndb.de>
To: Lucas Stach <l.stach@pengutronix.de>
Cc: "y2038 Mailman List" <y2038@lists.linaro.org>,
"David Airlie" <airlied@linux.ie>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Guido Günther" <agx@sigxcpu.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
"The etnaviv authors" <etnaviv@lists.freedesktop.org>,
"Russell King" <linux+etnaviv@armlinux.org.uk>,
"Sam Ravnborg" <sam@ravnborg.org>,
"Christian König" <christian.koenig@amd.com>,
"Emil Velikov" <emil.velikov@collabora.com>
Subject: Re: [PATCH 15/16] drm/etnaviv: use ktime_t for timeouts
Date: Sat, 9 Nov 2019 13:12:07 +0100 [thread overview]
Message-ID: <CAK8P3a13jSRqzZ-aDETdxk-BKgfXaAhdWiSn7aW+u3MFf06fWw@mail.gmail.com> (raw)
In-Reply-To: <3a0cfce79620152facfe31b442a735db1dcda436.camel@pengutronix.de>
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);
next prev parent reply other threads:[~2019-11-09 12:12 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-08 21:32 [PATCH 00/16] drivers: y2038 updates Arnd Bergmann
2019-11-08 21:32 ` [PATCH 01/16] staging: exfat: use prandom_u32() for i_generation Arnd Bergmann
2019-11-08 21:53 ` Valdis Klētnieks
2019-11-08 21:32 ` [PATCH 02/16] fat: " Arnd Bergmann
2019-11-08 21:32 ` [PATCH 03/16] net: sock: use __kernel_old_timespec instead of timespec Arnd Bergmann
2019-11-09 19:09 ` Deepa Dinamani
2019-11-08 21:32 ` [PATCH 04/16] dlm: use SO_SNDTIMEO_NEW instead of SO_SNDTIMEO_OLD Arnd Bergmann
2019-11-09 19:14 ` Deepa Dinamani
2019-11-08 21:32 ` [PATCH 05/16] xtensa: ISS: avoid struct timeval Arnd Bergmann
2019-11-08 21:38 ` Max Filippov
2019-11-08 21:32 ` [PATCH 06/16] um: ubd: use 64-bit time_t where possible Arnd Bergmann
2019-11-08 21:32 ` [PATCH 07/16] acct: stop using get_seconds() Arnd Bergmann
2019-11-08 21:32 ` [PATCH 08/16] tsacct: add 64-bit btime field Arnd Bergmann
2019-11-08 21:32 ` [PATCH 09/16] netfilter: nft_meta: use 64-bit time arithmetic Arnd Bergmann
2019-11-09 11:20 ` Phil Sutter
2019-11-15 22:44 ` Pablo Neira Ayuso
2019-11-08 21:32 ` [PATCH 10/16] packet: clarify timestamp overflow Arnd Bergmann
2019-11-08 21:32 ` [PATCH 11/16] quota: avoid time_t in v1_disk_dqblk definition Arnd Bergmann
2019-11-08 21:32 ` [PATCH 12/16] hostfs: pass 64-bit timestamps to/from user space Arnd Bergmann
2019-11-20 20:30 ` [Y2038] " Ben Hutchings
2019-11-20 20:35 ` Ben Hutchings
2019-11-08 21:32 ` [PATCH 13/16] hfs/hfsplus: use 64-bit inode timestamps Arnd Bergmann
2019-11-13 3:53 ` Ernesto A. Fernández
2019-11-13 5:59 ` Viacheslav Dubeyko
2019-11-13 8:06 ` [Y2038] " Arnd Bergmann
2019-11-13 17:03 ` Viacheslav Dubeyko
2019-11-08 21:32 ` [PATCH 14/16] drm/msm: avoid using 'timespec' Arnd Bergmann
2019-11-12 16:55 ` Jordan Crouse
2019-11-08 21:32 ` [PATCH 15/16] drm/etnaviv: use ktime_t for timeouts Arnd Bergmann
2019-11-08 23:03 ` Lucas Stach
2019-11-09 12:12 ` Arnd Bergmann [this message]
2019-11-11 9:55 ` Lucas Stach
2019-11-11 16:24 ` Arnd Bergmann
2019-11-08 21:32 ` [PATCH 16/16] firewire: ohci: stop using get_seconds() for BUS_TIME Arnd Bergmann
2019-11-13 20:04 ` Stefan Richter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAK8P3a13jSRqzZ-aDETdxk-BKgfXaAhdWiSn7aW+u3MFf06fWw@mail.gmail.com \
--to=arnd@arndb.de \
--cc=agx@sigxcpu.org \
--cc=airlied@linux.ie \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=emil.velikov@collabora.com \
--cc=etnaviv@lists.freedesktop.org \
--cc=l.stach@pengutronix.de \
--cc=linux+etnaviv@armlinux.org.uk \
--cc=linux-kernel@vger.kernel.org \
--cc=sam@ravnborg.org \
--cc=y2038@lists.linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).