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

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

[Maarten Lankhorst]

Op 13-01-15 om 10:36 schreef 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
>
>
Looks more sane, but where do you need this?

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

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

[Zhou, Jammy]

> but where do you need this?
With the new upcoming amdgpu driver, we would like to use 'timeout==0' to check if the fences are signaled or not (without waiting). 

Besides, the jiffies accuracy is not enough for UMDs if the specified timeout is nsecs or usecs, and then nsecs_to_jiffies/usecs_to_jiffies may return zero. So the 'timeout==0' case is easy to be hit for reservation_object_wait_timeout_rcu.

Regards,
Jammy

-----Original Message-----
From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com] 
Sent: Tuesday, January 13, 2015 5:04 PM
To: Zhou, Jammy; dri-devel@lists.freedesktop.org
Cc: Deucher, Alexander; Koenig, Christian
Subject: Re: [PATCH] reservation: wait only with non-zero timeout specified (v3)

Op 13-01-15 om 10:36 schreef 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
>
>
Looks more sane, but where do you need this?

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

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

[Christian König]

Am 13.01.2015 um 10:04 schrieb Maarten Lankhorst:
> Op 13-01-15 om 10:36 schreef 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
>>
>>
> Looks more sane, but where do you need this?

The rational is that we want a single function to call from the driver 
no matter what timeout we have and get a zero if the call timed out and 
a non zero value otherwise.

When we called reservation_object_wait_timeout_rcu with a timeout of 
zero we got a return value of zero as well independent of whether or not 
the fences were signaled. That behavior looked inconsistent and Jammy is 
trying to fix this with this patch.

We could of course do the check in the calling code as well, but to me 
it rather looked like the calling convention of 
reservation_object_wait_timeout_rcu should be thought over one more time.

Either calling it with timeout=0 is invalid and the driver needs to call 
reservation_object_test_signaled_rcu directly instead or we apply this 
patch or something similar to get an useful behavior in the case of 
timeout=0.

Regards,
Christian.

>
> ~Maarten

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

[PATCH] reservation: wait only with non-zero timeout specified (v3)

[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

v3: switch to reservation_object_test_signaled_rcu

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 3c97c8f..807ef15 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -327,6 +327,9 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
 	unsigned seq, shared_count, i = 0;
 	long ret = timeout;
 
+	if (!timeout)
+		return reservation_object_test_signaled_rcu(obj, wait_all);
+
 retry:
 	fence = NULL;
 	shared_count = 0;
-- 
1.9.1

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

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

[Maarten Lankhorst]

Op 13-01-15 om 10:22 schreef Christian König:
> Am 13.01.2015 um 10:04 schrieb Maarten Lankhorst:
>> Op 13-01-15 om 10:36 schreef 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
>>>
>>>
>> Looks more sane, but where do you need this?
>
> The rational is that we want a single function to call from the driver no matter what timeout we have and get a zero if the call timed out and a non zero value otherwise.
>
> When we called reservation_object_wait_timeout_rcu with a timeout of zero we got a return value of zero as well independent of whether or not the fences were signaled. That behavior looked inconsistent and Jammy is trying to fix this with this patch.
>
> We could of course do the check in the calling code as well, but to me it rather looked like the calling convention of reservation_object_wait_timeout_rcu should be thought over one more time.
>
> Either calling it with timeout=0 is invalid and the driver needs to call reservation_object_test_signaled_rcu directly instead or we apply this patch or something similar to get an useful behavior in the case of timeout=0.
I think it would be best to leave timeout=0 returning 0. Not handling it differently gives the same semantics as used by fence_wait_time and wait_event_timeout.
Are there really many cases in which you don't know if timeout = 0 before or not?

~Maarten

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

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

[Daniel Vetter]

On Tue, Jan 13, 2015 at 11:18:19AM +0100, Maarten Lankhorst wrote:
> Op 13-01-15 om 10:22 schreef Christian König:
> > Am 13.01.2015 um 10:04 schrieb Maarten Lankhorst:
> >> Op 13-01-15 om 10:36 schreef 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
> >>>
> >>>
> >> Looks more sane, but where do you need this?
> >
> > The rational is that we want a single function to call from the driver no matter what timeout we have and get a zero if the call timed out and a non zero value otherwise.
> >
> > When we called reservation_object_wait_timeout_rcu with a timeout of zero we got a return value of zero as well independent of whether or not the fences were signaled. That behavior looked inconsistent and Jammy is trying to fix this with this patch.
> >
> > We could of course do the check in the calling code as well, but to me it rather looked like the calling convention of reservation_object_wait_timeout_rcu should be thought over one more time.
> >
> > Either calling it with timeout=0 is invalid and the driver needs to call reservation_object_test_signaled_rcu directly instead or we apply this patch or something similar to get an useful behavior in the case of timeout=0.
> I think it would be best to leave timeout=0 returning 0. Not handling it differently gives the same semantics as used by fence_wait_time and wait_event_timeout.
> Are there really many cases in which you don't know if timeout = 0 before or not?

Yeah I think with this it's more important to be consistent with all the
other wait_something primitives the kernel exposes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Zhou, Jammy]

>> I think it would be best to leave timeout=0 returning 0. Not handling it differently gives the same semantics as used by fence_wait_time and wait_event_timeout.
>> Are there really many cases in which you don't know if timeout = 0 before or not?

>Yeah I think with this it's more important to be consistent with all the other wait_something primitives the kernel exposes.

Okay. I think we can live with that from driver perspective by handling timeout==0 and timeout>0 differently. 
But it should still be worth adding some notes for reservation_object_wait_timeout_rcu that  the return value cannot be used to judge if the fences are signaled or not when timeout==0.

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

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

[Maarten Lankhorst]

Hey,

Op 14-01-15 om 03:16 schreef Zhou, Jammy:
>>> I think it would be best to leave timeout=0 returning 0. Not handling it differently gives the same semantics as used by fence_wait_time and wait_event_timeout.
>>> Are there really many cases in which you don't know if timeout = 0 before or not?
>> Yeah I think with this it's more important to be consistent with all the other wait_something primitives the kernel exposes.
> Okay. I think we can live with that from driver perspective by handling timeout==0 and timeout>0 differently. 
> But it should still be worth adding some notes for reservation_object_wait_timeout_rcu that  the return value cannot be used to judge if the fences are signaled or not when timeout==0.
>
Oops it looks like I was wrong here..

Looking more closely at wait_event_timeout, ___wait_cond_timeout modifies __ret which makes it explicitly handle timeout = 0 by testing.

If you resend your patch I will ack it, but can you send a patch for fixing fence_wait_timeout too to clear any possible confusion?

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

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

[Zhou, Jammy]

Sure. I will send the patches out later.

Regards,
Jammy

-----Original Message-----
From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com] 
Sent: Wednesday, January 21, 2015 5:12 PM
To: Zhou, Jammy; Daniel Vetter
Cc: Koenig, Christian; dri-devel; Deucher, Alexander
Subject: Re: [PATCH] reservation: wait only with non-zero timeout specified (v3)

Hey,

Op 14-01-15 om 03:16 schreef Zhou, Jammy:
>>> I think it would be best to leave timeout=0 returning 0. Not handling it differently gives the same semantics as used by fence_wait_time and wait_event_timeout.
>>> Are there really many cases in which you don't know if timeout = 0 before or not?
>> Yeah I think with this it's more important to be consistent with all the other wait_something primitives the kernel exposes.
> Okay. I think we can live with that from driver perspective by handling timeout==0 and timeout>0 differently. 
> But it should still be worth adding some notes for reservation_object_wait_timeout_rcu that  the return value cannot be used to judge if the fences are signaled or not when timeout==0.
>
Oops it looks like I was wrong here..

Looking more closely at wait_event_timeout, ___wait_cond_timeout modifies __ret which makes it explicitly handle timeout = 0 by testing.

If you resend your patch I will ack it, but can you send a patch for fixing fence_wait_timeout too to clear any possible confusion?

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