[PATCH 1/1] reservation: wait only with non-zero timeout specified (v2)

[PATCH 1/1] reservation: wait only with non-zero timeout specified (v2)

[Jammy Zhou]

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:
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] reservation: wait only with non-zero timeout specified (v2)

[Maarten Lankhorst]

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:
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/1] reservation: wait only with non-zero timeout specified (v2)

[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.

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:
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] reservation: wait only with non-zero timeout specified (v2)

[Maarten Lankhorst]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/1] reservation: wait only with non-zero timeout specified (v2)

[Zhou, Jammy]

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel