[libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error value

[Laurent Pinchart]

Hi Hiro,

One more comment.

On Mon, Mar 29, 2021 at 07:52:52AM +0300, Laurent Pinchart wrote:
> On Mon, Mar 29, 2021 at 09:27:14AM +0900, Hirokazu Honda wrote:
> > libcamera::Request returned by Camera::requestComplete() is
> > always assumed. Therefore, an error occurred in
> > PipelineHandler::queueRequest() is ignored.
> > This adds an error value, which is zero on success, to
> > libcamera::Request() so that a Camera client can know the request
> > error in completing request.
> 
> As the result is only valid when status() is Request::Complete, it
> creates a risk for libcamera to set status() and result() to incoherent
> values, and for applications to badly interpret them. I think it would
> be better to instead extend Request::Status with a RequestError. We may
> later need to distinguish between different types of errors, but until
> we reach that point, extending the status() will be simpler.
> 
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  include/libcamera/request.h        |  2 ++
> >  src/libcamera/pipeline_handler.cpp |  6 ++++--
> >  src/libcamera/request.cpp          | 21 ++++++++++++++++-----
> >  3 files changed, 22 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 6e5aad5f..60e91d5f 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -52,6 +52,7 @@ public:
> >  
> >  	uint64_t cookie() const { return cookie_; }
> >  	Status status() const { return status_; }
> > +	int result() const { return result_; }
> >  
> >  	bool hasPendingBuffers() const { return !pending_.empty(); }
> >  
> > @@ -73,6 +74,7 @@ private:
> >  
> >  	const uint64_t cookie_;
> >  	Status status_;
> > +	int result_;
> >  	bool cancelled_;
> >  };
> >  
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 05b807d6..a9a5523b 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -383,8 +383,10 @@ void PipelineHandler::queueRequest(Request *request)
> >  	data->queuedRequests_.push_back(request);
> >  
> >  	int ret = queueRequestDevice(camera, request);
> > -	if (ret)
> > -		data->queuedRequests_.remove(request);
> > +	if (ret) {
> > +		request->result_ = ret;
> > +		completeRequest(request);

Shouldn't the buffers in the request be marked with FrameError ?

Interactions between buffer states and request states may need a
revisit, and this includes the way we handle cancelling requests. This
is a topic that has recently been brought up in the context of the "IPU3
Debug Improvements" series, or a previous version of it (I can't locate
the exact message I'm thinking about at the moment). To avoid delaying
this fix, let's go for the simplest solution for now, which means
avoiding the new result() function as that has an impact through the
whole code base, and adding a new status value instead. We'll then
decide how to rework this.

> > +	}
> >  }
> >  
> >  /**
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 0071667e..8106437e 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -72,7 +72,7 @@ LOG_DEFINE_CATEGORY(Request)
> >   *
> >   */
> >  Request::Request(Camera *camera, uint64_t cookie)
> > -	: camera_(camera), cookie_(cookie), status_(RequestPending),
> > +	: camera_(camera), cookie_(cookie), status_(RequestPending), result_(0),
> >  	  cancelled_(false)
> >  {
> >  	/**
> > @@ -236,15 +236,26 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
> >   * \fn Request::status()
> >   * \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
> > + * The request status indicates whether the request has pended, completed or
> > + * cancelled.. 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.
> >   *
> >   * \return The request completion status
> >   */
> >  
> > +/**
> > + * \fn Request::result()
> > + * \brief Retrieve the error value of the request .
> > + *
> > + * The request error indicates whether the request has completed successfully or
> > + * with an error. It is a negative value if an error happens in queueing and
> > + * processing the request, or 0 on success.
> > + *
> > + * \return The request error value.
> > + */
> > +
> >  /**
> >   * \fn Request::hasPendingBuffers()
> >   * \brief Check if a request has buffers yet to be completed

-- 
Regards,

Laurent Pinchart