| Message ID | 1421128234-11968-1-git-send-email-Jammy.Zhou@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey, Can't you simply add if (!timeout) return !reservation_object_test_signaled_rcu(obj, wait_all); to the beginning instead? Waiting with timeout = 0 is not really defined. Look at fence_default_wait for example. It returns timeout if the fence is signaled, but since this is 0 you can't distinguish between timed out wait and succesful wait. Also why do you need this? Why not simply return 0 with timeout = 0. ~Maarten On 13-01-15 06:50, Jammy Zhou wrote: > When the timeout value passed to reservation_object_wait_timeout_rcu > is zero, no wait should be done if the fences are not signaled. > > Return '1' for idle and '0' for busy if the specified timeout is '0' > to keep consistent with the case of non-zero timeout. > > v2: call fence_put if not signaled in the case of timeout==0 > > Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com> > Reviewed-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/dma-buf/reservation.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 3c97c8f..b1d554f 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -380,12 +380,19 @@ retry: > } > > rcu_read_unlock(); > - if (fence) { > + if (fence && timeout) { > ret = fence_wait_timeout(fence, intr, ret); > fence_put(fence); > if (ret > 0 && wait_all && (i + 1 < shared_count)) > goto retry; > } > + > + if (fence && !timeout) > + fence_put(fence); > + > + if (!fence && !timeout) > + ret = 1; > + > return ret; > > unlock_retry: >
Hi Maarten, > Can't you simply add if (!timeout) return !reservation_object_test_signaled_rcu(obj, wait_all); to the beginning instead? Hmm, after looking into it, I think that can achieve the same purpose. I will update the patch with this. > Also why do you need this? Why not simply return 0 with timeout = 0. The major purpose here is to use reservation_object_wait_timeout_rcu to handle all possible timeout values (and just to check status with timeout==0). If we simply return 0, we cannot determine the fence is signaled or not with the return value. Regards, Jammy -----Original Message----- From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com] Sent: Tuesday, January 13, 2015 2:50 PM To: Zhou, Jammy Cc: dri-devel@lists.freedesktop.org; Christian König; Deucher, Alexander Subject: Re: [PATCH 1/1] reservation: wait only with non-zero timeout specified (v2) Hey, Can't you simply add if (!timeout) return !reservation_object_test_signaled_rcu(obj, wait_all); to the beginning instead? Waiting with timeout = 0 is not really defined. Look at fence_default_wait for example. It returns timeout if the fence is signaled, but since this is 0 you can't distinguish between timed out wait and succesful wait. Also why do you need this? Why not simply return 0 with timeout = 0. ~Maarten On 13-01-15 06:50, Jammy Zhou wrote: > When the timeout value passed to reservation_object_wait_timeout_rcu > is zero, no wait should be done if the fences are not signaled. > > Return '1' for idle and '0' for busy if the specified timeout is '0' > to keep consistent with the case of non-zero timeout. > > v2: call fence_put if not signaled in the case of timeout==0 > > Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com> > Reviewed-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/dma-buf/reservation.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/reservation.c > b/drivers/dma-buf/reservation.c index 3c97c8f..b1d554f 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -380,12 +380,19 @@ retry: > } > > rcu_read_unlock(); > - if (fence) { > + if (fence && timeout) { > ret = fence_wait_timeout(fence, intr, ret); > fence_put(fence); > if (ret > 0 && wait_all && (i + 1 < shared_count)) > goto retry; > } > + > + if (fence && !timeout) > + fence_put(fence); > + > + if (!fence && !timeout) > + ret = 1; > + > return ret; > > unlock_retry: >
Op 13-01-15 om 08:59 schreef Zhou, Jammy: > Hi Maarten, > >> Can't you simply add if (!timeout) return !reservation_object_test_signaled_rcu(obj, wait_all); to the beginning instead? > Hmm, after looking into it, I think that can achieve the same purpose. I will update the patch with this. > >> Also why do you need this? Why not simply return 0 with timeout = 0. > The major purpose here is to use reservation_object_wait_timeout_rcu to handle all possible timeout values (and just to check status with timeout==0). If we simply return 0, we cannot determine the fence is signaled or not with the return value. You can't anyway when calling with timeout = 0. * fence_wait_timeout - sleep until the fence gets signaled * * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the * remaining timeout in jiffies on success. Other error values may be * returned on custom implementations. Since you have 0 returning jiffies, you would get 0 regardless of fence_wait being succesful or not. I think the only right way to handle this is by returning 0 immediately if timeout is 0. Why do you need it to return 1? Why not use reservation_object_test_signaled_rcu directly? ~Maarten
> You can't anyway when calling with timeout = 0. For consistency, we can always return 0 for busy (not signaled), and return '>0' for idle (signaled). This rule should be common for reservation_object_wait_timeout_rcu. So in my previous patch, '1' is returned for signaled case when specified timeout is zero. > Since you have 0 returning jiffies, you would get 0 regardless of fence_wait being succesful or not. IMHO, fence_wait_timeout shouldn't be called per se when timeout==0. > Why do you need it to return 1? Why not use reservation_object_test_signaled_rcu directly? Just as I mentioned above, return 1 is to make the returning values of reservation_object_wait_timeout_rcu consistent for both cases. And we can call reservation_object_test_signaled_rcu directly in driver side, but it will be better if we can also add 'timeout==0' support in reservation_object_wait_timeout_rcu (sometimes it isn't preventable). Regards, Jammy -----Original Message----- From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com] Sent: Tuesday, January 13, 2015 4:05 PM To: Zhou, Jammy Cc: dri-devel@lists.freedesktop.org; Christian König; Deucher, Alexander Subject: Re: [PATCH 1/1] reservation: wait only with non-zero timeout specified (v2) Op 13-01-15 om 08:59 schreef Zhou, Jammy: > Hi Maarten, > >> Can't you simply add if (!timeout) return !reservation_object_test_signaled_rcu(obj, wait_all); to the beginning instead? > Hmm, after looking into it, I think that can achieve the same purpose. I will update the patch with this. > >> Also why do you need this? Why not simply return 0 with timeout = 0. > The major purpose here is to use reservation_object_wait_timeout_rcu to handle all possible timeout values (and just to check status with timeout==0). If we simply return 0, we cannot determine the fence is signaled or not with the return value. You can't anyway when calling with timeout = 0. * fence_wait_timeout - sleep until the fence gets signaled * * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the * remaining timeout in jiffies on success. Other error values may be * returned on custom implementations. Since you have 0 returning jiffies, you would get 0 regardless of fence_wait being succesful or not. I think the only right way to handle this is by returning 0 immediately if timeout is 0. Why do you need it to return 1? Why not use reservation_object_test_signaled_rcu directly? ~Maarten
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 3c97c8f..b1d554f 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -380,12 +380,19 @@ retry: } rcu_read_unlock(); - if (fence) { + if (fence && timeout) { ret = fence_wait_timeout(fence, intr, ret); fence_put(fence); if (ret > 0 && wait_all && (i + 1 < shared_count)) goto retry; } + + if (fence && !timeout) + fence_put(fence); + + if (!fence && !timeout) + ret = 1; + return ret; unlock_retry: