[libcamera-devel,1/2] libcamera: request: Add support for error flags
diff mbox series

Message ID 20220719103144.3686313-2-kieran.bingham@ideasonboard.com
State New
Delegated to: Kieran Bingham
Headers show
Series
  • request: Support non fatal errors
Related show

Commit Message

Kieran Bingham July 19, 2022, 10:31 a.m. UTC 
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>
---
 include/libcamera/internal/request.h |  3 +++
 include/libcamera/request.h          |  9 +++++++
 src/libcamera/request.cpp            | 36 ++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

Comments

Jacopo Mondi July 19, 2022, 3:40 p.m. UTC | #1 
Hi Kieran

On Tue, Jul 19, 2022 at 11:31:43AM +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>
> ---
>  include/libcamera/internal/request.h |  3 +++
>  include/libcamera/request.h          |  9 +++++++
>  src/libcamera/request.cpp            | 36 ++++++++++++++++++++++++++++
>  3 files changed, 48 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 07613cb33709..d585ef0b0ae4 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,11 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)
>  	emitPrepareCompleted();
>  }
>
> +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 +324,22 @@ void Request::Private::timeout()
>   * Reuse the buffers that were previously added by addBuffer()
>   */
>
> +/**
> + * \enum Request::ErrorFlag
> + * Flags to report non-fatal errors

I'm afraid the documentation should likely be expanded a bit :)

In particular, what does it mean for application an error is non-fatal ?
That the request contains valid data ? That the request does not contain
valid data but the device is operating and new requests can be queued ?

> + * \var Request::NoError
> + * No error

Do we need a NoError ?

Isn't testing with:

        if (request->error())

enough ?

> + * \var Request::ControlError
> + * Control Error. At least on control was not able to be applied to the device.

s/on control/one control

> + * 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.
> + */
> +
>  /**
>   * \typedef Request::BufferMap
>   * \brief A map of Stream to FrameBuffer pointers
> @@ -560,6 +582,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
> + *

Here as well I think a better definition of success and what fatal
means is required. Linking to the flags enum documentation, when
expanded, might be enough

> + * \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
>   *
> --
> 2.34.1
>
Kieran Bingham July 20, 2022, 12:55 p.m. UTC | #2 
Quoting Jacopo Mondi (2022-07-19 16:40:12)
> Hi Kieran
> 
> On Tue, Jul 19, 2022 at 11:31:43AM +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>
> > ---
> >  include/libcamera/internal/request.h |  3 +++
> >  include/libcamera/request.h          |  9 +++++++
> >  src/libcamera/request.cpp            | 36 ++++++++++++++++++++++++++++
> >  3 files changed, 48 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 07613cb33709..d585ef0b0ae4 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,11 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)
> >       emitPrepareCompleted();
> >  }
> >
> > +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 +324,22 @@ void Request::Private::timeout()
> >   * Reuse the buffers that were previously added by addBuffer()
> >   */
> >
> > +/**
> > + * \enum Request::ErrorFlag
> > + * Flags to report non-fatal errors
> 
> I'm afraid the documentation should likely be expanded a bit :)

Well this was supposed to be marked RFC ;-) so sure.
 
> In particular, what does it mean for application an error is non-fatal ?
> That the request contains valid data ? That the request does not contain
> valid data but the device is operating and new requests can be queued ?
> 
> > + * \var Request::NoError
> > + * No error
> 
> Do we need a NoError ?

Yes, we need to be able to initialise to 'No Error'.
See Request::Private::reuse() above.

(You can't set error_ = 0, with Flags<>);

> 
> Isn't testing with:
> 
>         if (request->error())

Yes, that's also sufficient. These two are equivalent:

	if (request->error())
	if (request->error() != Request::NoError)



> enough ?
> 
> > + * \var Request::ControlError
> > + * Control Error. At least on control was not able to be applied to the device.
> 
> s/on control/one control
> 
> > + * 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.
> > + */
> > +
> >  /**
> >   * \typedef Request::BufferMap
> >   * \brief A map of Stream to FrameBuffer pointers
> > @@ -560,6 +582,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
> > + *
> 
> Here as well I think a better definition of success and what fatal
> means is required. Linking to the flags enum documentation, when
> expanded, might be enough

Linking to the detail of the enums is certainly a good addition.


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

Patch
diff mbox series 

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 07613cb33709..d585ef0b0ae4 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,11 @@  void Request::Private::notifierActivated(FrameBuffer *buffer)
 	emitPrepareCompleted();
 }
 
+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 +324,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.
+ * 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.
+ */
+
 /**
  * \typedef Request::BufferMap
  * \brief A map of Stream to FrameBuffer pointers
@@ -560,6 +582,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
+ */
+Request::ErrorFlags Request::error() const
+{
+	return _d()->error_;
+}
+
 /**
  * \brief Check if a request has buffers yet to be completed
  *