Re: [PATCH 02/28] dma-buf: add dma_resv_for_each_fence v2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/10/2021 09:40, Tvrtko Ursulin wrote:

On 05/10/2021 12:37, Christian König wrote:
A simpler version of the iterator to be used when the dma_resv object is
locked.

v2: fix index check here as well

Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
  drivers/dma-buf/dma-resv.c | 49 ++++++++++++++++++++++++++++++++++++++
  include/linux/dma-resv.h   | 19 +++++++++++++++
  2 files changed, 68 insertions(+)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 3cbcf66a137e..231bae173ef1 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -423,6 +423,55 @@ struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor)
  }
  EXPORT_SYMBOL(dma_resv_iter_next_unlocked);
+/**
+ * dma_resv_iter_first - first fence from a locked dma_resv object
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object while holding the
+ * &dma_resv.lock.
+ */
+struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor)
+{
+    struct dma_fence *fence;
+
+    dma_resv_assert_held(cursor->obj);
+
+    cursor->index = 0;
+    cursor->fences = dma_resv_shared_list(cursor->obj);
+
+    fence = dma_resv_excl_fence(cursor->obj);
+    if (!fence)
+        fence = dma_resv_iter_next(cursor);

"Is restarted" probably does not matter hugely for the locked iterator but I think if it hits this path (no exclusive fence, returns first shared) then it will show it as false. Which is not consistent with the unlocked iterator.

Sorry I was blind or I don't know which version of which patch I was looking at.. It is set to true a few lines below. :)

Regards,

Tvrtko


Bonus points if you make a debug build assert that makes querying "is restarted" warn when used with the locked iterator.

+
+    cursor->is_restarted = true;
+    return fence;
+}
+EXPORT_SYMBOL_GPL(dma_resv_iter_first);
+
+/**
+ * dma_resv_iter_next - next fence from a locked dma_resv object
+ * @cursor: cursor to record the current position
+ *
+ * Return all the fences in the dma_resv object while holding the
+ * &dma_resv.lock.

You probably want to replace "all the fences" with first and next, respectively, in here and in dma_resv_iter_first kerneldoc.

+ */
+struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor)
+{
+    unsigned int idx;
+
+    dma_resv_assert_held(cursor->obj);
+
+    cursor->is_restarted = false;
+    if (!cursor->all_fences || !cursor->fences ||
+        cursor->index >= cursor->fences->shared_count)
+        return NULL;

Theoretically you could store the shared count in the cursor and so could have a single condition here (assuming initialized to zero when !all_fences and !cursor->fences). For some value of optimisation. :) Probably not worth it.

But you could only assign cursor->fences if all_fences, in dma_resv_iter_first, so wouldn't have to duplicate the all_fences check here.

+
+    idx = cursor->index++;
+    return rcu_dereference_protected(cursor->fences->shared[idx],
+                     dma_resv_held(cursor->obj));
+}
+EXPORT_SYMBOL_GPL(dma_resv_iter_next);
+
  /**
   * 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 764138ad8583..3df7ef23712d 100644
--- a/include/linux/dma-resv.h
+++ b/include/linux/dma-resv.h
@@ -179,6 +179,8 @@ struct dma_resv_iter {
  struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor);   struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor);
+struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor);
+struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor);
  /**
   * dma_resv_iter_begin - initialize a dma_resv_iter object
@@ -244,6 +246,23 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor)
      for (fence = dma_resv_iter_first_unlocked(cursor);        \
           fence; fence = dma_resv_iter_next_unlocked(cursor))
+/**
+ * dma_resv_for_each_fence - fence iterator
+ * @cursor: a struct dma_resv_iter pointer
+ * @obj: a dma_resv object pointer
+ * @all_fences: true if all fences should be returned
+ * @fence: the current fence
+ *
+ * Iterate over the fences in a struct dma_resv object while holding the
+ * &dma_resv.lock. @all_fences controls if the shared fences are returned as + * well. The cursor initialisation is part of the iterator and the fence stays
+ * valid as long as the lock is held.

I'd be super cautious and explicitly spell out that reference is not held in contrast to the unlocked iterator.

+ */
+#define dma_resv_for_each_fence(cursor, obj, all_fences, fence)    \
+    for (dma_resv_iter_begin(cursor, obj, all_fences),    \
+         fence = dma_resv_iter_first(cursor); fence;    \
+         fence = dma_resv_iter_next(cursor))
+
  #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base)
  #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base)


Regards,

Tvrtko



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux