[libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests in the case a buffer shortage
Jacopo Mondi
jacopo at jmondi.org
Tue May 11 12:07:48 CEST 2021
Hi Hiro,
On Tue, May 11, 2021 at 01:18:53PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for reviewing.
>
> On Tue, May 11, 2021 at 12:57 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> > Hi Hiro,
> > sorry took me a while to re-read the whole discussion on the
> > previous version and put me in the same perspective as you and
> > Laurent had
> >
> > On Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda 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 IPU3CameraData to do that.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +-
> > > src/libcamera/pipeline/ipu3/frames.cpp | 4 +-
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++-------
> > > 3 files changed, 56 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp
> > b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > index 3cd777d1..8bbef174 100644
> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request
> > *request, FrameBuffer *rawBuffer)
> > > /* If no buffer is provided in the request, use an internal one. */
> > > if (!buffer) {
> > > if (availableBuffers_.empty()) {
> > > - LOG(IPU3, Error) << "CIO2 buffer underrun";
> > > + LOG(IPU3, Debug) << "CIO2 buffer underrun";
> > > return nullptr;
> > > }
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp
> > b/src/libcamera/pipeline/ipu3/frames.cpp
> > > index a1b014ee..2c4fe508 100644
> > > --- a/src/libcamera/pipeline/ipu3/frames.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> > > @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request
> > *request)
> > > unsigned int id = request->sequence();
> > >
> > > if (availableParamBuffers_.empty()) {
> > > - LOG(IPU3, Error) << "Parameters buffer underrun";
> > > + LOG(IPU3, Debug) << "Parameters buffer underrun";
> > > return nullptr;
> > > }
> > >
> > > if (availableStatBuffers_.empty()) {
> > > - LOG(IPU3, Error) << "Statistics buffer underrun";
> > > + LOG(IPU3, Debug) << "Statistics buffer underrun";
> > > return nullptr;
> > > }
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 73306cea..3f311e58 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -66,6 +66,8 @@ public:
> > > void cio2BufferReady(FrameBuffer *buffer);
> > > void paramBufferReady(FrameBuffer *buffer);
> > > void statBufferReady(FrameBuffer *buffer);
> > > + void queuePendingRequests();
> > > + void cancelPendingRequests();
> > >
> > > CIO2Device cio2_;
> > > ImgUDevice *imgu_;
> > > @@ -84,6 +86,8 @@ public:
> > >
> > > std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
> > >
> > > + std::queue<Request *> pendingRequests_;
> > > +
> > > private:
> > > void queueFrameAction(unsigned int id,
> > > const ipa::ipu3::IPU3Action &action);
> > > @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > > IPU3CameraData *data = cameraData(camera);
> > > int ret = 0;
> > >
> > > + data->cancelPendingRequests();
> > > +
> > > data->ipa_->stop();
> > >
> > > ret |= data->imgu_->stop();
> > > @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > > freeBuffers(camera);
> > > }
> > >
> > > -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request
> > *request)
> > > +void IPU3CameraData::cancelPendingRequests()
> > > {
> > > - IPU3CameraData *data = cameraData(camera);
> > > + while (!pendingRequests_.empty()) {
> > > + Request *request = pendingRequests_.front();
> > >
> > > - IPU3Frames::Info *info = data->frameInfos_.create(request);
> > > - if (!info)
> > > - return -ENOBUFS;
> > > + for (auto it : request->buffers()) {
> > > + FrameBuffer *buffer = it.second;
> > > + buffer->cancel();
> > > + pipe_->completeBuffer(request, buffer);
> > > + }
> > >
> > > - /*
> > > - * 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;
> > > + pipe_->completeRequest(request);
> > > + pendingRequests_.pop();
> > > }
> > > +}
> > >
> > > - info->rawBuffer = rawBuffer;
> > > +void IPU3CameraData::queuePendingRequests()
> > > +{
> > > + while (!pendingRequests_.empty()) {
> > > + Request *request = pendingRequests_.front();
> > >
> > > - ipa::ipu3::IPU3Event ev;
> > > - ev.op = ipa::ipu3::EventProcessControls;
> > > - ev.frame = info->id;
> > > - ev.controls = request->controls();
> > > - data->ipa_->processEvent(ev);
> > > + IPU3Frames::Info *info = 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(&rawStream_);
> > > + FrameBuffer *rawBuffer = cio2_.queueBuffer(request,
> > reqRawBuffer);
> > > + if (!rawBuffer) {
> > > + frameInfos_.remove(info);
> > > + break;
> > > + }
> > > +
> > > + info->rawBuffer = rawBuffer;
> > >
> > > + ipa::ipu3::IPU3Event ev;
> > > + ev.op = ipa::ipu3::EventProcessControls;
> > > + ev.frame = info->id;
> > > + ev.controls = request->controls();
> > > + ipa_->processEvent(ev);
> > > +
> > > + pendingRequests_.pop();
> > > + }
> > > +}
> > > +
> > > +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request
> > *request)
> > > +{
> > > + IPU3CameraData *data = cameraData(camera);
> > > + data->pendingRequests_.push(request);
> > > + data->queuePendingRequests();
> >
> > I originally wrote this comment:
> >
> > ----------------------------------------------------------
> > This makes it impossible to return errors to the pipeline handler base
> > class, even if those errors are not due to missing buffers but are
> > reported by the video device, in example at cio2_.queueBuffer() time.
> > I don't think that's desired, as errors would go unnonticed.
> >
> > I would
> >
> > int queueRequestDevice(Request *req)
> > {
> > pending.push(req);
> > while(!pending.empty()) {
> >
> > r = pending.front()
> >
> > info = frameInfo.create();
> > if (!info)
> > break;
> >
> > pending.pop();
> > rawB = request->findBuffer(req);
> > raw = cio2.queue(rawB);
> > if (!raw) {
> > frameInfo.remove(info);
> > return -ENOMEM;
> > }
> >
> > info->rawBuffer = raw;
> >
> > ...
> > ipa_->processEvent(ev);
> > }
> >
> > }
> >
> >
> > To maintain the ability to return errors from the video device
> > -------------------------------------------------------------
> >
> > But then I noticed CIO2::queueBuffer() is pretty idiotic, it behaves
> > differently if the raw buffer is nullptr or not, and to work around
> > this it cannot return an error code to signal an actual queueing
> > error, but it just return nullptr which can mean either "I don't have
> > memory available" or "there's been an error in queueing the buffer to
> > the device". It's impossible to decouple the two error conditions and
> > too much magic is happening in that function which makes it not easy
> > predictable from the caller.
> >
> > So, I would start by splitting the buffer retrieval from the CIO2 from
> > the buffer queueing as in:
> >
> > raw = request->findBuffer(req);
> > if (!raw)
> > raw = cio2->getRawBuffer();
> >
> > if (!raw)
> > break;
> >
> > ret = cio2->queueBuffer(raw);
> > if (ret)
> > return ret;
> >
> > But as you don't have to solve all the issues you encounter, the patch
> > is fine as it is with a little nit that I find
> >
> >
> Thanks for finding.
> I am also thinking of how to report the error.
> Since we delay queueing a request, it may be difficult to return the error
> properly with an associated request with the current solution.
> I honestly have no idea.
Don't you have a Request * to return an error on if queuing failed ?
>
>
> > void funcA()
> > {
> > while(something) {
> > code;
> > }
> > }
> >
> > void funcB()
> > {
> > funcA();
> > }
> >
> > worse to read compared to unfolding the while loop in the caller.
> >
> >
> I think you are saying about queuePendingRequests().
Yes, I meant that function.
> queuePendingRequests() is called in queueRequestDevice() in this patch.
> But it will be called from other places in 2/2.
Oh sorry, I missed that !
> So the current style is fine for you?
Considering 2/2 yes.
Please add
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
>
> -Hiro
>
> >
> > > return 0;
> > > }
> > >
> > > --
> > > 2.31.1.368.gbe11c130af-goog
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
More information about the libcamera-devel
mailing list