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.
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);
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);
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
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
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
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
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
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.free...
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 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 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 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.
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Christian König -
Daniel Vetter -
Dave Airlie -
Jani Nikula -
Lionel Landwerlin -
Sumit Semwal