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@xxxxxxx>
---
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;
