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

[Chris Wilson <chris@chris-wilson.co.uk>]

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