[PATCH 00/15] dma-fence cleanup v2

[PATCH 00/15] dma-fence cleanup v2

[Daniel Vetter]

Hi all,

Resending the entire pile because I shrugged of some CI noise which wasn't
noise. Silly me :-/

Besides the BUG_ON that also Chris spotted just minor changes, which I did
send out individually already.

As always, feedback very much welcome.

Thanks, Daniel

Daniel Vetter (15):
  dma-fence: remove fill_driver_data callback
  dma-fence: Make ->enable_signaling optional
  dma-fence: Allow wait_any_timeout for all fences
  dma-fence: Make ->wait callback optional
  drm/amdgpu: Remove unecessary dma_fence_ops
  drm: Remove unecessary dma_fence_ops
  drm/etnaviv: Remove unecessary dma_fence_ops
  drm/i915: Remove unecessary dma_fence_ops
  drm/msm: Remove unecessary dma_fence_ops
  drm/nouveau: Remove unecessary dma_fence_ops
  drm/qxl: Remove unecessary dma_fence_ops
  drm/vc4: Remove unecessary dma_fence_ops
  drm/vgem: Remove unecessary dma_fence_ops
  drm/virtio: 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-array.c             |   1 -
 drivers/dma-buf/dma-fence.c                   | 164 ++++++++++++------
 drivers/dma-buf/sw_sync.c                     |   1 -
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c  |   2 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c     |   1 -
 drivers/gpu/drm/drm_crtc.c                    |   7 -
 drivers/gpu/drm/drm_syncobj.c                 |   1 -
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c         |   7 -
 drivers/gpu/drm/i915/i915_gem_clflush.c       |   7 -
 .../gpu/drm/i915/selftests/i915_sw_fence.c    |   7 -
 drivers/gpu/drm/msm/msm_fence.c               |   7 -
 drivers/gpu/drm/nouveau/nouveau_fence.c       |   1 -
 drivers/gpu/drm/qxl/qxl_release.c             |   7 -
 drivers/gpu/drm/scheduler/sched_fence.c       |  11 --
 drivers/gpu/drm/vc4/vc4_fence.c               |   7 -
 drivers/gpu/drm/vgem/vgem_fence.c             |  14 --
 drivers/gpu/drm/virtio/virtgpu_fence.c        |   7 -
 include/linux/dma-fence.h                     |  32 ++--
 19 files changed, 131 insertions(+), 159 deletions(-)

-- 
2.17.0

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

[PATCH 01/15] dma-fence: remove fill_driver_data callback

[Daniel Vetter]

Noticed while I was typing docs. Entirely unused.

v2: Remove reference in @timeline_value_str too. While at it clarify
why timeline_value_str has a fence parameter - we don't have an
explicit timeline structure unfortunately.

Cc: Eric Anholt <eric@anholt.net>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Cc: Christian König <christian.koenig@amd.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/linux/dma-fence.h | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index eb9b05aa5aea..111aefe1c956 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -217,17 +217,6 @@ struct dma_fence_ops {
 	 */
 	void (*release)(struct dma_fence *fence);
 
-	/**
-	 * @fill_driver_data:
-	 *
-	 * Callback to fill in free-form debug info.
-	 *
-	 * Returns amount of bytes filled, or negative error on failure.
-	 *
-	 * This callback is optional.
-	 */
-	int (*fill_driver_data)(struct dma_fence *fence, void *data, int size);
-
 	/**
 	 * @fence_value_str:
 	 *
@@ -242,8 +231,9 @@ struct dma_fence_ops {
 	 * @timeline_value_str:
 	 *
 	 * Fills in the current value of the timeline as a string, like the
-	 * sequence number. This should match what @fill_driver_data prints for
-	 * the most recently signalled fence (assuming no delayed signalling).
+	 * sequence number. Note that the specific fence passed to this function
+	 * should not matter, drivers should only use it to look up the
+	 * corresponding timeline structures.
 	 */
 	void (*timeline_value_str)(struct dma_fence *fence,
 				   char *str, int size);
-- 
2.17.0

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

[PATCH 02/15] dma-fence: Make ->enable_signaling optional

Many drivers have a trivial implementation for ->enable_signaling.
Let's make it optional by assuming that signalling is already
available when the callback isn't present.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
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
---
 drivers/dma-buf/dma-fence.c | 13 ++++++++++++-
 include/linux/dma-fence.h   |  3 ++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 4edb9fd3cf47..7b5b40d6b70e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -181,6 +181,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);
@@ -560,7 +567,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
+	BUG_ON(!ops || !ops->wait ||
 	       !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
@@ -572,6 +579,10 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	fence->flags = 0UL;
 	fence->error = 0;
 
+	if (!ops->enable_signaling)
+		set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+			&fence->flags);
+
 	trace_dma_fence_init(fence);
 }
 EXPORT_SYMBOL(dma_fence_init);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 111aefe1c956..c053d19e1e24 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -166,7 +166,8 @@ struct dma_fence_ops {
 	 * released when the fence is signalled (through e.g. the interrupt
 	 * handler).
 	 *
-	 * This callback is mandatory.
+	 * This callback is optional. If this callback is not present, then the
+	 * driver must always have signaling enabled.
 	 */
 	bool (*enable_signaling)(struct dma_fence *fence);
 
-- 
2.17.0

[PATCH 02/15] dma-fence: Make ->enable_signaling optional

[Daniel Vetter]

Many drivers have a trivial implementation for ->enable_signaling.
Let's make it optional by assuming that signalling is already
available when the callback isn't present.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
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
---
 drivers/dma-buf/dma-fence.c | 13 ++++++++++++-
 include/linux/dma-fence.h   |  3 ++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 4edb9fd3cf47..7b5b40d6b70e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -181,6 +181,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);
@@ -560,7 +567,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
+	BUG_ON(!ops || !ops->wait ||
 	       !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
@@ -572,6 +579,10 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	fence->flags = 0UL;
 	fence->error = 0;
 
+	if (!ops->enable_signaling)
+		set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+			&fence->flags);
+
 	trace_dma_fence_init(fence);
 }
 EXPORT_SYMBOL(dma_fence_init);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 111aefe1c956..c053d19e1e24 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -166,7 +166,8 @@ struct dma_fence_ops {
 	 * released when the fence is signalled (through e.g. the interrupt
 	 * handler).
 	 *
-	 * This callback is mandatory.
+	 * This callback is optional. If this callback is not present, then the
+	 * driver must always have signaling enabled.
 	 */
 	bool (*enable_signaling)(struct dma_fence *fence);
 
-- 
2.17.0

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

[PATCH 03/15] dma-fence: Allow wait_any_timeout for all fences

[Daniel Vetter]

When this was introduced in

commit a519435a96597d8cd96123246fea4ae5a6c90b02
Author: Christian König <christian.koenig@amd.com>
Date:   Tue Oct 20 16:34:16 2015 +0200

    dma-buf/fence: add fence_wait_any_timeout function v2

there was a restriction added that this only works if the dma-fence
uses the dma_fence_default_wait hook. Which works for amdgpu, which is
the only caller. Well, until you share some buffers with e.g. i915,
then you get an -EINVAL.

But there's really no reason for this, because all drivers must
support callbacks. The special ->wait hook is only as an optimization;
if the driver needs to create a worker thread for an active callback,
then it can avoid to do that if it knows that there's a process
context available already. So ->wait is just an optimization, just
using the logic in dma_fence_default_wait() should work for all
drivers.

Let's remove this restriction.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
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
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/dma-buf/dma-fence.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 7b5b40d6b70e..59049375bd19 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -503,11 +503,6 @@ dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
 	for (i = 0; i < count; ++i) {
 		struct dma_fence *fence = fences[i];
 
-		if (fence->ops->wait != dma_fence_default_wait) {
-			ret = -EINVAL;
-			goto fence_rm_cb;
-		}
-
 		cb[i].task = current;
 		if (dma_fence_add_callback(fence, &cb[i].base,
 					   dma_fence_default_wait_cb)) {
-- 
2.17.0

[PATCH 03/15] dma-fence: Allow wait_any_timeout for all fences

[Daniel Vetter]

When this was introduced in

commit a519435a96597d8cd96123246fea4ae5a6c90b02
Author: Christian König <christian.koenig@amd.com>
Date:   Tue Oct 20 16:34:16 2015 +0200

    dma-buf/fence: add fence_wait_any_timeout function v2

there was a restriction added that this only works if the dma-fence
uses the dma_fence_default_wait hook. Which works for amdgpu, which is
the only caller. Well, until you share some buffers with e.g. i915,
then you get an -EINVAL.

But there's really no reason for this, because all drivers must
support callbacks. The special ->wait hook is only as an optimization;
if the driver needs to create a worker thread for an active callback,
then it can avoid to do that if it knows that there's a process
context available already. So ->wait is just an optimization, just
using the logic in dma_fence_default_wait() should work for all
drivers.

Let's remove this restriction.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
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
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/dma-buf/dma-fence.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 7b5b40d6b70e..59049375bd19 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -503,11 +503,6 @@ dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,
 	for (i = 0; i < count; ++i) {
 		struct dma_fence *fence = fences[i];
 
-		if (fence->ops->wait != dma_fence_default_wait) {
-			ret = -EINVAL;
-			goto fence_rm_cb;
-		}
-
 		cb[i].task = current;
 		if (dma_fence_add_callback(fence, &cb[i].base,
 					   dma_fence_default_wait_cb)) {
-- 
2.17.0

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

[PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

Almost everyone uses dma_fence_default_wait.

v2: Also remove the BUG_ON(!ops->wait) (Chris).

Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
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
---
 drivers/dma-buf/dma-fence-array.c |  1 -
 drivers/dma-buf/dma-fence.c       |  8 +++++---
 drivers/dma-buf/sw_sync.c         |  1 -
 include/linux/dma-fence.h         | 13 ++++++++-----
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index dd1edfb27b61..a8c254497251 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -104,7 +104,6 @@ const struct dma_fence_ops dma_fence_array_ops = {
 	.get_timeline_name = dma_fence_array_get_timeline_name,
 	.enable_signaling = dma_fence_array_enable_signaling,
 	.signaled = dma_fence_array_signaled,
-	.wait = dma_fence_default_wait,
 	.release = dma_fence_array_release,
 };
 EXPORT_SYMBOL(dma_fence_array_ops);
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 59049375bd19..41ec19c9efc7 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -158,7 +158,10 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 		return -EINVAL;
 
 	trace_dma_fence_wait_start(fence);
-	ret = fence->ops->wait(fence, intr, timeout);
+	if (fence->ops->wait)
+		ret = fence->ops->wait(fence, intr, timeout);
+	else
+		ret = dma_fence_default_wait(fence, intr, timeout);
 	trace_dma_fence_wait_end(fence);
 	return ret;
 }
@@ -562,8 +565,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait ||
-	       !ops->get_driver_name || !ops->get_timeline_name);
+	BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
 	fence->ops = ops;
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 3d78ca89a605..53c1d6d36a64 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -188,7 +188,6 @@ static const struct dma_fence_ops timeline_fence_ops = {
 	.get_timeline_name = timeline_fence_get_timeline_name,
 	.enable_signaling = timeline_fence_enable_signaling,
 	.signaled = timeline_fence_signaled,
-	.wait = dma_fence_default_wait,
 	.release = timeline_fence_release,
 	.fence_value_str = timeline_fence_value_str,
 	.timeline_value_str = timeline_fence_timeline_value_str,
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index c053d19e1e24..02dba8cd033d 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -191,11 +191,14 @@ struct dma_fence_ops {
 	/**
 	 * @wait:
 	 *
-	 * Custom wait implementation, or dma_fence_default_wait.
+	 * Custom wait implementation, defaults to dma_fence_default_wait() if
+	 * not set.
 	 *
-	 * Must not be NULL, set to dma_fence_default_wait for default implementation.
-	 * the dma_fence_default_wait implementation should work for any fence, as long
-	 * as enable_signaling works correctly.
+	 * The dma_fence_default_wait implementation should work for any fence, as long
+	 * as @enable_signaling works correctly. This hook allows drivers to
+	 * have an optimized version for the case where a process context is
+	 * already available, e.g. if @enable_signaling for the general case
+	 * needs to set up a worker thread.
 	 *
 	 * Must return -ERESTARTSYS if the wait is intr = true and the wait was
 	 * interrupted, and remaining jiffies if fence has signaled, or 0 if wait
@@ -203,7 +206,7 @@ struct dma_fence_ops {
 	 * which should be treated as if the fence is signaled. For example a hardware
 	 * lockup could be reported like that.
 	 *
-	 * This callback is mandatory.
+	 * This callback is optional.
 	 */
 	signed long (*wait)(struct dma_fence *fence,
 			    bool intr, signed long timeout);
-- 
2.17.0

[PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

Almost everyone uses dma_fence_default_wait.

v2: Also remove the BUG_ON(!ops->wait) (Chris).

Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
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
---
 drivers/dma-buf/dma-fence-array.c |  1 -
 drivers/dma-buf/dma-fence.c       |  8 +++++---
 drivers/dma-buf/sw_sync.c         |  1 -
 include/linux/dma-fence.h         | 13 ++++++++-----
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index dd1edfb27b61..a8c254497251 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -104,7 +104,6 @@ const struct dma_fence_ops dma_fence_array_ops = {
 	.get_timeline_name = dma_fence_array_get_timeline_name,
 	.enable_signaling = dma_fence_array_enable_signaling,
 	.signaled = dma_fence_array_signaled,
-	.wait = dma_fence_default_wait,
 	.release = dma_fence_array_release,
 };
 EXPORT_SYMBOL(dma_fence_array_ops);
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 59049375bd19..41ec19c9efc7 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -158,7 +158,10 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 		return -EINVAL;
 
 	trace_dma_fence_wait_start(fence);
-	ret = fence->ops->wait(fence, intr, timeout);
+	if (fence->ops->wait)
+		ret = fence->ops->wait(fence, intr, timeout);
+	else
+		ret = dma_fence_default_wait(fence, intr, timeout);
 	trace_dma_fence_wait_end(fence);
 	return ret;
 }
@@ -562,8 +565,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait ||
-	       !ops->get_driver_name || !ops->get_timeline_name);
+	BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
 	fence->ops = ops;
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 3d78ca89a605..53c1d6d36a64 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -188,7 +188,6 @@ static const struct dma_fence_ops timeline_fence_ops = {
 	.get_timeline_name = timeline_fence_get_timeline_name,
 	.enable_signaling = timeline_fence_enable_signaling,
 	.signaled = timeline_fence_signaled,
-	.wait = dma_fence_default_wait,
 	.release = timeline_fence_release,
 	.fence_value_str = timeline_fence_value_str,
 	.timeline_value_str = timeline_fence_timeline_value_str,
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index c053d19e1e24..02dba8cd033d 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -191,11 +191,14 @@ struct dma_fence_ops {
 	/**
 	 * @wait:
 	 *
-	 * Custom wait implementation, or dma_fence_default_wait.
+	 * Custom wait implementation, defaults to dma_fence_default_wait() if
+	 * not set.
 	 *
-	 * Must not be NULL, set to dma_fence_default_wait for default implementation.
-	 * the dma_fence_default_wait implementation should work for any fence, as long
-	 * as enable_signaling works correctly.
+	 * The dma_fence_default_wait implementation should work for any fence, as long
+	 * as @enable_signaling works correctly. This hook allows drivers to
+	 * have an optimized version for the case where a process context is
+	 * already available, e.g. if @enable_signaling for the general case
+	 * needs to set up a worker thread.
 	 *
 	 * Must return -ERESTARTSYS if the wait is intr = true and the wait was
 	 * interrupted, and remaining jiffies if fence has signaled, or 0 if wait
@@ -203,7 +206,7 @@ struct dma_fence_ops {
 	 * which should be treated as if the fence is signaled. For example a hardware
 	 * lockup could be reported like that.
 	 *
-	 * This callback is mandatory.
+	 * This callback is optional.
 	 */
 	signed long (*wait)(struct dma_fence *fence,
 			    bool intr, signed long timeout);
-- 
2.17.0

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

[PATCH 05/15] drm/amdgpu: Remove unecessary dma_fence_ops

[Daniel Vetter]

dma_fence_default_wait is the default now.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: Monk Liu <Monk.Liu@amd.com>
Cc: pding <Pixel.Ding@amd.com>
Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Cc: Evan Quan <evan.quan@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Kees Cook <keescook@chromium.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 2 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c        | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
index 2c14025e5e76..574c1181ae9a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
@@ -173,7 +173,5 @@ static const struct dma_fence_ops amdkfd_fence_ops = {
 	.get_driver_name = amdkfd_fence_get_driver_name,
 	.get_timeline_name = amdkfd_fence_get_timeline_name,
 	.enable_signaling = amdkfd_fence_enable_signaling,
-	.signaled = NULL,
-	.wait = dma_fence_default_wait,
 	.release = amdkfd_fence_release,
 };
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 97449e06a242..9dbbd69dd2b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -645,7 +645,6 @@ static const struct dma_fence_ops amdgpu_fence_ops = {
 	.get_driver_name = amdgpu_fence_get_driver_name,
 	.get_timeline_name = amdgpu_fence_get_timeline_name,
 	.enable_signaling = amdgpu_fence_enable_signaling,
-	.wait = dma_fence_default_wait,
 	.release = amdgpu_fence_release,
 };
 
-- 
2.17.0

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

[PATCH 06/15] drm: Remove unecessary dma_fence_ops

[Daniel Vetter]

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

Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/drm_crtc.c              |  7 -------
 drivers/gpu/drm/drm_syncobj.c           |  1 -
 drivers/gpu/drm/scheduler/sched_fence.c | 11 -----------
 3 files changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a231dd5dce16..e4d3285f4191 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -225,16 +225,9 @@ static const char *drm_crtc_fence_get_timeline_name(struct dma_fence *fence)
 	return crtc->timeline_name;
 }
 
-static bool drm_crtc_fence_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static const struct dma_fence_ops drm_crtc_fence_ops = {
 	.get_driver_name = drm_crtc_fence_get_driver_name,
 	.get_timeline_name = drm_crtc_fence_get_timeline_name,
-	.enable_signaling = drm_crtc_fence_enable_signaling,
-	.wait = dma_fence_default_wait,
 };
 
 struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index d4f4ce484529..adb3cb27d31e 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -207,7 +207,6 @@ static const struct dma_fence_ops drm_syncobj_null_fence_ops = {
 	.get_driver_name = drm_syncobj_null_fence_get_name,
 	.get_timeline_name = drm_syncobj_null_fence_get_name,
 	.enable_signaling = drm_syncobj_null_fence_enable_signaling,
-	.wait = dma_fence_default_wait,
 	.release = NULL,
 };
 
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index 69aab086b913..4843289cc8f0 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -81,11 +81,6 @@ static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
 	return (const char *)fence->sched->name;
 }
 
-static bool drm_sched_fence_enable_signaling(struct dma_fence *f)
-{
-	return true;
-}
-
 /**
  * amd_sched_fence_free - free up the fence memory
  *
@@ -134,18 +129,12 @@ static void drm_sched_fence_release_finished(struct dma_fence *f)
 const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
 	.get_driver_name = drm_sched_fence_get_driver_name,
 	.get_timeline_name = drm_sched_fence_get_timeline_name,
-	.enable_signaling = drm_sched_fence_enable_signaling,
-	.signaled = NULL,
-	.wait = dma_fence_default_wait,
 	.release = drm_sched_fence_release_scheduled,
 };
 
 const struct dma_fence_ops drm_sched_fence_ops_finished = {
 	.get_driver_name = drm_sched_fence_get_driver_name,
 	.get_timeline_name = drm_sched_fence_get_timeline_name,
-	.enable_signaling = drm_sched_fence_enable_signaling,
-	.signaled = NULL,
-	.wait = dma_fence_default_wait,
 	.release = drm_sched_fence_release_finished,
 };
 
-- 
2.17.0

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

[PATCH 07/15] drm/etnaviv: Remove unecessary dma_fence_ops

[Daniel Vetter]

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

Acked-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: etnaviv@lists.freedesktop.org
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 8a88799bf79b..b78d9f49aa23 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1038,11 +1038,6 @@ static const char *etnaviv_fence_get_timeline_name(struct dma_fence *fence)
 	return dev_name(f->gpu->dev);
 }
 
-static bool etnaviv_fence_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static bool etnaviv_fence_signaled(struct dma_fence *fence)
 {
 	struct etnaviv_fence *f = to_etnaviv_fence(fence);
@@ -1060,9 +1055,7 @@ static void etnaviv_fence_release(struct dma_fence *fence)
 static const struct dma_fence_ops etnaviv_fence_ops = {
 	.get_driver_name = etnaviv_fence_get_driver_name,
 	.get_timeline_name = etnaviv_fence_get_timeline_name,
-	.enable_signaling = etnaviv_fence_enable_signaling,
 	.signaled = etnaviv_fence_signaled,
-	.wait = dma_fence_default_wait,
 	.release = etnaviv_fence_release,
 };
 
-- 
2.17.0

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

[PATCH 08/15] drm/i915: Remove unecessary dma_fence_ops

[Daniel Vetter]

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

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 | 7 -------
 2 files changed, 14 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..0c02446f613d 100644
--- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
@@ -611,16 +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,
 };
 
-- 
2.17.0

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

[PATCH 09/15] drm/msm: Remove unecessary dma_fence_ops

[Daniel Vetter]

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

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 | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index 349c12f670eb..6076f55a603c 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,9 +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,
 };
 
-- 
2.17.0

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

[PATCH 10/15] 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 503fa94dc06d..444bbde65344 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -527,6 +527,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.17.0

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

[PATCH 11/15] drm/qxl: Remove unecessary dma_fence_ops

[Daniel Vetter]

The trivial enable_signaling implementation matches the default code.

v2: Fix up commit message to match patch better (Eric).

Cc: Eric Anholt <eric@anholt.net>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: virtualization@lists.linux-foundation.org
---
 drivers/gpu/drm/qxl/qxl_release.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 7cb214577275..e37f0097f744 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -50,12 +50,6 @@ static const char *qxl_get_timeline_name(struct dma_fence *fence)
 	return "release";
 }
 
-static bool qxl_nop_signaling(struct dma_fence *fence)
-{
-	/* fences are always automatically signaled, so just pretend we did this.. */
-	return true;
-}
-
 static long qxl_fence_wait(struct dma_fence *fence, bool intr,
 			   signed long timeout)
 {
@@ -119,7 +113,6 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr,
 static const struct dma_fence_ops qxl_fence_ops = {
 	.get_driver_name = qxl_get_driver_name,
 	.get_timeline_name = qxl_get_timeline_name,
-	.enable_signaling = qxl_nop_signaling,
 	.wait = qxl_fence_wait,
 };
 
-- 
2.17.0

[PATCH 12/15] drm/vc4: Remove unecessary dma_fence_ops

[Daniel Vetter]

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

Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_fence.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_fence.c b/drivers/gpu/drm/vc4/vc4_fence.c
index dbf5a5a5d5f5..9f40c0ddadc4 100644
--- a/drivers/gpu/drm/vc4/vc4_fence.c
+++ b/drivers/gpu/drm/vc4/vc4_fence.c
@@ -33,11 +33,6 @@ static const char *vc4_fence_get_timeline_name(struct dma_fence *fence)
 	return "vc4-v3d";
 }
 
-static bool vc4_fence_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static bool vc4_fence_signaled(struct dma_fence *fence)
 {
 	struct vc4_fence *f = to_vc4_fence(fence);
@@ -49,8 +44,6 @@ static bool vc4_fence_signaled(struct dma_fence *fence)
 const struct dma_fence_ops vc4_fence_ops = {
 	.get_driver_name = vc4_fence_get_driver_name,
 	.get_timeline_name = vc4_fence_get_timeline_name,
-	.enable_signaling = vc4_fence_enable_signaling,
 	.signaled = vc4_fence_signaled,
-	.wait = dma_fence_default_wait,
 	.release = dma_fence_free,
 };
-- 
2.17.0

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

[PATCH 13/15] 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.17.0

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

[PATCH 14/15] drm/virtio: Remove unecessary dma_fence_ops

[Daniel Vetter]

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

Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: virtualization@lists.linux-foundation.org
---
 drivers/gpu/drm/virtio/virtgpu_fence.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 23353521f903..00c742a441bf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -36,11 +36,6 @@ static const char *virtio_get_timeline_name(struct dma_fence *f)
 	return "controlq";
 }
 
-static bool virtio_enable_signaling(struct dma_fence *f)
-{
-	return true;
-}
-
 static bool virtio_signaled(struct dma_fence *f)
 {
 	struct virtio_gpu_fence *fence = to_virtio_fence(f);
@@ -67,9 +62,7 @@ static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
 static const struct dma_fence_ops virtio_fence_ops = {
 	.get_driver_name     = virtio_get_driver_name,
 	.get_timeline_name   = virtio_get_timeline_name,
-	.enable_signaling    = virtio_enable_signaling,
 	.signaled            = virtio_signaled,
-	.wait                = dma_fence_default_wait,
 	.fence_value_str     = virtio_fence_value_str,
 	.timeline_value_str  = virtio_timeline_value_str,
 };
-- 
2.17.0

[PATCH 15/15] 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

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          | 140 ++++++++++++++++++---------
 2 files changed, 102 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 41ec19c9efc7..0387c6a59055 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 fence->lock held.
+ * Unlike dma_fence_signal(), this function must be called with &dma_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 =
@@ -199,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)
 {
@@ -226,24 +274,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.
@@ -292,7 +340,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
@@ -317,8 +365,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
@@ -329,6 +377,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)
@@ -365,9 +416,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
@@ -460,12 +511,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
@@ -474,6 +525,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,
@@ -546,19 +599,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.17.0

[PATCH 15/15] 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

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          | 140 ++++++++++++++++++---------
 2 files changed, 102 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 41ec19c9efc7..0387c6a59055 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 fence->lock held.
+ * Unlike dma_fence_signal(), this function must be called with &dma_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 =
@@ -199,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)
 {
@@ -226,24 +274,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.
@@ -292,7 +340,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
@@ -317,8 +365,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
@@ -329,6 +377,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)
@@ -365,9 +416,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
@@ -460,12 +511,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
@@ -474,6 +525,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,
@@ -546,19 +599,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.17.0

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

✗ Fi.CI.CHECKPATCH: warning for dma-fence cleanup v2

[Patchwork]

== Series Details ==

Series: dma-fence cleanup v2
URL   : https://patchwork.freedesktop.org/series/42640/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
0506ef5446b8 dma-fence: remove fill_driver_data callback
79de0bebf6c8 dma-fence: Make ->enable_signaling optional
-:43: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#43: FILE: drivers/dma-buf/dma-fence.c:570:
+	BUG_ON(!ops || !ops->wait ||

total: 0 errors, 1 warnings, 0 checks, 40 lines checked
43673b80a6a9 dma-fence: Allow wait_any_timeout for all fences
-:11: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit a519435a9659 ("dma-buf/fence: add fence_wait_any_timeout function v2")'
#11: 
commit a519435a96597d8cd96123246fea4ae5a6c90b02

total: 1 errors, 0 warnings, 0 checks, 11 lines checked
a08d4f1d2a61 dma-fence: Make ->wait callback optional
-:55: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#55: FILE: drivers/dma-buf/dma-fence.c:568:
+	BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);

total: 0 errors, 1 warnings, 0 checks, 60 lines checked
76e30313c660 drm/amdgpu: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/amdgpu: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 14 lines checked
8cc15e52dc3d drm: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 52 lines checked
ae81bae61c7f drm/etnaviv: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/etnaviv: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 20 lines checked
c625e25d4f33 drm/i915: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/i915: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 35 lines checked
3faf83e36c3d drm/msm: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/msm: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 20 lines checked
a1528ba2050e drm/nouveau: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/nouveau: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 6 lines checked
661f509db383 drm/qxl: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/qxl: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 19 lines checked
381046beac61 drm/vc4: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/vc4: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 19 lines checked
00196d556c50 drm/vgem: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/vgem: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 27 lines checked
a8f8ed66b2e9 drm/virtio: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/virtio: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 20 lines checked
3c90eb98fee9 dma-fence: Polish kernel-doc for dma-fence.c

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

Re: [PATCH 02/15] dma-fence: Make ->enable_signaling optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-03 15:25:50)
> @@ -560,7 +567,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>                spinlock_t *lock, u64 context, unsigned seqno)
>  {
>         BUG_ON(!lock);
> -       BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> +       BUG_ON(!ops || !ops->wait ||
>                !ops->get_driver_name || !ops->get_timeline_name);

One thing I was wondering about (following the discussion of rhashtable
on lwn) was inlining this function and passing dma_fence_ops by value.
And seeing if that eliminates the branch and makes smaller code
(probably not, mostly idling wondering about that technique) and kills
off the BUGs (can then be BUILD_BUG_ON).
-Chris

Re: [PATCH 02/15] dma-fence: Make ->enable_signaling optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-03 15:25:50)
> @@ -560,7 +567,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>                spinlock_t *lock, u64 context, unsigned seqno)
>  {
>         BUG_ON(!lock);
> -       BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> +       BUG_ON(!ops || !ops->wait ||
>                !ops->get_driver_name || !ops->get_timeline_name);

One thing I was wondering about (following the discussion of rhashtable
on lwn) was inlining this function and passing dma_fence_ops by value.
And seeing if that eliminates the branch and makes smaller code
(probably not, mostly idling wondering about that technique) and kills
off the BUGs (can then be BUILD_BUG_ON).
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for dma-fence cleanup v2

[Patchwork]

== Series Details ==

Series: dma-fence cleanup v2
URL   : https://patchwork.freedesktop.org/series/42640/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4134 -> Patchwork_8898 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42640/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8898 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-no-display:
      fi-bsw-n3050:       DMESG-FAIL -> PASS

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS +1
      fi-cnl-y3:          DMESG-WARN (fdo#104951) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (40 -> 37) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4134 -> Patchwork_8898

  CI_DRM_4134: 3fc6b5905d2bac264ced33d06f0cb2245c62b83f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4460: f772d9a910130b3aec8efa4f09ed723618fae656 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8898: 3c90eb98fee9527b36025efda7adec83f4d9e45f @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4460: f74d92e704849610364b4474a2c67ea2008c14e0 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

3c90eb98fee9 dma-fence: Polish kernel-doc for dma-fence.c
a8f8ed66b2e9 drm/virtio: Remove unecessary dma_fence_ops
00196d556c50 drm/vgem: Remove unecessary dma_fence_ops
381046beac61 drm/vc4: Remove unecessary dma_fence_ops
661f509db383 drm/qxl: Remove unecessary dma_fence_ops
a1528ba2050e drm/nouveau: Remove unecessary dma_fence_ops
3faf83e36c3d drm/msm: Remove unecessary dma_fence_ops
c625e25d4f33 drm/i915: Remove unecessary dma_fence_ops
ae81bae61c7f drm/etnaviv: Remove unecessary dma_fence_ops
8cc15e52dc3d drm: Remove unecessary dma_fence_ops
76e30313c660 drm/amdgpu: Remove unecessary dma_fence_ops
a08d4f1d2a61 dma-fence: Make ->wait callback optional
43673b80a6a9 dma-fence: Allow wait_any_timeout for all fences
79de0bebf6c8 dma-fence: Make ->enable_signaling optional
0506ef5446b8 dma-fence: remove fill_driver_data callback

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8898/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for dma-fence cleanup v2

[Patchwork]

== Series Details ==

Series: dma-fence cleanup v2
URL   : https://patchwork.freedesktop.org/series/42640/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4134_full -> Patchwork_8898_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8898_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8898_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42640/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8898_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-bsd2:
      shard-kbl:          PASS -> SKIP

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-snb:          PASS -> SKIP

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-pwrite:
      shard-hsw:          PASS -> SKIP +1

    
== Known issues ==

  Here are the changes found in Patchwork_8898_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_hangman@error-state-basic:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@drv_selftest@live_contexts:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_cursor_legacy@flip-vs-cursor-atomic:
      shard-hsw:          PASS -> FAIL (fdo#102670)

    igt@kms_flip@2x-dpms-vs-vblank-race-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#103060)

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-apl:          PASS -> FAIL (fdo#103925)

    igt@kms_rotation_crc@sprite-rotation-270:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +10

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#103665, fdo#106023) -> PASS

    igt@kms_flip@2x-modeset-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-apl:          FAIL (fdo#105363, fdo#102887) -> PASS

    igt@kms_flip@wf_vblank-ts-check-interruptible:
      shard-glk:          FAIL (fdo#105312) -> PASS

    igt@kms_rotation_crc@primary-rotation-270:
      shard-apl:          FAIL (fdo#103925) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105312 https://bugs.freedesktop.org/show_bug.cgi?id=105312
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4134 -> Patchwork_8898

  CI_DRM_4134: 3fc6b5905d2bac264ced33d06f0cb2245c62b83f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4460: f772d9a910130b3aec8efa4f09ed723618fae656 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8898: 3c90eb98fee9527b36025efda7adec83f4d9e45f @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4460: f74d92e704849610364b4474a2c67ea2008c14e0 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8898/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/15] drm/etnaviv: Remove unecessary dma_fence_ops

[Christian Gmeiner]

Am Do., 3. Mai 2018 um 16:26 Uhr schrieb Daniel Vetter <
daniel.vetter@ffwll.ch>:

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


Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> Acked-by: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: etnaviv@lists.freedesktop.org
> ---
>   drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 7 -------
>   1 file changed, 7 deletions(-)

> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 8a88799bf79b..b78d9f49aa23 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1038,11 +1038,6 @@ static const char
*etnaviv_fence_get_timeline_name(struct dma_fence *fence)
>          return dev_name(f->gpu->dev);
>   }

> -static bool etnaviv_fence_enable_signaling(struct dma_fence *fence)
> -{
> -       return true;
> -}
> -
>   static bool etnaviv_fence_signaled(struct dma_fence *fence)
>   {
>          struct etnaviv_fence *f = to_etnaviv_fence(fence);
> @@ -1060,9 +1055,7 @@ static void etnaviv_fence_release(struct dma_fence
*fence)
>   static const struct dma_fence_ops etnaviv_fence_ops = {
>          .get_driver_name = etnaviv_fence_get_driver_name,
>          .get_timeline_name = etnaviv_fence_get_timeline_name,
> -       .enable_signaling = etnaviv_fence_enable_signaling,
>          .signaled = etnaviv_fence_signaled,
> -       .wait = dma_fence_default_wait,
>          .release = etnaviv_fence_release,
>   };

> --
> 2.17.0



-- 
greets
--
Christian Gmeiner, MSc

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

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-03 15:25:52)
> Almost everyone uses dma_fence_default_wait.
> 
> v2: Also remove the BUG_ON(!ops->wait) (Chris).

I just don't get the rationale for implicit over explicit.
-Chris

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-03 15:25:52)
> Almost everyone uses dma_fence_default_wait.
> 
> v2: Also remove the BUG_ON(!ops->wait) (Chris).

I just don't get the rationale for implicit over explicit.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-05-03 15:25:52)
> > Almost everyone uses dma_fence_default_wait.
> > 
> > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> 
> I just don't get the rationale for implicit over explicit.

Closer approximation of dwim semantics. There's been tons of patch series
all over drm and related places to get there, once we have a big pile of
implementations and know what the dwim semantics should be. Individually
they're all not much, in aggregate they substantially simplify simple
drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-05-03 15:25:52)
> > Almost everyone uses dma_fence_default_wait.
> > 
> > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> 
> I just don't get the rationale for implicit over explicit.

Closer approximation of dwim semantics. There's been tons of patch series
all over drm and related places to get there, once we have a big pile of
implementations and know what the dwim semantics should be. Individually
they're all not much, in aggregate they substantially simplify simple
drivers.
-Daniel
-- 
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 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > Almost everyone uses dma_fence_default_wait.
> > > 
> > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > 
> > I just don't get the rationale for implicit over explicit.
> 
> Closer approximation of dwim semantics. There's been tons of patch series
> all over drm and related places to get there, once we have a big pile of
> implementations and know what the dwim semantics should be. Individually
> they're all not much, in aggregate they substantially simplify simple
> drivers.

I also think clearer separation between optional optimization hooks and
mandatory core parts is useful in itself.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > Almost everyone uses dma_fence_default_wait.
> > > 
> > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > 
> > I just don't get the rationale for implicit over explicit.
> 
> Closer approximation of dwim semantics. There's been tons of patch series
> all over drm and related places to get there, once we have a big pile of
> implementations and know what the dwim semantics should be. Individually
> they're all not much, in aggregate they substantially simplify simple
> drivers.

I also think clearer separation between optional optimization hooks and
mandatory core parts is useful in itself.
-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

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-04 09:23:01)
> On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > Almost everyone uses dma_fence_default_wait.
> > > > 
> > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > 
> > > I just don't get the rationale for implicit over explicit.
> > 
> > Closer approximation of dwim semantics. There's been tons of patch series
> > all over drm and related places to get there, once we have a big pile of
> > implementations and know what the dwim semantics should be. Individually
> > they're all not much, in aggregate they substantially simplify simple
> > drivers.
> 
> I also think clearer separation between optional optimization hooks and
> mandatory core parts is useful in itself.

A new spelling of midlayer ;) I don't see the contradiction with a
driver saying use the default and simplicity. (I know which one the
compiler thinks is simpler ;)
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-05-04 09:23:01)
> > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > Almost everyone uses dma_fence_default_wait.
> > > > > 
> > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > 
> > > > I just don't get the rationale for implicit over explicit.
> > > 
> > > Closer approximation of dwim semantics. There's been tons of patch series
> > > all over drm and related places to get there, once we have a big pile of
> > > implementations and know what the dwim semantics should be. Individually
> > > they're all not much, in aggregate they substantially simplify simple
> > > drivers.
> > 
> > I also think clearer separation between optional optimization hooks and
> > mandatory core parts is useful in itself.
> 
> A new spelling of midlayer ;) I don't see the contradiction with a
> driver saying use the default and simplicity. (I know which one the
> compiler thinks is simpler ;)

If the compiler overhead is real then I guess it would makes to be
explicit. I don't expect that to be a problem though for a blocking
function.

I disagree on this being a midlayer - you can still overwrite everything
you please to. What it does help is people doing less copypasting (and
assorted bugs), at least in the grand scheme of things. And we do have a
_lot_ more random small drivers than just a few years ago. Reducing the
amount of explicit typing just to get default bahaviour has been an
ongoing theme for a few years now, and your objection here is about the
first that this is not a good idea. So I'm somewhat confused.

It's ofc not all that useful when looking only through the i915
perspective, where we overwrite almost everything anyway. But the
ecosystem is a bit bigger than just i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-05-04 09:23:01)
> > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > Almost everyone uses dma_fence_default_wait.
> > > > > 
> > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > 
> > > > I just don't get the rationale for implicit over explicit.
> > > 
> > > Closer approximation of dwim semantics. There's been tons of patch series
> > > all over drm and related places to get there, once we have a big pile of
> > > implementations and know what the dwim semantics should be. Individually
> > > they're all not much, in aggregate they substantially simplify simple
> > > drivers.
> > 
> > I also think clearer separation between optional optimization hooks and
> > mandatory core parts is useful in itself.
> 
> A new spelling of midlayer ;) I don't see the contradiction with a
> driver saying use the default and simplicity. (I know which one the
> compiler thinks is simpler ;)

If the compiler overhead is real then I guess it would makes to be
explicit. I don't expect that to be a problem though for a blocking
function.

I disagree on this being a midlayer - you can still overwrite everything
you please to. What it does help is people doing less copypasting (and
assorted bugs), at least in the grand scheme of things. And we do have a
_lot_ more random small drivers than just a few years ago. Reducing the
amount of explicit typing just to get default bahaviour has been an
ongoing theme for a few years now, and your objection here is about the
first that this is not a good idea. So I'm somewhat confused.

It's ofc not all that useful when looking only through the i915
perspective, where we overwrite almost everything anyway. But the
ecosystem is a bit bigger than just i915.
-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

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Chris Wilson]

Quoting Daniel Vetter (2018-05-04 09:57:59)
> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-05-04 09:23:01)
> > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > > Almost everyone uses dma_fence_default_wait.
> > > > > > 
> > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > > 
> > > > > I just don't get the rationale for implicit over explicit.
> > > > 
> > > > Closer approximation of dwim semantics. There's been tons of patch series
> > > > all over drm and related places to get there, once we have a big pile of
> > > > implementations and know what the dwim semantics should be. Individually
> > > > they're all not much, in aggregate they substantially simplify simple
> > > > drivers.
> > > 
> > > I also think clearer separation between optional optimization hooks and
> > > mandatory core parts is useful in itself.
> > 
> > A new spelling of midlayer ;) I don't see the contradiction with a
> > driver saying use the default and simplicity. (I know which one the
> > compiler thinks is simpler ;)
> 
> If the compiler overhead is real then I guess it would makes to be
> explicit. I don't expect that to be a problem though for a blocking
> function.
> 
> I disagree on this being a midlayer - you can still overwrite everything
> you please to. What it does help is people doing less copypasting (and
> assorted bugs), at least in the grand scheme of things. And we do have a
> _lot_ more random small drivers than just a few years ago. Reducing the
> amount of explicit typing just to get default bahaviour has been an
> ongoing theme for a few years now, and your objection here is about the
> first that this is not a good idea. So I'm somewhat confused.

I'm just saying I don't see any rationale for this patch.

	"Almost everyone uses dma_fence_default_wait."

Why change?

Making it look simpler on the surface, so that you don't have to think
about things straight away? I understand the appeal, but I do worry
about it just being an illusion. (Cutting and pasting a line saying
.wait = default_wait, doesn't feel that onerous, as you likely cut and
paste the ops anyway, and at the very least you are reminded about some
of the interactions. You could even have default initializers and/or
magic macros to hide the cut and paste; maybe a simple_dma_fence [now
that's a midlayer!] but I haven't looked.)
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Daniel Vetter (2018-05-04 09:57:59)
>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>> > Quoting Daniel Vetter (2018-05-04 09:23:01)
>> > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>> > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>> > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
>> > > > > > Almost everyone uses dma_fence_default_wait.
>> > > > > >
>> > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
>> > > > >
>> > > > > I just don't get the rationale for implicit over explicit.
>> > > >
>> > > > Closer approximation of dwim semantics. There's been tons of patch series
>> > > > all over drm and related places to get there, once we have a big pile of
>> > > > implementations and know what the dwim semantics should be. Individually
>> > > > they're all not much, in aggregate they substantially simplify simple
>> > > > drivers.
>> > >
>> > > I also think clearer separation between optional optimization hooks and
>> > > mandatory core parts is useful in itself.
>> >
>> > A new spelling of midlayer ;) I don't see the contradiction with a
>> > driver saying use the default and simplicity. (I know which one the
>> > compiler thinks is simpler ;)
>>
>> If the compiler overhead is real then I guess it would makes to be
>> explicit. I don't expect that to be a problem though for a blocking
>> function.
>>
>> I disagree on this being a midlayer - you can still overwrite everything
>> you please to. What it does help is people doing less copypasting (and
>> assorted bugs), at least in the grand scheme of things. And we do have a
>> _lot_ more random small drivers than just a few years ago. Reducing the
>> amount of explicit typing just to get default bahaviour has been an
>> ongoing theme for a few years now, and your objection here is about the
>> first that this is not a good idea. So I'm somewhat confused.
>
> I'm just saying I don't see any rationale for this patch.
>
>         "Almost everyone uses dma_fence_default_wait."
>
> Why change?
>
> Making it look simpler on the surface, so that you don't have to think
> about things straight away? I understand the appeal, but I do worry
> about it just being an illusion. (Cutting and pasting a line saying
> .wait = default_wait, doesn't feel that onerous, as you likely cut and
> paste the ops anyway, and at the very least you are reminded about some
> of the interactions. You could even have default initializers and/or
> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> that's a midlayer!] but I haven't looked.)

In really monolithic vtables like drm_driver we do use default
function macros, so you type 1 line, get them all. But dma_fence_ops
is pretty small, and most drivers only implement a few callbacks. Also
note that e.g. the ->release callback already works like that, so this
pattern is there already. I simply extended it to ->wait and
->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
you can still wrap dma_fence_default_wait if you wish to do so.

But I just realized that I didn't clean out the optional release
hooks, I guess I should do that too (for the few cases it's not yet
done) and respin.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Daniel Vetter (2018-05-04 09:57:59)
>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>> > Quoting Daniel Vetter (2018-05-04 09:23:01)
>> > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>> > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>> > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
>> > > > > > Almost everyone uses dma_fence_default_wait.
>> > > > > >
>> > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
>> > > > >
>> > > > > I just don't get the rationale for implicit over explicit.
>> > > >
>> > > > Closer approximation of dwim semantics. There's been tons of patch series
>> > > > all over drm and related places to get there, once we have a big pile of
>> > > > implementations and know what the dwim semantics should be. Individually
>> > > > they're all not much, in aggregate they substantially simplify simple
>> > > > drivers.
>> > >
>> > > I also think clearer separation between optional optimization hooks and
>> > > mandatory core parts is useful in itself.
>> >
>> > A new spelling of midlayer ;) I don't see the contradiction with a
>> > driver saying use the default and simplicity. (I know which one the
>> > compiler thinks is simpler ;)
>>
>> If the compiler overhead is real then I guess it would makes to be
>> explicit. I don't expect that to be a problem though for a blocking
>> function.
>>
>> I disagree on this being a midlayer - you can still overwrite everything
>> you please to. What it does help is people doing less copypasting (and
>> assorted bugs), at least in the grand scheme of things. And we do have a
>> _lot_ more random small drivers than just a few years ago. Reducing the
>> amount of explicit typing just to get default bahaviour has been an
>> ongoing theme for a few years now, and your objection here is about the
>> first that this is not a good idea. So I'm somewhat confused.
>
> I'm just saying I don't see any rationale for this patch.
>
>         "Almost everyone uses dma_fence_default_wait."
>
> Why change?
>
> Making it look simpler on the surface, so that you don't have to think
> about things straight away? I understand the appeal, but I do worry
> about it just being an illusion. (Cutting and pasting a line saying
> .wait = default_wait, doesn't feel that onerous, as you likely cut and
> paste the ops anyway, and at the very least you are reminded about some
> of the interactions. You could even have default initializers and/or
> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> that's a midlayer!] but I haven't looked.)

In really monolithic vtables like drm_driver we do use default
function macros, so you type 1 line, get them all. But dma_fence_ops
is pretty small, and most drivers only implement a few callbacks. Also
note that e.g. the ->release callback already works like that, so this
pattern is there already. I simply extended it to ->wait and
->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
you can still wrap dma_fence_default_wait if you wish to do so.

But I just realized that I didn't clean out the optional release
hooks, I guess I should do that too (for the few cases it's not yet
done) and respin.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Christian König]

Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> Quoting Daniel Vetter (2018-05-04 09:57:59)
>>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>>>> Quoting Daniel Vetter (2018-05-04 09:23:01)
>>>>> On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>>>>>> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>>>>>>> Quoting Daniel Vetter (2018-05-03 15:25:52)
>>>>>>>> Almost everyone uses dma_fence_default_wait.
>>>>>>>>
>>>>>>>> v2: Also remove the BUG_ON(!ops->wait) (Chris).
>>>>>>> I just don't get the rationale for implicit over explicit.
>>>>>> Closer approximation of dwim semantics. There's been tons of patch series
>>>>>> all over drm and related places to get there, once we have a big pile of
>>>>>> implementations and know what the dwim semantics should be. Individually
>>>>>> they're all not much, in aggregate they substantially simplify simple
>>>>>> drivers.
>>>>> I also think clearer separation between optional optimization hooks and
>>>>> mandatory core parts is useful in itself.
>>>> A new spelling of midlayer ;) I don't see the contradiction with a
>>>> driver saying use the default and simplicity. (I know which one the
>>>> compiler thinks is simpler ;)
>>> If the compiler overhead is real then I guess it would makes to be
>>> explicit. I don't expect that to be a problem though for a blocking
>>> function.
>>>
>>> I disagree on this being a midlayer - you can still overwrite everything
>>> you please to. What it does help is people doing less copypasting (and
>>> assorted bugs), at least in the grand scheme of things. And we do have a
>>> _lot_ more random small drivers than just a few years ago. Reducing the
>>> amount of explicit typing just to get default bahaviour has been an
>>> ongoing theme for a few years now, and your objection here is about the
>>> first that this is not a good idea. So I'm somewhat confused.
>> I'm just saying I don't see any rationale for this patch.
>>
>>          "Almost everyone uses dma_fence_default_wait."
>>
>> Why change?
>>
>> Making it look simpler on the surface, so that you don't have to think
>> about things straight away? I understand the appeal, but I do worry
>> about it just being an illusion. (Cutting and pasting a line saying
>> .wait = default_wait, doesn't feel that onerous, as you likely cut and
>> paste the ops anyway, and at the very least you are reminded about some
>> of the interactions. You could even have default initializers and/or
>> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
>> that's a midlayer!] but I haven't looked.)
> In really monolithic vtables like drm_driver we do use default
> function macros, so you type 1 line, get them all. But dma_fence_ops
> is pretty small, and most drivers only implement a few callbacks. Also
> note that e.g. the ->release callback already works like that, so this
> pattern is there already. I simply extended it to ->wait and
> ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> you can still wrap dma_fence_default_wait if you wish to do so.
>
> But I just realized that I didn't clean out the optional release
> hooks, I guess I should do that too (for the few cases it's not yet
> done) and respin.

I kind of agree with Chris here, but also see the practical problem to 
copy the default function in all the implementations.

We had the same problem in TTM and I also don't really like the result 
to always have that "if (some_callback) default(); else some_callback();".

Might be that the run time overhead is negligible, but it doesn't feels 
right from the coding style perspective.

Christian.

> -Daniel

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Christian König]

Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> Quoting Daniel Vetter (2018-05-04 09:57:59)
>>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>>>> Quoting Daniel Vetter (2018-05-04 09:23:01)
>>>>> On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>>>>>> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>>>>>>> Quoting Daniel Vetter (2018-05-03 15:25:52)
>>>>>>>> Almost everyone uses dma_fence_default_wait.
>>>>>>>>
>>>>>>>> v2: Also remove the BUG_ON(!ops->wait) (Chris).
>>>>>>> I just don't get the rationale for implicit over explicit.
>>>>>> Closer approximation of dwim semantics. There's been tons of patch series
>>>>>> all over drm and related places to get there, once we have a big pile of
>>>>>> implementations and know what the dwim semantics should be. Individually
>>>>>> they're all not much, in aggregate they substantially simplify simple
>>>>>> drivers.
>>>>> I also think clearer separation between optional optimization hooks and
>>>>> mandatory core parts is useful in itself.
>>>> A new spelling of midlayer ;) I don't see the contradiction with a
>>>> driver saying use the default and simplicity. (I know which one the
>>>> compiler thinks is simpler ;)
>>> If the compiler overhead is real then I guess it would makes to be
>>> explicit. I don't expect that to be a problem though for a blocking
>>> function.
>>>
>>> I disagree on this being a midlayer - you can still overwrite everything
>>> you please to. What it does help is people doing less copypasting (and
>>> assorted bugs), at least in the grand scheme of things. And we do have a
>>> _lot_ more random small drivers than just a few years ago. Reducing the
>>> amount of explicit typing just to get default bahaviour has been an
>>> ongoing theme for a few years now, and your objection here is about the
>>> first that this is not a good idea. So I'm somewhat confused.
>> I'm just saying I don't see any rationale for this patch.
>>
>>          "Almost everyone uses dma_fence_default_wait."
>>
>> Why change?
>>
>> Making it look simpler on the surface, so that you don't have to think
>> about things straight away? I understand the appeal, but I do worry
>> about it just being an illusion. (Cutting and pasting a line saying
>> .wait = default_wait, doesn't feel that onerous, as you likely cut and
>> paste the ops anyway, and at the very least you are reminded about some
>> of the interactions. You could even have default initializers and/or
>> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
>> that's a midlayer!] but I haven't looked.)
> In really monolithic vtables like drm_driver we do use default
> function macros, so you type 1 line, get them all. But dma_fence_ops
> is pretty small, and most drivers only implement a few callbacks. Also
> note that e.g. the ->release callback already works like that, so this
> pattern is there already. I simply extended it to ->wait and
> ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> you can still wrap dma_fence_default_wait if you wish to do so.
>
> But I just realized that I didn't clean out the optional release
> hooks, I guess I should do that too (for the few cases it's not yet
> done) and respin.

I kind of agree with Chris here, but also see the practical problem to 
copy the default function in all the implementations.

We had the same problem in TTM and I also don't really like the result 
to always have that "if (some_callback) default(); else some_callback();".

Might be that the run time overhead is negligible, but it doesn't feels 
right from the coding style perspective.

Christian.

> -Daniel

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

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
> Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> > On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Quoting Daniel Vetter (2018-05-04 09:57:59)
> > > > On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> > > > > Quoting Daniel Vetter (2018-05-04 09:23:01)
> > > > > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > > > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > > > > > Almost everyone uses dma_fence_default_wait.
> > > > > > > > > 
> > > > > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > > > > > I just don't get the rationale for implicit over explicit.
> > > > > > > Closer approximation of dwim semantics. There's been tons of patch series
> > > > > > > all over drm and related places to get there, once we have a big pile of
> > > > > > > implementations and know what the dwim semantics should be. Individually
> > > > > > > they're all not much, in aggregate they substantially simplify simple
> > > > > > > drivers.
> > > > > > I also think clearer separation between optional optimization hooks and
> > > > > > mandatory core parts is useful in itself.
> > > > > A new spelling of midlayer ;) I don't see the contradiction with a
> > > > > driver saying use the default and simplicity. (I know which one the
> > > > > compiler thinks is simpler ;)
> > > > If the compiler overhead is real then I guess it would makes to be
> > > > explicit. I don't expect that to be a problem though for a blocking
> > > > function.
> > > > 
> > > > I disagree on this being a midlayer - you can still overwrite everything
> > > > you please to. What it does help is people doing less copypasting (and
> > > > assorted bugs), at least in the grand scheme of things. And we do have a
> > > > _lot_ more random small drivers than just a few years ago. Reducing the
> > > > amount of explicit typing just to get default bahaviour has been an
> > > > ongoing theme for a few years now, and your objection here is about the
> > > > first that this is not a good idea. So I'm somewhat confused.
> > > I'm just saying I don't see any rationale for this patch.
> > > 
> > >          "Almost everyone uses dma_fence_default_wait."
> > > 
> > > Why change?
> > > 
> > > Making it look simpler on the surface, so that you don't have to think
> > > about things straight away? I understand the appeal, but I do worry
> > > about it just being an illusion. (Cutting and pasting a line saying
> > > .wait = default_wait, doesn't feel that onerous, as you likely cut and
> > > paste the ops anyway, and at the very least you are reminded about some
> > > of the interactions. You could even have default initializers and/or
> > > magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> > > that's a midlayer!] but I haven't looked.)
> > In really monolithic vtables like drm_driver we do use default
> > function macros, so you type 1 line, get them all. But dma_fence_ops
> > is pretty small, and most drivers only implement a few callbacks. Also
> > note that e.g. the ->release callback already works like that, so this
> > pattern is there already. I simply extended it to ->wait and
> > ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> > you can still wrap dma_fence_default_wait if you wish to do so.
> > 
> > But I just realized that I didn't clean out the optional release
> > hooks, I guess I should do that too (for the few cases it's not yet
> > done) and respin.
> 
> I kind of agree with Chris here, but also see the practical problem to copy
> the default function in all the implementations.
> 
> We had the same problem in TTM and I also don't really like the result to
> always have that "if (some_callback) default(); else some_callback();".
> 
> Might be that the run time overhead is negligible, but it doesn't feels
> right from the coding style perspective.

Hm, maybe I've seen too much bad code, but modeset helpers is choke full
of exactly that pattern. It's imo also a trade-off. If you have a fairly
specialized library like ttm that's used by relatively few things, doing
everything explicitly is probably better. It's also where kms started out
from.

But if you have a huge pile of fairly simple drivers, imo the balance
starts to tip the other way, and a bit of additional logic in the shared
code to make all the implementations a notch simpler is good. If we
wouldn't have acquired quite a pile of dma_fence implementations I
wouldn't have bothered with all this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
> Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> > On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Quoting Daniel Vetter (2018-05-04 09:57:59)
> > > > On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> > > > > Quoting Daniel Vetter (2018-05-04 09:23:01)
> > > > > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > > > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > > > > > Almost everyone uses dma_fence_default_wait.
> > > > > > > > > 
> > > > > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > > > > > I just don't get the rationale for implicit over explicit.
> > > > > > > Closer approximation of dwim semantics. There's been tons of patch series
> > > > > > > all over drm and related places to get there, once we have a big pile of
> > > > > > > implementations and know what the dwim semantics should be. Individually
> > > > > > > they're all not much, in aggregate they substantially simplify simple
> > > > > > > drivers.
> > > > > > I also think clearer separation between optional optimization hooks and
> > > > > > mandatory core parts is useful in itself.
> > > > > A new spelling of midlayer ;) I don't see the contradiction with a
> > > > > driver saying use the default and simplicity. (I know which one the
> > > > > compiler thinks is simpler ;)
> > > > If the compiler overhead is real then I guess it would makes to be
> > > > explicit. I don't expect that to be a problem though for a blocking
> > > > function.
> > > > 
> > > > I disagree on this being a midlayer - you can still overwrite everything
> > > > you please to. What it does help is people doing less copypasting (and
> > > > assorted bugs), at least in the grand scheme of things. And we do have a
> > > > _lot_ more random small drivers than just a few years ago. Reducing the
> > > > amount of explicit typing just to get default bahaviour has been an
> > > > ongoing theme for a few years now, and your objection here is about the
> > > > first that this is not a good idea. So I'm somewhat confused.
> > > I'm just saying I don't see any rationale for this patch.
> > > 
> > >          "Almost everyone uses dma_fence_default_wait."
> > > 
> > > Why change?
> > > 
> > > Making it look simpler on the surface, so that you don't have to think
> > > about things straight away? I understand the appeal, but I do worry
> > > about it just being an illusion. (Cutting and pasting a line saying
> > > .wait = default_wait, doesn't feel that onerous, as you likely cut and
> > > paste the ops anyway, and at the very least you are reminded about some
> > > of the interactions. You could even have default initializers and/or
> > > magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> > > that's a midlayer!] but I haven't looked.)
> > In really monolithic vtables like drm_driver we do use default
> > function macros, so you type 1 line, get them all. But dma_fence_ops
> > is pretty small, and most drivers only implement a few callbacks. Also
> > note that e.g. the ->release callback already works like that, so this
> > pattern is there already. I simply extended it to ->wait and
> > ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> > you can still wrap dma_fence_default_wait if you wish to do so.
> > 
> > But I just realized that I didn't clean out the optional release
> > hooks, I guess I should do that too (for the few cases it's not yet
> > done) and respin.
> 
> I kind of agree with Chris here, but also see the practical problem to copy
> the default function in all the implementations.
> 
> We had the same problem in TTM and I also don't really like the result to
> always have that "if (some_callback) default(); else some_callback();".
> 
> Might be that the run time overhead is negligible, but it doesn't feels
> right from the coding style perspective.

Hm, maybe I've seen too much bad code, but modeset helpers is choke full
of exactly that pattern. It's imo also a trade-off. If you have a fairly
specialized library like ttm that's used by relatively few things, doing
everything explicitly is probably better. It's also where kms started out
from.

But if you have a huge pile of fairly simple drivers, imo the balance
starts to tip the other way, and a bit of additional logic in the shared
code to make all the implementations a notch simpler is good. If we
wouldn't have acquired quite a pile of dma_fence implementations I
wouldn't have bothered with all this.
-Daniel
-- 
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

[PATCH] 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.17.0

[PATCH] 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.17.0

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

[PATCH] drm/vc4: 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.

Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_fence.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_fence.c b/drivers/gpu/drm/vc4/vc4_fence.c
index dbf5a5a5d5f5..580214e2158c 100644
--- a/drivers/gpu/drm/vc4/vc4_fence.c
+++ b/drivers/gpu/drm/vc4/vc4_fence.c
@@ -33,11 +33,6 @@ static const char *vc4_fence_get_timeline_name(struct dma_fence *fence)
 	return "vc4-v3d";
 }
 
-static bool vc4_fence_enable_signaling(struct dma_fence *fence)
-{
-	return true;
-}
-
 static bool vc4_fence_signaled(struct dma_fence *fence)
 {
 	struct vc4_fence *f = to_vc4_fence(fence);
@@ -49,8 +44,5 @@ static bool vc4_fence_signaled(struct dma_fence *fence)
 const struct dma_fence_ops vc4_fence_ops = {
 	.get_driver_name = vc4_fence_get_driver_name,
 	.get_timeline_name = vc4_fence_get_timeline_name,
-	.enable_signaling = vc4_fence_enable_signaling,
 	.signaled = vc4_fence_signaled,
-	.wait = dma_fence_default_wait,
-	.release = dma_fence_free,
 };
-- 
2.17.0

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

[PATCH] 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.17.0

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

✗ Fi.CI.BAT: failure for dma-fence cleanup v2 (rev3)

[Patchwork]

== Series Details ==

Series: dma-fence cleanup v2 (rev3)
URL   : https://patchwork.freedesktop.org/series/42640/
State : failure

== Summary ==

Applying: dma-fence: remove fill_driver_data callback
Applying: dma-fence: Make ->enable_signaling optional
Applying: dma-fence: Allow wait_any_timeout for all fences
Applying: dma-fence: Make ->wait callback optional
Applying: drm/amdgpu: Remove unecessary dma_fence_ops
Applying: drm: Remove unecessary dma_fence_ops
Applying: drm/etnaviv: Remove unecessary dma_fence_ops
Applying: drm/i915: Remove unecessary dma_fence_ops
Applying: drm/msm: Remove unecessary dma_fence_ops
Applying: drm/nouveau: Remove unecessary dma_fence_ops
Applying: drm/qxl: Remove unecessary dma_fence_ops
Applying: drm/vc4: Remove unecessary dma_fence_ops
Applying: drm/vgem: Remove unecessary dma_fence_ops
Applying: drm/virtio: Remove unecessary dma_fence_ops
Applying: dma-fence: Polish kernel-doc for dma-fence.c
error: sha1 information is lacking or useless (drivers/dma-buf/dma-fence.c).
error: could not build fake ancestor
Patch failed at 0015 dma-fence: Polish kernel-doc for dma-fence.c
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

[PATCH] 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.17.0

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

[PATCH] dma-fence: Make ->enable_signaling optional

[Daniel Vetter]

Many drivers have a trivial implementation for ->enable_signaling.
Let's make it optional by assuming that signalling is already
available when the callback isn't present.

v2: Don't do the trick to set the ENABLE_SIGNAL_BIT
unconditionally, it results in an expensive spinlock take for
everyone. Instead just check if the callback is present. Suggested by
Maarten.

Also move misplaced kerneldoc hunk to the right patch.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
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
---
 drivers/dma-buf/dma-fence.c | 9 +++++----
 include/linux/dma-fence.h   | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 4edb9fd3cf47..dd01a1720be9 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -200,7 +200,8 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence)
 
 	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
 			      &fence->flags) &&
-	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
+	    fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		spin_lock_irqsave(fence->lock, flags);
@@ -260,7 +261,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		ret = -ENOENT;
-	else if (!was_set) {
+	else if (!was_set && fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		if (!fence->ops->enable_signaling(fence)) {
@@ -388,7 +389,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		goto out;
 
-	if (!was_set) {
+	if (!was_set && fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		if (!fence->ops->enable_signaling(fence)) {
@@ -560,7 +561,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
+	BUG_ON(!ops || !ops->wait ||
 	       !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 111aefe1c956..c053d19e1e24 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -166,7 +166,8 @@ struct dma_fence_ops {
 	 * released when the fence is signalled (through e.g. the interrupt
 	 * handler).
 	 *
-	 * This callback is mandatory.
+	 * This callback is optional. If this callback is not present, then the
+	 * driver must always have signaling enabled.
 	 */
 	bool (*enable_signaling)(struct dma_fence *fence);
 
-- 
2.17.0

[PATCH] dma-fence: Make ->enable_signaling optional

[Daniel Vetter]

Many drivers have a trivial implementation for ->enable_signaling.
Let's make it optional by assuming that signalling is already
available when the callback isn't present.

v2: Don't do the trick to set the ENABLE_SIGNAL_BIT
unconditionally, it results in an expensive spinlock take for
everyone. Instead just check if the callback is present. Suggested by
Maarten.

Also move misplaced kerneldoc hunk to the right patch.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
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
---
 drivers/dma-buf/dma-fence.c | 9 +++++----
 include/linux/dma-fence.h   | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 4edb9fd3cf47..dd01a1720be9 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -200,7 +200,8 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence)
 
 	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
 			      &fence->flags) &&
-	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
+	    fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		spin_lock_irqsave(fence->lock, flags);
@@ -260,7 +261,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		ret = -ENOENT;
-	else if (!was_set) {
+	else if (!was_set && fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		if (!fence->ops->enable_signaling(fence)) {
@@ -388,7 +389,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		goto out;
 
-	if (!was_set) {
+	if (!was_set && fence->ops->enable_signaling) {
 		trace_dma_fence_enable_signal(fence);
 
 		if (!fence->ops->enable_signaling(fence)) {
@@ -560,7 +561,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	       spinlock_t *lock, u64 context, unsigned seqno)
 {
 	BUG_ON(!lock);
-	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
+	BUG_ON(!ops || !ops->wait ||
 	       !ops->get_driver_name || !ops->get_timeline_name);
 
 	kref_init(&fence->refcount);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 111aefe1c956..c053d19e1e24 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -166,7 +166,8 @@ struct dma_fence_ops {
 	 * released when the fence is signalled (through e.g. the interrupt
 	 * handler).
 	 *
-	 * This callback is mandatory.
+	 * This callback is optional. If this callback is not present, then the
+	 * driver must always have signaling enabled.
 	 */
 	bool (*enable_signaling)(struct dma_fence *fence);
 
-- 
2.17.0

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

✗ Fi.CI.CHECKPATCH: warning for dma-fence cleanup v2 (rev6)

[Patchwork]

== Series Details ==

Series: dma-fence cleanup v2 (rev6)
URL   : https://patchwork.freedesktop.org/series/42640/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
96d5c610719e dma-fence: remove fill_driver_data callback
81dd25d339aa dma-fence: Make ->enable_signaling optional
-:65: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#65: FILE: drivers/dma-buf/dma-fence.c:564:
+	BUG_ON(!ops || !ops->wait ||

total: 0 errors, 1 warnings, 0 checks, 42 lines checked
86afc920fcc1 dma-fence: Allow wait_any_timeout for all fences
-:11: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit a519435a9659 ("dma-buf/fence: add fence_wait_any_timeout function v2")'
#11: 
commit a519435a96597d8cd96123246fea4ae5a6c90b02

total: 1 errors, 0 warnings, 0 checks, 11 lines checked
7b7a27f48b3b dma-fence: Make ->wait callback optional
-:55: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#55: FILE: drivers/dma-buf/dma-fence.c:562:
+	BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);

total: 0 errors, 1 warnings, 0 checks, 60 lines checked
35812b01e77e drm/amdgpu: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/amdgpu: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 14 lines checked
0c0e35c2c937 drm: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 52 lines checked
56331afb0615 drm/etnaviv: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/etnaviv: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 20 lines checked
eb620f1b920c drm/i915: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/i915: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 36 lines checked
15f8146aba7f drm/msm: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/msm: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 21 lines checked
5fa76c9745a4 drm/nouveau: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/nouveau: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 6 lines checked
27ca31cde11c drm/qxl: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/qxl: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 19 lines checked
e7201791ca1d drm/vc4: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/vc4: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 19 lines checked
d6f42dfaa2a7 drm/vgem: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/vgem: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 27 lines checked
6d946c9e6ffa drm/virtio: Remove unecessary dma_fence_ops
-:4: WARNING:TYPO_SPELLING: 'unecessary' may be misspelled - perhaps 'unnecessary'?
#4: 
Subject: [PATCH] drm/virtio: Remove unecessary dma_fence_ops

total: 0 errors, 1 warnings, 0 checks, 20 lines checked
55843f9b52c9 dma-fence: Polish kernel-doc for dma-fence.c

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

✓ Fi.CI.BAT: success for dma-fence cleanup v2 (rev6)

[Patchwork]

== Series Details ==

Series: dma-fence cleanup v2 (rev6)
URL   : https://patchwork.freedesktop.org/series/42640/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4146 -> Patchwork_8909 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42640/revisions/6/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8909 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-cnl-psr:         PASS -> FAIL (fdo#103481)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-4200u:       DMESG-FAIL (fdo#102614) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713


== Participating hosts (39 -> 37) ==

  Additional (1): fi-skl-gvtdvm 
  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4146 -> Patchwork_8909

  CI_DRM_4146: be068d4cb8a8b4709af83ffa993d8fc8889f0230 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4461: f772d9a910130b3aec8efa4f09ed723618fae656 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8909: 55843f9b52c924508b1940b3ce954e7b3c442b6b @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4461: 55207ea5154dfaa6d2c128124c50e3be4f9b6440 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

55843f9b52c9 dma-fence: Polish kernel-doc for dma-fence.c
6d946c9e6ffa drm/virtio: Remove unecessary dma_fence_ops
d6f42dfaa2a7 drm/vgem: Remove unecessary dma_fence_ops
e7201791ca1d drm/vc4: Remove unecessary dma_fence_ops
27ca31cde11c drm/qxl: Remove unecessary dma_fence_ops
5fa76c9745a4 drm/nouveau: Remove unecessary dma_fence_ops
15f8146aba7f drm/msm: Remove unecessary dma_fence_ops
eb620f1b920c drm/i915: Remove unecessary dma_fence_ops
56331afb0615 drm/etnaviv: Remove unecessary dma_fence_ops
0c0e35c2c937 drm: Remove unecessary dma_fence_ops
35812b01e77e drm/amdgpu: Remove unecessary dma_fence_ops
7b7a27f48b3b dma-fence: Make ->wait callback optional
86afc920fcc1 dma-fence: Allow wait_any_timeout for all fences
81dd25d339aa dma-fence: Make ->enable_signaling optional
96d5c610719e dma-fence: remove fill_driver_data callback

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8909/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for dma-fence cleanup v2 (rev6)

[Patchwork]

== Series Details ==

Series: dma-fence cleanup v2 (rev6)
URL   : https://patchwork.freedesktop.org/series/42640/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4146_full -> Patchwork_8909_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8909_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8909_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42640/revisions/6/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8909_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-blt:
      shard-kbl:          SKIP -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-onoff:
      shard-hsw:          PASS -> SKIP

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    

  === Piglit changes ===

    ==== Possible regressions ====

    spec@!opengl es 3.0@gles-3.0-transform-feedback-uniform-buffer-object:
      {pig-snb-2600}:     NOTRUN -> FAIL +11

    
== Known issues ==

  Here are the changes found in Patchwork_8909_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@kms_flip@2x-plain-flip-ts-check-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@absolute-wf_vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#106087)

    igt@kms_rotation_crc@sprite-rotation-90-pos-100-0:
      shard-apl:          PASS -> FAIL (fdo#103925)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      shard-hsw:          FAIL (fdo#103928) -> PASS

    igt@kms_rotation_crc@primary-rotation-90:
      shard-apl:          FAIL (fdo#103925) -> PASS

    igt@kms_vblank@pipe-c-ts-continuation-suspend:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106087 https://bugs.freedesktop.org/show_bug.cgi?id=106087


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4146 -> Patchwork_8909

  CI_DRM_4146: be068d4cb8a8b4709af83ffa993d8fc8889f0230 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4461: f772d9a910130b3aec8efa4f09ed723618fae656 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8909: 55843f9b52c924508b1940b3ce954e7b3c442b6b @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4461: 55207ea5154dfaa6d2c128124c50e3be4f9b6440 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8909/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] dma-fence: Make ->enable_signaling optional

[Maarten Lankhorst]

Op 04-05-18 om 16:10 schreef Daniel Vetter:
> Many drivers have a trivial implementation for ->enable_signaling.
> Let's make it optional by assuming that signalling is already
> available when the callback isn't present.
>
> v2: Don't do the trick to set the ENABLE_SIGNAL_BIT
> unconditionally, it results in an expensive spinlock take for
> everyone. Instead just check if the callback is present. Suggested by
> Maarten.
>
> Also move misplaced kerneldoc hunk to the right patch.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 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
> ---
>  drivers/dma-buf/dma-fence.c | 9 +++++----
>  include/linux/dma-fence.h   | 3 ++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 4edb9fd3cf47..dd01a1720be9 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -200,7 +200,8 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>  
>  	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>  			      &fence->flags) &&
> -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
> +	    fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		spin_lock_irqsave(fence->lock, flags);
> @@ -260,7 +261,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
>  
>  	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  		ret = -ENOENT;
> -	else if (!was_set) {
> +	else if (!was_set && fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		if (!fence->ops->enable_signaling(fence)) {
> @@ -388,7 +389,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  		goto out;
>  
> -	if (!was_set) {
> +	if (!was_set && fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		if (!fence->ops->enable_signaling(fence)) {
> @@ -560,7 +561,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>  	       spinlock_t *lock, u64 context, unsigned seqno)
>  {
>  	BUG_ON(!lock);
> -	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> +	BUG_ON(!ops || !ops->wait ||
>  	       !ops->get_driver_name || !ops->get_timeline_name);
>  
>  	kref_init(&fence->refcount);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 111aefe1c956..c053d19e1e24 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -166,7 +166,8 @@ struct dma_fence_ops {
>  	 * released when the fence is signalled (through e.g. the interrupt
>  	 * handler).
>  	 *
> -	 * This callback is mandatory.
> +	 * This callback is optional. If this callback is not present, then the
> +	 * driver must always have signaling enabled.
>  	 */
>  	bool (*enable_signaling)(struct dma_fence *fence);
>  

Much better. :)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Re: [PATCH] dma-fence: Make ->enable_signaling optional

[Maarten Lankhorst]

Op 04-05-18 om 16:10 schreef Daniel Vetter:
> Many drivers have a trivial implementation for ->enable_signaling.
> Let's make it optional by assuming that signalling is already
> available when the callback isn't present.
>
> v2: Don't do the trick to set the ENABLE_SIGNAL_BIT
> unconditionally, it results in an expensive spinlock take for
> everyone. Instead just check if the callback is present. Suggested by
> Maarten.
>
> Also move misplaced kerneldoc hunk to the right patch.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 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
> ---
>  drivers/dma-buf/dma-fence.c | 9 +++++----
>  include/linux/dma-fence.h   | 3 ++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 4edb9fd3cf47..dd01a1720be9 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -200,7 +200,8 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>  
>  	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>  			      &fence->flags) &&
> -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
> +	    fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		spin_lock_irqsave(fence->lock, flags);
> @@ -260,7 +261,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
>  
>  	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  		ret = -ENOENT;
> -	else if (!was_set) {
> +	else if (!was_set && fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		if (!fence->ops->enable_signaling(fence)) {
> @@ -388,7 +389,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  		goto out;
>  
> -	if (!was_set) {
> +	if (!was_set && fence->ops->enable_signaling) {
>  		trace_dma_fence_enable_signal(fence);
>  
>  		if (!fence->ops->enable_signaling(fence)) {
> @@ -560,7 +561,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>  	       spinlock_t *lock, u64 context, unsigned seqno)
>  {
>  	BUG_ON(!lock);
> -	BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> +	BUG_ON(!ops || !ops->wait ||
>  	       !ops->get_driver_name || !ops->get_timeline_name);
>  
>  	kref_init(&fence->refcount);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 111aefe1c956..c053d19e1e24 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -166,7 +166,8 @@ struct dma_fence_ops {
>  	 * released when the fence is signalled (through e.g. the interrupt
>  	 * handler).
>  	 *
> -	 * This callback is mandatory.
> +	 * This callback is optional. If this callback is not present, then the
> +	 * driver must always have signaling enabled.
>  	 */
>  	bool (*enable_signaling)(struct dma_fence *fence);
>  

Much better. :)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
> > Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> > > On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > Quoting Daniel Vetter (2018-05-04 09:57:59)
> > > > > On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> > > > > > Quoting Daniel Vetter (2018-05-04 09:23:01)
> > > > > > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > > > > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > > > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > > > > > > Almost everyone uses dma_fence_default_wait.
> > > > > > > > > > 
> > > > > > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > > > > > > I just don't get the rationale for implicit over explicit.
> > > > > > > > Closer approximation of dwim semantics. There's been tons of patch series
> > > > > > > > all over drm and related places to get there, once we have a big pile of
> > > > > > > > implementations and know what the dwim semantics should be. Individually
> > > > > > > > they're all not much, in aggregate they substantially simplify simple
> > > > > > > > drivers.
> > > > > > > I also think clearer separation between optional optimization hooks and
> > > > > > > mandatory core parts is useful in itself.
> > > > > > A new spelling of midlayer ;) I don't see the contradiction with a
> > > > > > driver saying use the default and simplicity. (I know which one the
> > > > > > compiler thinks is simpler ;)
> > > > > If the compiler overhead is real then I guess it would makes to be
> > > > > explicit. I don't expect that to be a problem though for a blocking
> > > > > function.
> > > > > 
> > > > > I disagree on this being a midlayer - you can still overwrite everything
> > > > > you please to. What it does help is people doing less copypasting (and
> > > > > assorted bugs), at least in the grand scheme of things. And we do have a
> > > > > _lot_ more random small drivers than just a few years ago. Reducing the
> > > > > amount of explicit typing just to get default bahaviour has been an
> > > > > ongoing theme for a few years now, and your objection here is about the
> > > > > first that this is not a good idea. So I'm somewhat confused.
> > > > I'm just saying I don't see any rationale for this patch.
> > > > 
> > > >          "Almost everyone uses dma_fence_default_wait."
> > > > 
> > > > Why change?
> > > > 
> > > > Making it look simpler on the surface, so that you don't have to think
> > > > about things straight away? I understand the appeal, but I do worry
> > > > about it just being an illusion. (Cutting and pasting a line saying
> > > > .wait = default_wait, doesn't feel that onerous, as you likely cut and
> > > > paste the ops anyway, and at the very least you are reminded about some
> > > > of the interactions. You could even have default initializers and/or
> > > > magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> > > > that's a midlayer!] but I haven't looked.)
> > > In really monolithic vtables like drm_driver we do use default
> > > function macros, so you type 1 line, get them all. But dma_fence_ops
> > > is pretty small, and most drivers only implement a few callbacks. Also
> > > note that e.g. the ->release callback already works like that, so this
> > > pattern is there already. I simply extended it to ->wait and
> > > ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> > > you can still wrap dma_fence_default_wait if you wish to do so.
> > > 
> > > But I just realized that I didn't clean out the optional release
> > > hooks, I guess I should do that too (for the few cases it's not yet
> > > done) and respin.
> > 
> > I kind of agree with Chris here, but also see the practical problem to copy
> > the default function in all the implementations.
> > 
> > We had the same problem in TTM and I also don't really like the result to
> > always have that "if (some_callback) default(); else some_callback();".
> > 
> > Might be that the run time overhead is negligible, but it doesn't feels
> > right from the coding style perspective.
> 
> Hm, maybe I've seen too much bad code, but modeset helpers is choke full
> of exactly that pattern. It's imo also a trade-off. If you have a fairly
> specialized library like ttm that's used by relatively few things, doing
> everything explicitly is probably better. It's also where kms started out
> from.
> 
> But if you have a huge pile of fairly simple drivers, imo the balance
> starts to tip the other way, and a bit of additional logic in the shared
> code to make all the implementations a notch simpler is good. If we
> wouldn't have acquired quite a pile of dma_fence implementations I
> wouldn't have bothered with all this.

So ack/nack on this (i.e. do you retract your original r-b or not)? It's
kinda holding up all the cleanup patches below ...

I went ahead and applied the first three patches of this series meanwhile.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter]

On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
> > Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
> > > On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > Quoting Daniel Vetter (2018-05-04 09:57:59)
> > > > > On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
> > > > > > Quoting Daniel Vetter (2018-05-04 09:23:01)
> > > > > > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
> > > > > > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
> > > > > > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
> > > > > > > > > > Almost everyone uses dma_fence_default_wait.
> > > > > > > > > > 
> > > > > > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
> > > > > > > > > I just don't get the rationale for implicit over explicit.
> > > > > > > > Closer approximation of dwim semantics. There's been tons of patch series
> > > > > > > > all over drm and related places to get there, once we have a big pile of
> > > > > > > > implementations and know what the dwim semantics should be. Individually
> > > > > > > > they're all not much, in aggregate they substantially simplify simple
> > > > > > > > drivers.
> > > > > > > I also think clearer separation between optional optimization hooks and
> > > > > > > mandatory core parts is useful in itself.
> > > > > > A new spelling of midlayer ;) I don't see the contradiction with a
> > > > > > driver saying use the default and simplicity. (I know which one the
> > > > > > compiler thinks is simpler ;)
> > > > > If the compiler overhead is real then I guess it would makes to be
> > > > > explicit. I don't expect that to be a problem though for a blocking
> > > > > function.
> > > > > 
> > > > > I disagree on this being a midlayer - you can still overwrite everything
> > > > > you please to. What it does help is people doing less copypasting (and
> > > > > assorted bugs), at least in the grand scheme of things. And we do have a
> > > > > _lot_ more random small drivers than just a few years ago. Reducing the
> > > > > amount of explicit typing just to get default bahaviour has been an
> > > > > ongoing theme for a few years now, and your objection here is about the
> > > > > first that this is not a good idea. So I'm somewhat confused.
> > > > I'm just saying I don't see any rationale for this patch.
> > > > 
> > > >          "Almost everyone uses dma_fence_default_wait."
> > > > 
> > > > Why change?
> > > > 
> > > > Making it look simpler on the surface, so that you don't have to think
> > > > about things straight away? I understand the appeal, but I do worry
> > > > about it just being an illusion. (Cutting and pasting a line saying
> > > > .wait = default_wait, doesn't feel that onerous, as you likely cut and
> > > > paste the ops anyway, and at the very least you are reminded about some
> > > > of the interactions. You could even have default initializers and/or
> > > > magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> > > > that's a midlayer!] but I haven't looked.)
> > > In really monolithic vtables like drm_driver we do use default
> > > function macros, so you type 1 line, get them all. But dma_fence_ops
> > > is pretty small, and most drivers only implement a few callbacks. Also
> > > note that e.g. the ->release callback already works like that, so this
> > > pattern is there already. I simply extended it to ->wait and
> > > ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
> > > you can still wrap dma_fence_default_wait if you wish to do so.
> > > 
> > > But I just realized that I didn't clean out the optional release
> > > hooks, I guess I should do that too (for the few cases it's not yet
> > > done) and respin.
> > 
> > I kind of agree with Chris here, but also see the practical problem to copy
> > the default function in all the implementations.
> > 
> > We had the same problem in TTM and I also don't really like the result to
> > always have that "if (some_callback) default(); else some_callback();".
> > 
> > Might be that the run time overhead is negligible, but it doesn't feels
> > right from the coding style perspective.
> 
> Hm, maybe I've seen too much bad code, but modeset helpers is choke full
> of exactly that pattern. It's imo also a trade-off. If you have a fairly
> specialized library like ttm that's used by relatively few things, doing
> everything explicitly is probably better. It's also where kms started out
> from.
> 
> But if you have a huge pile of fairly simple drivers, imo the balance
> starts to tip the other way, and a bit of additional logic in the shared
> code to make all the implementations a notch simpler is good. If we
> wouldn't have acquired quite a pile of dma_fence implementations I
> wouldn't have bothered with all this.

So ack/nack on this (i.e. do you retract your original r-b or not)? It's
kinda holding up all the cleanup patches below ...

I went ahead and applied the first three patches of this series meanwhile.
-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

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Christian König]

Am 02.07.2018 um 10:23 schrieb Daniel Vetter:
> On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote:
>> On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
>>> Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
>>>> On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>> Quoting Daniel Vetter (2018-05-04 09:57:59)
>>>>>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>>>>>>> Quoting Daniel Vetter (2018-05-04 09:23:01)
>>>>>>>> On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>>>>>>>>> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>>>>>>>>>> Quoting Daniel Vetter (2018-05-03 15:25:52)
>>>>>>>>>>> Almost everyone uses dma_fence_default_wait.
>>>>>>>>>>>
>>>>>>>>>>> v2: Also remove the BUG_ON(!ops->wait) (Chris).
>>>>>>>>>> I just don't get the rationale for implicit over explicit.
>>>>>>>>> Closer approximation of dwim semantics. There's been tons of patch series
>>>>>>>>> all over drm and related places to get there, once we have a big pile of
>>>>>>>>> implementations and know what the dwim semantics should be. Individually
>>>>>>>>> they're all not much, in aggregate they substantially simplify simple
>>>>>>>>> drivers.
>>>>>>>> I also think clearer separation between optional optimization hooks and
>>>>>>>> mandatory core parts is useful in itself.
>>>>>>> A new spelling of midlayer ;) I don't see the contradiction with a
>>>>>>> driver saying use the default and simplicity. (I know which one the
>>>>>>> compiler thinks is simpler ;)
>>>>>> If the compiler overhead is real then I guess it would makes to be
>>>>>> explicit. I don't expect that to be a problem though for a blocking
>>>>>> function.
>>>>>>
>>>>>> I disagree on this being a midlayer - you can still overwrite everything
>>>>>> you please to. What it does help is people doing less copypasting (and
>>>>>> assorted bugs), at least in the grand scheme of things. And we do have a
>>>>>> _lot_ more random small drivers than just a few years ago. Reducing the
>>>>>> amount of explicit typing just to get default bahaviour has been an
>>>>>> ongoing theme for a few years now, and your objection here is about the
>>>>>> first that this is not a good idea. So I'm somewhat confused.
>>>>> I'm just saying I don't see any rationale for this patch.
>>>>>
>>>>>           "Almost everyone uses dma_fence_default_wait."
>>>>>
>>>>> Why change?
>>>>>
>>>>> Making it look simpler on the surface, so that you don't have to think
>>>>> about things straight away? I understand the appeal, but I do worry
>>>>> about it just being an illusion. (Cutting and pasting a line saying
>>>>> .wait = default_wait, doesn't feel that onerous, as you likely cut and
>>>>> paste the ops anyway, and at the very least you are reminded about some
>>>>> of the interactions. You could even have default initializers and/or
>>>>> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
>>>>> that's a midlayer!] but I haven't looked.)
>>>> In really monolithic vtables like drm_driver we do use default
>>>> function macros, so you type 1 line, get them all. But dma_fence_ops
>>>> is pretty small, and most drivers only implement a few callbacks. Also
>>>> note that e.g. the ->release callback already works like that, so this
>>>> pattern is there already. I simply extended it to ->wait and
>>>> ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
>>>> you can still wrap dma_fence_default_wait if you wish to do so.
>>>>
>>>> But I just realized that I didn't clean out the optional release
>>>> hooks, I guess I should do that too (for the few cases it's not yet
>>>> done) and respin.
>>> I kind of agree with Chris here, but also see the practical problem to copy
>>> the default function in all the implementations.
>>>
>>> We had the same problem in TTM and I also don't really like the result to
>>> always have that "if (some_callback) default(); else some_callback();".
>>>
>>> Might be that the run time overhead is negligible, but it doesn't feels
>>> right from the coding style perspective.
>> Hm, maybe I've seen too much bad code, but modeset helpers is choke full
>> of exactly that pattern. It's imo also a trade-off. If you have a fairly
>> specialized library like ttm that's used by relatively few things, doing
>> everything explicitly is probably better. It's also where kms started out
>> from.
>>
>> But if you have a huge pile of fairly simple drivers, imo the balance
>> starts to tip the other way, and a bit of additional logic in the shared
>> code to make all the implementations a notch simpler is good. If we
>> wouldn't have acquired quite a pile of dma_fence implementations I
>> wouldn't have bothered with all this.
> So ack/nack on this (i.e. do you retract your original r-b or not)? It's
> kinda holding up all the cleanup patches below ...

Feel free to add my Acked-by for now, but I still have a kind of a gut 
feeling that we might want to revisit this decision at some time.

Christian.

>
> I went ahead and applied the first three patches of this series meanwhile.
> -Daniel

Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Christian König]

Am 02.07.2018 um 10:23 schrieb Daniel Vetter:
> On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote:
>> On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote:
>>> Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
>>>> On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>> Quoting Daniel Vetter (2018-05-04 09:57:59)
>>>>>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>>>>>>> Quoting Daniel Vetter (2018-05-04 09:23:01)
>>>>>>>> On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>>>>>>>>> On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>>>>>>>>>> Quoting Daniel Vetter (2018-05-03 15:25:52)
>>>>>>>>>>> Almost everyone uses dma_fence_default_wait.
>>>>>>>>>>>
>>>>>>>>>>> v2: Also remove the BUG_ON(!ops->wait) (Chris).
>>>>>>>>>> I just don't get the rationale for implicit over explicit.
>>>>>>>>> Closer approximation of dwim semantics. There's been tons of patch series
>>>>>>>>> all over drm and related places to get there, once we have a big pile of
>>>>>>>>> implementations and know what the dwim semantics should be. Individually
>>>>>>>>> they're all not much, in aggregate they substantially simplify simple
>>>>>>>>> drivers.
>>>>>>>> I also think clearer separation between optional optimization hooks and
>>>>>>>> mandatory core parts is useful in itself.
>>>>>>> A new spelling of midlayer ;) I don't see the contradiction with a
>>>>>>> driver saying use the default and simplicity. (I know which one the
>>>>>>> compiler thinks is simpler ;)
>>>>>> If the compiler overhead is real then I guess it would makes to be
>>>>>> explicit. I don't expect that to be a problem though for a blocking
>>>>>> function.
>>>>>>
>>>>>> I disagree on this being a midlayer - you can still overwrite everything
>>>>>> you please to. What it does help is people doing less copypasting (and
>>>>>> assorted bugs), at least in the grand scheme of things. And we do have a
>>>>>> _lot_ more random small drivers than just a few years ago. Reducing the
>>>>>> amount of explicit typing just to get default bahaviour has been an
>>>>>> ongoing theme for a few years now, and your objection here is about the
>>>>>> first that this is not a good idea. So I'm somewhat confused.
>>>>> I'm just saying I don't see any rationale for this patch.
>>>>>
>>>>>           "Almost everyone uses dma_fence_default_wait."
>>>>>
>>>>> Why change?
>>>>>
>>>>> Making it look simpler on the surface, so that you don't have to think
>>>>> about things straight away? I understand the appeal, but I do worry
>>>>> about it just being an illusion. (Cutting and pasting a line saying
>>>>> .wait = default_wait, doesn't feel that onerous, as you likely cut and
>>>>> paste the ops anyway, and at the very least you are reminded about some
>>>>> of the interactions. You could even have default initializers and/or
>>>>> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
>>>>> that's a midlayer!] but I haven't looked.)
>>>> In really monolithic vtables like drm_driver we do use default
>>>> function macros, so you type 1 line, get them all. But dma_fence_ops
>>>> is pretty small, and most drivers only implement a few callbacks. Also
>>>> note that e.g. the ->release callback already works like that, so this
>>>> pattern is there already. I simply extended it to ->wait and
>>>> ->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
>>>> you can still wrap dma_fence_default_wait if you wish to do so.
>>>>
>>>> But I just realized that I didn't clean out the optional release
>>>> hooks, I guess I should do that too (for the few cases it's not yet
>>>> done) and respin.
>>> I kind of agree with Chris here, but also see the practical problem to copy
>>> the default function in all the implementations.
>>>
>>> We had the same problem in TTM and I also don't really like the result to
>>> always have that "if (some_callback) default(); else some_callback();".
>>>
>>> Might be that the run time overhead is negligible, but it doesn't feels
>>> right from the coding style perspective.
>> Hm, maybe I've seen too much bad code, but modeset helpers is choke full
>> of exactly that pattern. It's imo also a trade-off. If you have a fairly
>> specialized library like ttm that's used by relatively few things, doing
>> everything explicitly is probably better. It's also where kms started out
>> from.
>>
>> But if you have a huge pile of fairly simple drivers, imo the balance
>> starts to tip the other way, and a bit of additional logic in the shared
>> code to make all the implementations a notch simpler is good. If we
>> wouldn't have acquired quite a pile of dma_fence implementations I
>> wouldn't have bothered with all this.
> So ack/nack on this (i.e. do you retract your original r-b or not)? It's
> kinda holding up all the cleanup patches below ...

Feel free to add my Acked-by for now, but I still have a kind of a gut 
feeling that we might want to revisit this decision at some time.

Christian.

>
> I went ahead and applied the first three patches of this series meanwhile.
> -Daniel

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

Re: [PATCH 14/15] drm/virtio: Remove unecessary dma_fence_ops

[Daniel Vetter]

On Thu, May 03, 2018 at 04:26:02PM +0200, Daniel Vetter wrote:
> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: virtualization@lists.linux-foundation.org

Ok pulled in the remaining patches here with acks/r-bs, will resend the
others.
-Daniel

> ---
>  drivers/gpu/drm/virtio/virtgpu_fence.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
> index 23353521f903..00c742a441bf 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
> @@ -36,11 +36,6 @@ static const char *virtio_get_timeline_name(struct dma_fence *f)
>  	return "controlq";
>  }
>  
> -static bool virtio_enable_signaling(struct dma_fence *f)
> -{
> -	return true;
> -}
> -
>  static bool virtio_signaled(struct dma_fence *f)
>  {
>  	struct virtio_gpu_fence *fence = to_virtio_fence(f);
> @@ -67,9 +62,7 @@ static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
>  static const struct dma_fence_ops virtio_fence_ops = {
>  	.get_driver_name     = virtio_get_driver_name,
>  	.get_timeline_name   = virtio_get_timeline_name,
> -	.enable_signaling    = virtio_enable_signaling,
>  	.signaled            = virtio_signaled,
> -	.wait                = dma_fence_default_wait,
>  	.fence_value_str     = virtio_fence_value_str,
>  	.timeline_value_str  = virtio_timeline_value_str,
>  };
> -- 
> 2.17.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 14/15] drm/virtio: Remove unecessary dma_fence_ops

[Daniel Vetter]

On Thu, May 03, 2018 at 04:26:02PM +0200, Daniel Vetter wrote:
> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: virtualization@lists.linux-foundation.org

Ok pulled in the remaining patches here with acks/r-bs, will resend the
others.
-Daniel

> ---
>  drivers/gpu/drm/virtio/virtgpu_fence.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
> index 23353521f903..00c742a441bf 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
> @@ -36,11 +36,6 @@ static const char *virtio_get_timeline_name(struct dma_fence *f)
>  	return "controlq";
>  }
>  
> -static bool virtio_enable_signaling(struct dma_fence *f)
> -{
> -	return true;
> -}
> -
>  static bool virtio_signaled(struct dma_fence *f)
>  {
>  	struct virtio_gpu_fence *fence = to_virtio_fence(f);
> @@ -67,9 +62,7 @@ static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
>  static const struct dma_fence_ops virtio_fence_ops = {
>  	.get_driver_name     = virtio_get_driver_name,
>  	.get_timeline_name   = virtio_get_timeline_name,
> -	.enable_signaling    = virtio_enable_signaling,
>  	.signaled            = virtio_signaled,
> -	.wait                = dma_fence_default_wait,
>  	.fence_value_str     = virtio_fence_value_str,
>  	.timeline_value_str  = virtio_timeline_value_str,
>  };
> -- 
> 2.17.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