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

11804 diff mbox series

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@chromium.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 62 ++++++++++++++++++----------
 1 file changed, 41 insertions(+), 21 deletions(-)

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.

Thanks in advance,
-Hiro

On Wed, Mar 31, 2021 at 5:45 PM Hirokazu Honda <hiroh@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@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_;
>  };
>
>  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;
> --
> 2.31.0.291.g576ba9dcdaf-goog
>

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@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@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;