| Message ID | 20210329002715.74403-3-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. 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@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); > + } > } > > /** > 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
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@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
Hi Laurent, thanks for reviewing, On Mon, Mar 29, 2021 at 2:04 PM Laurent Pinchart <laurent.pinchart@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@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
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@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
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); + } } /** 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
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. Signed-off-by: Hirokazu Honda <hiroh@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(-)