[PATCH 1/3] dma-fence: Clear fence->status during dma_fence_init()

[PATCH 1/3] dma-fence: Clear fence->status during dma_fence_init()

[Chris Wilson]

As the fence->status is an optional field that may be set before
dma_fence_signal() is called to convey that the fence completed with an
error, we have to ensure that it is always set to zero on initialisation
so that the typical use (i.e. unset) always flags a successful completion.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/dma-buf/dma-fence.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 3444f293ad4a..9130f790ebf3 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	fence->context = context;
 	fence->seqno = seqno;
 	fence->flags = 0UL;
+	fence->status = 0;
 
 	trace_dma_fence_init(fence);
 }
-- 
2.11.0

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

[PATCH 2/3] dma-fence: Wrap querying the fence->status

[Chris Wilson]

The fence->status is an optional field that is only valid once the fence
has been signaled. (Driver may fill the fence->status with an error code
prior to calling dma_fence_signal().) Given the restriction upon its
validity, wrap querying of the fence->status into a helper
dma_fence_get_status().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/dma-fence.c  | 25 +++++++++++++++++++++++++
 drivers/dma-buf/sync_debug.c | 17 ++++++++---------
 drivers/dma-buf/sync_file.c  |  6 ++----
 include/linux/dma-fence.h    | 24 ++++++++++++++++++++++++
 4 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 9130f790ebf3..7d936d19e659 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -275,6 +275,31 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 EXPORT_SYMBOL(dma_fence_add_callback);
 
 /**
+ * dma_fence_get_status - returns the status upon completion
+ * @fence: [in]	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
+ * details.
+ *
+ * Returns 0 if the fence has not yet been signaled, 1 if the fence has
+ * been signaled without an error condition, or a negative error code
+ * if the fence has been completed in err.
+ */
+int dma_fence_get_status(struct dma_fence *fence)
+{
+	unsigned long flags;
+	int status;
+
+	spin_lock_irqsave(fence->lock, flags);
+	status = dma_fence_get_status_locked(fence);
+	spin_unlock_irqrestore(fence->lock, flags);
+
+	return status;
+}
+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
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index 48b20e34fb6d..c769dc653b34 100644
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -62,30 +62,29 @@ void sync_file_debug_remove(struct sync_file *sync_file)
 
 static const char *sync_status_str(int status)
 {
-	if (status == 0)
-		return "signaled";
+	if (status < 0)
+		return "error";
 
 	if (status > 0)
-		return "active";
+		return "signaled";
 
-	return "error";
+	return "active";
 }
 
 static void sync_print_fence(struct seq_file *s,
 			     struct dma_fence *fence, bool show)
 {
-	int status = 1;
 	struct sync_timeline *parent = dma_fence_parent(fence);
+	int status;
 
-	if (dma_fence_is_signaled_locked(fence))
-		status = fence->status;
+	status = dma_fence_get_status_locked(fence);
 
 	seq_printf(s, "  %s%sfence %s",
 		   show ? parent->name : "",
 		   show ? "_" : "",
 		   sync_status_str(status));
 
-	if (status <= 0) {
+	if (status) {
 		struct timespec64 ts64 =
 			ktime_to_timespec64(fence->timestamp);
 
@@ -136,7 +135,7 @@ static void sync_print_sync_file(struct seq_file *s,
 	int i;
 
 	seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
-		   sync_status_str(!dma_fence_is_signaled(sync_file->fence)));
+		   sync_status_str(dma_fence_get_status(sync_file->fence)));
 
 	if (dma_fence_is_array(sync_file->fence)) {
 		struct dma_fence_array *array = to_dma_fence_array(sync_file->fence);
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 07cb9b908f30..2321035f6204 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -373,10 +373,8 @@ static void sync_fill_fence_info(struct dma_fence *fence,
 		sizeof(info->obj_name));
 	strlcpy(info->driver_name, fence->ops->get_driver_name(fence),
 		sizeof(info->driver_name));
-	if (dma_fence_is_signaled(fence))
-		info->status = fence->status >= 0 ? 1 : fence->status;
-	else
-		info->status = 0;
+
+	info->status = dma_fence_get_status(fence);
 	info->timestamp_ns = ktime_to_ns(fence->timestamp);
 }
 
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index d51a7d23c358..19f7af905182 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -378,6 +378,30 @@ static inline struct dma_fence *dma_fence_later(struct dma_fence *f1,
 		return dma_fence_is_signaled(f2) ? NULL : f2;
 }
 
+/**
+ * dma_fence_get_status_locked - returns the status upon completion
+ * @fence: [in]	the dma_fence to query
+ *
+ * Drivers can supply an optional error status condition before they signal
+ * the fence (to indicate whether the fence was completed due to an error
+ * rather than success). The value of the status condition is only valid
+ * if the fence has been signaled, dma_fence_get_status_locked() first checks
+ * the signal state before reporting the error status.
+ *
+ * Returns 0 if the fence has not yet been signaled, 1 if the fence has
+ * been signaled without an error condition, or a negative error code
+ * if the fence has been completed in err.
+ */
+static inline int dma_fence_get_status_locked(struct dma_fence *fence)
+{
+	if (dma_fence_is_signaled_locked(fence))
+		return fence->status < 0 ? fence->status : 1;
+	else
+		return 0;
+}
+
+int dma_fence_get_status(struct dma_fence *fence);
+
 signed long dma_fence_wait_timeout(struct dma_fence *,
 				   bool intr, signed long timeout);
 signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
-- 
2.11.0

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

[PATCH 3/3] dma-fence: Introduce drm_fence_set_error() helper

[Chris Wilson]

The dma_fence.error field (formerly known as dma_fence.status) is an
optional field that may be set by drivers before calling
dma_fence_signal(). The field can be used to indicate that the fence was
completed in err rather than with success, and is visible to other
consumers of the fence and to userspace via sync_file.

This patch renames the field from status to error so that its meaning is
hopefully more clear (and distinct from dma_fence_get_status() which is
a composite between the error state and signal state) and adds a helper
that validates the preconditions of when it is suitable to adjust the
error field.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/dma-fence.c |  2 +-
 include/linux/dma-fence.h   | 30 +++++++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 7d936d19e659..ee82f36cb25e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -559,7 +559,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	fence->context = context;
 	fence->seqno = seqno;
 	fence->flags = 0UL;
-	fence->status = 0;
+	fence->error = 0;
 
 	trace_dma_fence_init(fence);
 }
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 19f7af905182..6048fa404e57 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -47,7 +47,7 @@ struct dma_fence_cb;
  * can be compared to decide which fence would be signaled later.
  * @flags: A mask of DMA_FENCE_FLAG_* defined below
  * @timestamp: Timestamp when the fence was signaled.
- * @status: Optional, only valid if < 0, must be set before calling
+ * @error: Optional, only valid if < 0, must be set before calling
  * dma_fence_signal, indicates that the fence has completed with an error.
  *
  * the flags member must be manipulated and read using the appropriate
@@ -79,7 +79,7 @@ struct dma_fence {
 	unsigned seqno;
 	unsigned long flags;
 	ktime_t timestamp;
-	int status;
+	int error;
 };
 
 enum dma_fence_flag_bits {
@@ -133,7 +133,7 @@ struct dma_fence_cb {
  * or some failure occurred that made it impossible to enable
  * signaling. True indicates successful enabling.
  *
- * fence->status may be set in enable_signaling, but only when false is
+ * fence->error may be set in enable_signaling, but only when false is
  * returned.
  *
  * Calling dma_fence_signal before enable_signaling is called allows
@@ -145,7 +145,7 @@ struct dma_fence_cb {
  * the second time will be a noop since it was already signaled.
  *
  * Notes on signaled:
- * May set fence->status if returning true.
+ * May set fence->error if returning true.
  *
  * Notes on wait:
  * Must not be NULL, set to dma_fence_default_wait for default implementation.
@@ -395,13 +395,33 @@ static inline struct dma_fence *dma_fence_later(struct dma_fence *f1,
 static inline int dma_fence_get_status_locked(struct dma_fence *fence)
 {
 	if (dma_fence_is_signaled_locked(fence))
-		return fence->status < 0 ? fence->status : 1;
+		return fence->error ?: 1;
 	else
 		return 0;
 }
 
 int dma_fence_get_status(struct dma_fence *fence);
 
+/**
+ * dma_fence_set_error - flag an error condition on the fence
+ * @fence: [in]	the dma_fence
+ * @error: [in]	the error to store
+ *
+ * Drivers can supply an optional error status condition before they signal
+ * the fence, to indicate that the fence was completed due to an error
+ * rather than success. This must be set before signaling (so that the value
+ * is visible before any waiters on the signal callback are woken). This
+ * helper exists to help catching erroneous setting of #dma_fence.error.
+ */
+static inline void dma_fence_set_error(struct dma_fence *fence,
+				       int error)
+{
+	BUG_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags));
+	BUG_ON(error >= 0 || error < -MAX_ERRNO);
+
+	fence->error = error;
+}
+
 signed long dma_fence_wait_timeout(struct dma_fence *,
 				   bool intr, signed long timeout);
 signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
-- 
2.11.0

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

Re: [PATCH 1/3] dma-fence: Clear fence->status during dma_fence_init()

[Daniel Vetter]

On Wed, Jan 04, 2017 at 02:12:20PM +0000, Chris Wilson wrote:
> As the fence->status is an optional field that may be set before
> dma_fence_signal() is called to convey that the fence completed with an
> error, we have to ensure that it is always set to zero on initialisation
> so that the typical use (i.e. unset) always flags a successful completion.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Yeah, this looks all pretty. On the series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'll defer to Gustavo for another pass over it and merging it to drm-misc.
-Daniel

> ---
>  drivers/dma-buf/dma-fence.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 3444f293ad4a..9130f790ebf3 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>  	fence->context = context;
>  	fence->seqno = seqno;
>  	fence->flags = 0UL;
> +	fence->status = 0;
>  
>  	trace_dma_fence_init(fence);
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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

✗ Fi.CI.BAT: warning for series starting with [1/3] dma-fence: Clear fence->status during dma_fence_init()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] dma-fence: Clear fence->status during dma_fence_init()
URL   : https://patchwork.freedesktop.org/series/17493/
State : warning

== Summary ==

Series 17493v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/17493/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-edid:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-bsw-n3050)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:206  dwarn:1   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:214  dwarn:1   dfail:0   fail:0   skip:31 

3ff9912451fd6840732ac0184f0561c9e6c4107f drm-tip: 2017y-01m-04d-11h-22m-41s UTC integration manifest
f893cc6 dma-fence: Introduce drm_fence_set_error() helper
388ab7e dma-fence: Wrap querying the fence->status
5426a73 dma-fence: Clear fence->status during dma_fence_init()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3430/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] dma-fence: Clear fence->status during dma_fence_init()

[Sumit Semwal]

Hi Chris,

Thanks for your patches!

On 4 January 2017 at 20:40, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 04, 2017 at 02:12:20PM +0000, Chris Wilson wrote:
>> As the fence->status is an optional field that may be set before
>> dma_fence_signal() is called to convey that the fence completed with an
>> error, we have to ensure that it is always set to zero on initialisation
>> so that the typical use (i.e. unset) always flags a successful completion.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Yeah, this looks all pretty. On the series:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
FWIW, please feel free, for this series, to apply my

Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>

> I'll defer to Gustavo for another pass over it and merging it to drm-misc.
> -Daniel
>

Best,
Sumit.
>> ---
>>  drivers/dma-buf/dma-fence.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 3444f293ad4a..9130f790ebf3 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -534,6 +534,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>>       fence->context = context;
>>       fence->seqno = seqno;
>>       fence->flags = 0UL;
>> +     fence->status = 0;
>>
>>       trace_dma_fence_init(fence);
>>  }
>> --
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> 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



-- 
Thanks and regards,

Sumit Semwal
Linaro Mobile Group - Kernel Team Lead
Linaro.org │ Open source software for ARM SoCs
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] dma-fence: Clear fence->status during dma_fence_init()

[Chris Wilson]

On Wed, Jan 04, 2017 at 09:23:34PM +0530, Sumit Semwal wrote:
> Hi Chris,
> 
> Thanks for your patches!
> 
> On 4 January 2017 at 20:40, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Jan 04, 2017 at 02:12:20PM +0000, Chris Wilson wrote:
> >> As the fence->status is an optional field that may be set before
> >> dma_fence_signal() is called to convey that the fence completed with an
> >> error, we have to ensure that it is always set to zero on initialisation
> >> so that the typical use (i.e. unset) always flags a successful completion.
> >>
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > Yeah, this looks all pretty. On the series:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> FWIW, please feel free, for this series, to apply my
> 
> Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>
> 
> > I'll defer to Gustavo for another pass over it and merging it to drm-misc.

It would be nice to have this in the next round of backmerges.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] dma-fence: Introduce drm_fence_set_error() helper

[Sumit Semwal]

Hi Chris,

On 4 January 2017 at 19:42, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The dma_fence.error field (formerly known as dma_fence.status) is an
> optional field that may be set by drivers before calling
> dma_fence_signal(). The field can be used to indicate that the fence was
> completed in err rather than with success, and is visible to other
> consumers of the fence and to userspace via sync_file.
>
> This patch renames the field from status to error so that its meaning is
> hopefully more clear (and distinct from dma_fence_get_status() which is
> a composite between the error state and signal state) and adds a helper
> that validates the preconditions of when it is suitable to adjust the
> error field.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/dma-buf/dma-fence.c |  2 +-
>  include/linux/dma-fence.h   | 30 +++++++++++++++++++++++++-----
>  2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 7d936d19e659..ee82f36cb25e 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -559,7 +559,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
>         fence->context = context;
>         fence->seqno = seqno;
>         fence->flags = 0UL;
> -       fence->status = 0;
> +       fence->error = 0;
>
>         trace_dma_fence_init(fence);
>  }
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 19f7af905182..6048fa404e57 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -47,7 +47,7 @@ struct dma_fence_cb;
>   * can be compared to decide which fence would be signaled later.
>   * @flags: A mask of DMA_FENCE_FLAG_* defined below
>   * @timestamp: Timestamp when the fence was signaled.
> - * @status: Optional, only valid if < 0, must be set before calling
> + * @error: Optional, only valid if < 0, must be set before calling
>   * dma_fence_signal, indicates that the fence has completed with an error.
>   *
>   * the flags member must be manipulated and read using the appropriate
> @@ -79,7 +79,7 @@ struct dma_fence {
>         unsigned seqno;
>         unsigned long flags;
>         ktime_t timestamp;
> -       int status;
> +       int error;
>  };
>
>  enum dma_fence_flag_bits {
> @@ -133,7 +133,7 @@ struct dma_fence_cb {
>   * or some failure occurred that made it impossible to enable
>   * signaling. True indicates successful enabling.
>   *
> - * fence->status may be set in enable_signaling, but only when false is
> + * fence->error may be set in enable_signaling, but only when false is
>   * returned.
>   *
>   * Calling dma_fence_signal before enable_signaling is called allows
> @@ -145,7 +145,7 @@ struct dma_fence_cb {
>   * the second time will be a noop since it was already signaled.
>   *
>   * Notes on signaled:
> - * May set fence->status if returning true.
> + * May set fence->error if returning true.
>   *
>   * Notes on wait:
>   * Must not be NULL, set to dma_fence_default_wait for default implementation.
> @@ -395,13 +395,33 @@ static inline struct dma_fence *dma_fence_later(struct dma_fence *f1,
>  static inline int dma_fence_get_status_locked(struct dma_fence *fence)
>  {
>         if (dma_fence_is_signaled_locked(fence))
> -               return fence->status < 0 ? fence->status : 1;
> +               return fence->error ?: 1;
>         else
>                 return 0;
>  }
>
>  int dma_fence_get_status(struct dma_fence *fence);
>
> +/**
> + * dma_fence_set_error - flag an error condition on the fence
> + * @fence: [in]        the dma_fence
> + * @error: [in]        the error to store
> + *
> + * Drivers can supply an optional error status condition before they signal
> + * the fence, to indicate that the fence was completed due to an error
> + * rather than success. This must be set before signaling (so that the value
> + * is visible before any waiters on the signal callback are woken). This
> + * helper exists to help catching erroneous setting of #dma_fence.error.
> + */
> +static inline void dma_fence_set_error(struct dma_fence *fence,
> +                                      int error)
> +{
> +       BUG_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags));
> +       BUG_ON(error >= 0 || error < -MAX_ERRNO);
> +
> +       fence->error = error;
> +}
Sorry I missed this earlier, but are you sure you want to use a BUG_ON
here, and not a WARN_ON?
> +
>  signed long dma_fence_wait_timeout(struct dma_fence *,
>                                    bool intr, signed long timeout);
>  signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 3/3] dma-fence: Introduce drm_fence_set_error() helper

[Chris Wilson]

On Mon, Jan 09, 2017 at 08:13:11PM +0530, Sumit Semwal wrote:
> Hi Chris,
> 
> On 4 January 2017 at 19:42, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > The dma_fence.error field (formerly known as dma_fence.status) is an
> > optional field that may be set by drivers before calling
> > dma_fence_signal(). The field can be used to indicate that the fence was
> > completed in err rather than with success, and is visible to other
> > consumers of the fence and to userspace via sync_file.
> >
> > This patch renames the field from status to error so that its meaning is
> > hopefully more clear (and distinct from dma_fence_get_status() which is
> > a composite between the error state and signal state) and adds a helper
> > that validates the preconditions of when it is suitable to adjust the
> > error field.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/dma-buf/dma-fence.c |  2 +-
> >  include/linux/dma-fence.h   | 30 +++++++++++++++++++++++++-----
> >  2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 7d936d19e659..ee82f36cb25e 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -559,7 +559,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
> >         fence->context = context;
> >         fence->seqno = seqno;
> >         fence->flags = 0UL;
> > -       fence->status = 0;
> > +       fence->error = 0;
> >
> >         trace_dma_fence_init(fence);
> >  }
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index 19f7af905182..6048fa404e57 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -47,7 +47,7 @@ struct dma_fence_cb;
> >   * can be compared to decide which fence would be signaled later.
> >   * @flags: A mask of DMA_FENCE_FLAG_* defined below
> >   * @timestamp: Timestamp when the fence was signaled.
> > - * @status: Optional, only valid if < 0, must be set before calling
> > + * @error: Optional, only valid if < 0, must be set before calling
> >   * dma_fence_signal, indicates that the fence has completed with an error.
> >   *
> >   * the flags member must be manipulated and read using the appropriate
> > @@ -79,7 +79,7 @@ struct dma_fence {
> >         unsigned seqno;
> >         unsigned long flags;
> >         ktime_t timestamp;
> > -       int status;
> > +       int error;
> >  };
> >
> >  enum dma_fence_flag_bits {
> > @@ -133,7 +133,7 @@ struct dma_fence_cb {
> >   * or some failure occurred that made it impossible to enable
> >   * signaling. True indicates successful enabling.
> >   *
> > - * fence->status may be set in enable_signaling, but only when false is
> > + * fence->error may be set in enable_signaling, but only when false is
> >   * returned.
> >   *
> >   * Calling dma_fence_signal before enable_signaling is called allows
> > @@ -145,7 +145,7 @@ struct dma_fence_cb {
> >   * the second time will be a noop since it was already signaled.
> >   *
> >   * Notes on signaled:
> > - * May set fence->status if returning true.
> > + * May set fence->error if returning true.
> >   *
> >   * Notes on wait:
> >   * Must not be NULL, set to dma_fence_default_wait for default implementation.
> > @@ -395,13 +395,33 @@ static inline struct dma_fence *dma_fence_later(struct dma_fence *f1,
> >  static inline int dma_fence_get_status_locked(struct dma_fence *fence)
> >  {
> >         if (dma_fence_is_signaled_locked(fence))
> > -               return fence->status < 0 ? fence->status : 1;
> > +               return fence->error ?: 1;
> >         else
> >                 return 0;
> >  }
> >
> >  int dma_fence_get_status(struct dma_fence *fence);
> >
> > +/**
> > + * dma_fence_set_error - flag an error condition on the fence
> > + * @fence: [in]        the dma_fence
> > + * @error: [in]        the error to store
> > + *
> > + * Drivers can supply an optional error status condition before they signal
> > + * the fence, to indicate that the fence was completed due to an error
> > + * rather than success. This must be set before signaling (so that the value
> > + * is visible before any waiters on the signal callback are woken). This
> > + * helper exists to help catching erroneous setting of #dma_fence.error.
> > + */
> > +static inline void dma_fence_set_error(struct dma_fence *fence,
> > +                                      int error)
> > +{
> > +       BUG_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags));
> > +       BUG_ON(error >= 0 || error < -MAX_ERRNO);
> > +
> > +       fence->error = error;
> > +}
> Sorry I missed this earlier, but are you sure you want to use a BUG_ON
> here, and not a WARN_ON?

Given that it is a void return, there's nowhere for the warning to go.
The second is a definite programming error and should hopefully be
eliminated at compiletime, but writing it as a BUILD_BUG or
__builtin_constant_p may not work - BUILD_BUG_ON(error...) happens to
work for my code. If you want to use

-       BUG_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags));
-       BUG_ON(error >= 0 || error < -MAX_ERRNO);
+       BUILD_BUG_ON(error >= 0 || error < -MAX_ERRNO);
+       WARN_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags));
 
and have a softlockup instead, be my guest.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx