[libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests in the case a buffer shortage
Hirokazu Honda
hiroh at chromium.org
Tue May 11 06:18:53 CEST 2021
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.
> 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().
queuePendingRequests() is called in queueRequestDevice() in this patch.
But it will be called from other places in 2/2.
So the current style is fine for you?
-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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210511/847a7cc2/attachment-0001.htm>
More information about the libcamera-devel
mailing list