[04/10] dma-buf: Report signaled links inside dma-fence-chain
diff mbox series

Message ID 20200403091300.14734-4-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/10] drm/i915/selftests: Add request throughput measurement to perf
Related show

Commit Message

Chris Wilson April 3, 2020, 9:12 a.m. UTC
Whenever we walk along the dma-fence-chain, we prune signaled links to
keep the chain nice and tidy. This leads to situations where we can
prune a link and report the earlier fence as the target seqno --
violating our own consistency checks that the seqno is not more advanced
than the last element in a dma-fence-chain.

Report a NULL fence and success if the seqno has already been signaled.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/dma-fence-chain.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Venkata Sandeep Dhanalakota April 8, 2020, 7:46 p.m. UTC | #1
On 20/04/03 10:12, Chris Wilson wrote:
> Whenever we walk along the dma-fence-chain, we prune signaled links to
> keep the chain nice and tidy. This leads to situations where we can
> prune a link and report the earlier fence as the target seqno --
> violating our own consistency checks that the seqno is not more advanced
> than the last element in a dma-fence-chain.
> 
> Report a NULL fence and success if the seqno has already been signaled.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/dma-buf/dma-fence-chain.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index 3d123502ff12..c435bbba851c 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -99,6 +99,12 @@ 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;
> +		}
> +
Looks good to me.

Tested-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
Reviewed-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>

>  		if ((*pfence)->context != chain->base.context ||
>  		    to_dma_fence_chain(*pfence)->prev_seqno < seqno)
>  			break;
> @@ -222,6 +228,7 @@ 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.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lionel Landwerlin April 8, 2020, 8 p.m. UTC | #2
On 03/04/2020 12:12, Chris Wilson wrote:
> Whenever we walk along the dma-fence-chain, we prune signaled links to
> keep the chain nice and tidy. This leads to situations where we can
> prune a link and report the earlier fence as the target seqno --
> violating our own consistency checks that the seqno is not more advanced
> than the last element in a dma-fence-chain.
>
> Report a NULL fence and success if the seqno has already been signaled.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/dma-buf/dma-fence-chain.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index 3d123502ff12..c435bbba851c 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -99,6 +99,12 @@ 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;
> +		}
> +


Wouldn't this condition been fulfilled in the previous check? :


chain = to_dma_fence_chain(*pfence);
if (!chain || chain->base.seqno < seqno)
         return -EINVAL;

-Lionel

>   		if ((*pfence)->context != chain->base.context ||
>   		    to_dma_fence_chain(*pfence)->prev_seqno < seqno)
>   			break;
> @@ -222,6 +228,7 @@ 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.
Chris Wilson April 9, 2020, 10:52 a.m. UTC | #3
Quoting Lionel Landwerlin (2020-04-08 21:00:59)
> On 03/04/2020 12:12, Chris Wilson wrote:
> > Whenever we walk along the dma-fence-chain, we prune signaled links to
> > keep the chain nice and tidy. This leads to situations where we can
> > prune a link and report the earlier fence as the target seqno --
> > violating our own consistency checks that the seqno is not more advanced
> > than the last element in a dma-fence-chain.
> >
> > Report a NULL fence and success if the seqno has already been signaled.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/dma-buf/dma-fence-chain.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> > index 3d123502ff12..c435bbba851c 100644
> > --- a/drivers/dma-buf/dma-fence-chain.c
> > +++ b/drivers/dma-buf/dma-fence-chain.c
> > @@ -99,6 +99,12 @@ 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;
> > +             }
> > +
> 
> 
> Wouldn't this condition been fulfilled in the previous check? :
> 
> 
> chain = to_dma_fence_chain(*pfence);
> if (!chain || chain->base.seqno < seqno)
>          return -EINVAL;

The problem is in the chain iteration. It assumes that an unordered set
of fences is in the order of the user's seqno. There are no restrictions
placed on the chain, so we must apply the ordering from the timeline seqno
directly.
-Chris
Lionel Landwerlin April 9, 2020, 11:16 a.m. UTC | #4
On 09/04/2020 13:52, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2020-04-08 21:00:59)
>> On 03/04/2020 12:12, Chris Wilson wrote:
>>> Whenever we walk along the dma-fence-chain, we prune signaled links to
>>> keep the chain nice and tidy. This leads to situations where we can
>>> prune a link and report the earlier fence as the target seqno --
>>> violating our own consistency checks that the seqno is not more advanced
>>> than the last element in a dma-fence-chain.
>>>
>>> Report a NULL fence and success if the seqno has already been signaled.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    drivers/dma-buf/dma-fence-chain.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
>>> index 3d123502ff12..c435bbba851c 100644
>>> --- a/drivers/dma-buf/dma-fence-chain.c
>>> +++ b/drivers/dma-buf/dma-fence-chain.c
>>> @@ -99,6 +99,12 @@ 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;
>>> +             }
>>> +
>>
>> Wouldn't this condition been fulfilled in the previous check? :
>>
>>
>> chain = to_dma_fence_chain(*pfence);
>> if (!chain || chain->base.seqno < seqno)
>>           return -EINVAL;
> The problem is in the chain iteration. It assumes that an unordered set
> of fences is in the order of the user's seqno. There are no restrictions
> placed on the chain, so we must apply the ordering from the timeline seqno
> directly.
> -Chris


I don't really understand that. chain->seqno should be ordered because 
chain->prev_seqno <= chain->seqno.

Do you have an example where this is not the case?


-Lionel
Chris Wilson April 9, 2020, 1:46 p.m. UTC | #5
Quoting Lionel Landwerlin (2020-04-09 12:16:48)
> On 09/04/2020 13:52, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2020-04-08 21:00:59)
> >> On 03/04/2020 12:12, Chris Wilson wrote:
> >>> Whenever we walk along the dma-fence-chain, we prune signaled links to
> >>> keep the chain nice and tidy. This leads to situations where we can
> >>> prune a link and report the earlier fence as the target seqno --
> >>> violating our own consistency checks that the seqno is not more advanced
> >>> than the last element in a dma-fence-chain.
> >>>
> >>> Report a NULL fence and success if the seqno has already been signaled.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> ---
> >>>    drivers/dma-buf/dma-fence-chain.c | 7 +++++++
> >>>    1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> >>> index 3d123502ff12..c435bbba851c 100644
> >>> --- a/drivers/dma-buf/dma-fence-chain.c
> >>> +++ b/drivers/dma-buf/dma-fence-chain.c
> >>> @@ -99,6 +99,12 @@ 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;
> >>> +             }
> >>> +
> >>
> >> Wouldn't this condition been fulfilled in the previous check? :
> >>
> >>
> >> chain = to_dma_fence_chain(*pfence);
> >> if (!chain || chain->base.seqno < seqno)
> >>           return -EINVAL;
> > The problem is in the chain iteration. It assumes that an unordered set
> > of fences is in the order of the user's seqno. There are no restrictions
> > placed on the chain, so we must apply the ordering from the timeline seqno
> > directly.
> > -Chris
> 
> 
> I don't really understand that. chain->seqno should be ordered because 
> chain->prev_seqno <= chain->seqno.
> 
> Do you have an example where this is not the case?

See the failing test case.
-Chris

Patch
diff mbox series

diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
index 3d123502ff12..c435bbba851c 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -99,6 +99,12 @@  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;
@@ -222,6 +228,7 @@  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.