Re: [Intel-gfx] [PATCH 1/2] Revert "dma-buf: Report signaled links inside dma-fence-chain"
Hi Christian, On Fri, 26 Jun 2020, 18:10 Daniel Vetter, wrote: > On Fri, Jun 26, 2020 at 9:03 AM Christian König > wrote: > > > > Am 26.06.20 um 06:43 schrieb Sumit Semwal: > > > On Fri, 26 Jun 2020 at 01:24, Daniel Vetter wrote: > > >> Ignoring everything else ... > > >> > > >> On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula < > jani.nik...@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. ___ 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"
On Fri, Jun 26, 2020 at 9:03 AM Christian König wrote: > > Am 26.06.20 um 06:43 schrieb Sumit Semwal: > > On Fri, 26 Jun 2020 at 01:24, Daniel Vetter wrote: > >> Ignoring everything else ... > >> > >> On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula > >> 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 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"
Am 26.06.20 um 06:43 schrieb Sumit Semwal: On Fri, 26 Jun 2020 at 01:24, Daniel Vetter wrote: Ignoring everything else ... On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula 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 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"
On Fri, 26 Jun 2020 at 01:24, Daniel Vetter wrote: > > Ignoring everything else ... > > On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula > 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 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"
On Fri, 26 Jun 2020 at 05:27, Jani Nikula wrote: > > On Fri, 26 Jun 2020, Dave Airlie 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"
Ignoring everything else ... On Thu, Jun 25, 2020 at 9:28 PM Jani Nikula 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 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"
On Fri, 26 Jun 2020, Dave Airlie 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
> Tested-by: Venkata Sandeep Dhanalakota
> Reviewed-by: Venkata Sandeep Dhanalakota
>
>
> On Thu, 25 Jun 2020 at 22: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
>>
>> Reviewed-by: Christian König
>>
>> > ---
>> > 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, >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-...@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"
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
Tested-by: Venkata Sandeep Dhanalakota
Reviewed-by: Venkata Sandeep Dhanalakota
On Thu, 25 Jun 2020 at 22: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
>
> Reviewed-by: Christian König
>
> > ---
> > 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, >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-...@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
