[PATCH 0/5] RESEND: dma-buf cleanup

[PATCH 0/5] RESEND: dma-buf cleanup

[Daniel Vetter]

Hi all,

This is a resend of my dma-buf cleanup with the patches that didn't yet
get and ack/r-b. Feedback very much welcome.

Thanks, Daniel

Daniel Vetter (5):
  drm/i915: Remove unecessary dma_fence_ops
  drm/msm: Remove unecessary dma_fence_ops
  drm/nouveau: Remove unecessary dma_fence_ops
  drm/vgem: Remove unecessary dma_fence_ops
  dma-fence: Polish kernel-doc for dma-fence.c

 Documentation/driver-api/dma-buf.rst          |   6 +
 drivers/dma-buf/dma-fence.c                   | 147 ++++++++++++------
 drivers/gpu/drm/i915/i915_gem_clflush.c       |   7 -
 .../gpu/drm/i915/selftests/i915_sw_fence.c    |   8 -
 drivers/gpu/drm/msm/msm_fence.c               |   8 -
 drivers/gpu/drm/nouveau/nouveau_fence.c       |   1 -
 drivers/gpu/drm/vgem/vgem_fence.c             |  14 --
 7 files changed, 109 insertions(+), 82 deletions(-)

-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops

[Daniel Vetter]

dma_fence_default_wait is the default now, same for the trivial
enable_signaling implementation.

v2: Also remove the relase hook, dma_fence_free is the default.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Colin Ian King <colin.king@canonical.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_gem_clflush.c        | 7 -------
 drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 --------
 2 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c
index f5c570d35b2a..8e74c23cbd91 100644
--- a/drivers/gpu/drm/i915/i915_gem_clflush.c
+++ b/drivers/gpu/drm/i915/i915_gem_clflush.c
@@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence)
 	return "clflush";
 }
 
-static bool i915_clflush_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static void i915_clflush_release(struct dma_fence *fence)
 {
 	struct clflush *clflush = container_of(fence, typeof(*clflush), dma);
@@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence)
 static const struct dma_fence_ops i915_clflush_ops = {
 	.get_driver_name = i915_clflush_get_driver_name,
 	.get_timeline_name = i915_clflush_get_timeline_name,
-	.enable_signaling = i915_clflush_enable_signaling,
-	.wait = dma_fence_default_wait,
 	.release = i915_clflush_release,
 };
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
index 570e325af93e..cdbc8f134e5e 100644
--- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
@@ -611,17 +611,9 @@ static const char *mock_name(struct dma_fence *fence)
 	return "mock";
 }
 
-static bool mock_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static const struct dma_fence_ops mock_fence_ops = {
 	.get_driver_name = mock_name,
 	.get_timeline_name = mock_name,
-	.enable_signaling = mock_enable_signaling,
-	.wait = dma_fence_default_wait,
-	.release = dma_fence_free,
 };
 
 static DEFINE_SPINLOCK(mock_fence_lock);
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/msm: Remove unecessary dma_fence_ops

[Daniel Vetter]

dma_fence_default_wait is the default now, same for the trivial
enable_signaling implementation.

v2: Also remove the relase hook, dma_fence_free is the default.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Rob Clark <robdclark@gmail.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
 drivers/gpu/drm/msm/msm_fence.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index 349c12f670eb..77263cf97b20 100644
--- a/drivers/gpu/drm/msm/msm_fence.c
+++ b/drivers/gpu/drm/msm/msm_fence.c
@@ -119,11 +119,6 @@ static const char *msm_fence_get_timeline_name(struct dma_fence *fence)
 	return f->fctx->name;
 }
 
-static bool msm_fence_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static bool msm_fence_signaled(struct dma_fence *fence)
 {
 	struct msm_fence *f = to_msm_fence(fence);
@@ -133,10 +128,7 @@ static bool msm_fence_signaled(struct dma_fence *fence)
 static const struct dma_fence_ops msm_fence_ops = {
 	.get_driver_name = msm_fence_get_driver_name,
 	.get_timeline_name = msm_fence_get_timeline_name,
-	.enable_signaling = msm_fence_enable_signaling,
 	.signaled = msm_fence_signaled,
-	.wait = dma_fence_default_wait,
-	.release = dma_fence_free,
 };
 
 struct dma_fence *
-- 
2.18.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 3/5] drm/nouveau: Remove unecessary dma_fence_ops

[Daniel Vetter]

dma_fence_default_wait is the default now.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: nouveau@lists.freedesktop.org
---
 drivers/gpu/drm/nouveau/nouveau_fence.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c
index 412d49bc6e56..99be61ddeb75 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -526,6 +526,5 @@ static const struct dma_fence_ops nouveau_fence_ops_uevent = {
 	.get_timeline_name = nouveau_fence_get_timeline_name,
 	.enable_signaling = nouveau_fence_enable_signaling,
 	.signaled = nouveau_fence_is_signaled,
-	.wait = dma_fence_default_wait,
 	.release = nouveau_fence_release
 };
-- 
2.18.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 4/5] drm/vgem: Remove unecessary dma_fence_ops

[Daniel Vetter]

dma_fence_default_wait is the default now, same for the trivial
enable_signaling implementation.

Also remove the ->signaled callback, vgem can't peek ahead with a
fastpath, returning false is the default implementation.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Kees Cook <keescook@chromium.org>
Cc: Cihangir Akturk <cakturk@gmail.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/vgem/vgem_fence.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
index b28876c222b4..75adedeaa384 100644
--- a/drivers/gpu/drm/vgem/vgem_fence.c
+++ b/drivers/gpu/drm/vgem/vgem_fence.c
@@ -43,16 +43,6 @@ static const char *vgem_fence_get_timeline_name(struct dma_fence *fence)
 	return "unbound";
 }
 
-static bool vgem_fence_signaled(struct dma_fence *fence)
-{
-	return false;
-}
-
-static bool vgem_fence_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static void vgem_fence_release(struct dma_fence *base)
 {
 	struct vgem_fence *fence = container_of(base, typeof(*fence), base);
@@ -76,11 +66,7 @@ static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
 static const struct dma_fence_ops vgem_fence_ops = {
 	.get_driver_name = vgem_fence_get_driver_name,
 	.get_timeline_name = vgem_fence_get_timeline_name,
-	.enable_signaling = vgem_fence_enable_signaling,
-	.signaled = vgem_fence_signaled,
-	.wait = dma_fence_default_wait,
 	.release = vgem_fence_release,
-
 	.fence_value_str = vgem_fence_value_str,
 	.timeline_value_str = vgem_fence_timeline_value_str,
 };
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] dma-fence: Polish kernel-doc for dma-fence.c

[Daniel Vetter]

- Intro section that links to how this is exposed to userspace.
- Lots more hyperlinks.
- Minor clarifications and style polish

v2: Add misplaced hunk of kerneldoc from a different patch.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
---
 Documentation/driver-api/dma-buf.rst |   6 ++
 drivers/dma-buf/dma-fence.c          | 147 +++++++++++++++++++--------
 2 files changed, 109 insertions(+), 44 deletions(-)

diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
index dc384f2f7f34..b541e97c7ab1 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -130,6 +130,12 @@ Reservation Objects
 DMA Fences
 ----------
 
+.. kernel-doc:: drivers/dma-buf/dma-fence.c
+   :doc: DMA fences overview
+
+DMA Fences Functions Reference
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 .. kernel-doc:: drivers/dma-buf/dma-fence.c
    :export:
 
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 7a92f85a4cec..1551ca7df394 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -38,12 +38,43 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal);
  */
 static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0);
 
+/**
+ * DOC: DMA fences overview
+ *
+ * DMA fences, represented by &struct dma_fence, are the kernel internal
+ * synchronization primitive for DMA operations like GPU rendering, video
+ * encoding/decoding, or displaying buffers on a screen.
+ *
+ * A fence is initialized using dma_fence_init() and completed using
+ * dma_fence_signal(). Fences are associated with a context, allocated through
+ * dma_fence_context_alloc(), and all fences on the same context are
+ * fully ordered.
+ *
+ * Since the purposes of fences is to facilitate cross-device and
+ * cross-application synchronization, there's multiple ways to use one:
+ *
+ * - Individual fences can be exposed as a &sync_file, accessed as a file
+ *   descriptor from userspace, created by calling sync_file_create(). This is
+ *   called explicit fencing, since userspace passes around explicit
+ *   synchronization points.
+ *
+ * - Some subsystems also have their own explicit fencing primitives, like
+ *   &drm_syncobj. Compared to &sync_file, a &drm_syncobj allows the underlying
+ *   fence to be updated.
+ *
+ * - Then there's also implicit fencing, where the synchronization points are
+ *   implicitly passed around as part of shared &dma_buf instances. Such
+ *   implicit fences are stored in &struct reservation_object through the
+ *   &dma_buf.resv pointer.
+ */
+
 /**
  * dma_fence_context_alloc - allocate an array of fence contexts
- * @num:	[in]	amount of contexts to allocate
+ * @num: amount of contexts to allocate
  *
- * This function will return the first index of the number of fences allocated.
- * The fence context is used for setting fence->context to a unique number.
+ * This function will return the first index of the number of fence contexts
+ * allocated.  The fence context is used for setting &dma_fence.context to a
+ * unique number by passing the context to dma_fence_init().
  */
 u64 dma_fence_context_alloc(unsigned num)
 {
@@ -59,10 +90,14 @@ EXPORT_SYMBOL(dma_fence_context_alloc);
  * Signal completion for software callbacks on a fence, this will unblock
  * dma_fence_wait() calls and run all the callbacks added with
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
- * can only go from unsignaled to signaled state, it will only be effective
- * the first time.
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time.
+ *
+ * Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
+ * held.
  *
- * Unlike dma_fence_signal, this function must be called with fence->lock held.
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
  */
 int dma_fence_signal_locked(struct dma_fence *fence)
 {
@@ -102,8 +137,11 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
  * Signal completion for software callbacks on a fence, this will unblock
  * dma_fence_wait() calls and run all the callbacks added with
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
- * can only go from unsignaled to signaled state, it will only be effective
- * the first time.
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time.
+ *
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
  */
 int dma_fence_signal(struct dma_fence *fence)
 {
@@ -136,9 +174,9 @@ EXPORT_SYMBOL(dma_fence_signal);
 /**
  * dma_fence_wait_timeout - sleep until the fence gets signaled
  * or until timeout elapses
- * @fence:	[in]	the fence to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @fence: the fence to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  *
  * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  * remaining timeout in jiffies on success. Other error values may be
@@ -148,6 +186,8 @@ EXPORT_SYMBOL(dma_fence_signal);
  * directly or indirectly (buf-mgr between reservation and committing)
  * holds a reference to the fence, otherwise the fence might be
  * freed before return, resulting in undefined behavior.
+ *
+ * See also dma_fence_wait() and dma_fence_wait_any_timeout().
  */
 signed long
 dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
@@ -167,6 +207,13 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 }
 EXPORT_SYMBOL(dma_fence_wait_timeout);
 
+/**
+ * dma_fence_release - default relese function for fences
+ * @kref: &dma_fence.recfount
+ *
+ * This is the default release functions for &dma_fence. Drivers shouldn't call
+ * this directly, but instead call dma_fence_put().
+ */
 void dma_fence_release(struct kref *kref)
 {
 	struct dma_fence *fence =
@@ -184,6 +231,13 @@ void dma_fence_release(struct kref *kref)
 }
 EXPORT_SYMBOL(dma_fence_release);
 
+/**
+ * dma_fence_free - default release function for &dma_fence.
+ * @fence: fence to release
+ *
+ * This is the default implementation for &dma_fence_ops.release. It calls
+ * kfree_rcu() on @fence.
+ */
 void dma_fence_free(struct dma_fence *fence)
 {
 	kfree_rcu(fence, rcu);
@@ -192,10 +246,11 @@ EXPORT_SYMBOL(dma_fence_free);
 
 /**
  * dma_fence_enable_sw_signaling - enable signaling on fence
- * @fence:	[in]	the fence to enable
+ * @fence: the fence to enable
  *
- * this will request for sw signaling to be enabled, to make the fence
- * complete as soon as possible
+ * This will request for sw signaling to be enabled, to make the fence
+ * complete as soon as possible. This calls &dma_fence_ops.enable_signaling
+ * internally.
  */
 void dma_fence_enable_sw_signaling(struct dma_fence *fence)
 {
@@ -220,24 +275,24 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
 /**
  * dma_fence_add_callback - add a callback to be called when the fence
  * is signaled
- * @fence:	[in]	the fence to wait on
- * @cb:		[in]	the callback to register
- * @func:	[in]	the function to call
+ * @fence: the fence to wait on
+ * @cb: the callback to register
+ * @func: the function to call
  *
- * cb will be initialized by dma_fence_add_callback, no initialization
+ * @cb will be initialized by dma_fence_add_callback(), no initialization
  * by the caller is required. Any number of callbacks can be registered
  * to a fence, but a callback can only be registered to one fence at a time.
  *
  * Note that the callback can be called from an atomic context.  If
  * fence is already signaled, this function will return -ENOENT (and
- * *not* call the callback)
+ * *not* call the callback).
  *
  * Add a software callback to the fence. Same restrictions apply to
- * refcount as it does to dma_fence_wait, however the caller doesn't need to
- * keep a refcount to fence afterwards: when software access is enabled,
- * the creator of the fence is required to keep the fence alive until
- * after it signals with dma_fence_signal. The callback itself can be called
- * from irq context.
+ * refcount as it does to dma_fence_wait(), however the caller doesn't need to
+ * keep a refcount to fence afterward dma_fence_add_callback() has returned:
+ * when software access is enabled, the creator of the fence is required to keep
+ * the fence alive until after it signals with dma_fence_signal(). The callback
+ * itself can be called from irq context.
  *
  * Returns 0 in case of success, -ENOENT if the fence is already signaled
  * and -EINVAL in case of error.
@@ -286,7 +341,7 @@ EXPORT_SYMBOL(dma_fence_add_callback);
 
 /**
  * dma_fence_get_status - returns the status upon completion
- * @fence: [in]	the dma_fence to query
+ * @fence: the dma_fence to query
  *
  * This wraps dma_fence_get_status_locked() to return the error status
  * condition on a signaled fence. See dma_fence_get_status_locked() for more
@@ -311,8 +366,8 @@ EXPORT_SYMBOL(dma_fence_get_status);
 
 /**
  * dma_fence_remove_callback - remove a callback from the signaling list
- * @fence:	[in]	the fence to wait on
- * @cb:		[in]	the callback to remove
+ * @fence: the fence to wait on
+ * @cb: the callback to remove
  *
  * Remove a previously queued callback from the fence. This function returns
  * true if the callback is successfully removed, or false if the fence has
@@ -323,6 +378,9 @@ EXPORT_SYMBOL(dma_fence_get_status);
  * doing, since deadlocks and race conditions could occur all too easily. For
  * this reason, it should only ever be done on hardware lockup recovery,
  * with a reference held to the fence.
+ *
+ * Behaviour is undefined if @cb has not been added to @fence using
+ * dma_fence_add_callback() beforehand.
  */
 bool
 dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
@@ -359,9 +417,9 @@ dma_fence_default_wait_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
 /**
  * dma_fence_default_wait - default sleep until the fence gets signaled
  * or until timeout elapses
- * @fence:	[in]	the fence to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @fence: the fence to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  *
  * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  * remaining timeout in jiffies on success. If timeout is zero the value one is
@@ -454,12 +512,12 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
 /**
  * dma_fence_wait_any_timeout - sleep until any fence gets signaled
  * or until timeout elapses
- * @fences:	[in]	array of fences to wait on
- * @count:	[in]	number of fences to wait on
- * @intr:	[in]	if true, do an interruptible wait
- * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
- * @idx:       [out]	the first signaled fence index, meaningful only on
- *			positive return
+ * @fences: array of fences to wait on
+ * @count: number of fences to wait on
+ * @intr: if true, do an interruptible wait
+ * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
+ * @idx: used to store the first signaled fence index, meaningful only on
+ *	positive return
  *
  * Returns -EINVAL on custom fence wait implementation, -ERESTARTSYS if
  * interrupted, 0 if the wait timed out, or the remaining timeout in jiffies
@@ -468,6 +526,8 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
  * Synchronous waits for the first fence in the array to be signaled. The
  * caller needs to hold a reference to all fences in the array, otherwise a
  * fence might be freed before return, resulting in undefined behavior.
+ *
+ * See also dma_fence_wait() and dma_fence_wait_timeout().
  */
 signed long
 dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
@@ -540,19 +600,18 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
 
 /**
  * dma_fence_init - Initialize a custom fence.
- * @fence:	[in]	the fence to initialize
- * @ops:	[in]	the dma_fence_ops for operations on this fence
- * @lock:	[in]	the irqsafe spinlock to use for locking this fence
- * @context:	[in]	the execution context this fence is run on
- * @seqno:	[in]	a linear increasing sequence number for this context
+ * @fence: the fence to initialize
+ * @ops: the dma_fence_ops for operations on this fence
+ * @lock: the irqsafe spinlock to use for locking this fence
+ * @context: the execution context this fence is run on
+ * @seqno: a linear increasing sequence number for this context
  *
  * Initializes an allocated fence, the caller doesn't have to keep its
  * refcount after committing with this fence, but it will need to hold a
- * refcount again if dma_fence_ops.enable_signaling gets called. This can
- * be used for other implementing other types of fence.
+ * refcount again if &dma_fence_ops.enable_signaling gets called.
  *
  * context and seqno are used for easy comparison between fences, allowing
- * to check which fence is later by simply using dma_fence_later.
+ * to check which fence is later by simply using dma_fence_later().
  */
 void
 dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] dma-fence: Polish kernel-doc for dma-fence.c

[Christian König]

Am 04.07.2018 um 11:29 schrieb Daniel Vetter:
> - Intro section that links to how this is exposed to userspace.
> - Lots more hyperlinks.
> - Minor clarifications and style polish
>
> v2: Add misplaced hunk of kerneldoc from a different patch.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   Documentation/driver-api/dma-buf.rst |   6 ++
>   drivers/dma-buf/dma-fence.c          | 147 +++++++++++++++++++--------
>   2 files changed, 109 insertions(+), 44 deletions(-)
>
> diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
> index dc384f2f7f34..b541e97c7ab1 100644
> --- a/Documentation/driver-api/dma-buf.rst
> +++ b/Documentation/driver-api/dma-buf.rst
> @@ -130,6 +130,12 @@ Reservation Objects
>   DMA Fences
>   ----------
>   
> +.. kernel-doc:: drivers/dma-buf/dma-fence.c
> +   :doc: DMA fences overview
> +
> +DMA Fences Functions Reference
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
>   .. kernel-doc:: drivers/dma-buf/dma-fence.c
>      :export:
>   
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 7a92f85a4cec..1551ca7df394 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -38,12 +38,43 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal);
>    */
>   static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0);
>   
> +/**
> + * DOC: DMA fences overview
> + *
> + * DMA fences, represented by &struct dma_fence, are the kernel internal
> + * synchronization primitive for DMA operations like GPU rendering, video
> + * encoding/decoding, or displaying buffers on a screen.
> + *
> + * A fence is initialized using dma_fence_init() and completed using
> + * dma_fence_signal(). Fences are associated with a context, allocated through
> + * dma_fence_context_alloc(), and all fences on the same context are
> + * fully ordered.
> + *
> + * Since the purposes of fences is to facilitate cross-device and
> + * cross-application synchronization, there's multiple ways to use one:
> + *
> + * - Individual fences can be exposed as a &sync_file, accessed as a file
> + *   descriptor from userspace, created by calling sync_file_create(). This is
> + *   called explicit fencing, since userspace passes around explicit
> + *   synchronization points.
> + *
> + * - Some subsystems also have their own explicit fencing primitives, like
> + *   &drm_syncobj. Compared to &sync_file, a &drm_syncobj allows the underlying
> + *   fence to be updated.
> + *
> + * - Then there's also implicit fencing, where the synchronization points are
> + *   implicitly passed around as part of shared &dma_buf instances. Such
> + *   implicit fences are stored in &struct reservation_object through the
> + *   &dma_buf.resv pointer.
> + */
> +
>   /**
>    * dma_fence_context_alloc - allocate an array of fence contexts
> - * @num:	[in]	amount of contexts to allocate
> + * @num: amount of contexts to allocate
>    *
> - * This function will return the first index of the number of fences allocated.
> - * The fence context is used for setting fence->context to a unique number.
> + * This function will return the first index of the number of fence contexts
> + * allocated.  The fence context is used for setting &dma_fence.context to a
> + * unique number by passing the context to dma_fence_init().
>    */
>   u64 dma_fence_context_alloc(unsigned num)
>   {
> @@ -59,10 +90,14 @@ EXPORT_SYMBOL(dma_fence_context_alloc);
>    * Signal completion for software callbacks on a fence, this will unblock
>    * dma_fence_wait() calls and run all the callbacks added with
>    * dma_fence_add_callback(). Can be called multiple times, but since a fence
> - * can only go from unsignaled to signaled state, it will only be effective
> - * the first time.
> + * can only go from the unsignaled to the signaled state and not back, it will
> + * only be effective the first time.
> + *
> + * Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
> + * held.
>    *
> - * Unlike dma_fence_signal, this function must be called with fence->lock held.
> + * Returns 0 on success and a negative error value when @fence has been
> + * signalled already.
>    */
>   int dma_fence_signal_locked(struct dma_fence *fence)
>   {
> @@ -102,8 +137,11 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
>    * Signal completion for software callbacks on a fence, this will unblock
>    * dma_fence_wait() calls and run all the callbacks added with
>    * dma_fence_add_callback(). Can be called multiple times, but since a fence
> - * can only go from unsignaled to signaled state, it will only be effective
> - * the first time.
> + * can only go from the unsignaled to the signaled state and not back, it will
> + * only be effective the first time.
> + *
> + * Returns 0 on success and a negative error value when @fence has been
> + * signalled already.
>    */
>   int dma_fence_signal(struct dma_fence *fence)
>   {
> @@ -136,9 +174,9 @@ EXPORT_SYMBOL(dma_fence_signal);
>   /**
>    * dma_fence_wait_timeout - sleep until the fence gets signaled
>    * or until timeout elapses
> - * @fence:	[in]	the fence to wait on
> - * @intr:	[in]	if true, do an interruptible wait
> - * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
> + * @fence: the fence to wait on
> + * @intr: if true, do an interruptible wait
> + * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
>    *
>    * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
>    * remaining timeout in jiffies on success. Other error values may be
> @@ -148,6 +186,8 @@ EXPORT_SYMBOL(dma_fence_signal);
>    * directly or indirectly (buf-mgr between reservation and committing)
>    * holds a reference to the fence, otherwise the fence might be
>    * freed before return, resulting in undefined behavior.
> + *
> + * See also dma_fence_wait() and dma_fence_wait_any_timeout().
>    */
>   signed long
>   dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
> @@ -167,6 +207,13 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>   }
>   EXPORT_SYMBOL(dma_fence_wait_timeout);
>   
> +/**
> + * dma_fence_release - default relese function for fences
> + * @kref: &dma_fence.recfount
> + *
> + * This is the default release functions for &dma_fence. Drivers shouldn't call
> + * this directly, but instead call dma_fence_put().
> + */
>   void dma_fence_release(struct kref *kref)
>   {
>   	struct dma_fence *fence =
> @@ -184,6 +231,13 @@ void dma_fence_release(struct kref *kref)
>   }
>   EXPORT_SYMBOL(dma_fence_release);
>   
> +/**
> + * dma_fence_free - default release function for &dma_fence.
> + * @fence: fence to release
> + *
> + * This is the default implementation for &dma_fence_ops.release. It calls
> + * kfree_rcu() on @fence.
> + */
>   void dma_fence_free(struct dma_fence *fence)
>   {
>   	kfree_rcu(fence, rcu);
> @@ -192,10 +246,11 @@ EXPORT_SYMBOL(dma_fence_free);
>   
>   /**
>    * dma_fence_enable_sw_signaling - enable signaling on fence
> - * @fence:	[in]	the fence to enable
> + * @fence: the fence to enable
>    *
> - * this will request for sw signaling to be enabled, to make the fence
> - * complete as soon as possible
> + * This will request for sw signaling to be enabled, to make the fence
> + * complete as soon as possible. This calls &dma_fence_ops.enable_signaling
> + * internally.
>    */
>   void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>   {
> @@ -220,24 +275,24 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
>   /**
>    * dma_fence_add_callback - add a callback to be called when the fence
>    * is signaled
> - * @fence:	[in]	the fence to wait on
> - * @cb:		[in]	the callback to register
> - * @func:	[in]	the function to call
> + * @fence: the fence to wait on
> + * @cb: the callback to register
> + * @func: the function to call
>    *
> - * cb will be initialized by dma_fence_add_callback, no initialization
> + * @cb will be initialized by dma_fence_add_callback(), no initialization
>    * by the caller is required. Any number of callbacks can be registered
>    * to a fence, but a callback can only be registered to one fence at a time.
>    *
>    * Note that the callback can be called from an atomic context.  If
>    * fence is already signaled, this function will return -ENOENT (and
> - * *not* call the callback)
> + * *not* call the callback).
>    *
>    * Add a software callback to the fence. Same restrictions apply to
> - * refcount as it does to dma_fence_wait, however the caller doesn't need to
> - * keep a refcount to fence afterwards: when software access is enabled,
> - * the creator of the fence is required to keep the fence alive until
> - * after it signals with dma_fence_signal. The callback itself can be called
> - * from irq context.
> + * refcount as it does to dma_fence_wait(), however the caller doesn't need to
> + * keep a refcount to fence afterward dma_fence_add_callback() has returned:
> + * when software access is enabled, the creator of the fence is required to keep
> + * the fence alive until after it signals with dma_fence_signal(). The callback
> + * itself can be called from irq context.
>    *
>    * Returns 0 in case of success, -ENOENT if the fence is already signaled
>    * and -EINVAL in case of error.
> @@ -286,7 +341,7 @@ EXPORT_SYMBOL(dma_fence_add_callback);
>   
>   /**
>    * dma_fence_get_status - returns the status upon completion
> - * @fence: [in]	the dma_fence to query
> + * @fence: the dma_fence to query
>    *
>    * This wraps dma_fence_get_status_locked() to return the error status
>    * condition on a signaled fence. See dma_fence_get_status_locked() for more
> @@ -311,8 +366,8 @@ EXPORT_SYMBOL(dma_fence_get_status);
>   
>   /**
>    * dma_fence_remove_callback - remove a callback from the signaling list
> - * @fence:	[in]	the fence to wait on
> - * @cb:		[in]	the callback to remove
> + * @fence: the fence to wait on
> + * @cb: the callback to remove
>    *
>    * Remove a previously queued callback from the fence. This function returns
>    * true if the callback is successfully removed, or false if the fence has
> @@ -323,6 +378,9 @@ EXPORT_SYMBOL(dma_fence_get_status);
>    * doing, since deadlocks and race conditions could occur all too easily. For
>    * this reason, it should only ever be done on hardware lockup recovery,
>    * with a reference held to the fence.
> + *
> + * Behaviour is undefined if @cb has not been added to @fence using
> + * dma_fence_add_callback() beforehand.
>    */
>   bool
>   dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> @@ -359,9 +417,9 @@ dma_fence_default_wait_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
>   /**
>    * dma_fence_default_wait - default sleep until the fence gets signaled
>    * or until timeout elapses
> - * @fence:	[in]	the fence to wait on
> - * @intr:	[in]	if true, do an interruptible wait
> - * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
> + * @fence: the fence to wait on
> + * @intr: if true, do an interruptible wait
> + * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
>    *
>    * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
>    * remaining timeout in jiffies on success. If timeout is zero the value one is
> @@ -454,12 +512,12 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
>   /**
>    * dma_fence_wait_any_timeout - sleep until any fence gets signaled
>    * or until timeout elapses
> - * @fences:	[in]	array of fences to wait on
> - * @count:	[in]	number of fences to wait on
> - * @intr:	[in]	if true, do an interruptible wait
> - * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
> - * @idx:       [out]	the first signaled fence index, meaningful only on
> - *			positive return
> + * @fences: array of fences to wait on
> + * @count: number of fences to wait on
> + * @intr: if true, do an interruptible wait
> + * @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
> + * @idx: used to store the first signaled fence index, meaningful only on
> + *	positive return
>    *
>    * Returns -EINVAL on custom fence wait implementation, -ERESTARTSYS if
>    * interrupted, 0 if the wait timed out, or the remaining timeout in jiffies
> @@ -468,6 +526,8 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
>    * Synchronous waits for the first fence in the array to be signaled. The
>    * caller needs to hold a reference to all fences in the array, otherwise a
>    * fence might be freed before return, resulting in undefined behavior.
> + *
> + * See also dma_fence_wait() and dma_fence_wait_timeout().
>    */
>   signed long
>   dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
> @@ -540,19 +600,18 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
>   
>   /**
>    * dma_fence_init - Initialize a custom fence.
> - * @fence:	[in]	the fence to initialize
> - * @ops:	[in]	the dma_fence_ops for operations on this fence
> - * @lock:	[in]	the irqsafe spinlock to use for locking this fence
> - * @context:	[in]	the execution context this fence is run on
> - * @seqno:	[in]	a linear increasing sequence number for this context
> + * @fence: the fence to initialize
> + * @ops: the dma_fence_ops for operations on this fence
> + * @lock: the irqsafe spinlock to use for locking this fence
> + * @context: the execution context this fence is run on
> + * @seqno: a linear increasing sequence number for this context
>    *
>    * Initializes an allocated fence, the caller doesn't have to keep its
>    * refcount after committing with this fence, but it will need to hold a
> - * refcount again if dma_fence_ops.enable_signaling gets called. This can
> - * be used for other implementing other types of fence.
> + * refcount again if &dma_fence_ops.enable_signaling gets called.
>    *
>    * context and seqno are used for easy comparison between fences, allowing
> - * to check which fence is later by simply using dma_fence_later.
> + * to check which fence is later by simply using dma_fence_later().
>    */
>   void
>   dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops

[Emil Velikov]

Hi Daniel,

On 4 July 2018 at 10:29, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.
>
> v2: Also remove the relase hook, dma_fence_free is the default.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Colin Ian King <colin.king@canonical.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/i915_gem_clflush.c        | 7 -------
>  drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 --------
>  2 files changed, 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c
> index f5c570d35b2a..8e74c23cbd91 100644
> --- a/drivers/gpu/drm/i915/i915_gem_clflush.c
> +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c
> @@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence)
>         return "clflush";
>  }
>
> -static bool i915_clflush_enable_signaling(struct dma_fence *fence)
> -{
> -       return true;
> -}
> -
>  static void i915_clflush_release(struct dma_fence *fence)
>  {
>         struct clflush *clflush = container_of(fence, typeof(*clflush), dma);
> @@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence)
>  static const struct dma_fence_ops i915_clflush_ops = {
>         .get_driver_name = i915_clflush_get_driver_name,
>         .get_timeline_name = i915_clflush_get_timeline_name,
> -       .enable_signaling = i915_clflush_enable_signaling,
From a quick look through drm-misc/drm-misc-next removing the
enable_signalling hook may cause functional changes.

Namely:
A call to trace_dma_fence_enable_signal() (in
dma_fence_enable_sw_signaling(), dma_fence_add_callback() and others)
will be omitted.

Removing the default .wait and .release hooks is fine.

HTH
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops

[Daniel Vetter]

On Wed, Jul 04, 2018 at 01:03:18PM +0100, Emil Velikov wrote:
> Hi Daniel,
> 
> On 4 July 2018 at 10:29, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > dma_fence_default_wait is the default now, same for the trivial
> > enable_signaling implementation.
> >
> > v2: Also remove the relase hook, dma_fence_free is the default.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Colin Ian King <colin.king@canonical.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem_clflush.c        | 7 -------
> >  drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 --------
> >  2 files changed, 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c
> > index f5c570d35b2a..8e74c23cbd91 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_clflush.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c
> > @@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence)
> >         return "clflush";
> >  }
> >
> > -static bool i915_clflush_enable_signaling(struct dma_fence *fence)
> > -{
> > -       return true;
> > -}
> > -
> >  static void i915_clflush_release(struct dma_fence *fence)
> >  {
> >         struct clflush *clflush = container_of(fence, typeof(*clflush), dma);
> > @@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence)
> >  static const struct dma_fence_ops i915_clflush_ops = {
> >         .get_driver_name = i915_clflush_get_driver_name,
> >         .get_timeline_name = i915_clflush_get_timeline_name,
> > -       .enable_signaling = i915_clflush_enable_signaling,
> From a quick look through drm-misc/drm-misc-next removing the
> enable_signalling hook may cause functional changes.
> 
> Namely:
> A call to trace_dma_fence_enable_signal() (in
> dma_fence_enable_sw_signaling(), dma_fence_add_callback() and others)
> will be omitted.

I'm not sure what this tracepoint is useful for in the absenve of a real
signaling mechanism that must be enabled (like interrupts).

For all the other bits (begin/end wait, fence signalling itsefl) we have
tracepoints already, so I think we're all covered. What do you think will
be lost with the tracepoint here? If there's a real need for it I think I
can rework the already merged patch to still call the tracpoint, while
avoiding everything else. I just don't see the use-case for that.
-Daniel

> 
> Removing the default .wait and .release hooks is fine.
> 
> HTH
> Emil

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops

[Emil Velikov]

On 4 July 2018 at 13:34, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 04, 2018 at 01:03:18PM +0100, Emil Velikov wrote:
>> Hi Daniel,
>>
>> On 4 July 2018 at 10:29, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > dma_fence_default_wait is the default now, same for the trivial
>> > enable_signaling implementation.
>> >
>> > v2: Also remove the relase hook, dma_fence_free is the default.
>> >
>> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> > Cc: Colin Ian King <colin.king@canonical.com>
>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> > Cc: intel-gfx@lists.freedesktop.org
>> > ---
>> >  drivers/gpu/drm/i915/i915_gem_clflush.c        | 7 -------
>> >  drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 --------
>> >  2 files changed, 15 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c
>> > index f5c570d35b2a..8e74c23cbd91 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem_clflush.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c
>> > @@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence)
>> >         return "clflush";
>> >  }
>> >
>> > -static bool i915_clflush_enable_signaling(struct dma_fence *fence)
>> > -{
>> > -       return true;
>> > -}
>> > -
>> >  static void i915_clflush_release(struct dma_fence *fence)
>> >  {
>> >         struct clflush *clflush = container_of(fence, typeof(*clflush), dma);
>> > @@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence)
>> >  static const struct dma_fence_ops i915_clflush_ops = {
>> >         .get_driver_name = i915_clflush_get_driver_name,
>> >         .get_timeline_name = i915_clflush_get_timeline_name,
>> > -       .enable_signaling = i915_clflush_enable_signaling,
>> From a quick look through drm-misc/drm-misc-next removing the
>> enable_signalling hook may cause functional changes.
>>
>> Namely:
>> A call to trace_dma_fence_enable_signal() (in
>> dma_fence_enable_sw_signaling(), dma_fence_add_callback() and others)
>> will be omitted.
>
> I'm not sure what this tracepoint is useful for in the absenve of a real
> signaling mechanism that must be enabled (like interrupts).
>
> For all the other bits (begin/end wait, fence signalling itsefl) we have
> tracepoints already, so I think we're all covered. What do you think will
> be lost with the tracepoint here? If there's a real need for it I think I
> can rework the already merged patch to still call the tracpoint, while
> avoiding everything else. I just don't see the use-case for that.

Nothing obvious comes to mind, yet again my knowledge of the fence API
is limited.
Was simply pointing out something that was removed without a small
note covering it.

A fraction of your explanation would have been great, but obviously
not a big deal either way.

Thanks
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops

[Daniel Vetter]

On Wed, Jul 4, 2018 at 7:22 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 4 July 2018 at 13:34, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Jul 04, 2018 at 01:03:18PM +0100, Emil Velikov wrote:
>>> Hi Daniel,
>>>
>>> On 4 July 2018 at 10:29, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> > dma_fence_default_wait is the default now, same for the trivial
>>> > enable_signaling implementation.
>>> >
>>> > v2: Also remove the relase hook, dma_fence_free is the default.
>>> >
>>> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> > Cc: Colin Ian King <colin.king@canonical.com>
>>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>> > Cc: intel-gfx@lists.freedesktop.org
>>> > ---
>>> >  drivers/gpu/drm/i915/i915_gem_clflush.c        | 7 -------
>>> >  drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 --------
>>> >  2 files changed, 15 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c
>>> > index f5c570d35b2a..8e74c23cbd91 100644
>>> > --- a/drivers/gpu/drm/i915/i915_gem_clflush.c
>>> > +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c
>>> > @@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence)
>>> >         return "clflush";
>>> >  }
>>> >
>>> > -static bool i915_clflush_enable_signaling(struct dma_fence *fence)
>>> > -{
>>> > -       return true;
>>> > -}
>>> > -
>>> >  static void i915_clflush_release(struct dma_fence *fence)
>>> >  {
>>> >         struct clflush *clflush = container_of(fence, typeof(*clflush), dma);
>>> > @@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence)
>>> >  static const struct dma_fence_ops i915_clflush_ops = {
>>> >         .get_driver_name = i915_clflush_get_driver_name,
>>> >         .get_timeline_name = i915_clflush_get_timeline_name,
>>> > -       .enable_signaling = i915_clflush_enable_signaling,
>>> From a quick look through drm-misc/drm-misc-next removing the
>>> enable_signalling hook may cause functional changes.
>>>
>>> Namely:
>>> A call to trace_dma_fence_enable_signal() (in
>>> dma_fence_enable_sw_signaling(), dma_fence_add_callback() and others)
>>> will be omitted.
>>
>> I'm not sure what this tracepoint is useful for in the absenve of a real
>> signaling mechanism that must be enabled (like interrupts).
>>
>> For all the other bits (begin/end wait, fence signalling itsefl) we have
>> tracepoints already, so I think we're all covered. What do you think will
>> be lost with the tracepoint here? If there's a real need for it I think I
>> can rework the already merged patch to still call the tracpoint, while
>> avoiding everything else. I just don't see the use-case for that.
>
> Nothing obvious comes to mind, yet again my knowledge of the fence API
> is limited.
> Was simply pointing out something that was removed without a small
> note covering it.
>
> A fraction of your explanation would have been great, but obviously
> not a big deal either way.

Yeah would have been good to add to the commit message that made
->enable_signaling optional, but that's now baked into history already
:-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/vgem: Remove unecessary dma_fence_ops

[Daniel Vetter]

On Wed, Jul 04, 2018 at 11:29:08AM +0200, Daniel Vetter wrote:
> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.
> 
> Also remove the ->signaled callback, vgem can't peek ahead with a
> fastpath, returning false is the default implementation.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Cihangir Akturk <cakturk@gmail.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Anyone feel like reviewing patches 1-4 here?

Thanks, Daniel

> ---
>  drivers/gpu/drm/vgem/vgem_fence.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
> index b28876c222b4..75adedeaa384 100644
> --- a/drivers/gpu/drm/vgem/vgem_fence.c
> +++ b/drivers/gpu/drm/vgem/vgem_fence.c
> @@ -43,16 +43,6 @@ static const char *vgem_fence_get_timeline_name(struct dma_fence *fence)
>  	return "unbound";
>  }
>  
> -static bool vgem_fence_signaled(struct dma_fence *fence)
> -{
> -	return false;
> -}
> -
> -static bool vgem_fence_enable_signaling(struct dma_fence *fence)
> -{
> -	return true;
> -}
> -
>  static void vgem_fence_release(struct dma_fence *base)
>  {
>  	struct vgem_fence *fence = container_of(base, typeof(*fence), base);
> @@ -76,11 +66,7 @@ static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
>  static const struct dma_fence_ops vgem_fence_ops = {
>  	.get_driver_name = vgem_fence_get_driver_name,
>  	.get_timeline_name = vgem_fence_get_timeline_name,
> -	.enable_signaling = vgem_fence_enable_signaling,
> -	.signaled = vgem_fence_signaled,
> -	.wait = dma_fence_default_wait,
>  	.release = vgem_fence_release,
> -
>  	.fence_value_str = vgem_fence_value_str,
>  	.timeline_value_str = vgem_fence_timeline_value_str,
>  };
> -- 
> 2.18.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/vgem: Remove unecessary dma_fence_ops

[Chris Wilson]

Quoting Daniel Vetter (2018-08-09 09:33:49)
> On Wed, Jul 04, 2018 at 11:29:08AM +0200, Daniel Vetter wrote:
> >  static void vgem_fence_release(struct dma_fence *base)
> >  {
> >       struct vgem_fence *fence = container_of(base, typeof(*fence), base);
> > @@ -76,11 +66,7 @@ static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
> >  static const struct dma_fence_ops vgem_fence_ops = {
> >       .get_driver_name = vgem_fence_get_driver_name,
> >       .get_timeline_name = vgem_fence_get_timeline_name,
> > -     .enable_signaling = vgem_fence_enable_signaling,
> > -     .signaled = vgem_fence_signaled,
> > -     .wait = dma_fence_default_wait,
> >       .release = vgem_fence_release,
> > -

That space was to separate the interesting ops from the debug!
-Chris

> >       .fence_value_str = vgem_fence_value_str,
> >       .timeline_value_str = vgem_fence_timeline_value_str,
> >  };
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/vgem: Remove unecessary dma_fence_ops

[Daniel Vetter]

dma_fence_default_wait is the default now, same for the trivial
enable_signaling implementation.

Also remove the ->signaled callback, vgem can't peek ahead with a
fastpath, returning false is the default implementation.

v2: Protect the meaningful space! (Chris)

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Kees Cook <keescook@chromium.org>
Cc: Cihangir Akturk <cakturk@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/vgem/vgem_fence.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c
index b28876c222b4..e6ee71323a66 100644
--- a/drivers/gpu/drm/vgem/vgem_fence.c
+++ b/drivers/gpu/drm/vgem/vgem_fence.c
@@ -43,16 +43,6 @@ static const char *vgem_fence_get_timeline_name(struct dma_fence *fence)
 	return "unbound";
 }
 
-static bool vgem_fence_signaled(struct dma_fence *fence)
-{
-	return false;
-}
-
-static bool vgem_fence_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static void vgem_fence_release(struct dma_fence *base)
 {
 	struct vgem_fence *fence = container_of(base, typeof(*fence), base);
@@ -76,9 +66,6 @@ static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str,
 static const struct dma_fence_ops vgem_fence_ops = {
 	.get_driver_name = vgem_fence_get_driver_name,
 	.get_timeline_name = vgem_fence_get_timeline_name,
-	.enable_signaling = vgem_fence_enable_signaling,
-	.signaled = vgem_fence_signaled,
-	.wait = dma_fence_default_wait,
 	.release = vgem_fence_release,
 
 	.fence_value_str = vgem_fence_value_str,
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/vgem: Remove unecessary dma_fence_ops

[Chris Wilson]

Quoting Daniel Vetter (2018-08-09 13:45:44)
> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.
> 
> Also remove the ->signaled callback, vgem can't peek ahead with a
> fastpath, returning false is the default implementation.
> 
> v2: Protect the meaningful space! (Chris)
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Cihangir Akturk <cakturk@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

1-4,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vgem: Remove unecessary dma_fence_ops

[Daniel Vetter]

On Thu, Aug 09, 2018 at 01:48:42PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-08-09 13:45:44)
> > dma_fence_default_wait is the default now, same for the trivial
> > enable_signaling implementation.
> > 
> > Also remove the ->signaled callback, vgem can't peek ahead with a
> > fastpath, returning false is the default implementation.
> > 
> > v2: Protect the meaningful space! (Chris)
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Cihangir Akturk <cakturk@gmail.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 1-4,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for your reviews, all four pushed to drm-misc-next now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel