[libcamera-devel] [PATCH] pipeline: ipu3: Store requests in the case a buffer shortage

[Laurent Pinchart]

Hi Hiro,

On Wed, Mar 31, 2021 at 05:51:48PM +0900, Hirokazu Honda wrote:
> I should still work for this patch series.
> The missing part is to trigger queuePendingRequests() appropriately
> when buffers are available.
> I couldn't find a good way as availableBuffers notification functions
> are separated and trying queuePendingRequests() is not likely
> successful and thus inefficient.
> I would like to ask anyone for a good idea for this.

Request queuing to the kernel can fail if we are out of either ImgU
stats/params buffers, or internal CIO2 raw buffers. New buffers become
available for the ImgU in IPU3Frames::remove(), and for the CIO2 in
CIO2Device::tryReturnBuffer(). Both could be modified to emit a signal,
which could be connected to queuePendingRequests().

> On Wed, Mar 31, 2021 at 5:45 PM Hirokazu Honda <hiroh at chromium.org> wrote:
> >
> > PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a
> > request when there are not sufficient buffers for the request.
> > Since the request will be successful if it is queued later when
> > enough buffers are available. The requests failed due to a buffer
> > shortage should be stored and retried later in the FIFO order.
> > This introduces the queue in PipelineHandlerIPU3 to do that.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 62 ++++++++++++++++++----------
> >  1 file changed, 41 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 519cad4f..71dd311f 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -153,12 +153,16 @@ private:
> >         int allocateBuffers(Camera *camera);
> >         int freeBuffers(Camera *camera);
> >
> > +       int queuePendingRequests();
> > +
> >         ImgUDevice imgu0_;
> >         ImgUDevice imgu1_;
> >         MediaDevice *cio2MediaDev_;
> >         MediaDevice *imguMediaDev_;
> >
> >         std::vector<IPABuffer> ipaBuffers_;
> > +
> > +       std::queue<std::pair<IPU3CameraData *, Request *>> pendingRequests_;

As requests are per-camera, shouldn't the queue be stored in
IPU3CameraData ? queuePendingRequests() should also be moved to the
IPU3CameraData class.

> >  };
> >
> >  IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)
> > @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >         IPU3CameraData *data = cameraData(camera);
> >         int ret = 0;
> >
> > +       pendingRequests_ = {};
> > +
> >         data->ipa_->stop();
> >
> >         ret |= data->imgu_->stop();
> > @@ -774,36 +780,50 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >         freeBuffers(camera);
> >  }
> >
> > -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> > +int PipelineHandlerIPU3::queuePendingRequests()
> >  {
> > -       IPU3CameraData *data = cameraData(camera);
> > +       while (!pendingRequests_.empty()) {
> > +               IPU3CameraData *data = pendingRequests_.front().first;
> > +               Request *request = pendingRequests_.front().second;
> >
> > -       IPU3Frames::Info *info = data->frameInfos_.create(request);
> > -       if (!info)
> > -               return -ENOBUFS;
> > +               IPU3Frames::Info *info = data->frameInfos_.create(request);
> > +               if (!info)
> > +                       break;
> >
> > -       /*
> > -        * Queue a buffer on the CIO2, using the raw stream buffer provided in
> > -        * the request, if any, or a CIO2 internal buffer otherwise.
> > -        */
> > -       FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
> > -       FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);
> > -       if (!rawBuffer) {
> > -               data->frameInfos_.remove(info);
> > -               return -ENOMEM;
> > -       }
> > +               /*
> > +                * Queue a buffer on the CIO2, using the raw stream buffer
> > +                * provided in the request, if any, or a CIO2 internal buffer
> > +                * otherwise.
> > +                */
> > +               FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
> > +               FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);
> > +               if (!rawBuffer) {
> > +                       data->frameInfos_.remove(info);
> > +                       break;
> > +               }
> >
> > -       info->rawBuffer = rawBuffer;
> > +               info->rawBuffer = rawBuffer;
> >
> > -       ipa::ipu3::IPU3Event ev;
> > -       ev.op = ipa::ipu3::EventProcessControls;
> > -       ev.frame = info->id;
> > -       ev.controls = request->controls();
> > -       data->ipa_->processEvent(ev);
> > +               ipa::ipu3::IPU3Event ev;
> > +               ev.op = ipa::ipu3::EventProcessControls;
> > +               ev.frame = info->id;
> > +               ev.controls = request->controls();
> > +               data->ipa_->processEvent(ev);
> > +
> > +               pendingRequests_.pop();
> > +       }
> >
> >         return 0;
> >  }
> >
> > +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> > +{
> > +       IPU3CameraData *data = cameraData(camera);
> > +
> > +       pendingRequests_.emplace(data, request);
> > +       return queuePendingRequests();
> > +}
> > +
> >  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> >  {
> >         int ret;

-- 
Regards,

Laurent Pinchart