<div dir="ltr"><div dir="ltr">Hi Kieran and Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 1 Feb 2022 at 10:35, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">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">Hello,<br>
<br>
On Tue, Feb 01, 2022 at 10:18:23AM +0000, Kieran Bingham wrote:<br>
> Quoting David Plowman (2022-02-01 09:47:09)<br>
> > Hi Naush<br>
> > <br>
> > Thanks for this patch. Weird how we never noticed this problem before!<br>
> > <br>
> > On Tue, 1 Feb 2022 at 09:27, Naushir Patuck wrote:<br>
> > ><br>
> > > The ISP input stream currently only allocates a single slot in the<br>
> > > V4L2VideoDevice cache as it follows the number of buffers allocated for use.<br>
> > > However, this is wrong as the ISP input stream imports buffers from Unicam<br>
> > > image stream. As a consequence of this, only one cache slot was used during<br>
> > > runtime for the ISP input stream, and if multiple buffers were to be queued<br>
> > > simultaneously, the queue operation would return a failure.<br>
> > ><br>
> > > Fix this by passing the same number of RAW buffers available from the Unicam<br>
> > > image stream. Additionally, double this count in the cases where buffers could<br>
> > > be allocated externally from the application.<br>
> > <br>
> > I agree this sounds like it wants a better solution, but is definitely<br>
> > beyond the scope of this (relatively small but important) fix. So:<br>
> <br>
> Did you ever dig further into what the maximum number of V4L2 buffers<br>
> can be allocated are?<br>
> <br>
> Should we change the V4L2Device to always just allocate the maximum<br>
> there instead?<br>
> <br>
> These are just 'structures' to be able to pass the buffer information<br>
> right ? They're not the actual expensive memory backing?<br>
<br>
When importing, that's correct, but note that V4L2 doesn't provide an<br>
"unprepare" buffer operation. It means that once you queue a dmabuf fd<br>
to a given V4L2 buffer, the dmabuf will be mapped to the device and the<br>
CPU until a new dmabuf comes to replace it for the same V4L2 buffer, or<br>
until the V4L2 buffer is freed (which can only be done with<br>
VIDIOC_REQBUFS(0) at the moment). It also means that a reference to the<br>
dmabuf will be kept, which will prevent the memory from being released.<br>
If an application queues different buffers from time to time, with a<br>
large number of V4L2 buffer, it may take a long time before stale cache<br>
entries are being pushed out (if ever). I'd thus be quite cautious about<br>
allocating too many V4L2 buffers.<br>
<br>
This seems a difficult to solve problem, and it stems from the fact that<br>
we don't have explicit release calls in V4L2. We may want to address<br>
this in the libcamera public API nonetheless at some point, with hints<br>
that applications can pass, for instance to tell if a buffer will be<br>
submitted again or not. V4L2 extensions will then likely be required.<br></blockquote><div><br></div><div>I was going to suggest that we setup the cache to use VB2_MAX_FRAME (32)</div><div>entries, but given the above explanation this may not be the right thing to</div><div>do to fix this.</div><div><br></div><div>Naush</div><div><br></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>
> That would also then give V4L2BufferCache the best chance of supporting<br>
> more external buffers?<br>
> <br>
> > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> > ><br>
> > > Bug: <a href="https://github.com/raspberrypi/libcamera-apps/issues/236" rel="noreferrer" target="_blank">https://github.com/raspberrypi/libcamera-apps/issues/236</a><br>
> > > <a href="https://github.com/raspberrypi/libcamera-apps/issues/238" rel="noreferrer" target="_blank">https://github.com/raspberrypi/libcamera-apps/issues/238</a><br>
> > > <a href="https://github.com/raspberrypi/libcamera-apps/issues/240" rel="noreferrer" target="_blank">https://github.com/raspberrypi/libcamera-apps/issues/240</a><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++<br>
> > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 3 ++-<br>
> > > 2 files changed, 10 insertions(+), 1 deletion(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index 6a46e6bc89fa..49af56edc1f9 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -1382,6 +1382,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
> > > * minimise frame drops.<br>
> > > */<br>
> > > numBuffers = std::max<int>(2, minBuffers - numRawBuffers);<br>
> > > + } else if (stream == &data->isp_[Isp::Input]) {<br>
> > > + /*<br>
> > > + * ISP input buffers are imported from Unicam, so follow<br>
> > > + * similar logic as above to count all the RAW buffers<br>
> > > + * available.<br>
> > > + */<br>
> > > + numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers);<br>
> > > +<br>
> > > } else if (stream == &data->unicam_[Unicam::Embedded]) {<br>
> > > /*<br>
> > > * Embedded data buffers are (currently) for internal use,<br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> > > index a4159e20b068..a421ad09ba50 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> > > @@ -111,11 +111,12 @@ int Stream::prepareBuffers(unsigned int count)<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 (isExternal())<br>
> > > + if (isExternal() || importOnly_)<br>
> > > count = count * 2;<br>
> > ><br>
> > > return dev_->importBuffers(count);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>