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

16713 diff mbox series

From: Paul Elder <paul.elder@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@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v2:
 - Add documentation for Request::Private::setErrorFlags

---
v2
 - Extend documentation

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/request.h |  3 ++
 include/libcamera/request.h          |  9 ++++++
 src/libcamera/request.cpp            | 46 ++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

Hi Kieran,

On 7/21/22 17:42, Kieran Bingham via libcamera-devel wrote:
> From: Paul Elder <paul.elder@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@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> ---
> v2:
>   - Add documentation for Request::Private::setErrorFlags
>
> ---
> v2
>   - Extend documentation
>
> Signed-off-by: Kieran Bingham <kieran.bingham@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);
>   
>   	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_;
>   };
>   
>   } /* 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 {
> +		NoError = 0,
> +		ControlError = (1 << 0),
> +	};
> +
> +	using ErrorFlags = Flags<ErrorFlag>;
> +
>   	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.


s/to report to the application/which will get reported to the application/

> + *
> + * 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
> + * \var Request::ControlError
> + * Control Error. At least on control was not able to be applied to the device.


s/on/one/ maybe?

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


s/that the request completed with/with which the request was completed.

> + *
> + * \return Flags of non-fatal errors that the request completed with
> + */
> +Request::ErrorFlags Request::error() const
> +{
> +	return _d()->error_;
> +}
> +
>   /**
>    * \brief Check if a request has buffers yet to be completed
>    *

Hi Kieran

On Thu, Jul 21, 2022 at 01:12:59PM +0100, Kieran Bingham via libcamera-devel wrote:
> From: Paul Elder <paul.elder@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@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> ---
> v2:
>  - Add documentation for Request::Private::setErrorFlags
>
> ---
> v2
>  - Extend documentation
>
> Signed-off-by: Kieran Bingham <kieran.bingham@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.

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

>  };
>
>  } /* 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

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

and s/ErrorFlags/Errors/

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

> + * \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 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)


/**
 * \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
 */


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

Quoting Umang Jain (2022-07-22 16:15:05)
> Hi Kieran,
> 
> On 7/21/22 17:42, Kieran Bingham via libcamera-devel wrote:
> > From: Paul Elder <paul.elder@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@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > ---
> > v2:
> >   - Add documentation for Request::Private::setErrorFlags
> >
> > ---
> > v2
> >   - Extend documentation
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@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);
> >   
> >       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_;
> >   };
> >   
> >   } /* 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 {
> > +             NoError = 0,
> > +             ControlError = (1 << 0),
> > +     };
> > +
> > +     using ErrorFlags = Flags<ErrorFlag>;
> > +
> >       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.
> 
> 
> s/to report to the application/which will get reported to the application/

Ack.


> 
> > + *
> > + * 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
> > + * \var Request::ControlError
> > + * Control Error. At least on control was not able to be applied to the device.
> 
> 
> s/on/one/ maybe?

Ack.


> 
> > + * 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
> 
> 
> s/that the request completed with/with which the request was completed.

Ack, but I think Jacopo has comments here too.


> 
> > + *
> > + * \return Flags of non-fatal errors that the request completed with
> > + */
> > +Request::ErrorFlags Request::error() const
> > +{
> > +     return _d()->error_;
> > +}
> > +
> >   /**
> >    * \brief Check if a request has buffers yet to be completed
> >    *

@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@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@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > ---
> > v2:
> >  - Add documentation for Request::Private::setErrorFlags
> >
> > ---
> > v2
> >  - Extend documentation
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@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
> >