On Wed, Jul 04, 2018 at 01:03:18PM +0100, Emil Velikov wrote:

Hi Daniel,

On 4 July 2018 at 10:29, Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

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

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

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com Cc: Colin Ian King colin.king@canonical.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Mika Kuoppala mika.kuoppala@intel.com Cc: intel-gfx@lists.freedesktop.org

---

drivers/gpu/drm/i915/i915_gem_clflush.c | 7 ------- drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 -------- 2 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c index f5c570d35b2a..8e74c23cbd91 100644 --- a/drivers/gpu/drm/i915/i915_gem_clflush.c +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c @@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence) return "clflush"; }

-static bool i915_clflush_enable_signaling(struct dma_fence *fence) -{

•   return true;
  

-}

static void i915_clflush_release(struct dma_fence *fence) { struct clflush *clflush = container_of(fence, typeof(*clflush), dma); @@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence) static const struct dma_fence_ops i915_clflush_ops = { .get_driver_name = i915_clflush_get_driver_name, .get_timeline_name = i915_clflush_get_timeline_name,

•   .enable_signaling = i915_clflush_enable_signaling,
  

From a quick look through drm-misc/drm-misc-next removing the enable_signalling hook may cause functional changes.

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

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

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

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

HTH Emil

On Wed, Jul 4, 2018 at 7:22 PM, Emil Velikov emil.l.velikov@gmail.com wrote:

On 4 July 2018 at 13:34, Daniel Vetter daniel@ffwll.ch wrote:

...

On Wed, Jul 04, 2018 at 01:03:18PM +0100, Emil Velikov wrote:

...

Hi Daniel,

On 4 July 2018 at 10:29, Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

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

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

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com Cc: Colin Ian King colin.king@canonical.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Mika Kuoppala mika.kuoppala@intel.com Cc: intel-gfx@lists.freedesktop.org

---

drivers/gpu/drm/i915/i915_gem_clflush.c | 7 ------- drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 -------- 2 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c index f5c570d35b2a..8e74c23cbd91 100644 --- a/drivers/gpu/drm/i915/i915_gem_clflush.c +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c @@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence) return "clflush"; }

-static bool i915_clflush_enable_signaling(struct dma_fence *fence) -{

•   return true;
  

-}

static void i915_clflush_release(struct dma_fence *fence) { struct clflush *clflush = container_of(fence, typeof(*clflush), dma); @@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence) static const struct dma_fence_ops i915_clflush_ops = { .get_driver_name = i915_clflush_get_driver_name, .get_timeline_name = i915_clflush_get_timeline_name,

•   .enable_signaling = i915_clflush_enable_signaling,
  

From a quick look through drm-misc/drm-misc-next removing the enable_signalling hook may cause functional changes.

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

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

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

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

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

Yeah would have been good to add to the commit message that made ->enable_signaling optional, but that's now baked into history already :-/ -Daniel

On Wed, Jul 04, 2018 at 11:29:08AM +0200, Daniel Vetter wrote:

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

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

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Cc: Kees Cook keescook@chromium.org Cc: Cihangir Akturk cakturk@gmail.com Cc: Sean Paul seanpaul@chromium.org Cc: Daniel Vetter daniel.vetter@ffwll.ch

Anyone feel like reviewing patches 1-4 here?

Thanks, Daniel

---

drivers/gpu/drm/vgem/vgem_fence.c | 14 -------------- 1 file changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c index b28876c222b4..75adedeaa384 100644 --- a/drivers/gpu/drm/vgem/vgem_fence.c +++ b/drivers/gpu/drm/vgem/vgem_fence.c @@ -43,16 +43,6 @@ static const char *vgem_fence_get_timeline_name(struct dma_fence *fence) return "unbound"; }

-static bool vgem_fence_signaled(struct dma_fence *fence) -{

• return false;

-}

-static bool vgem_fence_enable_signaling(struct dma_fence *fence) -{

• return true;

-}

static void vgem_fence_release(struct dma_fence *base) { struct vgem_fence *fence = container_of(base, typeof(*fence), base); @@ -76,11 +66,7 @@ static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str, static const struct dma_fence_ops vgem_fence_ops = { .get_driver_name = vgem_fence_get_driver_name, .get_timeline_name = vgem_fence_get_timeline_name,

• .enable_signaling = vgem_fence_enable_signaling,
• .signaled = vgem_fence_signaled,
• .wait = dma_fence_default_wait, .release = vgem_fence_release,
• .fence_value_str = vgem_fence_value_str, .timeline_value_str = vgem_fence_timeline_value_str,

};

2.18.0

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

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

v2: Protect the meaningful space! (Chris)

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

diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c index b28876c222b4..e6ee71323a66 100644 --- a/drivers/gpu/drm/vgem/vgem_fence.c +++ b/drivers/gpu/drm/vgem/vgem_fence.c @@ -43,16 +43,6 @@ static const char *vgem_fence_get_timeline_name(struct dma_fence *fence) return "unbound"; }

-static bool vgem_fence_signaled(struct dma_fence *fence) -{ - return false; -} - -static bool vgem_fence_enable_signaling(struct dma_fence *fence) -{ - return true; -} - static void vgem_fence_release(struct dma_fence *base) { struct vgem_fence *fence = container_of(base, typeof(*fence), base); @@ -76,9 +66,6 @@ static void vgem_fence_timeline_value_str(struct dma_fence *fence, char *str, static const struct dma_fence_ops vgem_fence_ops = { .get_driver_name = vgem_fence_get_driver_name, .get_timeline_name = vgem_fence_get_timeline_name, - .enable_signaling = vgem_fence_enable_signaling, - .signaled = vgem_fence_signaled, - .wait = dma_fence_default_wait, .release = vgem_fence_release,

.fence_value_str = vgem_fence_value_str,

Am 04.07.2018 um 11:29 schrieb Daniel Vetter:

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

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

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

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

---

Documentation/driver-api/dma-buf.rst | 6 ++ drivers/dma-buf/dma-fence.c | 147 +++++++++++++++++++-------- 2 files changed, 109 insertions(+), 44 deletions(-)

diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index dc384f2f7f34..b541e97c7ab1 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -130,6 +130,12 @@ Reservation Objects DMA Fences

---

+.. kernel-doc:: drivers/dma-buf/dma-fence.c

• :doc: DMA fences overview

+DMA Fences Functions Reference +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

• .. kernel-doc:: drivers/dma-buf/dma-fence.c :export:

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 7a92f85a4cec..1551ca7df394 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -38,12 +38,43 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal); */ static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0);

+/**

• • DOC: DMA fences overview
• • DMA fences, represented by &struct dma_fence, are the kernel internal
• • synchronization primitive for DMA operations like GPU rendering, video
• • encoding/decoding, or displaying buffers on a screen.
• • A fence is initialized using dma_fence_init() and completed using
• • dma_fence_signal(). Fences are associated with a context, allocated through
• • dma_fence_context_alloc(), and all fences on the same context are
• • fully ordered.
• • Since the purposes of fences is to facilitate cross-device and
• • cross-application synchronization, there's multiple ways to use one:
• • • Individual fences can be exposed as a &sync_file, accessed as a file
• • descriptor from userspace, created by calling sync_file_create(). This is
• • called explicit fencing, since userspace passes around explicit
• • synchronization points.
• • • Some subsystems also have their own explicit fencing primitives, like
• • &drm_syncobj. Compared to &sync_file, a &drm_syncobj allows the underlying
• • fence to be updated.
• • • Then there's also implicit fencing, where the synchronization points are
• • implicitly passed around as part of shared &dma_buf instances. Such
• • implicit fences are stored in &struct reservation_object through the
• • &dma_buf.resv pointer.
• */
• /**
  • dma_fence_context_alloc - allocate an array of fence contexts

• • @num: [in] amount of contexts to allocate

• • @num: amount of contexts to allocate

• • This function will return the first index of the number of fences allocated.
• • The fence context is used for setting fence->context to a unique number.

• • This function will return the first index of the number of fence contexts
• • allocated. The fence context is used for setting &dma_fence.context to a
• • unique number by passing the context to dma_fence_init().
  */ u64 dma_fence_context_alloc(unsigned num) {

@@ -59,10 +90,14 @@ EXPORT_SYMBOL(dma_fence_context_alloc);

• Signal completion for software callbacks on a fence, this will unblock
• dma_fence_wait() calls and run all the callbacks added with
• dma_fence_add_callback(). Can be called multiple times, but since a fence

• • can only go from unsignaled to signaled state, it will only be effective
• • the first time.

• • can only go from the unsignaled to the signaled state and not back, it will
• • only be effective the first time.
• • Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
• • held.

• • Unlike dma_fence_signal, this function must be called with fence->lock held.

• • Returns 0 on success and a negative error value when @fence has been
• • signalled already.
  */ int dma_fence_signal_locked(struct dma_fence *fence) {

@@ -102,8 +137,11 @@ EXPORT_SYMBOL(dma_fence_signal_locked);

• Signal completion for software callbacks on a fence, this will unblock
• dma_fence_wait() calls and run all the callbacks added with
• dma_fence_add_callback(). Can be called multiple times, but since a fence

• • can only go from unsignaled to signaled state, it will only be effective
• • the first time.

• • can only go from the unsignaled to the signaled state and not back, it will
• • only be effective the first time.
• • Returns 0 on success and a negative error value when @fence has been
• • signalled already.
  */ int dma_fence_signal(struct dma_fence *fence) {

@@ -136,9 +174,9 @@ EXPORT_SYMBOL(dma_fence_signal); /**

• dma_fence_wait_timeout - sleep until the fence gets signaled
• or until timeout elapses

• • @fence: [in] the fence to wait on
• • @intr: [in] if true, do an interruptible wait
• • @timeout: [in] timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT

• • @fence: the fence to wait on
• • @intr: if true, do an interruptible wait
• • @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  • Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  • remaining timeout in jiffies on success. Other error values may be

@@ -148,6 +186,8 @@ EXPORT_SYMBOL(dma_fence_signal);

• directly or indirectly (buf-mgr between reservation and committing)
• holds a reference to the fence, otherwise the fence might be
• freed before return, resulting in undefined behavior.

• • See also dma_fence_wait() and dma_fence_wait_any_timeout().
  */ signed long dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)

@@ -167,6 +207,13 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) } EXPORT_SYMBOL(dma_fence_wait_timeout);

+/**

• • dma_fence_release - default relese function for fences
• • @kref: &dma_fence.recfount
• • This is the default release functions for &dma_fence. Drivers shouldn't call
• • this directly, but instead call dma_fence_put().
• */ void dma_fence_release(struct kref *kref) { struct dma_fence *fence =

@@ -184,6 +231,13 @@ void dma_fence_release(struct kref *kref) } EXPORT_SYMBOL(dma_fence_release);

+/**

• • dma_fence_free - default release function for &dma_fence.
• • @fence: fence to release
• • This is the default implementation for &dma_fence_ops.release. It calls
• • kfree_rcu() on @fence.
• */ void dma_fence_free(struct dma_fence *fence) { kfree_rcu(fence, rcu);

@@ -192,10 +246,11 @@ EXPORT_SYMBOL(dma_fence_free);

/**

• dma_fence_enable_sw_signaling - enable signaling on fence

• • @fence: [in] the fence to enable

• • @fence: the fence to enable

• • this will request for sw signaling to be enabled, to make the fence
• • complete as soon as possible

• • This will request for sw signaling to be enabled, to make the fence
• • complete as soon as possible. This calls &dma_fence_ops.enable_signaling
• • internally.
  */ void dma_fence_enable_sw_signaling(struct dma_fence *fence) {

@@ -220,24 +275,24 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); /**

• dma_fence_add_callback - add a callback to be called when the fence
• is signaled

• • @fence: [in] the fence to wait on
• • @cb: [in] the callback to register
• • @func: [in] the function to call

• • @fence: the fence to wait on
• • @cb: the callback to register
• • @func: the function to call

• • cb will be initialized by dma_fence_add_callback, no initialization

• • @cb will be initialized by dma_fence_add_callback(), no initialization
  • by the caller is required. Any number of callbacks can be registered
  • to a fence, but a callback can only be registered to one fence at a time.
  • Note that the callback can be called from an atomic context. If
  • fence is already signaled, this function will return -ENOENT (and

• • *not* call the callback)

• • *not* call the callback).
  • Add a software callback to the fence. Same restrictions apply to

• • refcount as it does to dma_fence_wait, however the caller doesn't need to
• • keep a refcount to fence afterwards: when software access is enabled,
• • the creator of the fence is required to keep the fence alive until
• • after it signals with dma_fence_signal. The callback itself can be called
• • from irq context.

• • refcount as it does to dma_fence_wait(), however the caller doesn't need to
• • keep a refcount to fence afterward dma_fence_add_callback() has returned:
• • when software access is enabled, the creator of the fence is required to keep
• • the fence alive until after it signals with dma_fence_signal(). The callback
• • itself can be called from irq context.
  • Returns 0 in case of success, -ENOENT if the fence is already signaled
  • and -EINVAL in case of error.

@@ -286,7 +341,7 @@ EXPORT_SYMBOL(dma_fence_add_callback);

/**

• dma_fence_get_status - returns the status upon completion

• • @fence: [in] the dma_fence to query

• • @fence: the dma_fence to query
  • This wraps dma_fence_get_status_locked() to return the error status
  • condition on a signaled fence. See dma_fence_get_status_locked() for more

@@ -311,8 +366,8 @@ EXPORT_SYMBOL(dma_fence_get_status);

/**

• dma_fence_remove_callback - remove a callback from the signaling list

• • @fence: [in] the fence to wait on
• • @cb: [in] the callback to remove

• • @fence: the fence to wait on
• • @cb: the callback to remove
  • Remove a previously queued callback from the fence. This function returns
  • true if the callback is successfully removed, or false if the fence has

@@ -323,6 +378,9 @@ EXPORT_SYMBOL(dma_fence_get_status);

• doing, since deadlocks and race conditions could occur all too easily. For
• this reason, it should only ever be done on hardware lockup recovery,
• with a reference held to the fence.

• • Behaviour is undefined if @cb has not been added to @fence using
• • dma_fence_add_callback() beforehand.
  */ bool dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)

@@ -359,9 +417,9 @@ dma_fence_default_wait_cb(struct dma_fence *fence, struct dma_fence_cb *cb) /**

• dma_fence_default_wait - default sleep until the fence gets signaled
• or until timeout elapses

• • @fence: [in] the fence to wait on
• • @intr: [in] if true, do an interruptible wait
• • @timeout: [in] timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT

• • @fence: the fence to wait on
• • @intr: if true, do an interruptible wait
• • @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
  • Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the
  • remaining timeout in jiffies on success. If timeout is zero the value one is

@@ -454,12 +512,12 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count, /**

• dma_fence_wait_any_timeout - sleep until any fence gets signaled
• or until timeout elapses

• • @fences: [in] array of fences to wait on
• • @count: [in] number of fences to wait on
• • @intr: [in] if true, do an interruptible wait
• • @timeout: [in] timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
• • @idx: [out] the first signaled fence index, meaningful only on

• • 	positive return
    

• • @fences: array of fences to wait on
• • @count: number of fences to wait on
• • @intr: if true, do an interruptible wait
• • @timeout: timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
• • @idx: used to store the first signaled fence index, meaningful only on
• • positive return
  • Returns -EINVAL on custom fence wait implementation, -ERESTARTSYS if
  • interrupted, 0 if the wait timed out, or the remaining timeout in jiffies

@@ -468,6 +526,8 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,

• Synchronous waits for the first fence in the array to be signaled. The
• caller needs to hold a reference to all fences in the array, otherwise a
• fence might be freed before return, resulting in undefined behavior.

• • See also dma_fence_wait() and dma_fence_wait_timeout().
  */ signed long dma_fence_wait_any_timeout(struct dma_fence **fences, uint32_t count,

@@ -540,19 +600,18 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);

/**

• dma_fence_init - Initialize a custom fence.

• • @fence: [in] the fence to initialize
• • @ops: [in] the dma_fence_ops for operations on this fence
• • @lock: [in] the irqsafe spinlock to use for locking this fence
• • @context: [in] the execution context this fence is run on
• • @seqno: [in] a linear increasing sequence number for this context

• • @fence: the fence to initialize
• • @ops: the dma_fence_ops for operations on this fence
• • @lock: the irqsafe spinlock to use for locking this fence
• • @context: the execution context this fence is run on
• • @seqno: a linear increasing sequence number for this context
  • Initializes an allocated fence, the caller doesn't have to keep its
  • refcount after committing with this fence, but it will need to hold a

• • refcount again if dma_fence_ops.enable_signaling gets called. This can
• • be used for other implementing other types of fence.

• • refcount again if &dma_fence_ops.enable_signaling gets called.
  • context and seqno are used for easy comparison between fences, allowing

• • to check which fence is later by simply using dma_fence_later.

• • to check which fence is later by simply using dma_fence_later().
  */ void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,