[libcamera-devel,RFC,2/2] libcamera: PipelineHandler: Retry queuing a capture request

11756 diff mbox series

PipelineHandler::queueRequestDevice() fails due to a buffer
shortage. We should retry queuing a capture request later in the
case.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/pipeline_handler.h |  2 +
 src/libcamera/pipeline_handler.cpp            | 57 +++++++++++++++++--
 2 files changed, 54 insertions(+), 5 deletions(-)

Hey Hirokazu,

Thanks for the patch.

This patch currently doesn't apply on the master branch of libcamera:
```
error: sha1 information is lacking or useless (include/libcamera/internal/pipeline_handler.h).
error: could not build fake ancestor
Patch failed at 0002 libcamera: PipelineHandler: Retry queuing a capture request
```


On 29.03.2021 11:26, Hirokazu Honda wrote:
>PipelineHandler::queueRequestDevice() fails due to a buffer
>shortage. We should retry queuing a capture request later in the
>case.

s/in the case/in that case/

>
>Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>---
> include/libcamera/internal/pipeline_handler.h |  2 +
> src/libcamera/pipeline_handler.cpp            | 57 +++++++++++++++++--
> 2 files changed, 54 insertions(+), 5 deletions(-)
>
>diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>index 763da63e..e6f771a6 100644
>--- a/include/libcamera/internal/pipeline_handler.h
>+++ b/include/libcamera/internal/pipeline_handler.h
>@@ -44,6 +44,7 @@ public:
> 	virtual ~CameraData() = default;
>
> 	PipelineHandler *pipe_;
>+	std::queue<Request *> waitedRequests_;

s/waitedRequests_/waitingRequests_/ ?

Waited is the past tense of wait and therefore this variable sounds like
those are requests, that we waited for in the past, but we still wait
for them. Therefore we should use the present progressive, which in the
case of wait is waiting.
As an alternative, we might also call the variable: failedRequests_,
as it describes requests, that failed to be inserted into the queue
and need to be queued again.

The correction applies to all other cases below, where that variable is
used.

> 	std::deque<Request *> queuedRequests_;
> 	ControlInfoMap controlInfo_;
> 	ControlList properties_;
>@@ -99,6 +100,7 @@ protected:
> 	CameraManager *manager_;
>
> private:
>+	void tryQueueRequests(CameraData *data);
> 	void mediaDeviceDisconnected(MediaDevice *media);
> 	virtual void disconnect();
>
>diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>index 4cb58084..18952a1b 100644
>--- a/src/libcamera/pipeline_handler.cpp
>+++ b/src/libcamera/pipeline_handler.cpp
>@@ -70,6 +70,16 @@ LOG_DEFINE_CATEGORY(Pipeline)
>  * and remains valid until the instance is destroyed.
>  */
>
>+/**
>+ * \var CameraData::waitedRequests_
>+ * \brief The queue of not yet queued request
>+ *
>+ * The queue of not yet queued request is used to track requests that are not
>+ * queued in order to retry queueing them when a pipeline is ready to accept.

I find this sentence confusing to read, there is too much use of the
word queue.  How about:
This queue of failed requests is used to retry queuing as soon as the pipeline
is ready to accept them.


>+ *
>+ * \sa PipelineHandler::queueRequest(), PipelineHandler::tryQueueRequests().
>+ */
>+
> /**
>  * \var CameraData::queuedRequests_
>  * \brief The queue of queued and not yet completed request
>@@ -378,12 +388,44 @@ void PipelineHandler::queueRequest(Request *request)
>
> 	Camera *camera = request->camera_;
> 	CameraData *data = cameraData(camera);
>-	data->queuedRequests_.push_back(request);
>+	data->waitedRequests_.push(request);
>+
>+	tryQueueRequests(data);
>+}
>+
>+/**
>+ * \fn PipelineHandler::tryQueueRequests()
>+ * \brief Queue requests that are not yet queued.
>+ * \param[in] data The camera data whose waited requests are tried to queue.

s/The camera data whose waited requests are tried to queue./
   CameraData containing a queue of failed requests to retry queuing./
>+ *
>+ * This tries to queue as many requests as possible in order of the
>+ * waitedRequests_ in data. If queuing fails due to a buffer shortage, this
>+ * method stops and the next call of this method starts from the request that
>+ * fails in the previous call. On any other failures, the request is marked as

s/fails/failed/
s/any other failures/any other failure/

>+ * complete and proceed the successive requests.

s/proceed the/proceed with/

>+ *
>+ * \context This function shall be called from the CameraManager thread.
>+ */
>+void PipelineHandler::tryQueueRequests(CameraData *data)
>+{
>+	while (!data->waitedRequests_.empty()) {
>+		Request *request = data->waitedRequests_.front();
>+		Camera *camera = request->camera_;
>+		ASSERT(data == cameraData(camera));
>+
>+		data->queuedRequests_.push_back(request);
>+		int ret = queueRequestDevice(camera, request);
>+		if (ret == -ENOBUFS || ret == -ENOMEM) {
>+			data->queuedRequests_.pop_back();
>+			break;
>+		}
>
>-	int ret = queueRequestDevice(camera, request);
>-	if (ret) {
>-		request->result_ = ret;
>-		completeRequest(request);
>+		data->waitedRequests_.pop();
>+		if (ret) {
>+			request->result_ = ret;
>+			completeRequest(request);
>+			break;
>+		}
> 	}
> }
>
>@@ -440,6 +482,9 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>  * submission order, the pipeline handler may call it on any complete request
>  * without any ordering constraint.
>  *
>+ * There might be requests that are waiting to be queued, this triggers
>+ * tryQueueRequests().
>+ *
>  * \context This function shall be called from the CameraManager thread.
>  */
> void PipelineHandler::completeRequest(Request *request)
>@@ -459,6 +504,8 @@ void PipelineHandler::completeRequest(Request *request)
> 		data->queuedRequests_.pop_front();
> 		camera->requestComplete(req);
> 	}
>+
>+	tryQueueRequests(data);
> }

It is difficult for me to follow the code here and the patch currently
doesn't apply could you rebase it on master and repost?

Greetings,
Sebastian
>
> /**
>-- 
>2.31.0.291.g576ba9dcdaf-goog
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel

Hi Hiro,

Thank you for the patch.

On Mon, Mar 29, 2021 at 11:26:04AM +0900, Hirokazu Honda wrote:
> PipelineHandler::queueRequestDevice() fails due to a buffer
> shortage. We should retry queuing a capture request later in the
> case.

I don't think this should be handled by the base PipelineHandler class,
but by individual pipeline handlers. There may be needs for pipeline
handlers to peek ahead in the queue of requests to get metadata needed
by the algorithms, with a queue depth larger than the number of buffers.
You queue those requests on waitedRequests_ so the pipeline handler can
have access to them, but the semantics isn't well defined, and I think
that will be quite error prone. Furthermore, the call to
tryQueueRequests() in PipelineHandler::completeRequest() will result in
the pipeline handler's queueRequestDevice() function being called from
the request completion handler, making the pipeline handler code
reentrant, and pipeline handlers are not prepared for that. This may
create race conditions, I'd rather not go that route.

This being said, duplicating request queues in all pipeline handlers
isn't a great idea either. We should share code, but that should be
implemented by helper classes that pipeline handlers can opt-in to use,
not in a mandatory middle layer. Now that we have multiple pipeline
handler implementations, it's time to start cleaning things up, and the
Raspberry Pi and IPU3 pipeline handlers are good candidates for code
sharing as they share a common hardware architecture.

If you want to fix this issue for the IPU3 pipeline handler without
depending on the creation of those helper classes, then the fix should
be implemented in the IPU3 pipeline handler itself in the meantime.

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/internal/pipeline_handler.h |  2 +
>  src/libcamera/pipeline_handler.cpp            | 57 +++++++++++++++++--
>  2 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 763da63e..e6f771a6 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -44,6 +44,7 @@ public:
>  	virtual ~CameraData() = default;
>  
>  	PipelineHandler *pipe_;
> +	std::queue<Request *> waitedRequests_;
>  	std::deque<Request *> queuedRequests_;
>  	ControlInfoMap controlInfo_;
>  	ControlList properties_;
> @@ -99,6 +100,7 @@ protected:
>  	CameraManager *manager_;
>  
>  private:
> +	void tryQueueRequests(CameraData *data);
>  	void mediaDeviceDisconnected(MediaDevice *media);
>  	virtual void disconnect();
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 4cb58084..18952a1b 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -70,6 +70,16 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * and remains valid until the instance is destroyed.
>   */
>  
> +/**
> + * \var CameraData::waitedRequests_
> + * \brief The queue of not yet queued request
> + *
> + * The queue of not yet queued request is used to track requests that are not
> + * queued in order to retry queueing them when a pipeline is ready to accept.
> + *
> + * \sa PipelineHandler::queueRequest(), PipelineHandler::tryQueueRequests().
> + */
> +
>  /**
>   * \var CameraData::queuedRequests_
>   * \brief The queue of queued and not yet completed request
> @@ -378,12 +388,44 @@ void PipelineHandler::queueRequest(Request *request)
>  
>  	Camera *camera = request->camera_;
>  	CameraData *data = cameraData(camera);
> -	data->queuedRequests_.push_back(request);
> +	data->waitedRequests_.push(request);
> +
> +	tryQueueRequests(data);
> +}
> +
> +/**
> + * \fn PipelineHandler::tryQueueRequests()
> + * \brief Queue requests that are not yet queued.
> + * \param[in] data The camera data whose waited requests are tried to queue.
> + *
> + * This tries to queue as many requests as possible in order of the
> + * waitedRequests_ in data. If queuing fails due to a buffer shortage, this
> + * method stops and the next call of this method starts from the request that
> + * fails in the previous call. On any other failures, the request is marked as
> + * complete and proceed the successive requests.
> + *
> + * \context This function shall be called from the CameraManager thread.
> + */
> +void PipelineHandler::tryQueueRequests(CameraData *data)
> +{
> +	while (!data->waitedRequests_.empty()) {
> +		Request *request = data->waitedRequests_.front();
> +		Camera *camera = request->camera_;
> +		ASSERT(data == cameraData(camera));
> +
> +		data->queuedRequests_.push_back(request);
> +		int ret = queueRequestDevice(camera, request);
> +		if (ret == -ENOBUFS || ret == -ENOMEM) {
> +			data->queuedRequests_.pop_back();
> +			break;
> +		}
>  
> -	int ret = queueRequestDevice(camera, request);
> -	if (ret) {
> -		request->result_ = ret;
> -		completeRequest(request);
> +		data->waitedRequests_.pop();
> +		if (ret) {
> +			request->result_ = ret;
> +			completeRequest(request);
> +			break;
> +		}
>  	}
>  }
>  
> @@ -440,6 +482,9 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>   * submission order, the pipeline handler may call it on any complete request
>   * without any ordering constraint.
>   *
> + * There might be requests that are waiting to be queued, this triggers
> + * tryQueueRequests().
> + *
>   * \context This function shall be called from the CameraManager thread.
>   */
>  void PipelineHandler::completeRequest(Request *request)
> @@ -459,6 +504,8 @@ void PipelineHandler::completeRequest(Request *request)
>  		data->queuedRequests_.pop_front();
>  		camera->requestComplete(req);
>  	}
> +
> +	tryQueueRequests(data);
>  }
>  
>  /**

Hi Sebastian and Laurent, thanks for reviewing.

On Mon, Mar 29, 2021 at 2:28 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Mar 29, 2021 at 11:26:04AM +0900, Hirokazu Honda wrote:
> > PipelineHandler::queueRequestDevice() fails due to a buffer
> > shortage. We should retry queuing a capture request later in the
> > case.
>
> I don't think this should be handled by the base PipelineHandler class,
> but by individual pipeline handlers. There may be needs for pipeline
> handlers to peek ahead in the queue of requests to get metadata needed
> by the algorithms, with a queue depth larger than the number of buffers.
> You queue those requests on waitedRequests_ so the pipeline handler can
> have access to them, but the semantics isn't well defined, and I think
> that will be quite error prone. Furthermore, the call to
> tryQueueRequests() in PipelineHandler::completeRequest() will result in
> the pipeline handler's queueRequestDevice() function being called from
> the request completion handler, making the pipeline handler code
> reentrant, and pipeline handlers are not prepared for that. This may
> create race conditions, I'd rather not go that route.
>
> This being said, duplicating request queues in all pipeline handlers
> isn't a great idea either. We should share code, but that should be
> implemented by helper classes that pipeline handlers can opt-in to use,
> not in a mandatory middle layer. Now that we have multiple pipeline
> handler implementations, it's time to start cleaning things up, and the
> Raspberry Pi and IPU3 pipeline handlers are good candidates for code
> sharing as they share a common hardware architecture.
>
> If you want to fix this issue for the IPU3 pipeline handler without
> depending on the creation of those helper classes, then the fix should
> be implemented in the IPU3 pipeline handler itself in the meantime.
>

Thanks for comments.
My first idea is to avoid code duplication caused by handling in each
pipeline handler.
But your comment makes sense.
I upload the change for PipelineHandlerIPU3.
https://patchwork.libcamera.org/patch/11804/

-Hiro




> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  include/libcamera/internal/pipeline_handler.h |  2 +
> >  src/libcamera/pipeline_handler.cpp            | 57 +++++++++++++++++--
> >  2 files changed, 54 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 763da63e..e6f771a6 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -44,6 +44,7 @@ public:
> >       virtual ~CameraData() = default;
> >
> >       PipelineHandler *pipe_;
> > +     std::queue<Request *> waitedRequests_;
> >       std::deque<Request *> queuedRequests_;
> >       ControlInfoMap controlInfo_;
> >       ControlList properties_;
> > @@ -99,6 +100,7 @@ protected:
> >       CameraManager *manager_;
> >
> >  private:
> > +     void tryQueueRequests(CameraData *data);
> >       void mediaDeviceDisconnected(MediaDevice *media);
> >       virtual void disconnect();
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 4cb58084..18952a1b 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -70,6 +70,16 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >   * and remains valid until the instance is destroyed.
> >   */
> >
> > +/**
> > + * \var CameraData::waitedRequests_
> > + * \brief The queue of not yet queued request
> > + *
> > + * The queue of not yet queued request is used to track requests that are not
> > + * queued in order to retry queueing them when a pipeline is ready to accept.
> > + *
> > + * \sa PipelineHandler::queueRequest(), PipelineHandler::tryQueueRequests().
> > + */
> > +
> >  /**
> >   * \var CameraData::queuedRequests_
> >   * \brief The queue of queued and not yet completed request
> > @@ -378,12 +388,44 @@ void PipelineHandler::queueRequest(Request *request)
> >
> >       Camera *camera = request->camera_;
> >       CameraData *data = cameraData(camera);
> > -     data->queuedRequests_.push_back(request);
> > +     data->waitedRequests_.push(request);
> > +
> > +     tryQueueRequests(data);
> > +}
> > +
> > +/**
> > + * \fn PipelineHandler::tryQueueRequests()
> > + * \brief Queue requests that are not yet queued.
> > + * \param[in] data The camera data whose waited requests are tried to queue.
> > + *
> > + * This tries to queue as many requests as possible in order of the
> > + * waitedRequests_ in data. If queuing fails due to a buffer shortage, this
> > + * method stops and the next call of this method starts from the request that
> > + * fails in the previous call. On any other failures, the request is marked as
> > + * complete and proceed the successive requests.
> > + *
> > + * \context This function shall be called from the CameraManager thread.
> > + */
> > +void PipelineHandler::tryQueueRequests(CameraData *data)
> > +{
> > +     while (!data->waitedRequests_.empty()) {
> > +             Request *request = data->waitedRequests_.front();
> > +             Camera *camera = request->camera_;
> > +             ASSERT(data == cameraData(camera));
> > +
> > +             data->queuedRequests_.push_back(request);
> > +             int ret = queueRequestDevice(camera, request);
> > +             if (ret == -ENOBUFS || ret == -ENOMEM) {
> > +                     data->queuedRequests_.pop_back();
> > +                     break;
> > +             }
> >
> > -     int ret = queueRequestDevice(camera, request);
> > -     if (ret) {
> > -             request->result_ = ret;
> > -             completeRequest(request);
> > +             data->waitedRequests_.pop();
> > +             if (ret) {
> > +                     request->result_ = ret;
> > +                     completeRequest(request);
> > +                     break;
> > +             }
> >       }
> >  }
> >
> > @@ -440,6 +482,9 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> >   * submission order, the pipeline handler may call it on any complete request
> >   * without any ordering constraint.
> >   *
> > + * There might be requests that are waiting to be queued, this triggers
> > + * tryQueueRequests().
> > + *
> >   * \context This function shall be called from the CameraManager thread.
> >   */
> >  void PipelineHandler::completeRequest(Request *request)
> > @@ -459,6 +504,8 @@ void PipelineHandler::completeRequest(Request *request)
> >               data->queuedRequests_.pop_front();
> >               camera->requestComplete(req);
> >       }
> > +
> > +     tryQueueRequests(data);
> >  }
> >
> >  /**
>
> --
> Regards,
>
> Laurent Pinchart