[libcamera-devel] [RFC PATCH 01/12] libcamera: request: Add support for error flags

[Kieran Bingham]

@Quoting Jacopo Mondi (2022-07-25 08:54:25)
> Hi Kieran
> 
> On Thu, Jul 21, 2022 at 01:12:59PM +0100, Kieran Bingham via libcamera-devel wrote:
> > From: Paul Elder <paul.elder at ideasonboard.com>
> >
> > Add error flags to the Request to indicate non-fatal errors.
> >
> > Applications should check the error() state of the Request to determine
> > if any fault occured while processing the request.
> >
> > Initially, a single error flag ControlError is added to allow a pipeline
> > handler to report a failure to set controls on a hardware device while
> > processing the request.
> >
> > ControlErrors occur when a Request was asked to set a control and the
> > pipeline handler attempted to do so, but it was rejected by the kernel.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > ---
> > v2:
> >  - Add documentation for Request::Private::setErrorFlags
> >
> > ---
> > v2
> >  - Extend documentation
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/internal/request.h |  3 ++
> >  include/libcamera/request.h          |  9 ++++++
> >  src/libcamera/request.cpp            | 46 ++++++++++++++++++++++++++++
> >  3 files changed, 58 insertions(+)
> >
> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> > index 9dadd6c60361..8e592272cfae 100644
> > --- a/include/libcamera/internal/request.h
> > +++ b/include/libcamera/internal/request.h
> > @@ -38,6 +38,7 @@ public:
> >       void complete();
> >       void cancel();
> >       void reuse();
> > +     void setErrorFlags(ErrorFlags flags);
> 
> Please move after the 'prepared' signal with an empy line to match the
> private fields declaration order, where error_ follows the
> fence-related members.

Ok.


> >
> >       void prepare(std::chrono::milliseconds timeout = 0ms);
> >       Signal<> prepared;
> > @@ -59,6 +60,8 @@ private:
> >       std::unordered_set<FrameBuffer *> pending_;
> >       std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
> >       std::unique_ptr<Timer> timer_;
> > +
> > +     ErrorFlags error_;
> 
> I wonder if s/Flags// almost everywhere
> 
> Is this initialized ? Or just set in reuse() it doesn't seem to me
> that reuse is called at class construction time

Flags<> are always initiliased to '0':

 https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/base/flags.cpp#n36
 https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/base/flags.h#n23


> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index dffde1536cad..992629e11aa4 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -15,6 +15,7 @@
> >  #include <unordered_set>
> >
> >  #include <libcamera/base/class.h>
> > +#include <libcamera/base/flags.h>
> >  #include <libcamera/base/signal.h>
> >
> >  #include <libcamera/controls.h>
> > @@ -43,6 +44,13 @@ public:
> >               ReuseBuffers = (1 << 0),
> >       };
> >
> > +     enum ErrorFlag {
> 
> Like here having this as ErrorId or similar

I don't mind changing Flag for Id, I hadn't given it much thought of the
name itself.


> 
> > +             NoError = 0,
> > +             ControlError = (1 << 0),
> > +     };
> > +
> > +     using ErrorFlags = Flags<ErrorFlag>;
> 
> and s/ErrorFlags/Errors/

If we don't need to call them flags, I think that's fine yes.


> 
> > +
> >       using BufferMap = std::map<const Stream *, FrameBuffer *>;
> >
> >       Request(Camera *camera, uint64_t cookie = 0);
> > @@ -60,6 +68,7 @@ public:
> >       uint32_t sequence() const;
> >       uint64_t cookie() const { return cookie_; }
> >       Status status() const { return status_; }
> > +     ErrorFlags error() const;
> >
> >       bool hasPendingBuffers() const;
> >
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index d2af1d2212ad..8b82757ea7e3 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -162,6 +162,7 @@ void Request::Private::cancel()
> >   */
> >  void Request::Private::reuse()
> >  {
> > +     error_ = Request::NoError;
> >       sequence_ = 0;
> >       cancelled_ = false;
> >       prepared_ = false;
> > @@ -284,6 +285,21 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)
> >       emitPrepareCompleted();
> >  }
> >
> > +/**
> > + * \brief Update the error flags of the Request
> > + * \param[in] flags Flags to apply on the Request
> > + *
> > + * Apply \a flags to the Request to report to the application when the Request
> > + * completes.
> > + *
> > + * Setting an Error flag does not cause a Request to fail, and once set it can
> > + * only be cleared by the application destroying the Request or calling reuse().
> > + */
> > +void Request::Private::setErrorFlags(ErrorFlags flags)
> > +{
> > +     error_ |= flags;
> > +}
> > +
> >  void Request::Private::timeout()
> >  {
> >       /* A timeout can only happen if there are fences not yet signalled. */
> > @@ -318,6 +334,22 @@ void Request::Private::timeout()
> >   * Reuse the buffers that were previously added by addBuffer()
> >   */
> >
> > +/**
> > + * \enum Request::ErrorFlag
> > + * Flags to report non-fatal errors
> > + * \var Request::NoError
> > + * No error
> 
> No error. The Request completed succesfully and all its buffer contain
> valid data.

Yes, I think that's a good expansion.


> 
> > + * \var Request::ControlError
> > + * Control Error. At least on control was not able to be applied to the device.
> 
> s/on/one
> 
> Isn't "was not able to be applied" a bit convoluted ?
> 
> At lest one control was not correctly applied to the Camera.

The issue I have here, is that as I understand it - failing to set one
- probably means failing to set more than one control. All following
controls in the control list are in indetermined state, while previous
controls (of the list) are set.

Essentially - it means our overall state becomes a bit undefined. :-(


 
> > + * The application should compare the metadata to the requested control values
> > + * to check which controls weren't applied.
> > + */
> > +
> > +/**
> > + * \typedef Request::ErrorFlags
> > + * The error state of the request defined by \a Request::ErrorFlag
> > + */
> > +
> >  /**
> >   * \typedef Request::BufferMap
> >   * \brief A map of Stream to FrameBuffer pointers
> > @@ -560,6 +592,20 @@ uint32_t Request::sequence() const
> >   * \return The request completion status
> >   */
> >
> > +/**
> > + * \brief Retrieve the error flags
> > + *
> > + * The request could complete with non-fatal error. The completion status will
> > + * indicate success. This function returns the non-fatal errors that the
> > + * request completed with
> > + *
> > + * \return Flags of non-fatal errors that the request completed with
> 
> I understand we have debated fatal vs non-fatal errors, but I would
> leave it out from here. As the error flags description doesn't mention
> that the request is cancelled I don't think it's necessary to describe
> this as non-fatal, mostly because there's not clear definition in the
> documentation of what fatal means.
> 
> I would then remove non-fatal from the documentation and expand this a
> bit to make it clear this is separate from Request::Status.
> 
> Speaking of which, are errors meaningful when a request completes in
> Cancelled state ? (to be eventually added below)


No - if a request is cancelled, then the error flags are meaningless. As
I udnerstand it currently Request state Cancelled - means do not requeue
the request. And do not even look into the request, it's all invalid.


> /**
>  * \brief Retrieve the mask of error flags associated with a completed request
>  *
>  * The request could complete with errors, which indicate failures in
>  * completing correctly parts of the request submitted by the application.
>  *
>  * The possible failure reasons are defined by the error flags defined
>  * by Request::ErrorFlag and application are expected to retrieve the
>  * mask of error flags by using this function before accessing the
>  * buffers and data associated with a completed request.
>  *
>  * Error conditions reported through this function do not change the
>  * request completion status retrieved through Request::status() which
>  * indicates if the Request has been processed or not.
>  *
>  * \return Mask of ErrorFlags that the request completed with
>  */
> 
> Please note that the documentation of Request::status() mentions
> "errors" as well.
> 
> If we go towards a model where errors are reported through this
> function and the completion status is reported through "status()" the
> documentation there should be updated.
> 
> In example, if we want "status" to report if the Request has been
> processed or not (as a Cancelled request has not been processed at
> all, and that's the only failure status we have there) I would updated
> the documentation with
> 
> /*
>  * \fn Request::status()
>  * \brief Retrieve the request completion status
>  *
>  * The request status indicates whether the request has completed successfully
>  * or has been cancelled before being processed.
>  *
>  * Requests are created with their status set to RequestPending. When
>  * a Request is successfully processed and completed by the Camera its
>  * status is set to RequestComplete. If a Request is cancelled before
>  * being processed, in example because the Camera has been stopped
>  * before the request is completed, its status is set to RequestCancelled.
>  *
>  * Successfully completed requests can complete with errors,
>  * applications shall inspect the error mask returned by
>  * Request::error() before accessing buffers and data associated with
>  * a completed request.
>  *
>  * \return The request completion status
>  */

That sounds good too. (With a very small grammar fix to 
  "/in example/for example"

> 
> 
> > + */
> > +Request::ErrorFlags Request::error() const
> > +{
> > +     return _d()->error_;
> > +}
> > +
> >  /**
> >   * \brief Check if a request has buffers yet to be completed
> >   *
> > --
> > 2.34.1
> >