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

[Hirokazu Honda]

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