<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 11 Nov 2021 at 15:49, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Thu, Nov 11, 2021 at 02:58:21PM +0000, Naushir Patuck wrote:<br>
> On Thu, 11 Nov 2021 at 14:35, Laurent Pinchart wrote:<br>
> > On Wed, Nov 10, 2021 at 10:08:01AM +0000, Naushir Patuck wrote:<br>
> > > For simplicity, the pipeline handler would look at the maximum number of<br>
> > > buffers set in the StreamConfiguration and allocate the same number of internal<br>
> > > buffers for all device nodes. This would likely overallocate buffers for some<br>
> > > nodes. Rework this logic to try and minimise overallcations without compromising<br>
> > > performance.<br>
> > ><br>
> > > For ISP nodes, we only ever need 1 set of internal buffers, as the hardware runs<br>
> > > synchronous with the requests and IPA.<br>
> > ><br>
> > > For Unicam nodes, allocate a minimum for 4 buffers (exported + internal), but<br>
> > > also require at least 1 internal buffer.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > > .../pipeline/raspberrypi/raspberrypi.cpp   | 44 ++++++++++++++-----<br>
> > >Â 1 file changed, 33 insertions(+), 11 deletions(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index 11d3c2b120dd..5f0f00aacd59 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)<br>
> > >Â int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
> > >Â {<br>
> > >Â Â Â Â RPiCameraData *data = cameraData(camera);<br>
> > > +Â Â Â unsigned int numBuffers;<br>
> > I'd name the variable numRawBuffers.<br>
> ><br>
> > > +Â Â Â bool rawStream = false;<br>
> > >Â Â Â Â int ret;<br>
> > ><br>
> > > -Â Â Â /*<br>
> > > -Â Â Â * Decide how many internal buffers to allocate. For now, simply look<br>
> > > -Â Â Â * at how many external buffers will be provided. We'll need to improve<br>
> > > -Â Â Â * this logic. However, we really must have all streams allocate the same<br>
> > > -Â Â Â * number of buffers to simplify error handling in queueRequestDevice().<br>
> ><br>
> > Does the error logic still work after this change ?<br>
> <br>
> Yes, the comment above has been bit-rotted, and this is handled correctly<br>
> now.<br>
> <br>
> > > -Â Â Â */<br>
> > > -Â Â Â unsigned int maxBuffers = 0;<br>
> > > -Â Â Â for (const Stream *s : camera->streams())<br>
> > > -Â Â Â Â Â Â Â if (static_cast<const RPi::Stream *>(s)->isExternal())<br>
> > > -Â Â Â Â Â Â Â Â Â Â Â maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);<br>
> > > +Â Â Â for (Stream *s : camera->streams()) {<br>
> > > +Â Â Â Â Â Â Â if (isRaw(s->configuration().pixelFormat)) {<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â numBuffers = s->configuration().bufferCount;<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â rawStream = true;<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â break;<br>
> > > +Â Â Â Â Â Â Â }<br>
> > > +Â Â Â }<br>
> > ><br>
> > > +Â Â Â /* Decide how many internal buffers to allocate. */<br>
> > >Â Â Â Â for (auto const stream : data->streams_) {<br>
> > > -Â Â Â Â Â Â Â ret = stream->prepareBuffers(maxBuffers);<br>
> ><br>
> > And here add a numBuffers local variable.<br>
> ><br>
> > > +Â Â Â Â Â Â Â if (stream == &data->unicam_[Unicam::Image] ||<br>
> > > +Â Â Â Â Â Â Â Â Â stream == &data->unicam_[Unicam::Embedded]) {<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â /*<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * For Unicam, allocate a minimum of 4 buffers as we want<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * to avoid any frame drops. If an application has configured<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * a RAW stream, allocate additional buffers to make up the<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * minimum, but ensure we have at least 1 set of internal<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * buffers to use to minimise frame drops.<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â */<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â constexpr unsigned int minBuffers = 4;<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â numBuffers = rawStream ? std::max<int>(1, minBuffers - numBuffers)<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : minBuffers;<br>
> ><br>
> > You could initialize numRawBuffers to 0 and drop rawStream, as if<br>
> > !rawStream, then numRawBuffers will be equal to 0, so<br>
> ><br>
> >Â Â Â Â Â Â Â Â Â Â Â Â Â numBuffers = std::max<int>(1, minBuffers - numRawBuffers);<br>
> ><br>
> > would do the right thing.<br>
> ><br>
> > > +Â Â Â Â Â Â Â } else {<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â /*<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * Since the ISP runs synchronous with the IPA and requests,<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * we only ever need one set of internal buffers. Any buffers<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * the application wants to hold onto will already be exported<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â * through PipelineHandlerRPi::exportFrameBuffers().<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â */<br>
> > > +Â Â Â Â Â Â Â Â Â Â Â numBuffers = 1;<br>
> > > +Â Â Â Â Â Â Â }<br>
> > > +<br>
> > > +Â Â Â Â Â Â Â ret = stream->prepareBuffers(numBuffers);<br>
> ><br>
> > I have a hard time follow this. Before the patch, the pipeline handler<br>
> > calls prepareBuffers() with the number of buffers requested by the<br>
> > application. Now it calls it with a number of internal buffers only,<br>
> > without any change to the prepareBuffers() function itself. This would<br>
> > benefit from an explanation in the commit message, as it's not clear why<br>
> > it's correct.<br>
> <br>
> For clarification, prepareBuffers() would (and still will) allocate internal use<br>
> buffers, and call importBuffers() to allocate the buffer cache with the internal<br>
> + user requested buffer count. Buffer allocations for the user requested buffer<br>
> count goes through PipelineHandler::exportFrameBuffers().<br>
<br>
I'm looking at prepareBuffers():<br>
<br>
int Stream::prepareBuffers(unsigned int count)<br>
{<br>
    int ret;<br>
<br>
    if (!importOnly_) {<br>
        if (count) {<br>
            /* Export some frame buffers for internal use. */<br>
            ret = dev_->exportBuffers(count, &internalBuffers_);<br>
            if (ret < 0)<br>
                return ret;<br>
<br>
            /* Add these exported buffers to the internal/external buffer list. */<br>
            setExportedBuffers(&internalBuffers_);<br>
<br>
            /* Add these buffers to the queue of internal usable buffers. */<br>
            for (auto const &buffer : internalBuffers_)<br>
                availableBuffers_.push(buffer.get());<br>
        }<br>
<br>
        /* We must import all internal/external exported buffers. */<br>
        count = bufferMap_.size();<br>
    }<br>
<br>
    return dev_->importBuffers(count);<br>
}<br>
<br>
In the !importOnly_ case, we add count buffers to bufferMap_ by a call<br>
to setExportedBuffers(). setExportedBuffers() is also called by<br>
PipelineHandlerRPi::exportFrameBuffers(), so we currently add<br>
max(s->configuration().bufferCount) buffers to any buffers already<br>
exported by the application. This certainly overallocates, as we<br>
essentially allocations bufferCount * 2 buffers.<br>
<br>
With this patch, overallocation is reduced. However, there's no<br>
requirement for applications to use the FrameBufferAllocator (which ends<br>
up calling PipelineHandlerRPi::exportFrameBuffers()) to allocate<br>
buffers. If an application allocates buffers through different means<br>
(for instance exporting them from a display device), then we will call<br>
dev_->importBuffers() with a very low count.<br></blockquote><div><br></div><div>Yes, I see how this could go wrong. Should I just assume the worst, and</div><div>pass VIDEO_MAX_FRAME into importBuffers. Would that be advisable?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> So this change (somewhat) decouples the user count from the internal count.<br>
> I will clarify this in the commit message.<br>
> <br>
> > Furthermore, what will happen if an application requests 4 buffers for<br>
> > the raw stream and any number of buffers for a processed streams, and<br>
> > repeatedly queues requests with no buffer for the raw stream ? It seems<br>
> > to me that you'll now allocate a single extra buffer for the raw stream,<br>
> > which won't be enough to keep the unicam queue running.<br>
> <br>
> This is a tricky one. Without any hints from the application as to what it intends<br>
> to do, I have to balance memory usage with performance. You are right that<br>
> with only a single internal buffer allocated, the performance may be degraded<br>
> if the application does not provide buffers through the Request. However,<br>
> If the application has allocated 4x 18Mbyte buffers (12mpix 12-bit), I really don't<br>
> want to be allocating 2x more of them when they possibly may never be used<br>
> (assuming the application is circulating all these buffers constantly). I would hope<br>
> application writers would be using those buffers sensibly :-)<br>
> <br>
> Some of our platforms have very small CMA heaps to work with, memory will<br>
> get very tight for these use-cases, so I do want to limit memory usage at the<br>
> expense of performance. Some hints from the application on how it intends to<br>
> use buffers might help here with this balance.<br>
<br>
I agree with you on the problem statement, but I'm wondering if we<br>
shouldn't start by implementing the safe option to avoid frame drops,<br>
and then improve performance on top (possibly with a hint API).<br></blockquote><div><br></div><div>I think maybe we can start with 2 internal buffer allocations, then see if</div><div>that may ever need reducing.</div><div><br></div><div>Regards,</div><div>Naush</div><div>Â </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> David, do you think we can bump the internal allocation up to 2 buffers?<br>
> <br>
> > >Â Â Â Â Â Â Â Â if (ret < 0)<br>
> > >Â Â Â Â Â Â Â Â Â Â Â Â return ret;<br>
> > >Â Â Â Â }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>