<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thanks for the review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 4 Sept 2023 at 09:44, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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">Quoting Jacopo Mondi via libcamera-devel (2023-07-27 10:31:28)<br>
> Hi Naush<br>
> <br>
> On Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> > Hardcode the maximum number of buffers imported to the V4L2 video device<br>
> > to 32. This only has a minor disadvantage of over-allocating cache slots<br>
> > and V4L2 buffer indexes, but does allow more headroom for using dma<br>
> > buffers allocated from outside of libcamera.<br>
> ><br>
> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > ---<br>
> >  .../pipeline/rpi/common/rpi_stream.cpp        | 33 +++++--------------<br>
> >  1 file changed, 8 insertions(+), 25 deletions(-)<br>
> ><br>
> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp<br>
> > index c158843cb0ed..e9ad1e6f0848 100644<br>
> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp<br>
> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp<br>
> > @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)<br>
> >       int ret;<br>
> ><br>
> >       if (!(flags_ & StreamFlag::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>
> > -                     resetBuffers();<br>
> > -             }<br>
> > +             /* Export some frame buffers for internal use. */<br>
> > +             ret = dev_->exportBuffers(count, &internalBuffers_);<br>
> > +             if (ret < 0)<br>
> > +                     return ret;<br>
> ><br>
> > -             /* We must import all internal/external exported buffers. */<br>
> > -             count = bufferMap_.size();<br>
> > +             /* Add these exported buffers to the internal/external buffer list. */<br>
> > +             setExportedBuffers(&internalBuffers_);<br>
> > +             resetBuffers();<br>
> >       }<br>
> ><br>
> > -     /*<br>
> > -      * If this is an external stream, we must allocate slots for buffers that<br>
> > -      * might be externally allocated. We have no indication of how many buffers<br>
> > -      * may be used, so this might overallocate slots in the buffer cache.<br>
> > -      * Similarly, if this stream is only importing buffers, we do the same.<br>
> > -      *<br>
> > -      * \todo Find a better heuristic, or, even better, an exact solution to<br>
> > -      * this issue.<br>
> > -      */<br>
> > -     if ((flags_ & StreamFlag::External) || (flags_ & StreamFlag::ImportOnly))<br>
> > -             count = count * 2;<br>
> > -<br>
> > -     return dev_->importBuffers(count);<br>
> > +     return dev_->importBuffers(32);<br>
> <br>
> Might be good to define 32 somewhere as constexpr to avoid magic<br>
> numbers, but apart from this<br>
<br>
Agreed here. Something like<br>
<br>
/** The maximum buffer allocation possible with V4L2 */<br>
constexpr maximumV4L2Buffers = 32;<br></blockquote><div><br></div><div>Ack, I'll make this change for v3.</div><div><br></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>
to express that this is the maximum possible with V4L2....<br>
<br>
But otherwise<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> <br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> <br>
> Thanks<br>
>   j<br>
> <br>
> >  }<br>
> ><br>
> >  int Stream::queueBuffer(FrameBuffer *buffer)<br>
> > --<br>
> > 2.34.1<br>
> ><br>
</blockquote></div></div>