| Message ID | 20200625123443.19680-1-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain" | expand |
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.
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
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
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
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.
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.
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.
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. >
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.
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. >
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. >> >
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.
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(-)