<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 13 Mar 2022 at 16:02, 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">H Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> Currently, all framebuffer allocations get freed and cleared on a stop in<br>
> PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then called<br>
> without an intermediate PipelineHandlerRPi::configure(), it will re-allocate and<br>
> prepare all the buffers again, which is unnecessary.<br>
> <br>
> Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), but<br>
> insted doing it in PipelineHandlerRPi::configure(), as the buffers might have<br>
> to be resized.<br>
<br>
I see where you're going, but doesn't this mean that buffers will stay<br>
allocated forever ? If an application uses a camera for some time and<br>
then stops it, memory won't be released until the application<br>
terminates. That's trading an issue for another one, and which one is<br>
worse really depends on the use case.<br>
<br>
There are (at least) two ways to address this. The simplest one would be<br>
to fire a timer at stop() time, and free buffers when it elapses. The<br>
timer would be cancelled if the camera is restarted first.<br>
<br>
The second option is to make this controllable by the application. We<br>
could hook it up to the Camera::release() call for instance, adding a<br>
new pipeline handler operation to handle it. release() may not be the<br>
best option though, maybe we need a new cleanup function. Or maybe an<br>
argument to stop(), to tell if the camera is expected to be restarted<br>
soon ? I haven't really thought about the pros and cons of the different<br>
options, I'm just brainstorming here.<br></blockquote><div><br></div><div>Yes, I do see a possible issue here with holding onto buffers for longer than</div><div>expected.  My preferred option would be to have a Camera::release() call</div><div>that explicitly requests the pipeline handler to remove all buffer allocations.</div><div>That way, the application controls the intended behavior if it chooses to, but</div><div>the pipeline handler keeps buffers allocated otherwise.</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>
> 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 | 10 +++++++---<br>
>  1 file changed, 7 insertions(+), 3 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 3781e4e0e3c4..8bb9fc429912 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -189,6 +189,11 @@ public:<br>
>       {<br>
>       }<br>
>  <br>
> +     ~RPiCameraData()<br>
> +     {<br>
> +             freeBuffers();<br>
> +     }<br>
> +<br>
>       void freeBuffers();<br>
>       void frameStarted(uint32_t sequence);<br>
>  <br>
> @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
>       RPiCameraData *data = cameraData(camera);<br>
>       int ret;<br>
>  <br>
> -     /* Start by resetting the Unicam and ISP stream states. */<br>
> +     /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */<br>
> +     data->freeBuffers();<br>
>       for (auto const stream : data->streams_)<br>
>               stream->reset();<br>
>  <br>
> @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)<br>
>  <br>
>       /* Stop the IPA. */<br>
>       data->ipa_->stop();<br>
> -<br>
> -     data->freeBuffers();<br>
>  }<br>
>  <br>
>  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>