| Message ID | 20200403091300.14734-4-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm/i915/selftests: Add request throughput measurement to perf | expand |
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
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.
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
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
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
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.
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(+)