<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thanks for all the reviews!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 10 Nov 2021 at 10:33, 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 Naushir Patuck (2021-11-10 10:08:02)<br>
> If a stream is marked as external, double the number of V4L2BufferCache slots<br>
> that are allocated. This is to account for additional buffers that may be<br>
> allocated directly by the application.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++<br>
>  1 file changed, 8 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> index b3265d0e8aab..67901936d6b6 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count)<br>
>                 count = bufferMap_.size();<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>
> +        */<br>
<br>
Perhaps we'll need a better system here (not in this patch) to handle<br>
this ... but I think overallocating is cheap - and the whole point of<br>
the buffer cache is to try to re-use the same buffers where possible. So<br>
I /think/ overallocations is the right solution for now anyway.<br></blockquote><div><br></div><div>I think this over allocation is the only change needed to "fix" Roman's original</div><div>issue.  But I did mean to tidy up the buffer allocations for some time now, so</div><div>why not :-)</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +       if (isExternal())<br>
> +               count = count * 2;<br>
<br>
Of course it's hard to know /how/ far to overallocate ... But this will<br>
do until we figure it out ;-)<br></blockquote><div><br></div><div>I wonder if we can perhaps allow the cache to grow in size dynamically</div><div>instead of relying on a fixed number of slots?</div><div><br></div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> +<br>
>         return dev_->importBuffers(count);<br>
>  }<br>
>  <br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>