[PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

If the fences in the fence_array signal on the fence_array does not have
signalling enabled num_pending will not be updated accordingly.

So when signaling is disabled check the signal of every fence with
fence_is_signaled() and then compare with num_pending to learn if the
fence_array was signalled or not.

If we want to keep the poll_does_not_wait optimization I think we need
something like this. It keeps the same behaviour if signalling is enabled
but tries to calculated the state otherwise.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/fence-array.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
index f1989fc..1eec271 100644
--- a/drivers/dma-buf/fence-array.c
+++ b/drivers/dma-buf/fence-array.c
@@ -75,8 +75,25 @@ static bool fence_array_enable_signaling(struct fence *fence)
 static bool fence_array_signaled(struct fence *fence)
 {
 	struct fence_array *array = to_fence_array(fence);
+	int i, num_pending;
+
+	num_pending = atomic_read(&array->num_pending);
+
+	/*
+	 * Before signaling is enabled, num_pending is static (set during array
+	 * construction as a count of all fences or set to 1 if signal_on_any
+	 * flag is passed. To ensure forward progress, i.e. a while
+	 * (!fence_is_signaled()) ; busy-loop eventually proceeds, we need to
+	 * check the current status of our fences.
+	 */
+	if (!test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {
+		for (i = 0 ; i < array->num_fences; ++i) {
+			if (fence_is_signaled(array->fences[i]))
+				num_pending--;
+		}
+	}
 
-	return atomic_read(&array->num_pending) <= 0;
+	return num_pending <= 0;
 }
 
 static void fence_array_release(struct fence *fence)
-- 
2.5.5

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Christian König]

Am 21.09.2016 um 13:36 schrieb Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> If the fences in the fence_array signal on the fence_array does not have
> signalling enabled num_pending will not be updated accordingly.
>
> So when signaling is disabled check the signal of every fence with
> fence_is_signaled() and then compare with num_pending to learn if the
> fence_array was signalled or not.
>
> If we want to keep the poll_does_not_wait optimization I think we need
> something like this. It keeps the same behaviour if signalling is enabled
> but tries to calculated the state otherwise.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

First of all the patch is horrible wrong because fence_array_signaled() 
is called without any locks held. So you can run into a race condition 
between checking the fences here and enable signaling.

Additional to that I'm not sure if that is such a good idea or not, 
cause fence_array_signaled() should be light weight and without calling 
enable_signaling there is not guarantee that fences will ever signal.

Regards,
Christian.

> ---
>   drivers/dma-buf/fence-array.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index f1989fc..1eec271 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -75,8 +75,25 @@ static bool fence_array_enable_signaling(struct fence *fence)
>   static bool fence_array_signaled(struct fence *fence)
>   {
>   	struct fence_array *array = to_fence_array(fence);
> +	int i, num_pending;
> +
> +	num_pending = atomic_read(&array->num_pending);
> +
> +	/*
> +	 * Before signaling is enabled, num_pending is static (set during array
> +	 * construction as a count of all fences or set to 1 if signal_on_any
> +	 * flag is passed. To ensure forward progress, i.e. a while
> +	 * (!fence_is_signaled()) ; busy-loop eventually proceeds, we need to
> +	 * check the current status of our fences.
> +	 */
> +	if (!test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {
> +		for (i = 0 ; i < array->num_fences; ++i) {
> +			if (fence_is_signaled(array->fences[i]))
> +				num_pending--;
> +		}
> +	}
>   
> -	return atomic_read(&array->num_pending) <= 0;
> +	return num_pending <= 0;
>   }
>   
>   static void fence_array_release(struct fence *fence)


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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Gustavo Padovan]

Hi Christian,

2016-09-21 Christian König <christian.koenig@amd.com>:

> Am 21.09.2016 um 13:36 schrieb Gustavo Padovan:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > If the fences in the fence_array signal on the fence_array does not have
> > signalling enabled num_pending will not be updated accordingly.
> > 
> > So when signaling is disabled check the signal of every fence with
> > fence_is_signaled() and then compare with num_pending to learn if the
> > fence_array was signalled or not.
> > 
> > If we want to keep the poll_does_not_wait optimization I think we need
> > something like this. It keeps the same behaviour if signalling is enabled
> > but tries to calculated the state otherwise.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> First of all the patch is horrible wrong because fence_array_signaled() is
> called without any locks held. So you can run into a race condition between
> checking the fences here and enable signaling.

Yes. it can, but I don't think the race condition is actually a problem.
Maybe you have some use case that we are not seeing?
 
> Additional to that I'm not sure if that is such a good idea or not, cause
> fence_array_signaled() should be light weight and without calling
> enable_signaling there is not guarantee that fences will ever signal.

It is still lightweight for the case when signaling is enabled and
fences can signal with signaling enabled or disabled so I don't see that
as problem too.

Gustavo

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Christian König]

Am 22.09.2016 um 09:44 schrieb Gustavo Padovan:
> Hi Christian,
>
> 2016-09-21 Christian König <christian.koenig@amd.com>:
>
>> Am 21.09.2016 um 13:36 schrieb Gustavo Padovan:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> If the fences in the fence_array signal on the fence_array does not have
>>> signalling enabled num_pending will not be updated accordingly.
>>>
>>> So when signaling is disabled check the signal of every fence with
>>> fence_is_signaled() and then compare with num_pending to learn if the
>>> fence_array was signalled or not.
>>>
>>> If we want to keep the poll_does_not_wait optimization I think we need
>>> something like this. It keeps the same behaviour if signalling is enabled
>>> but tries to calculated the state otherwise.
>>>
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> First of all the patch is horrible wrong because fence_array_signaled() is
>> called without any locks held. So you can run into a race condition between
>> checking the fences here and enable signaling.
> Yes. it can, but I don't think the race condition is actually a problem.
> Maybe you have some use case that we are not seeing?

I'm not sure if that can really race, but if it does the check would 
return true while not all necessary fences are signaled yet.

That would be really really bad for things like TTM where we just do a 
quick check in the page fault handler for example.

I need to double check if that really could be a problem.

>> Additional to that I'm not sure if that is such a good idea or not, cause
>> fence_array_signaled() should be light weight and without calling
>> enable_signaling there is not guarantee that fences will ever signal.
> It is still lightweight for the case when signaling is enabled and
> fences can signal with signaling enabled or disabled
Nope that's not correct. The signaled callback is only optional.

See the comment on the fence_is_signaled function:
>  * Returns true if the fence was already signaled, false if not. Since 
> this
>  * function doesn't enable signaling, it is not guaranteed to ever return
>  * true if fence_add_callback, fence_wait or fence_enable_sw_signaling
>  * haven't been called before.

E.g. for example it is illegal to do something like 
"while(!fence_is_signaled(f)) sleep();" without enabling signaling 
before doing this.

Could just be a misunderstanding, but the comments on your patch 
actually sounds a bit like somebody is trying to do exactly that.

Regards,
Christian.

>   so I don't see that
> as problem too.
>
> Gustavo
>

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Gustavo Padovan]

2016-09-22 Christian König <christian.koenig@amd.com>:

> Am 22.09.2016 um 09:44 schrieb Gustavo Padovan:
> > Hi Christian,
> > 
> > 2016-09-21 Christian König <christian.koenig@amd.com>:
> > 
> > > Am 21.09.2016 um 13:36 schrieb Gustavo Padovan:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > If the fences in the fence_array signal on the fence_array does not have
> > > > signalling enabled num_pending will not be updated accordingly.
> > > > 
> > > > So when signaling is disabled check the signal of every fence with
> > > > fence_is_signaled() and then compare with num_pending to learn if the
> > > > fence_array was signalled or not.
> > > > 
> > > > If we want to keep the poll_does_not_wait optimization I think we need
> > > > something like this. It keeps the same behaviour if signalling is enabled
> > > > but tries to calculated the state otherwise.
> > > > 
> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > First of all the patch is horrible wrong because fence_array_signaled() is
> > > called without any locks held. So you can run into a race condition between
> > > checking the fences here and enable signaling.
> > Yes. it can, but I don't think the race condition is actually a problem.
> > Maybe you have some use case that we are not seeing?
> 
> I'm not sure if that can really race, but if it does the check would return
> true while not all necessary fences are signaled yet.

How? If signaling is disabled num_pending is equal to the number of
fences (or 1 if signal_on_any) then we just check all fences. If all of
them are signaled then num_pending will be zero.

> 
> That would be really really bad for things like TTM where we just do a quick
> check in the page fault handler for example.
> 
> I need to double check if that really could be a problem.
> 
> > > Additional to that I'm not sure if that is such a good idea or not, cause
> > > fence_array_signaled() should be light weight and without calling
> > > enable_signaling there is not guarantee that fences will ever signal.
> > It is still lightweight for the case when signaling is enabled and
> > fences can signal with signaling enabled or disabled
> Nope that's not correct. The signaled callback is only optional.
> 
> See the comment on the fence_is_signaled function:
> >  * Returns true if the fence was already signaled, false if not. Since
> > this
> >  * function doesn't enable signaling, it is not guaranteed to ever return
> >  * true if fence_add_callback, fence_wait or fence_enable_sw_signaling
> >  * haven't been called before.

Right, I was with explicit fencing in mind, we only enable signaling
there if userspace polls.

> E.g. for example it is illegal to do something like
> "while(!fence_is_signaled(f)) sleep();" without enabling signaling before
> doing this.
> 
> Could just be a misunderstanding, but the comments on your patch actually
> sounds a bit like somebody is trying to do exactly that.

I think the usecase in mind here is poll(fd, timeout=0)

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 859 bytes --]

Dropping the rest of the patch, cause that really doesn't make sense any 
more.

Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
>> E.g. for example it is illegal to do something like
>> >"while(!fence_is_signaled(f)) sleep();" without enabling signaling before
>> >doing this.
>> >
>> >Could just be a misunderstanding, but the comments on your patch actually
>> >sounds a bit like somebody is trying to do exactly that.
> I think the usecase in mind here is poll(fd, timeout=0)

Exactly as I feared. Even if userspace polls with timeout=0 you still 
need to call enable_signaling().

Otherwise you can run into a situation where userspace only uses 
timeout=0 and so never activates the signaling check in the driver.

This would in turn result in an endless loop on implementations where 
the driver never signals fences on their own.

Regards,
Christian.

[-- Attachment #1.2: Type: text/html, Size: 1588 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Gustavo Padovan]

2016-09-22 Christian König <christian.koenig@amd.com>:

> Dropping the rest of the patch, cause that really doesn't make sense any
> more.
> 
> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
> > > E.g. for example it is illegal to do something like
> > > >"while(!fence_is_signaled(f)) sleep();" without enabling signaling before
> > > >doing this.
> > > >
> > > >Could just be a misunderstanding, but the comments on your patch actually
> > > >sounds a bit like somebody is trying to do exactly that.
> > I think the usecase in mind here is poll(fd, timeout=0)
> 
> Exactly as I feared. Even if userspace polls with timeout=0 you still need
> to call enable_signaling().
> 
> Otherwise you can run into a situation where userspace only uses timeout=0
> and so never activates the signaling check in the driver.
> 
> This would in turn result in an endless loop on implementations where the
> driver never signals fences on their own.

Polling is optional, userspace may never call it. And DRM/KMS or GPU            
drivers will be doing fence_wait() themselves so signaling is enabled at        
some point. 

Gustavo

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Christian König]

Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
> 2016-09-22 Christian König <christian.koenig@amd.com>:
>
>> Dropping the rest of the patch, cause that really doesn't make sense any
>> more.
>>
>> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
>>>> E.g. for example it is illegal to do something like
>>>>> "while(!fence_is_signaled(f)) sleep();" without enabling signaling before
>>>>> doing this.
>>>>>
>>>>> Could just be a misunderstanding, but the comments on your patch actually
>>>>> sounds a bit like somebody is trying to do exactly that.
>>> I think the usecase in mind here is poll(fd, timeout=0)
>> Exactly as I feared. Even if userspace polls with timeout=0 you still need
>> to call enable_signaling().
>>
>> Otherwise you can run into a situation where userspace only uses timeout=0
>> and so never activates the signaling check in the driver.
>>
>> This would in turn result in an endless loop on implementations where the
>> driver never signals fences on their own.
> Polling is optional, userspace may never call it. And DRM/KMS or GPU
> drivers will be doing fence_wait() themselves so signaling is enabled at
> some point.

No they won't. We have an use case where we clearly want to avoid that 
as much as possible because and so the driver never calls 
enable_signaling() on it's own.

Exposing this poll function to userspace without enabling signaling is a 
clear NAK from my side.

Christian.

>
> Gustavo
>

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Christian König]

Am 22.09.2016 um 16:11 schrieb Christian König:
> Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
>> 2016-09-22 Christian König <christian.koenig@amd.com>:
>>
>>> Dropping the rest of the patch, cause that really doesn't make sense 
>>> any
>>> more.
>>>
>>> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
>>>>> E.g. for example it is illegal to do something like
>>>>>> "while(!fence_is_signaled(f)) sleep();" without enabling 
>>>>>> signaling before
>>>>>> doing this.
>>>>>>
>>>>>> Could just be a misunderstanding, but the comments on your patch 
>>>>>> actually
>>>>>> sounds a bit like somebody is trying to do exactly that.
>>>> I think the usecase in mind here is poll(fd, timeout=0)
>>> Exactly as I feared. Even if userspace polls with timeout=0 you 
>>> still need
>>> to call enable_signaling().
>>>
>>> Otherwise you can run into a situation where userspace only uses 
>>> timeout=0
>>> and so never activates the signaling check in the driver.
>>>
>>> This would in turn result in an endless loop on implementations 
>>> where the
>>> driver never signals fences on their own.
>> Polling is optional, userspace may never call it. And DRM/KMS or GPU
>> drivers will be doing fence_wait() themselves so signaling is enabled at
>> some point.
>
> No they won't. We have an use case where we clearly want to avoid that 
> as much as possible because and so the driver never calls 
> enable_signaling() on it's own.

Sorry hitting send to soon. That should read "because of the extreme 
overhead".

Christian.

>
> Exposing this poll function to userspace without enabling signaling is 
> a clear NAK from my side.
>
> Christian.
>
>>
>> Gustavo
>>
>

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Gustavo Padovan]

2016-09-22 Christian König <christian.koenig@amd.com>:

> Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
> > 2016-09-22 Christian König <christian.koenig@amd.com>:
> > 
> > > Dropping the rest of the patch, cause that really doesn't make sense any
> > > more.
> > > 
> > > Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
> > > > > E.g. for example it is illegal to do something like
> > > > > > "while(!fence_is_signaled(f)) sleep();" without enabling signaling before
> > > > > > doing this.
> > > > > > 
> > > > > > Could just be a misunderstanding, but the comments on your patch actually
> > > > > > sounds a bit like somebody is trying to do exactly that.
> > > > I think the usecase in mind here is poll(fd, timeout=0)
> > > Exactly as I feared. Even if userspace polls with timeout=0 you still need
> > > to call enable_signaling().
> > > 
> > > Otherwise you can run into a situation where userspace only uses timeout=0
> > > and so never activates the signaling check in the driver.
> > > 
> > > This would in turn result in an endless loop on implementations where the
> > > driver never signals fences on their own.
> > Polling is optional, userspace may never call it. And DRM/KMS or GPU
> > drivers will be doing fence_wait() themselves so signaling is enabled at
> > some point.
> 
> No they won't. We have an use case where we clearly want to avoid that as
> much as possible because and so the driver never calls enable_signaling() on
> it's own.
> 
> Exposing this poll function to userspace without enabling signaling is a
> clear NAK from my side.

Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert
that one then? It is already broken.

Gustavo

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Christian König]

Am 23.09.2016 um 13:30 schrieb Gustavo Padovan:
> 2016-09-22 Christian König <christian.koenig@amd.com>:
>
>> Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
>>> 2016-09-22 Christian König <christian.koenig@amd.com>:
>>>
>>>> Dropping the rest of the patch, cause that really doesn't make sense any
>>>> more.
>>>>
>>>> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
>>>>>> E.g. for example it is illegal to do something like
>>>>>>> "while(!fence_is_signaled(f)) sleep();" without enabling signaling before
>>>>>>> doing this.
>>>>>>>
>>>>>>> Could just be a misunderstanding, but the comments on your patch actually
>>>>>>> sounds a bit like somebody is trying to do exactly that.
>>>>> I think the usecase in mind here is poll(fd, timeout=0)
>>>> Exactly as I feared. Even if userspace polls with timeout=0 you still need
>>>> to call enable_signaling().
>>>>
>>>> Otherwise you can run into a situation where userspace only uses timeout=0
>>>> and so never activates the signaling check in the driver.
>>>>
>>>> This would in turn result in an endless loop on implementations where the
>>>> driver never signals fences on their own.
>>> Polling is optional, userspace may never call it. And DRM/KMS or GPU
>>> drivers will be doing fence_wait() themselves so signaling is enabled at
>>> some point.
>> No they won't. We have an use case where we clearly want to avoid that as
>> much as possible because and so the driver never calls enable_signaling() on
>> it's own.
>>
>> Exposing this poll function to userspace without enabling signaling is a
>> clear NAK from my side.
> Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert
> that one then? It is already broken.

Yeah, that would probably a good idea. The AMD driver changes which 
really rely on this aren't upstream yet, so if you point me to the 
commit hash I could revert that as well when we send that out.

On the other hand the idea behind fence_is_signaled() is really that you 
check the status multiple times after enabling signaling. So I would 
prefer if you would revert this change preliminary.

Double checking this patch (and thinking about it a bit more) reveals 
that it is most likely correct. So feel free to commit this one if it is 
still needed for something.

Regards,
Christian.

>
> Gustavo
>

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Gustavo Padovan]

2016-09-23 Christian König <christian.koenig@amd.com>:

> Am 23.09.2016 um 13:30 schrieb Gustavo Padovan:
> > 2016-09-22 Christian König <christian.koenig@amd.com>:
> > 
> > > Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
> > > > 2016-09-22 Christian König <christian.koenig@amd.com>:
> > > > 
> > > > > Dropping the rest of the patch, cause that really doesn't make sense any
> > > > > more.
> > > > > 
> > > > > Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
> > > > > > > E.g. for example it is illegal to do something like
> > > > > > > > "while(!fence_is_signaled(f)) sleep();" without enabling signaling before
> > > > > > > > doing this.
> > > > > > > > 
> > > > > > > > Could just be a misunderstanding, but the comments on your patch actually
> > > > > > > > sounds a bit like somebody is trying to do exactly that.
> > > > > > I think the usecase in mind here is poll(fd, timeout=0)
> > > > > Exactly as I feared. Even if userspace polls with timeout=0 you still need
> > > > > to call enable_signaling().
> > > > > 
> > > > > Otherwise you can run into a situation where userspace only uses timeout=0
> > > > > and so never activates the signaling check in the driver.
> > > > > 
> > > > > This would in turn result in an endless loop on implementations where the
> > > > > driver never signals fences on their own.
> > > > Polling is optional, userspace may never call it. And DRM/KMS or GPU
> > > > drivers will be doing fence_wait() themselves so signaling is enabled at
> > > > some point.
> > > No they won't. We have an use case where we clearly want to avoid that as
> > > much as possible because and so the driver never calls enable_signaling() on
> > > it's own.
> > > 
> > > Exposing this poll function to userspace without enabling signaling is a
> > > clear NAK from my side.
> > Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert
> > that one then? It is already broken.
> 
> Yeah, that would probably a good idea. The AMD driver changes which really
> rely on this aren't upstream yet, so if you point me to the commit hash I
> could revert that as well when we send that out.
> 
> On the other hand the idea behind fence_is_signaled() is really that you
> check the status multiple times after enabling signaling. So I would prefer
> if you would revert this change preliminary.
> 
> Double checking this patch (and thinking about it a bit more) reveals that
> it is most likely correct. So feel free to commit this one if it is still
> needed for something.

It is this patch:

ecebca7 dma-buf/sync-file: Avoid enable fence signaling if poll(.timeout=0)

But if we revert it as you are a proposing we don't need my patch here
anymore. However we would need to revert it now because it is broken.
Shall I send a revert part?

Gustavo

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

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Chris Wilson]

On Sun, Sep 25, 2016 at 10:43:37PM +0200, Gustavo Padovan wrote:
> 2016-09-23 Christian König <christian.koenig@amd.com>:
> 
> > Am 23.09.2016 um 13:30 schrieb Gustavo Padovan:
> > > 2016-09-22 Christian König <christian.koenig@amd.com>:
> > > 
> > > > Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
> > > > > 2016-09-22 Christian König <christian.koenig@amd.com>:
> > > > > 
> > > > > > Dropping the rest of the patch, cause that really doesn't make sense any
> > > > > > more.
> > > > > > 
> > > > > > Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
> > > > > > > > E.g. for example it is illegal to do something like
> > > > > > > > > "while(!fence_is_signaled(f)) sleep();" without enabling signaling before
> > > > > > > > > doing this.
> > > > > > > > > 
> > > > > > > > > Could just be a misunderstanding, but the comments on your patch actually
> > > > > > > > > sounds a bit like somebody is trying to do exactly that.
> > > > > > > I think the usecase in mind here is poll(fd, timeout=0)
> > > > > > Exactly as I feared. Even if userspace polls with timeout=0 you still need
> > > > > > to call enable_signaling().
> > > > > > 
> > > > > > Otherwise you can run into a situation where userspace only uses timeout=0
> > > > > > and so never activates the signaling check in the driver.
> > > > > > 
> > > > > > This would in turn result in an endless loop on implementations where the
> > > > > > driver never signals fences on their own.
> > > > > Polling is optional, userspace may never call it. And DRM/KMS or GPU
> > > > > drivers will be doing fence_wait() themselves so signaling is enabled at
> > > > > some point.
> > > > No they won't. We have an use case where we clearly want to avoid that as
> > > > much as possible because and so the driver never calls enable_signaling() on
> > > > it's own.
> > > > 
> > > > Exposing this poll function to userspace without enabling signaling is a
> > > > clear NAK from my side.
> > > Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert
> > > that one then? It is already broken.
> > 
> > Yeah, that would probably a good idea. The AMD driver changes which really
> > rely on this aren't upstream yet, so if you point me to the commit hash I
> > could revert that as well when we send that out.
> > 
> > On the other hand the idea behind fence_is_signaled() is really that you
> > check the status multiple times after enabling signaling. So I would prefer
> > if you would revert this change preliminary.
> > 
> > Double checking this patch (and thinking about it a bit more) reveals that
> > it is most likely correct. So feel free to commit this one if it is still
> > needed for something.
> 
> It is this patch:
> 
> ecebca7 dma-buf/sync-file: Avoid enable fence signaling if poll(.timeout=0)
> 
> But if we revert it as you are a proposing we don't need my patch here
> anymore. However we would need to revert it now because it is broken.
> Shall I send a revert part?

Adding and propagating:

diff --git a/include/linux/fence.h b/include/linux/fence.h
index 0d763053f97a..ed5e88d2b664 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -56,6 +56,7 @@ struct fence_cb;
  *
  * FENCE_FLAG_SIGNALED_BIT - fence is already signaled
  * FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called*
+ * FENCE_FLAG_AUTO_SIGNAL_BIT - ops->signaled() is accurate by itself
  * FENCE_FLAG_USER_BITS - start of the unused bits, can be used by the
  * implementer of the fence for its own purposes. Can be used in different
  * ways by different fence implementers, so do not rely on this.
@@ -85,6 +86,7 @@ struct fence {
 enum fence_flag_bits {
        FENCE_FLAG_SIGNALED_BIT,
        FENCE_FLAG_ENABLE_SIGNAL_BIT,
+       FENCE_FLAG_AUTO_SIGNAL_BIT,
        FENCE_FLAG_USER_BITS, /* must always be last member */
 };
 
is trivial (I'm just stuck on naming and explaining it in kerneldoc). The
race Christian described in this patch is simple to prevent (on x86 all
that is missing is a compiler barrier, smp_mb__before_atomic() to be
generic), and it allows for external-fences to be as cheap as native
fences.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[zhoucm1]

On 2016年09月21日 19:36, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> If the fences in the fence_array signal on the fence_array does not have
> signalling enabled num_pending will not be updated accordingly.
>
> So when signaling is disabled check the signal of every fence with
> fence_is_signaled() and then compare with num_pending to learn if the
> fence_array was signalled or not.
>
> If we want to keep the poll_does_not_wait optimization I think we need
> something like this. It keeps the same behaviour if signalling is enabled
> but tries to calculated the state otherwise.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chunming Zhou <david1.zhou@amd.com>

Regards,
David Zhou
> ---
>   drivers/dma-buf/fence-array.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index f1989fc..1eec271 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -75,8 +75,25 @@ static bool fence_array_enable_signaling(struct fence *fence)
>   static bool fence_array_signaled(struct fence *fence)
>   {
>   	struct fence_array *array = to_fence_array(fence);
> +	int i, num_pending;
> +
> +	num_pending = atomic_read(&array->num_pending);
> +
> +	/*
> +	 * Before signaling is enabled, num_pending is static (set during array
> +	 * construction as a count of all fences or set to 1 if signal_on_any
> +	 * flag is passed. To ensure forward progress, i.e. a while
> +	 * (!fence_is_signaled()) ; busy-loop eventually proceeds, we need to
> +	 * check the current status of our fences.
> +	 */
> +	if (!test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {
> +		for (i = 0 ; i < array->num_fences; ++i) {
> +			if (fence_is_signaled(array->fences[i]))
> +				num_pending--;
> +		}
> +	}
>   
> -	return atomic_read(&array->num_pending) <= 0;
> +	return num_pending <= 0;
>   }
>   
>   static void fence_array_release(struct fence *fence)

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