[PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Lionel Landwerlin]

This reverts commit 5de376bb434f80a13138f0ebedc8351ab73d8b0d.

This change breaks synchronization of a timeline.
dma_fence_chain_find_seqno() might be a bit of a confusing name but
this function is not trying to find a particular seqno, is supposed to
give a fence to wait on for a particular point in the timeline.

In a timeline, a particular value is reached when all the points up to
and including that value have signaled.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/dma-buf/dma-fence-chain.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index c435bbba851c..3d123502ff12 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -99,12 +99,6 @@ int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)
 		return -EINVAL;
 
 	dma_fence_chain_for_each(*pfence, &chain->base) {
-		if ((*pfence)->seqno < seqno) { /* already signaled */
-			dma_fence_put(*pfence);
-			*pfence = NULL;
-			break;
-		}
-
 		if ((*pfence)->context != chain->base.context ||
 		    to_dma_fence_chain(*pfence)->prev_seqno < seqno)
 			break;
@@ -228,7 +222,6 @@ EXPORT_SYMBOL(dma_fence_chain_ops);
  * @chain: the chain node to initialize
  * @prev: the previous fence
  * @fence: the current fence
- * @seqno: the sequence number (syncpt) of the fence within the chain
  *
  * Initialize a new chain node and either start a new chain or add the node to
  * the existing chain of the previous fence.
-- 
2.27.0

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

[PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Lionel Landwerlin]

There was probably a misunderstand on how the dma-fence-chain is
supposed to work or what dma_fence_chain_find_seqno() is supposed to
return.

dma_fence_chain_find_seqno() is here to give us the fence to wait upon
for a particular point in the timeline. The timeline progresses only
when all the points prior to a given number have completed.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: dc2f7e67a28a5c ("dma-buf: Exercise dma-fence-chain under selftests")
---
 drivers/dma-buf/st-dma-fence-chain.c | 43 ++++++++++++++--------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c
index 5d45ba7ba3cd..9525f7f56119 100644
--- a/drivers/dma-buf/st-dma-fence-chain.c
+++ b/drivers/dma-buf/st-dma-fence-chain.c
@@ -318,15 +318,16 @@ static int find_out_of_order(void *arg)
 		goto err;
 	}
 
-	if (fence && fence != fc.chains[1]) {
+	/*
+	 * We signaled the middle fence (2) of the 1-2-3 chain. The behavior
+	 * of the dma-fence-chain is to make us wait for all the fences up to
+	 * the point we want. Since fence 1 is still not signaled, this what
+	 * we should get as fence to wait upon (fence 2 being garbage
+	 * collected during the traversal of the chain).
+	 */
+	if (fence != fc.chains[0]) {
 		pr_err("Incorrect chain-fence.seqno:%lld reported for completed seqno:2\n",
-		       fence->seqno);
-
-		dma_fence_get(fence);
-		err = dma_fence_chain_find_seqno(&fence, 2);
-		dma_fence_put(fence);
-		if (err)
-			pr_err("Reported %d for finding self!\n", err);
+		       fence ? fence->seqno : 0);
 
 		err = -EINVAL;
 	}
@@ -415,20 +416,18 @@ static int __find_race(void *arg)
 		if (!fence)
 			goto signal;
 
-		err = dma_fence_chain_find_seqno(&fence, seqno);
-		if (err) {
-			pr_err("Reported an invalid fence for find-self:%d\n",
-			       seqno);
-			dma_fence_put(fence);
-			break;
-		}
-
-		if (fence->seqno < seqno) {
-			pr_err("Reported an earlier fence.seqno:%lld for seqno:%d\n",
-			       fence->seqno, seqno);
-			err = -EINVAL;
-			dma_fence_put(fence);
-			break;
+		/*
+		 * We can only find ourselves if we are on fence we were
+		 * looking for.
+		 */
+		if (fence->seqno == seqno) {
+			err = dma_fence_chain_find_seqno(&fence, seqno);
+			if (err) {
+				pr_err("Reported an invalid fence for find-self:%d\n",
+				       seqno);
+				dma_fence_put(fence);
+				break;
+			}
 		}
 
 		dma_fence_put(fence);
-- 
2.27.0

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

Re: [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Christian König]

Am 25.06.20 um 14:34 schrieb Lionel Landwerlin:
> This reverts commit 5de376bb434f80a13138f0ebedc8351ab73d8b0d.
>
> This change breaks synchronization of a timeline.
> dma_fence_chain_find_seqno() might be a bit of a confusing name but
> this function is not trying to find a particular seqno, is supposed to
> give a fence to wait on for a particular point in the timeline.
>
> In a timeline, a particular value is reached when all the points up to
> and including that value have signaled.
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/dma-buf/dma-fence-chain.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index c435bbba851c..3d123502ff12 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -99,12 +99,6 @@ int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)
>   		return -EINVAL;
>   
>   	dma_fence_chain_for_each(*pfence, &chain->base) {
> -		if ((*pfence)->seqno < seqno) { /* already signaled */
> -			dma_fence_put(*pfence);
> -			*pfence = NULL;
> -			break;
> -		}
> -
>   		if ((*pfence)->context != chain->base.context ||
>   		    to_dma_fence_chain(*pfence)->prev_seqno < seqno)
>   			break;
> @@ -228,7 +222,6 @@ EXPORT_SYMBOL(dma_fence_chain_ops);
>    * @chain: the chain node to initialize
>    * @prev: the previous fence
>    * @fence: the current fence
> - * @seqno: the sequence number (syncpt) of the fence within the chain
>    *
>    * Initialize a new chain node and either start a new chain or add the node to
>    * the existing chain of the previous fence.

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

Re: [PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Christian König]

Am 25.06.20 um 14:34 schrieb Lionel Landwerlin:
> There was probably a misunderstand on how the dma-fence-chain is
> supposed to work or what dma_fence_chain_find_seqno() is supposed to
> return.
>
> dma_fence_chain_find_seqno() is here to give us the fence to wait upon
> for a particular point in the timeline. The timeline progresses only
> when all the points prior to a given number have completed.
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: dc2f7e67a28a5c ("dma-buf: Exercise dma-fence-chain under selftests")

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/dma-buf/st-dma-fence-chain.c | 43 ++++++++++++++--------------
>   1 file changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c
> index 5d45ba7ba3cd..9525f7f56119 100644
> --- a/drivers/dma-buf/st-dma-fence-chain.c
> +++ b/drivers/dma-buf/st-dma-fence-chain.c
> @@ -318,15 +318,16 @@ static int find_out_of_order(void *arg)
>   		goto err;
>   	}
>   
> -	if (fence && fence != fc.chains[1]) {
> +	/*
> +	 * We signaled the middle fence (2) of the 1-2-3 chain. The behavior
> +	 * of the dma-fence-chain is to make us wait for all the fences up to
> +	 * the point we want. Since fence 1 is still not signaled, this what
> +	 * we should get as fence to wait upon (fence 2 being garbage
> +	 * collected during the traversal of the chain).
> +	 */
> +	if (fence != fc.chains[0]) {
>   		pr_err("Incorrect chain-fence.seqno:%lld reported for completed seqno:2\n",
> -		       fence->seqno);
> -
> -		dma_fence_get(fence);
> -		err = dma_fence_chain_find_seqno(&fence, 2);
> -		dma_fence_put(fence);
> -		if (err)
> -			pr_err("Reported %d for finding self!\n", err);
> +		       fence ? fence->seqno : 0);
>   
>   		err = -EINVAL;
>   	}
> @@ -415,20 +416,18 @@ static int __find_race(void *arg)
>   		if (!fence)
>   			goto signal;
>   
> -		err = dma_fence_chain_find_seqno(&fence, seqno);
> -		if (err) {
> -			pr_err("Reported an invalid fence for find-self:%d\n",
> -			       seqno);
> -			dma_fence_put(fence);
> -			break;
> -		}
> -
> -		if (fence->seqno < seqno) {
> -			pr_err("Reported an earlier fence.seqno:%lld for seqno:%d\n",
> -			       fence->seqno, seqno);
> -			err = -EINVAL;
> -			dma_fence_put(fence);
> -			break;
> +		/*
> +		 * We can only find ourselves if we are on fence we were
> +		 * looking for.
> +		 */
> +		if (fence->seqno == seqno) {
> +			err = dma_fence_chain_find_seqno(&fence, seqno);
> +			if (err) {
> +				pr_err("Reported an invalid fence for find-self:%d\n",
> +				       seqno);
> +				dma_fence_put(fence);
> +				break;
> +			}
>   		}
>   
>   		dma_fence_put(fence);

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

Re: [Intel-gfx] [PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Chris Wilson]

Quoting Lionel Landwerlin (2020-06-25 13:34:43)
> There was probably a misunderstand on how the dma-fence-chain is
> supposed to work or what dma_fence_chain_find_seqno() is supposed to
> return.
> 
> dma_fence_chain_find_seqno() is here to give us the fence to wait upon
> for a particular point in the timeline. The timeline progresses only
> when all the points prior to a given number have completed.

Hmm, the question was what point is it supposed to wait for.

For the simple chain of [1, 3], does 1 being signaled imply that all
points up to 3 are signaled, or does 3 not being signaled imply that all
points after 1 are not. If that's mentioned already somewhere, my bad.
If not, could you put the answer somewhere.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Lionel Landwerlin]

On 25/06/2020 16:18, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2020-06-25 13:34:43)
>> There was probably a misunderstand on how the dma-fence-chain is
>> supposed to work or what dma_fence_chain_find_seqno() is supposed to
>> return.
>>
>> dma_fence_chain_find_seqno() is here to give us the fence to wait upon
>> for a particular point in the timeline. The timeline progresses only
>> when all the points prior to a given number have completed.
> Hmm, the question was what point is it supposed to wait for.
>
> For the simple chain of [1, 3], does 1 being signaled imply that all
> points up to 3 are signaled, or does 3 not being signaled imply that all
> points after 1 are not. If that's mentioned already somewhere, my bad.
> If not, could you put the answer somewhere.
> -Chris

In [1, 3], if 1 is signaled, the timeline value is 1. And find_seqno(2) 
should return NULL.


In the out_of_order selftest the chain was [1, 2, 3], 2 was signaled and 
the test was expecting no fence to be returned by find_seqno(2).

But we still have to wait on 1 to complete before find_seqno(2) can 
return NULL (as in you don't have to wait on anything).


Hope that answer the question.


-Lionel

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

Re: [Intel-gfx] [PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Chris Wilson]

Quoting Lionel Landwerlin (2020-06-25 14:23:25)
> On 25/06/2020 16:18, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2020-06-25 13:34:43)
> >> There was probably a misunderstand on how the dma-fence-chain is
> >> supposed to work or what dma_fence_chain_find_seqno() is supposed to
> >> return.
> >>
> >> dma_fence_chain_find_seqno() is here to give us the fence to wait upon
> >> for a particular point in the timeline. The timeline progresses only
> >> when all the points prior to a given number have completed.
> > Hmm, the question was what point is it supposed to wait for.
> >
> > For the simple chain of [1, 3], does 1 being signaled imply that all
> > points up to 3 are signaled, or does 3 not being signaled imply that all
> > points after 1 are not. If that's mentioned already somewhere, my bad.
> > If not, could you put the answer somewhere.
> > -Chris
> 
> In [1, 3], if 1 is signaled, the timeline value is 1. And find_seqno(2) 
> should return NULL.
> 
> 
> In the out_of_order selftest the chain was [1, 2, 3], 2 was signaled and 
> the test was expecting no fence to be returned by find_seqno(2).
> 
> But we still have to wait on 1 to complete before find_seqno(2) can 
> return NULL (as in you don't have to wait on anything).

* scratches head

I thought it was meant to be expecting fc.chain[1] to still be present
as the chain at that point was not yet signaled.

Oh well, a mistake compounded. :|
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Lionel Landwerlin]

On 25/06/2020 16:47, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2020-06-25 14:23:25)
>> On 25/06/2020 16:18, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2020-06-25 13:34:43)
>>>> There was probably a misunderstand on how the dma-fence-chain is
>>>> supposed to work or what dma_fence_chain_find_seqno() is supposed to
>>>> return.
>>>>
>>>> dma_fence_chain_find_seqno() is here to give us the fence to wait upon
>>>> for a particular point in the timeline. The timeline progresses only
>>>> when all the points prior to a given number have completed.
>>> Hmm, the question was what point is it supposed to wait for.
>>>
>>> For the simple chain of [1, 3], does 1 being signaled imply that all
>>> points up to 3 are signaled, or does 3 not being signaled imply that all
>>> points after 1 are not. If that's mentioned already somewhere, my bad.
>>> If not, could you put the answer somewhere.
>>> -Chris
>> In [1, 3], if 1 is signaled, the timeline value is 1. And find_seqno(2)
>> should return NULL.
>>
>>
>> In the out_of_order selftest the chain was [1, 2, 3], 2 was signaled and
>> the test was expecting no fence to be returned by find_seqno(2).
>>
>> But we still have to wait on 1 to complete before find_seqno(2) can
>> return NULL (as in you don't have to wait on anything).
> * scratches head
>
> I thought it was meant to be expecting fc.chain[1] to still be present
> as the chain at that point was not yet signaled.


You're right that the point is not yet signaled.

But it doesn't need to stay in the chain if you can wait on a previous 
point.


chain[1] gets removed as we walk the chain backward in dma_fence_chain_walk.


-Lionel


>
> Oh well, a mistake compounded. :|
> -Chris


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

Re: [Intel-gfx] [PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Daniel Vetter]

On Thu, Jun 25, 2020 at 3:23 PM Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
>
> On 25/06/2020 16:18, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2020-06-25 13:34:43)
> >> There was probably a misunderstand on how the dma-fence-chain is
> >> supposed to work or what dma_fence_chain_find_seqno() is supposed to
> >> return.
> >>
> >> dma_fence_chain_find_seqno() is here to give us the fence to wait upon
> >> for a particular point in the timeline. The timeline progresses only
> >> when all the points prior to a given number have completed.
> > Hmm, the question was what point is it supposed to wait for.
> >
> > For the simple chain of [1, 3], does 1 being signaled imply that all
> > points up to 3 are signaled, or does 3 not being signaled imply that all
> > points after 1 are not. If that's mentioned already somewhere, my bad.
> > If not, could you put the answer somewhere.
> > -Chris
>
> In [1, 3], if 1 is signaled, the timeline value is 1. And find_seqno(2)
> should return NULL.
>
>
> In the out_of_order selftest the chain was [1, 2, 3], 2 was signaled and
> the test was expecting no fence to be returned by find_seqno(2).
>
> But we still have to wait on 1 to complete before find_seqno(2) can
> return NULL (as in you don't have to wait on anything).
>
>
> Hope that answer the question.

I asked Christian to document why timeline works like this, but I
can't find it in the kerneldoc right now. If it's missing I think we
should fix that and add the explanation, iirc it was around gpu reset
creating too much havoc otherwise.
-Daniel

>
>
> -Lionel
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Dave Airlie]

WTUF?

How did this ever land in my tree, there is no ACK on this from anyone
in core dma-buf,

Intel team, clean your house up here, I'm going to have to ask you to
stop Chris merging stuff without oversight, if this sort of thing
happens, this is totally unacceptable.

Dave.


 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Tested-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
    Reviewed-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>


On Thu, 25 Jun 2020 at 22:43, Christian König <christian.koenig@amd.com> wrote:
>
> Am 25.06.20 um 14:34 schrieb Lionel Landwerlin:
> > This reverts commit 5de376bb434f80a13138f0ebedc8351ab73d8b0d.
> >
> > This change breaks synchronization of a timeline.
> > dma_fence_chain_find_seqno() might be a bit of a confusing name but
> > this function is not trying to find a particular seqno, is supposed to
> > give a fence to wait on for a particular point in the timeline.
> >
> > In a timeline, a particular value is reached when all the points up to
> > and including that value have signaled.
> >
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> > ---
> >   drivers/dma-buf/dma-fence-chain.c | 7 -------
> >   1 file changed, 7 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> > index c435bbba851c..3d123502ff12 100644
> > --- a/drivers/dma-buf/dma-fence-chain.c
> > +++ b/drivers/dma-buf/dma-fence-chain.c
> > @@ -99,12 +99,6 @@ int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)
> >               return -EINVAL;
> >
> >       dma_fence_chain_for_each(*pfence, &chain->base) {
> > -             if ((*pfence)->seqno < seqno) { /* already signaled */
> > -                     dma_fence_put(*pfence);
> > -                     *pfence = NULL;
> > -                     break;
> > -             }
> > -
> >               if ((*pfence)->context != chain->base.context ||
> >                   to_dma_fence_chain(*pfence)->prev_seqno < seqno)
> >                       break;
> > @@ -228,7 +222,6 @@ EXPORT_SYMBOL(dma_fence_chain_ops);
> >    * @chain: the chain node to initialize
> >    * @prev: the previous fence
> >    * @fence: the current fence
> > - * @seqno: the sequence number (syncpt) of the fence within the chain
> >    *
> >    * Initialize a new chain node and either start a new chain or add the node to
> >    * the existing chain of the previous fence.
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Jani Nikula]

On Fri, 26 Jun 2020, Dave Airlie <airlied@gmail.com> wrote:
> WTUF?
>
> How did this ever land in my tree, there is no ACK on this from anyone
> in core dma-buf,
>
> Intel team, clean your house up here, I'm going to have to ask you to
> stop Chris merging stuff without oversight, if this sort of thing
> happens, this is totally unacceptable.

There's no argument, an ack is required.

In fairness to the i915 maintainers, though, this particular commit was
merged via drm-misc-next [1].

As a side note, there seem to be extra checks in place for acks when
applying non-i915 patches to drm-intel; there are no such checks for
drm-misc.


BR,
Jani.


[1] http://lore.kernel.org/r/20200414090738.GA16827@linux-uq9g

>
> Dave.
>
>
>  Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Tested-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
>     Reviewed-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
>
>
> On Thu, 25 Jun 2020 at 22:43, Christian König <christian.koenig@amd.com> wrote:
>>
>> Am 25.06.20 um 14:34 schrieb Lionel Landwerlin:
>> > This reverts commit 5de376bb434f80a13138f0ebedc8351ab73d8b0d.
>> >
>> > This change breaks synchronization of a timeline.
>> > dma_fence_chain_find_seqno() might be a bit of a confusing name but
>> > this function is not trying to find a particular seqno, is supposed to
>> > give a fence to wait on for a particular point in the timeline.
>> >
>> > In a timeline, a particular value is reached when all the points up to
>> > and including that value have signaled.
>> >
>> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>
>> > ---
>> >   drivers/dma-buf/dma-fence-chain.c | 7 -------
>> >   1 file changed, 7 deletions(-)
>> >
>> > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
>> > index c435bbba851c..3d123502ff12 100644
>> > --- a/drivers/dma-buf/dma-fence-chain.c
>> > +++ b/drivers/dma-buf/dma-fence-chain.c
>> > @@ -99,12 +99,6 @@ int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno)
>> >               return -EINVAL;
>> >
>> >       dma_fence_chain_for_each(*pfence, &chain->base) {
>> > -             if ((*pfence)->seqno < seqno) { /* already signaled */
>> > -                     dma_fence_put(*pfence);
>> > -                     *pfence = NULL;
>> > -                     break;
>> > -             }
>> > -
>> >               if ((*pfence)->context != chain->base.context ||
>> >                   to_dma_fence_chain(*pfence)->prev_seqno < seqno)
>> >                       break;
>> > @@ -228,7 +222,6 @@ EXPORT_SYMBOL(dma_fence_chain_ops);
>> >    * @chain: the chain node to initialize
>> >    * @prev: the previous fence
>> >    * @fence: the current fence
>> > - * @seqno: the sequence number (syncpt) of the fence within the chain
>> >    *
>> >    * Initialize a new chain node and either start a new chain or add the node to
>> >    * the existing chain of the previous fence.
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Daniel Vetter]

Ignoring everything else ...

On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> As a side note, there seem to be extra checks in place for acks when
> applying non-i915 patches to drm-intel; there are no such checks for
> drm-misc.

One option to generalize that that I pondered is to consult
get_maintainers.pl asking for git repo link, and if that returns
something else, then insist that there's an ack from a relevant
maintainer. It's a bit of typing, but I think the bigger problem is
that there's a ton of false positives.

But maybe that's a good thing, would give some motivation to keep
MAINTAINERS updated.

The other issue is though that drm-misc is plenty used to merge
patches even when the respective maintainers are absent for weeks, or
unresponsive. If we just blindly implement that rule, then the only
possible Ack for these would be Dave&me as subsystem maintainers, and
I don't want to be in the business of stamping approvals for all this
stuff. Much better if people just collaborate.

So I think an ack check would be nice, but probably not practical.

Plus in this situation here drm-misc.git actually is the main repo,
and we wont ever be able to teach a script to make a judgement call of
whether that patch has the right amount of review on it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Dave Airlie]

On Fri, 26 Jun 2020 at 05:27, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Fri, 26 Jun 2020, Dave Airlie <airlied@gmail.com> wrote:
> > WTUF?
> >
> > How did this ever land in my tree, there is no ACK on this from anyone
> > in core dma-buf,
> >
> > Intel team, clean your house up here, I'm going to have to ask you to
> > stop Chris merging stuff without oversight, if this sort of thing
> > happens, this is totally unacceptable.
>
> There's no argument, an ack is required.
>
> In fairness to the i915 maintainers, though, this particular commit was
> merged via drm-misc-next [1].
>
> As a side note, there seem to be extra checks in place for acks when
> applying non-i915 patches to drm-intel; there are no such checks for
> drm-misc.

Sorry Jani, thanks for chasing that down.

drm-misc we need to oversight a bit more, I don't think we should be
landing things that affect core code with single company acks.

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

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Sumit Semwal]

On Fri, 26 Jun 2020 at 01:24, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> Ignoring everything else ...
>
> On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > As a side note, there seem to be extra checks in place for acks when
> > applying non-i915 patches to drm-intel; there are no such checks for
> > drm-misc.
>
> One option to generalize that that I pondered is to consult
> get_maintainers.pl asking for git repo link, and if that returns
> something else, then insist that there's an ack from a relevant
> maintainer. It's a bit of typing, but I think the bigger problem is
> that there's a ton of false positives.

Right; for the particular patch, I wasn't even in the to: or cc: field
and that made it slip from my radar. I would definitely ask any one
sending patches for dma-buf directory to follow the get_maintainers.pl
religiously.
>
> But maybe that's a good thing, would give some motivation to keep
> MAINTAINERS updated.
>
> The other issue is though that drm-misc is plenty used to merge
> patches even when the respective maintainers are absent for weeks, or
> unresponsive. If we just blindly implement that rule, then the only
> possible Ack for these would be Dave&me as subsystem maintainers, and
> I don't want to be in the business of stamping approvals for all this
> stuff. Much better if people just collaborate.
>
> So I think an ack check would be nice, but probably not practical.
>
> Plus in this situation here drm-misc.git actually is the main repo,
> and we wont ever be able to teach a script to make a judgement call of
> whether that patch has the right amount of review on it.
> -Daniel

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

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Christian König]

Am 26.06.20 um 06:43 schrieb Sumit Semwal:
> On Fri, 26 Jun 2020 at 01:24, Daniel Vetter <daniel@ffwll.ch> wrote:
>> Ignoring everything else ...
>>
>> On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>> As a side note, there seem to be extra checks in place for acks when
>>> applying non-i915 patches to drm-intel; there are no such checks for
>>> drm-misc.
>> One option to generalize that that I pondered is to consult
>> get_maintainers.pl asking for git repo link, and if that returns
>> something else, then insist that there's an ack from a relevant
>> maintainer. It's a bit of typing, but I think the bigger problem is
>> that there's a ton of false positives.
> Right; for the particular patch, I wasn't even in the to: or cc: field
> and that made it slip from my radar. I would definitely ask any one
> sending patches for dma-buf directory to follow the get_maintainers.pl
> religiously.
>> But maybe that's a good thing, would give some motivation to keep
>> MAINTAINERS updated.

Should I maybe add myself as maintainer as well? I've written enough 
stuff in there to know the code quite a bit.

Christian.

>>
>> The other issue is though that drm-misc is plenty used to merge
>> patches even when the respective maintainers are absent for weeks, or
>> unresponsive. If we just blindly implement that rule, then the only
>> possible Ack for these would be Dave&me as subsystem maintainers, and
>> I don't want to be in the business of stamping approvals for all this
>> stuff. Much better if people just collaborate.
>>
>> So I think an ack check would be nice, but probably not practical.
>>
>> Plus in this situation here drm-misc.git actually is the main repo,
>> and we wont ever be able to teach a script to make a judgement call of
>> whether that patch has the right amount of review on it.
>> -Daniel
> Best,
> Sumit.

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

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Daniel Vetter]

On Fri, Jun 26, 2020 at 9:03 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 26.06.20 um 06:43 schrieb Sumit Semwal:
> > On Fri, 26 Jun 2020 at 01:24, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> Ignoring everything else ...
> >>
> >> On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>> As a side note, there seem to be extra checks in place for acks when
> >>> applying non-i915 patches to drm-intel; there are no such checks for
> >>> drm-misc.
> >> One option to generalize that that I pondered is to consult
> >> get_maintainers.pl asking for git repo link, and if that returns
> >> something else, then insist that there's an ack from a relevant
> >> maintainer. It's a bit of typing, but I think the bigger problem is
> >> that there's a ton of false positives.
> > Right; for the particular patch, I wasn't even in the to: or cc: field
> > and that made it slip from my radar. I would definitely ask any one
> > sending patches for dma-buf directory to follow the get_maintainers.pl
> > religiously.
> >> But maybe that's a good thing, would give some motivation to keep
> >> MAINTAINERS updated.
>
> Should I maybe add myself as maintainer as well? I've written enough
> stuff in there to know the code quite a bit.

I think that makes lots of sense, since defacto you already are :-)

If you feel like bikeshed, get_maintainers.pl also supports R: for
reviewer, but given that you also push patches to drm-misc M: for
maintainer feels more accurate.
-Daniel


>
> Christian.
>
> >>
> >> The other issue is though that drm-misc is plenty used to merge
> >> patches even when the respective maintainers are absent for weeks, or
> >> unresponsive. If we just blindly implement that rule, then the only
> >> possible Ack for these would be Dave&me as subsystem maintainers, and
> >> I don't want to be in the business of stamping approvals for all this
> >> stuff. Much better if people just collaborate.
> >>
> >> So I think an ack check would be nice, but probably not practical.
> >>
> >> Plus in this situation here drm-misc.git actually is the main repo,
> >> and we wont ever be able to teach a script to make a judgement call of
> >> whether that patch has the right amount of review on it.
> >> -Daniel
> > Best,
> > Sumit.
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Sumit Semwal]

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

Hi Christian,

On Fri, 26 Jun 2020, 18:10 Daniel Vetter, <daniel@ffwll.ch> wrote:

> On Fri, Jun 26, 2020 at 9:03 AM Christian König
> <christian.koenig@amd.com> wrote:
> >
> > Am 26.06.20 um 06:43 schrieb Sumit Semwal:
> > > On Fri, 26 Jun 2020 at 01:24, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >> Ignoring everything else ...
> > >>
> > >> On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula <
> jani.nikula@linux.intel.com> wrote:
> > >>> As a side note, there seem to be extra checks in place for acks when
> > >>> applying non-i915 patches to drm-intel; there are no such checks for
> > >>> drm-misc.
> > >> One option to generalize that that I pondered is to consult
> > >> get_maintainers.pl asking for git repo link, and if that returns
> > >> something else, then insist that there's an ack from a relevant
> > >> maintainer. It's a bit of typing, but I think the bigger problem is
> > >> that there's a ton of false positives.
> > > Right; for the particular patch, I wasn't even in the to: or cc: field
> > > and that made it slip from my radar. I would definitely ask any one
> > > sending patches for dma-buf directory to follow the get_maintainers.pl
> > > religiously.
> > >> But maybe that's a good thing, would give some motivation to keep
> > >> MAINTAINERS updated.
> >
> > Should I maybe add myself as maintainer as well? I've written enough
> > stuff in there to know the code quite a bit.
>
> I think that makes lots of sense, since defacto you already are :-)
>
> If you feel like bikeshed, get_maintainers.pl also supports R: for
> reviewer, but given that you also push patches to drm-misc M: for
> maintainer feels more accurate.
>

I think given you've been reviewing and changing most of the code around
dma-fences, it should be ok to add you as the maintainer for those bits?

-Daniel
>

Best,
Sumit.

[-- Attachment #1.2: Type: text/html, Size: 3195 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 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Lionel Landwerlin]

On 25/06/2020 15:43, Christian König wrote:
> Am 25.06.20 um 14:34 schrieb Lionel Landwerlin:
>> This reverts commit 5de376bb434f80a13138f0ebedc8351ab73d8b0d.
>>
>> This change breaks synchronization of a timeline.
>> dma_fence_chain_find_seqno() might be a bit of a confusing name but
>> this function is not trying to find a particular seqno, is supposed to
>> give a fence to wait on for a particular point in the timeline.
>>
>> In a timeline, a particular value is reached when all the points up to
>> and including that value have signaled.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>


Now that you are a maintainer, feel free to merge this and the test changes.


Thanks,


-Lionel


>
>> ---
>>   drivers/dma-buf/dma-fence-chain.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence-chain.c 
>> b/drivers/dma-buf/dma-fence-chain.c
>> index c435bbba851c..3d123502ff12 100644
>> --- a/drivers/dma-buf/dma-fence-chain.c
>> +++ b/drivers/dma-buf/dma-fence-chain.c
>> @@ -99,12 +99,6 @@ int dma_fence_chain_find_seqno(struct dma_fence 
>> **pfence, uint64_t seqno)
>>           return -EINVAL;
>>         dma_fence_chain_for_each(*pfence, &chain->base) {
>> -        if ((*pfence)->seqno < seqno) { /* already signaled */
>> -            dma_fence_put(*pfence);
>> -            *pfence = NULL;
>> -            break;
>> -        }
>> -
>>           if ((*pfence)->context != chain->base.context ||
>>               to_dma_fence_chain(*pfence)->prev_seqno < seqno)
>>               break;
>> @@ -228,7 +222,6 @@ EXPORT_SYMBOL(dma_fence_chain_ops);
>>    * @chain: the chain node to initialize
>>    * @prev: the previous fence
>>    * @fence: the current fence
>> - * @seqno: the sequence number (syncpt) of the fence within the chain
>>    *
>>    * Initialize a new chain node and either start a new chain or add 
>> the node to
>>    * the existing chain of the previous fence.
>

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

Re: [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"

[Christian König]

Am 02.07.20 um 10:26 schrieb Lionel Landwerlin:
> On 25/06/2020 15:43, Christian König wrote:
>> Am 25.06.20 um 14:34 schrieb Lionel Landwerlin:
>>> This reverts commit 5de376bb434f80a13138f0ebedc8351ab73d8b0d.
>>>
>>> This change breaks synchronization of a timeline.
>>> dma_fence_chain_find_seqno() might be a bit of a confusing name but
>>> this function is not trying to find a particular seqno, is supposed to
>>> give a fence to wait on for a particular point in the timeline.
>>>
>>> In a timeline, a particular value is reached when all the points up to
>>> and including that value have signaled.
>>>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>
>
> Now that you are a maintainer, feel free to merge this and the test 
> changes.

Sure, I will pick that up later today :)

Christian.

>
>
> Thanks,
>
>
> -Lionel
>
>
>>
>>> ---
>>>   drivers/dma-buf/dma-fence-chain.c | 7 -------
>>>   1 file changed, 7 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence-chain.c 
>>> b/drivers/dma-buf/dma-fence-chain.c
>>> index c435bbba851c..3d123502ff12 100644
>>> --- a/drivers/dma-buf/dma-fence-chain.c
>>> +++ b/drivers/dma-buf/dma-fence-chain.c
>>> @@ -99,12 +99,6 @@ int dma_fence_chain_find_seqno(struct dma_fence 
>>> **pfence, uint64_t seqno)
>>>           return -EINVAL;
>>>         dma_fence_chain_for_each(*pfence, &chain->base) {
>>> -        if ((*pfence)->seqno < seqno) { /* already signaled */
>>> -            dma_fence_put(*pfence);
>>> -            *pfence = NULL;
>>> -            break;
>>> -        }
>>> -
>>>           if ((*pfence)->context != chain->base.context ||
>>>               to_dma_fence_chain(*pfence)->prev_seqno < seqno)
>>>               break;
>>> @@ -228,7 +222,6 @@ EXPORT_SYMBOL(dma_fence_chain_ops);
>>>    * @chain: the chain node to initialize
>>>    * @prev: the previous fence
>>>    * @fence: the current fence
>>> - * @seqno: the sequence number (syncpt) of the fence within the chain
>>>    *
>>>    * Initialize a new chain node and either start a new chain or add 
>>> the node to
>>>    * the existing chain of the previous fence.
>>
>

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

Re: [Intel-gfx] [PATCH 2/2] dma-buf: fix dma-fence-chain out of order test

[Christian König]

Am 25.06.20 um 15:59 schrieb Daniel Vetter:
> On Thu, Jun 25, 2020 at 3:23 PM Lionel Landwerlin
> <lionel.g.landwerlin@intel.com> wrote:
>> On 25/06/2020 16:18, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2020-06-25 13:34:43)
>>>> There was probably a misunderstand on how the dma-fence-chain is
>>>> supposed to work or what dma_fence_chain_find_seqno() is supposed to
>>>> return.
>>>>
>>>> dma_fence_chain_find_seqno() is here to give us the fence to wait upon
>>>> for a particular point in the timeline. The timeline progresses only
>>>> when all the points prior to a given number have completed.
>>> Hmm, the question was what point is it supposed to wait for.
>>>
>>> For the simple chain of [1, 3], does 1 being signaled imply that all
>>> points up to 3 are signaled, or does 3 not being signaled imply that all
>>> points after 1 are not. If that's mentioned already somewhere, my bad.
>>> If not, could you put the answer somewhere.
>>> -Chris
>> In [1, 3], if 1 is signaled, the timeline value is 1. And find_seqno(2)
>> should return NULL.
>>
>>
>> In the out_of_order selftest the chain was [1, 2, 3], 2 was signaled and
>> the test was expecting no fence to be returned by find_seqno(2).
>>
>> But we still have to wait on 1 to complete before find_seqno(2) can
>> return NULL (as in you don't have to wait on anything).
>>
>>
>> Hope that answer the question.
> I asked Christian to document why timeline works like this, but I
> can't find it in the kerneldoc right now. If it's missing I think we
> should fix that and add the explanation, iirc it was around gpu reset
> creating too much havoc otherwise.

I do remember that I wrote a patch to improve the kerneldoc for timeline 
semaphores, but then somebody else came along with an even better 
description.

Unfortunately it looks like neither was ever merged.

Need to dig through my mails,
Christian.

> -Daniel
>
>>
>> -Lionel
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fintel-gfx&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7Cfd87640cd9bd422971bf08d8191004d2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637286903879074805&amp;sdata=M3WGWbuyQKZeGC0J3wEKtgQ1oKYo6GOAMvKU2mU3r%2FM%3D&amp;reserved=0
>
>

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