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

[Hirokazu Honda]

Hi Laurent, thanks for reviewing,


On Mon, Mar 29, 2021 at 2:04 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
> >

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

Best Regards,
-Hiro

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