<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 15 Mar 2022 at 08:03, 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">Hi Naush,<br>
<br>
On Mon, Mar 14, 2022 at 11:13:44AM +0000, Naushir Patuck wrote:<br>
> On Sun, 13 Mar 2022 at 16:02, Laurent Pinchart wrote:<br>
> > On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via<br>
> > 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>
> <br>
> Yes, I do see a possible issue here with holding onto buffers for longer than<br>
> expected. My preferred option would be to have a Camera::release() call<br>
> that explicitly requests the pipeline handler to remove all buffer allocations.<br>
> That way, the application controls the intended behavior if it chooses to, but<br>
> the pipeline handler keeps buffers allocated otherwise.<br>
<br>
The downside of tying this to Camera::release() is that the application<br>
will need to let go of exclusive ownership of the camera to free the<br>
buffers. Someone else could then acquire() it. It's probably a corner<br>
case, but I'd prefer not hardcoding this limitation in the API.<br></blockquote><div><br></div><div>I agree, this may not be the best approach then.</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>
I'm also wondering if adding a hint flag to stop() to tell if streaming<br>
will soon tbe restarted could also have other use cases. A pipeline<br>
handler could avoid powering down hardware for instance, to decrease the<br>
time needed when restarting (although with recent drivers PM is handled<br>
using runtime PM in the sensor driver, with auto-suspend, so the sensor<br>
itself won't benefit much).<br></blockquote><div><br></div><div>I'm actually coming round to the idea that the pipeline handler has an internal</div><div>timeout that is used to free the buffers. Partly because I need a timeout</div><div>functionality for something entirely unrelated :) But it does remove the decision</div><div>from the application side. I'll prototype something along these lines to see </div><div>how it works.</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>
While brainstorming on this topic, would it be useful to have a mean to<br>
allocate internal buffers large enough for different resolutions, and<br>
use them in different capture cycles instead of reallocating ?<br></blockquote><div><br></div><div>It would certainly help, but is the extra complexity worth the savings on allocation</div><div>time, I can't say...</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>