Re: [Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Am 04.10.21 um 12:50 schrieb Tvrtko Ursulin:
On 04/10/2021 11:44, Christian König wrote:
Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin:
On 04/10/2021 10:53, Christian König wrote:
Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin:
On 01/10/2021 11:05, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100
+
include/linux/dma-resv.h | 95
+++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct
dma_resv *obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor
object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter
*cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not
yet signaled.
+ * The returned fence has an extra local reference so will stay
alive.
+ * If a concurrent modify is detected the whole iteration is
started over again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter
*cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
Worth having an assert dma_fence_get_rcu does not fail here? Not
sure that I have seen debug build only asserts though on the DRM
core side.
That won't work. It's perfectly valid for dma_fence_get_rcu() to
return NULL when we are racing here. Keep in mind that we don't
hold any locks.
Ah yes.. No need to change anything then, sorry for the confusion. I
did not find any holes, the rest was just about how to maybe make
the flow more obvious. Let me know if you want r-b now or later.
Now would be good. I've tried to make that more cleaner, but this
only lead to repeating the code more often.
Reviewed-by: Tvrtko Ursulin
Thanks, but what about the rest?
The selftests in this version still have some bugs which I already
fixed, but I think we could push most of the set.
Christian.
Regards,
Tvrtko
Regards,
Christian.
Regards,
Tvrtko
What we could do is to return NULL and repeat with a new sequence
immediately though.
On the bike shedding front, would it be clearer if the continue
condition on signaled fences was standalone, using the continue
statement? I'd also possibly re-arrange the three if-else blocks
so that the end of iteration is not sandwiched between blocks
handling exclusive and shared, and flow tweaked a bit, like:
struct dma_fence *fence = cursor->fence;
int index = cursor->index;
dma_fence_put(fence);
fence = NULL;
next:
if (index == -1) {
/* Try picking the exclusive fence. */
index++;
fence = dma_resv_excl_fence(obj);
if (!fence)
goto next;
} else if (cursor->fences && index <
cursor->fences->shared_count) {
/* Try picking next shared fence. */
struct dma_resv_list *fences = cursor->fences;
fence =
Re: [Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 04/10/2021 13:59, Christian König wrote:
Am 04.10.21 um 12:50 schrieb Tvrtko Ursulin:
On 04/10/2021 11:44, Christian König wrote:
Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin:
On 04/10/2021 10:53, Christian König wrote:
Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin:
On 01/10/2021 11:05, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100
+
include/linux/dma-resv.h | 95
+++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct
dma_resv *obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor
object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter
*cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not
yet signaled.
+ * The returned fence has an extra local reference so will stay
alive.
+ * If a concurrent modify is detected the whole iteration is
started over again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter
*cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
Worth having an assert dma_fence_get_rcu does not fail here? Not
sure that I have seen debug build only asserts though on the DRM
core side.
That won't work. It's perfectly valid for dma_fence_get_rcu() to
return NULL when we are racing here. Keep in mind that we don't
hold any locks.
Ah yes.. No need to change anything then, sorry for the confusion. I
did not find any holes, the rest was just about how to maybe make
the flow more obvious. Let me know if you want r-b now or later.
Now would be good. I've tried to make that more cleaner, but this
only lead to repeating the code more often.
Reviewed-by: Tvrtko Ursulin
Thanks, but what about the rest?
I'll go through the core patches, it just taking time.
i915 patches, again, I'd prefer you drop the busy ioctl but at least you
have i915_request_await_object as a pilot. The rest of i915 I'd prefer
someone who knows the display paths can answer whether locked or
unlocked iterator is the right one.
The selftests in this version still have some bugs which I already
fixed, but I think we could push most of the set.
Ah.. I just replied on that one.
Regards,
Tvrtko
Christian.
Regards,
Tvrtko
Regards,
Christian.
Regards,
Tvrtko
What we could do is to return NULL and repeat with a new sequence
immediately though.
On the bike shedding front, would it be clearer if the continue
condition on signaled fences was standalone, using the continue
statement? I'd also possibly re-arrange the three if-else blocks
so that the end of iteration is not sandwiched between blocks
handling exclusive and shared, and flow tweaked a bit, like:
struct dma_fence *fence = cursor->fence;
Re: [Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin:
On 01/10/2021 11:05, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100 +
include/linux/dma-resv.h | 95 +++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv
*obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter
*cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not yet
signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iteration is started
over again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
Worth having an assert dma_fence_get_rcu does not fail here? Not sure
that I have seen debug build only asserts though on the DRM core side.
That won't work. It's perfectly valid for dma_fence_get_rcu() to return
NULL when we are racing here. Keep in mind that we don't hold any locks.
What we could do is to return NULL and repeat with a new sequence
immediately though.
On the bike shedding front, would it be clearer if the continue
condition on signaled fences was standalone, using the continue
statement? I'd also possibly re-arrange the three if-else blocks so
that the end of iteration is not sandwiched between blocks handling
exclusive and shared, and flow tweaked a bit, like:
struct dma_fence *fence = cursor->fence;
int index = cursor->index;
dma_fence_put(fence);
fence = NULL;
next:
if (index == -1) {
/* Try picking the exclusive fence. */
index++;
fence = dma_resv_excl_fence(obj);
if (!fence)
goto next;
} else if (cursor->fences && index < cursor->fences->shared_count) {
/* Try picking next shared fence. */
struct dma_resv_list *fences = cursor->fences;
fence = rcu_dereference(fences->shared[index++]);
}
if (fence) {
if (dma_fence_is_signaled(fence))
goto next; /* Skip signaled. */
fence = dma_fence_get_rcu(fence);
WARN_ON(!fence);
}
cursor->fence = fence;
cursor->index = index;
(I started with a loop here but ended with goto based flow since it
ended up more succinct.)
At least if I don't have a handling flaw in there it looks like easier
to follow flow to me. Plus picking a not signaled fence works without
a reference FWIW.
I strongly don't think that this will work correctly. You need to grab a
reference first when you want to call dma_fence_is_signaled(), that's
why I used the testbit approach initially.
How does it look to you?
Mhm, let me try to reorder the loop once
Re: [Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin:
On 04/10/2021 10:53, Christian König wrote:
Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin:
On 01/10/2021 11:05, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100
+
include/linux/dma-resv.h | 95 +++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv
*obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter
*cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not yet
signaled.
+ * The returned fence has an extra local reference so will stay
alive.
+ * If a concurrent modify is detected the whole iteration is
started over again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
Worth having an assert dma_fence_get_rcu does not fail here? Not
sure that I have seen debug build only asserts though on the DRM
core side.
That won't work. It's perfectly valid for dma_fence_get_rcu() to
return NULL when we are racing here. Keep in mind that we don't hold
any locks.
Ah yes.. No need to change anything then, sorry for the confusion. I
did not find any holes, the rest was just about how to maybe make the
flow more obvious. Let me know if you want r-b now or later.
Now would be good. I've tried to make that more cleaner, but this only
lead to repeating the code more often.
Regards,
Christian.
Regards,
Tvrtko
What we could do is to return NULL and repeat with a new sequence
immediately though.
On the bike shedding front, would it be clearer if the continue
condition on signaled fences was standalone, using the continue
statement? I'd also possibly re-arrange the three if-else blocks so
that the end of iteration is not sandwiched between blocks handling
exclusive and shared, and flow tweaked a bit, like:
struct dma_fence *fence = cursor->fence;
int index = cursor->index;
dma_fence_put(fence);
fence = NULL;
next:
if (index == -1) {
/* Try picking the exclusive fence. */
index++;
fence = dma_resv_excl_fence(obj);
if (!fence)
goto next;
} else if (cursor->fences && index < cursor->fences->shared_count) {
/* Try picking next shared fence. */
struct dma_resv_list *fences = cursor->fences;
fence = rcu_dereference(fences->shared[index++]);
}
if (fence) {
if (dma_fence_is_signaled(fence))
goto next; /* Skip signaled. */
fence = dma_fence_get_rcu(fence);
WARN_ON(!fence);
}
cursor->fence = fence;
cursor->index = index;
(I started with a loop here but ended with goto based flow since
Re: [Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 04/10/2021 11:44, Christian König wrote:
Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin:
On 04/10/2021 10:53, Christian König wrote:
Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin:
On 01/10/2021 11:05, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100
+
include/linux/dma-resv.h | 95 +++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv
*obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter
*cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not yet
signaled.
+ * The returned fence has an extra local reference so will stay
alive.
+ * If a concurrent modify is detected the whole iteration is
started over again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
Worth having an assert dma_fence_get_rcu does not fail here? Not
sure that I have seen debug build only asserts though on the DRM
core side.
That won't work. It's perfectly valid for dma_fence_get_rcu() to
return NULL when we are racing here. Keep in mind that we don't hold
any locks.
Ah yes.. No need to change anything then, sorry for the confusion. I
did not find any holes, the rest was just about how to maybe make the
flow more obvious. Let me know if you want r-b now or later.
Now would be good. I've tried to make that more cleaner, but this only
lead to repeating the code more often.
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
Regards,
Christian.
Regards,
Tvrtko
What we could do is to return NULL and repeat with a new sequence
immediately though.
On the bike shedding front, would it be clearer if the continue
condition on signaled fences was standalone, using the continue
statement? I'd also possibly re-arrange the three if-else blocks so
that the end of iteration is not sandwiched between blocks handling
exclusive and shared, and flow tweaked a bit, like:
struct dma_fence *fence = cursor->fence;
int index = cursor->index;
dma_fence_put(fence);
fence = NULL;
next:
if (index == -1) {
/* Try picking the exclusive fence. */
index++;
fence = dma_resv_excl_fence(obj);
if (!fence)
goto next;
} else if (cursor->fences && index < cursor->fences->shared_count) {
/* Try picking next shared fence. */
struct dma_resv_list *fences = cursor->fences;
fence = rcu_dereference(fences->shared[index++]);
}
if (fence) {
if (dma_fence_is_signaled(fence))
goto next; /* Skip signaled. */
fence = dma_fence_get_rcu(fence);
WARN_ON(!fence);
}
cursor->fence =
Re: [Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 04/10/2021 10:53, Christian König wrote:
Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin:
On 01/10/2021 11:05, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100 +
include/linux/dma-resv.h | 95 +++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv
*obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter
*cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not yet
signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iteration is started
over again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
Worth having an assert dma_fence_get_rcu does not fail here? Not sure
that I have seen debug build only asserts though on the DRM core side.
That won't work. It's perfectly valid for dma_fence_get_rcu() to return
NULL when we are racing here. Keep in mind that we don't hold any locks.
Ah yes.. No need to change anything then, sorry for the confusion. I did
not find any holes, the rest was just about how to maybe make the flow
more obvious. Let me know if you want r-b now or later.
Regards,
Tvrtko
What we could do is to return NULL and repeat with a new sequence
immediately though.
On the bike shedding front, would it be clearer if the continue
condition on signaled fences was standalone, using the continue
statement? I'd also possibly re-arrange the three if-else blocks so
that the end of iteration is not sandwiched between blocks handling
exclusive and shared, and flow tweaked a bit, like:
struct dma_fence *fence = cursor->fence;
int index = cursor->index;
dma_fence_put(fence);
fence = NULL;
next:
if (index == -1) {
/* Try picking the exclusive fence. */
index++;
fence = dma_resv_excl_fence(obj);
if (!fence)
goto next;
} else if (cursor->fences && index < cursor->fences->shared_count) {
/* Try picking next shared fence. */
struct dma_resv_list *fences = cursor->fences;
fence = rcu_dereference(fences->shared[index++]);
}
if (fence) {
if (dma_fence_is_signaled(fence))
goto next; /* Skip signaled. */
fence = dma_fence_get_rcu(fence);
WARN_ON(!fence);
}
cursor->fence = fence;
cursor->index = index;
(I started with a loop here but ended with goto based flow since it
ended up more succinct.)
At least if I don't have a handling flaw in there it looks like easier
to follow flow to me. Plus picking a not signaled fence works without
a
Re: [Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 01/10/2021 11:05, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100 +
include/linux/dma-resv.h | 95 +++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct
dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not yet signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iteration is started over
again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
Worth having an assert dma_fence_get_rcu does not fail here? Not sure
that I have seen debug build only asserts though on the DRM core side.
On the bike shedding front, would it be clearer if the continue
condition on signaled fences was standalone, using the continue
statement? I'd also possibly re-arrange the three if-else blocks so that
the end of iteration is not sandwiched between blocks handling exclusive
and shared, and flow tweaked a bit, like:
struct dma_fence *fence = cursor->fence;
int index = cursor->index;
dma_fence_put(fence);
fence = NULL;
next:
if (index == -1) {
/* Try picking the exclusive fence. */
index++;
fence = dma_resv_excl_fence(obj);
if (!fence)
goto next;
} else if (cursor->fences && index < cursor->fences->shared_count) {
/* Try picking next shared fence. */
struct dma_resv_list *fences = cursor->fences;
fence = rcu_dereference(fences->shared[index++]);
}
if (fence) {
if (dma_fence_is_signaled(fence))
goto next; /* Skip signaled. */
fence = dma_fence_get_rcu(fence);
WARN_ON(!fence);
}
cursor->fence = fence;
cursor->index = index;
(I started with a loop here but ended with goto based flow since it
ended up more succinct.)
At least if I don't have a handling flaw in there it looks like easier
to follow flow to me. Plus picking a not signaled fence works without a
reference FWIW. How does it look to you?
Regards,
Tvrtko
+ if (!cursor->fence || !dma_fence_is_signaled(cursor->fence))
+ break;
+ } while (true);
+}
+
+/**
+ * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj.
+ * @cursor: the cursor with
[Intel-gfx] [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
v3: restructor the code, move rcu_read_lock()/unlock() into the
iterator, add dma_resv_iter_is_restarted()
v4: fix NULL deref when no explicit fence exists, drop superflous
rcu_read_lock()/unlock() calls.
v5: fix typos in the documentation
v6: fix coding error when excl fence is NULL
v7: one more logic fix
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 100 +
include/linux/dma-resv.h | 95 +++
2 files changed, 195 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3cbcf66a137e 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct
dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_restart_unlocked - restart the unlocked iterator
+ * @cursor: The dma_resv_iter object to restart
+ *
+ * Restart the unlocked iteration by initializing the cursor object.
+ */
+static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor)
+{
+ cursor->seq = read_seqcount_begin(>obj->seq);
+ cursor->index = -1;
+ if (cursor->all_fences)
+ cursor->fences = dma_resv_shared_list(cursor->obj);
+ else
+ cursor->fences = NULL;
+ cursor->is_restarted = true;
+}
+
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object which are not yet signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iteration is started over
again.
+ */
+static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ if (cursor->index == -1) {
+ cursor->fence = dma_resv_excl_fence(obj);
+ cursor->index++;
+ if (!cursor->fence)
+ continue;
+
+ } else if (!cursor->fences ||
+ cursor->index >= cursor->fences->shared_count) {
+ cursor->fence = NULL;
+ break;
+
+ } else {
+ struct dma_resv_list *fences = cursor->fences;
+ unsigned int idx = cursor->index++;
+
+ cursor->fence = rcu_dereference(fences->shared[idx]);
+ }
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
+ if (!cursor->fence || !dma_fence_is_signaled(cursor->fence))
+ break;
+ } while (true);
+}
+
+/**
+ * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj.
+ * @cursor: the cursor with the current position
+ *
+ * Returns the first fence from an unlocked dma_resv obj.
+ */
+struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor)
+{
+ rcu_read_lock();
+ do {
+ dma_resv_iter_restart_unlocked(cursor);
+ dma_resv_iter_walk_unlocked(cursor);
+ } while (read_seqcount_retry(>obj->seq, cursor->seq));
+ rcu_read_unlock();
+
+ return cursor->fence;
+}
+EXPORT_SYMBOL(dma_resv_iter_first_unlocked);
+
+/**
+ * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv obj.
+ * @cursor: the cursor with the current position
+ *
+ * Returns the next fence from an unlocked dma_resv obj.
+ */
+struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor)
+{
+ bool restart;
+
+ rcu_read_lock();
+ cursor->is_restarted = false;
+ restart = read_seqcount_retry(>obj->seq, cursor->seq);
+ do {
+ if (restart)
+ dma_resv_iter_restart_unlocked(cursor);
+ dma_resv_iter_walk_unlocked(cursor);
+ restart = true;
+ } while (read_seqcount_retry(>obj->seq, cursor->seq));
+ rcu_read_unlock();
+
+ return cursor->fence;
+}
+EXPORT_SYMBOL(dma_resv_iter_next_unlocked);
+
/**
* dma_resv_copy_fences - Copy all fences from src to dst.
* @dst: the destination reservation object
diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
index 9100dd3dc21f..5d7d28cb9008 100644
--- a/include/linux/dma-resv.h
+++ b/include/linux/dma-resv.h
@@ -149,6 +149,101 @@ struct
