| Message ID | 20170104141222.6992-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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,
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(-)