[libcamera-devel,v2,01/10] libcamera: request: Add support for error flags

16976 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 occurred 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.

ControlError occurs 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>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/request.h |  4 ++
 include/libcamera/request.h          |  9 ++++
 src/libcamera/request.cpp            | 72 ++++++++++++++++++++++++++--
 3 files changed, 81 insertions(+), 4 deletions(-)

Hi Jacopo,

Thank you for the patch.

On 8/5/22 19:23, Jacopo Mondi 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 occurred 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.
>
> ControlError occurs 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>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>


Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   include/libcamera/internal/request.h |  4 ++
>   include/libcamera/request.h          |  9 ++++
>   src/libcamera/request.cpp            | 72 ++++++++++++++++++++++++++--
>   3 files changed, 81 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> index 9dadd6c60361..76cc32f73f9c 100644
> --- a/include/libcamera/internal/request.h
> +++ b/include/libcamera/internal/request.h
> @@ -42,6 +42,8 @@ public:
>   	void prepare(std::chrono::milliseconds timeout = 0ms);
>   	Signal<> prepared;
>   
> +	void setError(Errors error);
> +
>   private:
>   	friend class PipelineHandler;
>   	friend std::ostream &operator<<(std::ostream &out, const Request &r);
> @@ -59,6 +61,8 @@ private:
>   	std::unordered_set<FrameBuffer *> pending_;
>   	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
>   	std::unique_ptr<Timer> timer_;
> +
> +	Errors errors_;
>   };
>   
>   } /* namespace libcamera */
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index dffde1536cad..3304da31200d 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 Errors = 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_; }
> +	Errors error() const;
>   
>   	bool hasPendingBuffers() const;
>   
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index d2af1d2212ad..9a808f19fc03 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -162,6 +162,7 @@ void Request::Private::cancel()
>    */
>   void Request::Private::reuse()
>   {
> +	errors_ = 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] error Error flags to apply on the Request
> + *
> + * Flag \a error in the Request which will get reported 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::setError(Errors error)
> +{
> +	errors_ |= error;
> +}
> +
>   void Request::Private::timeout()
>   {
>   	/* A timeout can only happen if there are fences not yet signalled. */
> @@ -318,6 +334,25 @@ void Request::Private::timeout()
>    * Reuse the buffers that were previously added by addBuffer()
>    */
>   
> +/**
> + * \enum Request::ErrorFlag
> + * Flags to report errors on a completed request
> + *
> + * \var Request::NoError
> + * No error. The Request completed succesfully and all its buffer contain
> + * valid data
> + *
> + * \var Request::ControlError
> + * Control Error. At least one control was not applied correctly to the camera.
> + * The application should compare the metadata to the requested control values
> + * to check which controls weren't applied.
> + */
> +
> +/**
> + * \typedef Request::Errors
> + * The error state of the request defined by \a Request::ErrorFlag
> + */
> +
>   /**
>    * \typedef Request::BufferMap
>    * \brief A map of Stream to FrameBuffer pointers
> @@ -552,14 +587,43 @@ uint32_t Request::sequence() const
>    * \brief Retrieve the request completion status
>    *
>    * The request status indicates whether the request has completed successfully
> - * or with an error. When requests are created and before they complete the
> - * request status is set to RequestPending, and is updated at completion time
> - * to RequestComplete. If a request is cancelled at capture stop before it has
> - * completed, its status is set to RequestCancelled.
> + * 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, for 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
>    */
>   
> +/**
> + * \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 A mask of error identifier with which the request was completed
> + */
> +Request::Errors Request::error() const
> +{
> +	return _d()->errors_;
> +}
> +
>   /**
>    * \brief Check if a request has buffers yet to be completed
>    *

Hi Jacopo and Paul,

Thank you for the patch.

On Fri, Aug 05, 2022 at 03:53:03PM +0200, Jacopo Mondi via libcamera-devel wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
> 
> Add error flags to the Request to indicate non-fatal errors.

What differentiates fatal and non-fatal errors ? Asked differently, if
these are non-fatal errors, what are the fatal errors ? I don't think we
define that concept anywhere.

> Applications should check the error() state of the Request to determine
> if any fault occurred while processing the request.

I think this needs to be documented in appropriate places in
Documentation/, especially in the guides, with an update to simple-cam.
The cam and qcam applications, as well as the adaptation layers, also
need to be updated.

> 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.
> 
> ControlError occurs when a Request was asked to set a control and the
> pipeline handler attempted to do so, but it was rejected by the kernel.

This doesn't really match the definition in the documentation below.

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/request.h |  4 ++
>  include/libcamera/request.h          |  9 ++++
>  src/libcamera/request.cpp            | 72 ++++++++++++++++++++++++++--
>  3 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> index 9dadd6c60361..76cc32f73f9c 100644
> --- a/include/libcamera/internal/request.h
> +++ b/include/libcamera/internal/request.h
> @@ -42,6 +42,8 @@ public:
>  	void prepare(std::chrono::milliseconds timeout = 0ms);
>  	Signal<> prepared;
>  
> +	void setError(Errors error);
> +
>  private:
>  	friend class PipelineHandler;
>  	friend std::ostream &operator<<(std::ostream &out, const Request &r);
> @@ -59,6 +61,8 @@ private:
>  	std::unordered_set<FrameBuffer *> pending_;
>  	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
>  	std::unique_ptr<Timer> timer_;
> +
> +	Errors errors_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index dffde1536cad..3304da31200d 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 Errors = Flags<ErrorFlag>;

I'd s/Errors/ErrorFlags/ to be more explicit (and to match MapFlags from
the File and MappedFrameBuffer classes).

> +
>  	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_; }
> +	Errors error() const;

The function name is confusing, singular implies a single error. There's
a similar issue with the error parameter to setError. Renaming the
functions to errorFlags and setErrorFlags could help, there may be
better names.

>  
>  	bool hasPendingBuffers() const;
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index d2af1d2212ad..9a808f19fc03 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -162,6 +162,7 @@ void Request::Private::cancel()
>   */
>  void Request::Private::reuse()
>  {
> +	errors_ = 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] error Error flags to apply on the Request
> + *
> + * Flag \a error in the Request which will get reported to the application when
> + * the Request completes.

Same as above, this should be adjusted depending on whether the
setError() function is meant to set a single error (it should take an
ErrorFlag then) or possibly multiple errors. Thinking ahead about
asynchronous error reporting, as well as typical usage patterns for this
function (would a pipeline handler have to set multiple error flags at
once ?) could help decide.

> + *
> + * 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::setError(Errors error)
> +{
> +	errors_ |= error;
> +}
> +
>  void Request::Private::timeout()
>  {
>  	/* A timeout can only happen if there are fences not yet signalled. */
> @@ -318,6 +334,25 @@ void Request::Private::timeout()
>   * Reuse the buffers that were previously added by addBuffer()
>   */
>  
> +/**
> + * \enum Request::ErrorFlag
> + * Flags to report errors on a completed request
> + *
> + * \var Request::NoError
> + * No error. The Request completed succesfully and all its buffer contain
> + * valid data
> + *
> + * \var Request::ControlError
> + * Control Error. At least one control was not applied correctly to the camera.
> + * The application should compare the metadata to the requested control values
> + * to check which controls weren't applied.
> + */
> +
> +/**
> + * \typedef Request::Errors
> + * The error state of the request defined by \a Request::ErrorFlag
> + */
> +
>  /**
>   * \typedef Request::BufferMap
>   * \brief A map of Stream to FrameBuffer pointers
> @@ -552,14 +587,43 @@ uint32_t Request::sequence() const
>   * \brief Retrieve the request completion status
>   *
>   * The request status indicates whether the request has completed successfully
> - * or with an error. When requests are created and before they complete the
> - * request status is set to RequestPending, and is updated at completion time
> - * to RequestComplete. If a request is cancelled at capture stop before it has
> - * completed, its status is set to RequestCancelled.
> + * 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, for example because the Camera has been stopped
> + * before the request is completed, its status is set to RequestCancelled.

I wonder if request cancellation should be reported as an error flag. We
may then possibly drop the request status. No need to do so now, but
opinions will be welcome.

> + *
> + * 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
>   */
>  
> +/**
> + * \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 A mask of error identifier with which the request was completed
> + */
> +Request::Errors Request::error() const
> +{
> +	return _d()->errors_;
> +}

One point that bothers me a bit is that I don't have a good view on what
other type of errors could be reported later through this mechanism. If
we end up with a single error flag for the request, then usage of the
Flags<> class will be overkill.

> +
>  /**
>   * \brief Check if a request has buffers yet to be completed
>   *

Hi Laurent

On Tue, Aug 16, 2022 at 06:19:43AM +0300, Laurent Pinchart wrote:
> Hi Jacopo and Paul,
>
> Thank you for the patch.
>
> On Fri, Aug 05, 2022 at 03:53:03PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> >
> > Add error flags to the Request to indicate non-fatal errors.
>
> What differentiates fatal and non-fatal errors ? Asked differently, if
> these are non-fatal errors, what are the fatal errors ? I don't think we
> define that concept anywhere.
>

I pushed to remove the terms "fatal" vs "non-fatal" in the PFC
design and in this series, and this is a left-over.

Fatal vs non-fatal comes from the requirement of defining what errors
cause a Request to be cancelled vs errors that are set on a Request
that completes correctly. As we have no errors that cause a Request to
be cancelled I agree the distinction creates more confusion to a
reader that is not aware of the background. I'll remove it.


> > Applications should check the error() state of the Request to determine
> > if any fault occurred while processing the request.
>
> I think this needs to be documented in appropriate places in
> Documentation/, especially in the guides, with an update to simple-cam.
> The cam and qcam applications, as well as the adaptation layers, also
> need to be updated.
>

That's a good point.

Looking at your comments, it seems you prefer to merge from 04 on and
break-out error flags, as they clearly need more attention.

> > 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.
> >
> > ControlError occurs when a Request was asked to set a control and the
> > pipeline handler attempted to do so, but it was rejected by the kernel.
>
> This doesn't really match the definition in the documentation below.

This is the error flag documentation

        + * \var Request::ControlError
        + * Control Error. At least one control was not applied correctly to the camera.
        + * The application should compare the metadata to the requested control values
        + * to check which controls weren't applied.

Is it the "by the kernel" part that bothers you ?

>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/internal/request.h |  4 ++
> >  include/libcamera/request.h          |  9 ++++
> >  src/libcamera/request.cpp            | 72 ++++++++++++++++++++++++++--
> >  3 files changed, 81 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> > index 9dadd6c60361..76cc32f73f9c 100644
> > --- a/include/libcamera/internal/request.h
> > +++ b/include/libcamera/internal/request.h
> > @@ -42,6 +42,8 @@ public:
> >  	void prepare(std::chrono::milliseconds timeout = 0ms);
> >  	Signal<> prepared;
> >
> > +	void setError(Errors error);
> > +
> >  private:
> >  	friend class PipelineHandler;
> >  	friend std::ostream &operator<<(std::ostream &out, const Request &r);
> > @@ -59,6 +61,8 @@ private:
> >  	std::unordered_set<FrameBuffer *> pending_;
> >  	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
> >  	std::unique_ptr<Timer> timer_;
> > +
> > +	Errors errors_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index dffde1536cad..3304da31200d 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 Errors = Flags<ErrorFlag>;
>
> I'd s/Errors/ErrorFlags/ to be more explicit (and to match MapFlags from
> the File and MappedFrameBuffer classes).
>

That's what Kieran had and I shortened it.

I can revert it back, but it really seems a duplication and what is
relevant for applications is knowing that this is a mask of errors,
the fact it is implemented with Flags<> is secondary.

Anyway, no need to bikeshed. As Kieran had it in this way and you feel
the same (I don't recall about Umang's opinion here) I'll change it
back.

> > +
> >  	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_; }
> > +	Errors error() const;
>
> The function name is confusing, singular implies a single error. There's

errors() ?

> a similar issue with the error parameter to setError. Renaming the

setError(ErrorFlags errors) ?

> functions to errorFlags and setErrorFlags could help, there may be
> better names.
>
> >
> >  	bool hasPendingBuffers() const;
> >
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index d2af1d2212ad..9a808f19fc03 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -162,6 +162,7 @@ void Request::Private::cancel()
> >   */
> >  void Request::Private::reuse()
> >  {
> > +	errors_ = 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] error Error flags to apply on the Request
> > + *
> > + * Flag \a error in the Request which will get reported to the application when
> > + * the Request completes.
>
> Same as above, this should be adjusted depending on whether the
> NetError() function is meant to set a single error (it should take an
> ErrorFlag then) or possibly multiple errors. Thinking ahead about
> asynchronous error reporting, as well as typical usage patterns for this
> function (would a pipeline handler have to set multiple error flags at
> once ?) could help decide.
>

I do feel like, in the context of async error signalling, that
setting/signalling single errors might be more opportune. Can we start
with that ?

> > + *
> > + * 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::setError(Errors error)
> > +{
> > +	errors_ |= error;
> > +}
> > +
> >  void Request::Private::timeout()
> >  {
> >  	/* A timeout can only happen if there are fences not yet signalled. */
> > @@ -318,6 +334,25 @@ void Request::Private::timeout()
> >   * Reuse the buffers that were previously added by addBuffer()
> >   */
> >
> > +/**
> > + * \enum Request::ErrorFlag
> > + * Flags to report errors on a completed request
> > + *
> > + * \var Request::NoError
> > + * No error. The Request completed succesfully and all its buffer contain
> > + * valid data
> > + *
> > + * \var Request::ControlError
> > + * Control Error. At least one control was not applied correctly to the camera.
> > + * The application should compare the metadata to the requested control values
> > + * to check which controls weren't applied.
> > + */
> > +
> > +/**
> > + * \typedef Request::Errors
> > + * The error state of the request defined by \a Request::ErrorFlag
> > + */
> > +
> >  /**
> >   * \typedef Request::BufferMap
> >   * \brief A map of Stream to FrameBuffer pointers
> > @@ -552,14 +587,43 @@ uint32_t Request::sequence() const
> >   * \brief Retrieve the request completion status
> >   *
> >   * The request status indicates whether the request has completed successfully
> > - * or with an error. When requests are created and before they complete the
> > - * request status is set to RequestPending, and is updated at completion time
> > - * to RequestComplete. If a request is cancelled at capture stop before it has
> > - * completed, its status is set to RequestCancelled.
> > + * 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, for example because the Camera has been stopped
> > + * before the request is completed, its status is set to RequestCancelled.
>
> I wonder if request cancellation should be reported as an error flag. We
> may then possibly drop the request status. No need to do so now, but
> opinions will be welcome.
>

We should really think if we want to maintain the semantic currently
associated with StatusCancelled. Right now, it only serves to report
to application that the camera has been stopped and no requests can be
queued anymore. It doesn't feel like an async error would fit there.
We're back to discuss what's fatal vs what's not fatal, also
considering what future error flags we might have to add.

Currently I'm fine with "non fatal" errors being set through flags and
request cancellation being signalled by the status. It nicely matches
the idea that "Completed" requests have been processed and might
complete with errors, "Cancelled" requests have been returned before
being processed because the camera device has stopped.

Asking the same question again: do we foresee errors that might cause
the device to stop and make it impossible to process all future requests ?
Currently we have none, and we thought it over a bit and so far all
the use cases for error flags are "non-fatal" ones.


> > + *
> > + * 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
> >   */
> >
> > +/**
> > + * \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 A mask of error identifier with which the request was completed
> > + */
> > +Request::Errors Request::error() const
> > +{
> > +	return _d()->errors_;
> > +}
>
> One point that bothers me a bit is that I don't have a good view on what
> other type of errors could be reported later through this mechanism. If
> we end up with a single error flag for the request, then usage of the
> Flags<> class will be overkill.
>

We'll have more, and we tried collecting them in the PFC design
document.

> > +
> >  /**
> >   * \brief Check if a request has buffers yet to be completed
> >   *
>
> --
> Regards,
>
> Laurent Pinchart