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

[Laurent Pinchart]

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 at 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 at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at 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
>   *

-- 
Regards,

Laurent Pinchart