It is illegal to add a dma_fence_chain as timeline point. Flatten out the fences into a dma_fence_array instead.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/drm_syncobj.c | 61 ++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index c313a5b4549c..7e48dcd1bee4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); }
+ +/* + * Try to flatten a dma_fence_chain into a dma_fence_array so that it can be + * added as timeline fence to a chain again. + */ +static int drm_syncobj_flatten_chain(struct dma_fence **f) +{ + struct dma_fence_chain *chain = to_dma_fence_chain(*f); + struct dma_fence *tmp, **fences; + struct dma_fence_array *array; + unsigned int count; + + if (!chain) + return 0; + + count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + ++count; + + fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL); + if (!fences) + return -ENOMEM; + + count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + fences[count++] = dma_fence_get(tmp); + + array = dma_fence_array_create(count, fences, + dma_fence_context_alloc(1), + 1, false); + if (!array) + goto free_fences; + + dma_fence_put(*f); + *f = &array->base; + return 0; + +free_fences: + while (count--) + dma_fence_put(fences[count]); + + kfree(fences); + return -ENOMEM; +} + static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, struct drm_syncobj_transfer *args) { struct drm_syncobj *timeline_syncobj = NULL; - struct dma_fence *fence; struct dma_fence_chain *chain; + struct dma_fence *fence; int ret;
timeline_syncobj = drm_syncobj_find(file_private, args->dst_handle); @@ -869,16 +914,22 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, args->src_point, args->flags, &fence); if (ret) - goto err; + goto err_put_timeline; + + ret = drm_syncobj_flatten_chain(&fence); + if (ret) + goto err_free_fence; + chain = dma_fence_chain_alloc(); if (!chain) { ret = -ENOMEM; - goto err1; + goto err_free_fence; } + drm_syncobj_add_point(timeline_syncobj, chain, fence, args->dst_point); -err1: +err_free_fence: dma_fence_put(fence); -err: +err_put_timeline: drm_syncobj_put(timeline_syncobj);
return ret;
On 09/02/2022 20:26, Christian König wrote:
It is illegal to add a dma_fence_chain as timeline point. Flatten out the fences into a dma_fence_array instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_syncobj.c | 61 ++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index c313a5b4549c..7e48dcd1bee4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); }
+/*
- Try to flatten a dma_fence_chain into a dma_fence_array so that it can be
- added as timeline fence to a chain again.
- */
+static int drm_syncobj_flatten_chain(struct dma_fence **f) +{
- struct dma_fence_chain *chain = to_dma_fence_chain(*f);
- struct dma_fence *tmp, **fences;
- struct dma_fence_array *array;
- unsigned int count;
- if (!chain)
return 0;- count = 0;
- dma_fence_chain_for_each(tmp, &chain->base)
++count;- fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
- if (!fences)
return -ENOMEM;- count = 0;
- dma_fence_chain_for_each(tmp, &chain->base)
fences[count++] = dma_fence_get(tmp);- array = dma_fence_array_create(count, fences,
dma_fence_context_alloc(1),
Hi Christian,
Sorry for the late answer to this.
It appears this commit is trying to remove the warnings added by "dma-buf: Warn about dma_fence_chain container rules"
But the context allocation you added just above is breaking some tests. In particular igt@syncobj_timeline@transfer-timeline-point
That test transfer points into the timeline at point 3 and expects that we'll still on the previous points to complete.
In my opinion we should be reusing the previous context number if there is one and only allocate if we don't have a point.
If the application wants to not depend on previous points for wait operations, it can reset the syncobj prior to adding a new point.
Cheers,
-Lionel
1, false);- if (!array)
goto free_fences;- dma_fence_put(*f);
- *f = &array->base;
- return 0;
+free_fences:
- while (count--)
dma_fence_put(fences[count]);- kfree(fences);
- return -ENOMEM;
+}
- static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, struct drm_syncobj_transfer *args) { struct drm_syncobj *timeline_syncobj = NULL;
- struct dma_fence *fence; struct dma_fence_chain *chain;
struct dma_fence *fence; int ret;
timeline_syncobj = drm_syncobj_find(file_private, args->dst_handle);
@@ -869,16 +914,22 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, args->src_point, args->flags, &fence); if (ret)
goto err;
goto err_put_timeline;- ret = drm_syncobj_flatten_chain(&fence);
- if (ret)
goto err_free_fence;- chain = dma_fence_chain_alloc(); if (!chain) { ret = -ENOMEM;
goto err1;
goto err_free_fence;- drm_syncobj_add_point(timeline_syncobj, chain, fence, args->dst_point);
-err1: +err_free_fence: dma_fence_put(fence); -err: +err_put_timeline: drm_syncobj_put(timeline_syncobj);
return ret;
Am 25.05.22 um 08:47 schrieb Lionel Landwerlin:
On 09/02/2022 20:26, Christian König wrote:
It is illegal to add a dma_fence_chain as timeline point. Flatten out the fences into a dma_fence_array instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_syncobj.c | 61 ++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index c313a5b4549c..7e48dcd1bee4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); } + +/*
- Try to flatten a dma_fence_chain into a dma_fence_array so that
it can be
- added as timeline fence to a chain again.
- */
+static int drm_syncobj_flatten_chain(struct dma_fence **f) +{ + struct dma_fence_chain *chain = to_dma_fence_chain(*f); + struct dma_fence *tmp, **fences; + struct dma_fence_array *array; + unsigned int count;
+ if (!chain) + return 0;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + ++count;
+ fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL); + if (!fences) + return -ENOMEM;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + fences[count++] = dma_fence_get(tmp);
+ array = dma_fence_array_create(count, fences, + dma_fence_context_alloc(1),
Hi Christian,
Sorry for the late answer to this.
It appears this commit is trying to remove the warnings added by "dma-buf: Warn about dma_fence_chain container rules"
Yes, correct. We are now enforcing some rules with warnings and this here bubbled up.
But the context allocation you added just above is breaking some tests. In particular igt@syncobj_timeline@transfer-timeline-point
That test transfer points into the timeline at point 3 and expects that we'll still on the previous points to complete.
Hui what? I don't understand the problem you are seeing here. What exactly is the test doing?
In my opinion we should be reusing the previous context number if there is one and only allocate if we don't have a point.
Scratching my head what you mean with that. The functionality transfers a synchronization fence from one timeline to another.
So as far as I can see the new point should be part of the timeline of the syncobj we are transferring to.
If the application wants to not depend on previous points for wait operations, it can reset the syncobj prior to adding a new point.
Well we should never lose synchronization. So what happens is that when we do the transfer all the fences of the source are flattened out into an array. And that array is then added as new point into the destination timeline.
Where exactly is the problem?
Regards, Christian.
Cheers,
-Lionel
+ 1, false); + if (!array) + goto free_fences;
+ dma_fence_put(*f); + *f = &array->base; + return 0;
+free_fences: + while (count--) + dma_fence_put(fences[count]);
+ kfree(fences); + return -ENOMEM; +}
static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, struct drm_syncobj_transfer *args) { struct drm_syncobj *timeline_syncobj = NULL; - struct dma_fence *fence; struct dma_fence_chain *chain; + struct dma_fence *fence; int ret; timeline_syncobj = drm_syncobj_find(file_private, args->dst_handle); @@ -869,16 +914,22 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, args->src_point, args->flags, &fence); if (ret) - goto err; + goto err_put_timeline;
+ ret = drm_syncobj_flatten_chain(&fence); + if (ret) + goto err_free_fence;
chain = dma_fence_chain_alloc(); if (!chain) { ret = -ENOMEM; - goto err1; + goto err_free_fence; }
drm_syncobj_add_point(timeline_syncobj, chain, fence, args->dst_point); -err1: +err_free_fence: dma_fence_put(fence); -err: +err_put_timeline: drm_syncobj_put(timeline_syncobj); return ret;
On 25/05/2022 11:24, Christian König wrote:
Am 25.05.22 um 08:47 schrieb Lionel Landwerlin:
On 09/02/2022 20:26, Christian König wrote:
It is illegal to add a dma_fence_chain as timeline point. Flatten out the fences into a dma_fence_array instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_syncobj.c | 61 ++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index c313a5b4549c..7e48dcd1bee4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); } + +/*
- Try to flatten a dma_fence_chain into a dma_fence_array so that
it can be
- added as timeline fence to a chain again.
- */
+static int drm_syncobj_flatten_chain(struct dma_fence **f) +{ + struct dma_fence_chain *chain = to_dma_fence_chain(*f); + struct dma_fence *tmp, **fences; + struct dma_fence_array *array; + unsigned int count;
+ if (!chain) + return 0;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + ++count;
+ fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL); + if (!fences) + return -ENOMEM;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + fences[count++] = dma_fence_get(tmp);
+ array = dma_fence_array_create(count, fences, + dma_fence_context_alloc(1),
Hi Christian,
Sorry for the late answer to this.
It appears this commit is trying to remove the warnings added by "dma-buf: Warn about dma_fence_chain container rules"
Yes, correct. We are now enforcing some rules with warnings and this here bubbled up.
But the context allocation you added just above is breaking some tests. In particular igt@syncobj_timeline@transfer-timeline-point
That test transfer points into the timeline at point 3 and expects that we'll still on the previous points to complete.
Hui what? I don't understand the problem you are seeing here. What exactly is the test doing?
In my opinion we should be reusing the previous context number if there is one and only allocate if we don't have a point.
Scratching my head what you mean with that. The functionality transfers a synchronization fence from one timeline to another.
So as far as I can see the new point should be part of the timeline of the syncobj we are transferring to.
If the application wants to not depend on previous points for wait operations, it can reset the syncobj prior to adding a new point.
Well we should never lose synchronization. So what happens is that when we do the transfer all the fences of the source are flattened out into an array. And that array is then added as new point into the destination timeline.
In this case would be broken :
syncobjA <- signal point 1
syncobjA <- import syncobjB point 1 into syncobjA point 2
syncobjA <- query returns 0
-Lionel
Where exactly is the problem?
Regards, Christian.
Cheers,
-Lionel
+ 1, false); + if (!array) + goto free_fences;
+ dma_fence_put(*f); + *f = &array->base; + return 0;
+free_fences: + while (count--) + dma_fence_put(fences[count]);
+ kfree(fences); + return -ENOMEM; +}
static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, struct drm_syncobj_transfer *args) { struct drm_syncobj *timeline_syncobj = NULL; - struct dma_fence *fence; struct dma_fence_chain *chain; + struct dma_fence *fence; int ret; timeline_syncobj = drm_syncobj_find(file_private, args->dst_handle); @@ -869,16 +914,22 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, args->src_point, args->flags, &fence); if (ret) - goto err; + goto err_put_timeline;
+ ret = drm_syncobj_flatten_chain(&fence); + if (ret) + goto err_free_fence;
chain = dma_fence_chain_alloc(); if (!chain) { ret = -ENOMEM; - goto err1; + goto err_free_fence; }
drm_syncobj_add_point(timeline_syncobj, chain, fence, args->dst_point); -err1: +err_free_fence: dma_fence_put(fence); -err: +err_put_timeline: drm_syncobj_put(timeline_syncobj); return ret;
On 25/05/2022 12:26, Lionel Landwerlin wrote:
On 25/05/2022 11:24, Christian König wrote:
Am 25.05.22 um 08:47 schrieb Lionel Landwerlin:
On 09/02/2022 20:26, Christian König wrote:
It is illegal to add a dma_fence_chain as timeline point. Flatten out the fences into a dma_fence_array instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_syncobj.c | 61 ++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index c313a5b4549c..7e48dcd1bee4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); } + +/*
- Try to flatten a dma_fence_chain into a dma_fence_array so that
it can be
- added as timeline fence to a chain again.
- */
+static int drm_syncobj_flatten_chain(struct dma_fence **f) +{ + struct dma_fence_chain *chain = to_dma_fence_chain(*f); + struct dma_fence *tmp, **fences; + struct dma_fence_array *array; + unsigned int count;
+ if (!chain) + return 0;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + ++count;
+ fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL); + if (!fences) + return -ENOMEM;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + fences[count++] = dma_fence_get(tmp);
+ array = dma_fence_array_create(count, fences, + dma_fence_context_alloc(1),
Hi Christian,
Sorry for the late answer to this.
It appears this commit is trying to remove the warnings added by "dma-buf: Warn about dma_fence_chain container rules"
Yes, correct. We are now enforcing some rules with warnings and this here bubbled up.
But the context allocation you added just above is breaking some tests. In particular igt@syncobj_timeline@transfer-timeline-point
That test transfer points into the timeline at point 3 and expects that we'll still on the previous points to complete.
Hui what? I don't understand the problem you are seeing here. What exactly is the test doing?
In my opinion we should be reusing the previous context number if there is one and only allocate if we don't have a point.
Scratching my head what you mean with that. The functionality transfers a synchronization fence from one timeline to another.
So as far as I can see the new point should be part of the timeline of the syncobj we are transferring to.
If the application wants to not depend on previous points for wait operations, it can reset the syncobj prior to adding a new point.
Well we should never lose synchronization. So what happens is that when we do the transfer all the fences of the source are flattened out into an array. And that array is then added as new point into the destination timeline.
In this case would be broken :
syncobjA <- signal point 1
syncobjA <- import syncobjB point 1 into syncobjA point 2
syncobjA <- query returns 0
-Lionel
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch, but now I have doubts that it's where the problem lies.
-Lionel
Where exactly is the problem?
Regards, Christian.
Cheers,
-Lionel
+ 1, false); + if (!array) + goto free_fences;
+ dma_fence_put(*f); + *f = &array->base; + return 0;
+free_fences: + while (count--) + dma_fence_put(fences[count]);
+ kfree(fences); + return -ENOMEM; +}
static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, struct drm_syncobj_transfer *args) { struct drm_syncobj *timeline_syncobj = NULL; - struct dma_fence *fence; struct dma_fence_chain *chain; + struct dma_fence *fence; int ret; timeline_syncobj = drm_syncobj_find(file_private, args->dst_handle); @@ -869,16 +914,22 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, args->src_point, args->flags, &fence); if (ret) - goto err; + goto err_put_timeline;
+ ret = drm_syncobj_flatten_chain(&fence); + if (ret) + goto err_free_fence;
chain = dma_fence_chain_alloc(); if (!chain) { ret = -ENOMEM; - goto err1; + goto err_free_fence; }
drm_syncobj_add_point(timeline_syncobj, chain, fence, args->dst_point); -err1: +err_free_fence: dma_fence_put(fence); -err: +err_put_timeline: drm_syncobj_put(timeline_syncobj); return ret;
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
On 25/05/2022 12:26, Lionel Landwerlin wrote:
On 25/05/2022 11:24, Christian König wrote:
Am 25.05.22 um 08:47 schrieb Lionel Landwerlin:
On 09/02/2022 20:26, Christian König wrote:
It is illegal to add a dma_fence_chain as timeline point. Flatten out the fences into a dma_fence_array instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_syncobj.c | 61 ++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index c313a5b4549c..7e48dcd1bee4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); } + +/*
- Try to flatten a dma_fence_chain into a dma_fence_array so
that it can be
- added as timeline fence to a chain again.
- */
+static int drm_syncobj_flatten_chain(struct dma_fence **f) +{ + struct dma_fence_chain *chain = to_dma_fence_chain(*f); + struct dma_fence *tmp, **fences; + struct dma_fence_array *array; + unsigned int count;
+ if (!chain) + return 0;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + ++count;
+ fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL); + if (!fences) + return -ENOMEM;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + fences[count++] = dma_fence_get(tmp);
+ array = dma_fence_array_create(count, fences, + dma_fence_context_alloc(1),
Hi Christian,
Sorry for the late answer to this.
It appears this commit is trying to remove the warnings added by "dma-buf: Warn about dma_fence_chain container rules"
Yes, correct. We are now enforcing some rules with warnings and this here bubbled up.
But the context allocation you added just above is breaking some tests. In particular igt@syncobj_timeline@transfer-timeline-point
That test transfer points into the timeline at point 3 and expects that we'll still on the previous points to complete.
Hui what? I don't understand the problem you are seeing here. What exactly is the test doing?
In my opinion we should be reusing the previous context number if there is one and only allocate if we don't have a point.
Scratching my head what you mean with that. The functionality transfers a synchronization fence from one timeline to another.
So as far as I can see the new point should be part of the timeline of the syncobj we are transferring to.
If the application wants to not depend on previous points for wait operations, it can reset the syncobj prior to adding a new point.
Well we should never lose synchronization. So what happens is that when we do the transfer all the fences of the source are flattened out into an array. And that array is then added as new point into the destination timeline.
In this case would be broken :
syncobjA <- signal point 1
syncobjA <- import syncobjB point 1 into syncobjA point 2
syncobjA <- query returns 0
-Lionel
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch, but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this, Christian.
-Lionel
Where exactly is the problem?
Regards, Christian.
Cheers,
-Lionel
+ 1, false); + if (!array) + goto free_fences;
+ dma_fence_put(*f); + *f = &array->base; + return 0;
+free_fences: + while (count--) + dma_fence_put(fences[count]);
+ kfree(fences); + return -ENOMEM; +}
static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, struct drm_syncobj_transfer *args) { struct drm_syncobj *timeline_syncobj = NULL; - struct dma_fence *fence; struct dma_fence_chain *chain; + struct dma_fence *fence; int ret; timeline_syncobj = drm_syncobj_find(file_private, args->dst_handle); @@ -869,16 +914,22 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, args->src_point, args->flags, &fence); if (ret) - goto err; + goto err_put_timeline;
+ ret = drm_syncobj_flatten_chain(&fence); + if (ret) + goto err_free_fence;
chain = dma_fence_chain_alloc(); if (!chain) { ret = -ENOMEM; - goto err1; + goto err_free_fence; }
drm_syncobj_add_point(timeline_syncobj, chain, fence, args->dst_point); -err1: +err_free_fence: dma_fence_put(fence); -err: +err_put_timeline: drm_syncobj_put(timeline_syncobj); return ret;
On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
On 25/05/2022 12:26, Lionel Landwerlin wrote:
On 25/05/2022 11:24, Christian König wrote:
Am 25.05.22 um 08:47 schrieb Lionel Landwerlin:
On 09/02/2022 20:26, Christian König wrote:
It is illegal to add a dma_fence_chain as timeline point. Flatten out the fences into a dma_fence_array instead.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_syncobj.c | 61 ++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index c313a5b4549c..7e48dcd1bee4 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); } + +/*
- Try to flatten a dma_fence_chain into a dma_fence_array
so that it can be
- added as timeline fence to a chain again.
- */
+static int drm_syncobj_flatten_chain(struct dma_fence **f) +{ + struct dma_fence_chain *chain = to_dma_fence_chain(*f); + struct dma_fence *tmp, **fences; + struct dma_fence_array *array; + unsigned int count;
+ if (!chain) + return 0;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + ++count;
+ fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL); + if (!fences) + return -ENOMEM;
+ count = 0; + dma_fence_chain_for_each(tmp, &chain->base) + fences[count++] = dma_fence_get(tmp);
+ array = dma_fence_array_create(count, fences, + dma_fence_context_alloc(1),
Hi Christian,
Sorry for the late answer to this.
It appears this commit is trying to remove the warnings added by "dma-buf: Warn about dma_fence_chain container rules"
Yes, correct. We are now enforcing some rules with warnings and this here bubbled up.
But the context allocation you added just above is breaking some tests. In particular igt@syncobj_timeline@transfer-timeline-point
That test transfer points into the timeline at point 3 and expects that we'll still on the previous points to complete.
Hui what? I don't understand the problem you are seeing here. What exactly is the test doing?
In my opinion we should be reusing the previous context number if there is one and only allocate if we don't have a point.
Scratching my head what you mean with that. The functionality transfers a synchronization fence from one timeline to another.
So as far as I can see the new point should be part of the timeline of the syncobj we are transferring to.
If the application wants to not depend on previous points for wait operations, it can reset the syncobj prior to adding a new point.
Well we should never lose synchronization. So what happens is that when we do the transfer all the fences of the source are flattened out into an array. And that array is then added as new point into the destination timeline.
In this case would be broken :
syncobjA <- signal point 1
syncobjA <- import syncobjB point 1 into syncobjA point 2
syncobjA <- query returns 0
-Lionel
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch, but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this, Christian.
With some additional prints:
[ 210.742634] Console: switching to colour dummy device 80x25 [ 210.742686] [IGT] syncobj_timeline: executing [ 210.756988] [IGT] syncobj_timeline: starting subtest transfer-timeline-point [ 210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding fence0 signaled=1 [ 210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting array fence signaled=0 [ 210.800469] [IGT] syncobj_timeline: exiting, ret=98 [ 210.825426] Console: switching to colour frame buffer device 240x67
still learning this part of the code but AFAICS the problem is because when we are creating the array, the 'signaled' doesn't propagate to the array.
dma_fence_array_create() { ... atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); ... }
This is not considering the fact that some of the fences could already have been signaled as is the case in the igt@syncobj_timeline@transfer-timeline-point test. See https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@synco...
Quick patch on this function fixes it for me:
---------8<---------------- Subject: [PATCH] dma-buf: Honor already signaled fences on array creation
When creating an array, array->num_pending is marked with the number of fences. However the fences could alredy have been signaled. Propagate num_pending to the array by looking at each individual fence the array contains.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com --- drivers/dma-buf/dma-fence-array.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 5c8a7084577b..32f491c32fa0 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -158,6 +158,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, { struct dma_fence_array *array; size_t size = sizeof(*array); + unsigned num_pending = 0; + struct dma_fence **f;
WARN_ON(!num_fences || !fences);
@@ -173,7 +175,14 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, init_irq_work(&array->work, irq_dma_fence_array_work);
array->num_fences = num_fences; - atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); + + for (f = fences; f < fences + num_fences; f++) + num_pending += !dma_fence_is_signaled(*f); + + if (signal_on_any) + num_pending = !!num_pending; + + atomic_set(&array->num_pending, num_pending); array->fences = fences;
array->base.error = PENDING_ERROR;
Am 25.05.22 um 23:59 schrieb Lucas De Marchi:
On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
[SNIP]
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch, but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this, Christian.
With some additional prints:
[ 210.742634] Console: switching to colour dummy device 80x25 [ 210.742686] [IGT] syncobj_timeline: executing [ 210.756988] [IGT] syncobj_timeline: starting subtest transfer-timeline-point [ 210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding fence0 signaled=1 [ 210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting array fence signaled=0 [ 210.800469] [IGT] syncobj_timeline: exiting, ret=98 [ 210.825426] Console: switching to colour frame buffer device 240x67
still learning this part of the code but AFAICS the problem is because when we are creating the array, the 'signaled' doesn't propagate to the array.
Yeah, but that is intentionally. The array should only signal when requested.
I still don't get what the test case here is checking.
Regards, Christian.
dma_fence_array_create() { ... atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); ... }
This is not considering the fact that some of the fences could already have been signaled as is the case in the igt@syncobj_timeline@transfer-timeline-point test. See https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@synco...
Quick patch on this function fixes it for me:
---------8<---------------- Subject: [PATCH] dma-buf: Honor already signaled fences on array creation
When creating an array, array->num_pending is marked with the number of fences. However the fences could alredy have been signaled. Propagate num_pending to the array by looking at each individual fence the array contains.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/dma-buf/dma-fence-array.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 5c8a7084577b..32f491c32fa0 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -158,6 +158,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, { struct dma_fence_array *array; size_t size = sizeof(*array); + unsigned num_pending = 0; + struct dma_fence **f;
WARN_ON(!num_fences || !fences);
@@ -173,7 +175,14 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, init_irq_work(&array->work, irq_dma_fence_array_work);
array->num_fences = num_fences; - atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
+ for (f = fences; f < fences + num_fences; f++) + num_pending += !dma_fence_is_signaled(*f);
+ if (signal_on_any) + num_pending = !!num_pending;
+ atomic_set(&array->num_pending, num_pending); array->fences = fences;
array->base.error = PENDING_ERROR;
On 30/05/2022 12:52, Christian König wrote:
Am 25.05.22 um 23:59 schrieb Lucas De Marchi:
On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
[SNIP]
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch, but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this, Christian.
With some additional prints:
[ 210.742634] Console: switching to colour dummy device 80x25 [ 210.742686] [IGT] syncobj_timeline: executing [ 210.756988] [IGT] syncobj_timeline: starting subtest transfer-timeline-point [ 210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding fence0 signaled=1 [ 210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting array fence signaled=0 [ 210.800469] [IGT] syncobj_timeline: exiting, ret=98 [ 210.825426] Console: switching to colour frame buffer device 240x67
still learning this part of the code but AFAICS the problem is because when we are creating the array, the 'signaled' doesn't propagate to the array.
Yeah, but that is intentionally. The array should only signal when requested.
I still don't get what the test case here is checking.
There must be something I don't know about fence arrays.
You seem to say that creating an array of signaled fences will not make the array signaled.
This is the situation with this IGT test.
We started with a syncobj with point 1 & 2 signaled.
We take point 2 and import it as a new point 3 on the same syncobj.
We expect point 3 to be signaled as well and it's not.
Thanks,
-Lionel
Regards, Christian.
dma_fence_array_create() { ... atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); ... }
This is not considering the fact that some of the fences could already have been signaled as is the case in the igt@syncobj_timeline@transfer-timeline-point test. See https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@synco...
Quick patch on this function fixes it for me:
---------8<---------------- Subject: [PATCH] dma-buf: Honor already signaled fences on array creation
When creating an array, array->num_pending is marked with the number of fences. However the fences could alredy have been signaled. Propagate num_pending to the array by looking at each individual fence the array contains.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/dma-buf/dma-fence-array.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 5c8a7084577b..32f491c32fa0 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -158,6 +158,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, { struct dma_fence_array *array; size_t size = sizeof(*array); + unsigned num_pending = 0; + struct dma_fence **f;
WARN_ON(!num_fences || !fences);
@@ -173,7 +175,14 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, init_irq_work(&array->work, irq_dma_fence_array_work);
array->num_fences = num_fences; - atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
+ for (f = fences; f < fences + num_fences; f++) + num_pending += !dma_fence_is_signaled(*f);
+ if (signal_on_any) + num_pending = !!num_pending;
+ atomic_set(&array->num_pending, num_pending); array->fences = fences;
array->base.error = PENDING_ERROR;
Am 30.05.22 um 12:09 schrieb Lionel Landwerlin:
On 30/05/2022 12:52, Christian König wrote:
Am 25.05.22 um 23:59 schrieb Lucas De Marchi:
On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
[SNIP]
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch, but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this, Christian.
With some additional prints:
[ 210.742634] Console: switching to colour dummy device 80x25 [ 210.742686] [IGT] syncobj_timeline: executing [ 210.756988] [IGT] syncobj_timeline: starting subtest transfer-timeline-point [ 210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding fence0 signaled=1 [ 210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting array fence signaled=0 [ 210.800469] [IGT] syncobj_timeline: exiting, ret=98 [ 210.825426] Console: switching to colour frame buffer device 240x67
still learning this part of the code but AFAICS the problem is because when we are creating the array, the 'signaled' doesn't propagate to the array.
Yeah, but that is intentionally. The array should only signal when requested.
I still don't get what the test case here is checking.
There must be something I don't know about fence arrays.
You seem to say that creating an array of signaled fences will not make the array signaled.
Exactly that, yes. The array delays it's signaling until somebody asks for it.
In other words the fences inside the array are check only after someone calls dma_fence_enable_sw_signaling() which in turn calls dma_fence_array_enable_signaling().
It is certainly possible that nobody does that in the drm_syncobj and because of this the array never signals.
Regards, Christian.
This is the situation with this IGT test.
We started with a syncobj with point 1 & 2 signaled.
We take point 2 and import it as a new point 3 on the same syncobj.
We expect point 3 to be signaled as well and it's not.
Thanks,
-Lionel
Regards, Christian.
dma_fence_array_create() { ... atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); ... }
This is not considering the fact that some of the fences could already have been signaled as is the case in the igt@syncobj_timeline@transfer-timeline-point test. See https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@synco...
Quick patch on this function fixes it for me:
---------8<---------------- Subject: [PATCH] dma-buf: Honor already signaled fences on array creation
When creating an array, array->num_pending is marked with the number of fences. However the fences could alredy have been signaled. Propagate num_pending to the array by looking at each individual fence the array contains.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/dma-buf/dma-fence-array.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 5c8a7084577b..32f491c32fa0 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -158,6 +158,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, { struct dma_fence_array *array; size_t size = sizeof(*array); + unsigned num_pending = 0; + struct dma_fence **f;
WARN_ON(!num_fences || !fences);
@@ -173,7 +175,14 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, init_irq_work(&array->work, irq_dma_fence_array_work);
array->num_fences = num_fences; - atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
+ for (f = fences; f < fences + num_fences; f++) + num_pending += !dma_fence_is_signaled(*f);
+ if (signal_on_any) + num_pending = !!num_pending;
+ atomic_set(&array->num_pending, num_pending); array->fences = fences;
array->base.error = PENDING_ERROR;
On 30/05/2022 14:40, Christian König wrote:
Am 30.05.22 um 12:09 schrieb Lionel Landwerlin:
On 30/05/2022 12:52, Christian König wrote:
Am 25.05.22 um 23:59 schrieb Lucas De Marchi:
On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
[SNIP]
Err... Let's double check with my colleagues.
It seems we're running into a test failure in IGT with this patch, but now I have doubts that it's where the problem lies.
Yeah, exactly that's what I couldn't understand as well.
What you describe above should still work fine.
Thanks for taking a look into this, Christian.
With some additional prints:
[ 210.742634] Console: switching to colour dummy device 80x25 [ 210.742686] [IGT] syncobj_timeline: executing [ 210.756988] [IGT] syncobj_timeline: starting subtest transfer-timeline-point [ 210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding fence0 signaled=1 [ 210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting array fence signaled=0 [ 210.800469] [IGT] syncobj_timeline: exiting, ret=98 [ 210.825426] Console: switching to colour frame buffer device 240x67
still learning this part of the code but AFAICS the problem is because when we are creating the array, the 'signaled' doesn't propagate to the array.
Yeah, but that is intentionally. The array should only signal when requested.
I still don't get what the test case here is checking.
There must be something I don't know about fence arrays.
You seem to say that creating an array of signaled fences will not make the array signaled.
Exactly that, yes. The array delays it's signaling until somebody asks for it.
In other words the fences inside the array are check only after someone calls dma_fence_enable_sw_signaling() which in turn calls dma_fence_array_enable_signaling().
It is certainly possible that nobody does that in the drm_syncobj and because of this the array never signals.
Regards, Christian.
Thanks,
Yeah I guess dma_fence_enable_sw_signaling() is never called for sw_sync.
Don't we also want to call it right at the end of drm_syncobj_flatten_chain() ?
-Lionel
This is the situation with this IGT test.
We started with a syncobj with point 1 & 2 signaled.
We take point 2 and import it as a new point 3 on the same syncobj.
We expect point 3 to be signaled as well and it's not.
Thanks,
-Lionel
Regards, Christian.
dma_fence_array_create() { ... atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); ... }
This is not considering the fact that some of the fences could already have been signaled as is the case in the igt@syncobj_timeline@transfer-timeline-point test. See https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@synco...
Quick patch on this function fixes it for me:
---------8<---------------- Subject: [PATCH] dma-buf: Honor already signaled fences on array creation
When creating an array, array->num_pending is marked with the number of fences. However the fences could alredy have been signaled. Propagate num_pending to the array by looking at each individual fence the array contains.
Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
drivers/dma-buf/dma-fence-array.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 5c8a7084577b..32f491c32fa0 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -158,6 +158,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, { struct dma_fence_array *array; size_t size = sizeof(*array); + unsigned num_pending = 0; + struct dma_fence **f;
WARN_ON(!num_fences || !fences);
@@ -173,7 +175,14 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, init_irq_work(&array->work, irq_dma_fence_array_work);
array->num_fences = num_fences; - atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
+ for (f = fences; f < fences + num_fences; f++) + num_pending += !dma_fence_is_signaled(*f);
+ if (signal_on_any) + num_pending = !!num_pending;
+ atomic_set(&array->num_pending, num_pending); array->fences = fences;
array->base.error = PENDING_ERROR;
dri-devel@lists.freedesktop.org
-
Christian König -
Lionel Landwerlin -
Lucas De Marchi