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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
