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

[Laurent Pinchart]

Hi Hiro,

On Mon, Mar 29, 2021 at 03:32:47PM +0900, Hirokazu Honda wrote:
> On Mon, Mar 29, 2021 at 2:04 PM Laurent Pinchart wrote:
> > 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.
> > >
> 
> I got it. I am adding the new status enum value.
> status_ is set in complete(). Currently it is decided by cancelled_,
> so that the status_ is either Complete or Cancelled.
> Do you have any good idea how to solve this issue?
> I would add error_ (error code, int) and decide the status value by
> cancelled_ and error_.

Let's keep it simple for now as we know we'll have to rework this.
Adding a private bool error_ member to Request should be fine.

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