[libcamera-devel] [RFC PATCH 2/2] libcamera: PipelineHandler: Retry queuing a capture request
Hirokazu Honda
hiroh at chromium.org
Wed Mar 31 10:46:57 CEST 2021
Hi Sebastian and Laurent, thanks for reviewing.
On Mon, Mar 29, 2021 at 2:28 PM Laurent Pinchart
<laurent.pinchart at 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 at 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
More information about the libcamera-devel
mailing list