| Message ID | 20220307124633.115452-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch. On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Currently, all framebuffer allocations get freed and cleared on a stop in > PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then called > without an intermediate PipelineHandlerRPi::configure(), it will re-allocate and > prepare all the buffers again, which is unnecessary. > > Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), but > insted doing it in PipelineHandlerRPi::configure(), as the buffers might have > to be resized. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 3781e4e0e3c4..8bb9fc429912 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -189,6 +189,11 @@ public: > { > } > > + ~RPiCameraData() > + { > + freeBuffers(); > + } > + > void freeBuffers(); > void frameStarted(uint32_t sequence); > > @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > RPiCameraData *data = cameraData(camera); > int ret; > > - /* Start by resetting the Unicam and ISP stream states. */ > + /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */ > + data->freeBuffers(); > for (auto const stream : data->streams_) > stream->reset(); > > @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > /* Stop the IPA. */ > data->ipa_->stop(); > - > - data->freeBuffers(); > } > > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) > -- > 2.25.1 > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David
H Naush, Thank you for the patch. On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via libcamera-devel wrote: > Currently, all framebuffer allocations get freed and cleared on a stop in > PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then called > without an intermediate PipelineHandlerRPi::configure(), it will re-allocate and > prepare all the buffers again, which is unnecessary. > > Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), but > insted doing it in PipelineHandlerRPi::configure(), as the buffers might have > to be resized. I see where you're going, but doesn't this mean that buffers will stay allocated forever ? If an application uses a camera for some time and then stops it, memory won't be released until the application terminates. That's trading an issue for another one, and which one is worse really depends on the use case. There are (at least) two ways to address this. The simplest one would be to fire a timer at stop() time, and free buffers when it elapses. The timer would be cancelled if the camera is restarted first. The second option is to make this controllable by the application. We could hook it up to the Camera::release() call for instance, adding a new pipeline handler operation to handle it. release() may not be the best option though, maybe we need a new cleanup function. Or maybe an argument to stop(), to tell if the camera is expected to be restarted soon ? I haven't really thought about the pros and cons of the different options, I'm just brainstorming here. > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 3781e4e0e3c4..8bb9fc429912 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -189,6 +189,11 @@ public: > { > } > > + ~RPiCameraData() > + { > + freeBuffers(); > + } > + > void freeBuffers(); > void frameStarted(uint32_t sequence); > > @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > RPiCameraData *data = cameraData(camera); > int ret; > > - /* Start by resetting the Unicam and ISP stream states. */ > + /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */ > + data->freeBuffers(); > for (auto const stream : data->streams_) > stream->reset(); > > @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > /* Stop the IPA. */ > data->ipa_->stop(); > - > - data->freeBuffers(); > } > > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
Hi Laurent, Thank you for your feedback! On Sun, 13 Mar 2022 at 16:02, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > H Naush, > > Thank you for the patch. > > On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via > libcamera-devel wrote: > > Currently, all framebuffer allocations get freed and cleared on a stop in > > PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then > called > > without an intermediate PipelineHandlerRPi::configure(), it will > re-allocate and > > prepare all the buffers again, which is unnecessary. > > > > Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), > but > > insted doing it in PipelineHandlerRPi::configure(), as the buffers might > have > > to be resized. > > I see where you're going, but doesn't this mean that buffers will stay > allocated forever ? If an application uses a camera for some time and > then stops it, memory won't be released until the application > terminates. That's trading an issue for another one, and which one is > worse really depends on the use case. > > There are (at least) two ways to address this. The simplest one would be > to fire a timer at stop() time, and free buffers when it elapses. The > timer would be cancelled if the camera is restarted first. > > The second option is to make this controllable by the application. We > could hook it up to the Camera::release() call for instance, adding a > new pipeline handler operation to handle it. release() may not be the > best option though, maybe we need a new cleanup function. Or maybe an > argument to stop(), to tell if the camera is expected to be restarted > soon ? I haven't really thought about the pros and cons of the different > options, I'm just brainstorming here. > Yes, I do see a possible issue here with holding onto buffers for longer than expected. My preferred option would be to have a Camera::release() call that explicitly requests the pipeline handler to remove all buffer allocations. That way, the application controls the intended behavior if it chooses to, but the pipeline handler keeps buffers allocated otherwise. Regards, Naush > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 3781e4e0e3c4..8bb9fc429912 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -189,6 +189,11 @@ public: > > { > > } > > > > + ~RPiCameraData() > > + { > > + freeBuffers(); > > + } > > + > > void freeBuffers(); > > void frameStarted(uint32_t sequence); > > > > @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > RPiCameraData *data = cameraData(camera); > > int ret; > > > > - /* Start by resetting the Unicam and ISP stream states. */ > > + /* Start by freeing all buffers and resetting the Unicam and ISP > stream states. */ > > + data->freeBuffers(); > > for (auto const stream : data->streams_) > > stream->reset(); > > > > @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > > > /* Stop the IPA. */ > > data->ipa_->stop(); > > - > > - data->freeBuffers(); > > } > > > > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request > *request) > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Mon, Mar 14, 2022 at 11:13:44AM +0000, Naushir Patuck wrote: > On Sun, 13 Mar 2022 at 16:02, Laurent Pinchart wrote: > > On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via > > libcamera-devel wrote: > > > Currently, all framebuffer allocations get freed and cleared on a stop in > > > PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then called > > > without an intermediate PipelineHandlerRPi::configure(), it will re-allocate and > > > prepare all the buffers again, which is unnecessary. > > > > > > Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), but > > > insted doing it in PipelineHandlerRPi::configure(), as the buffers might have > > > to be resized. > > > > I see where you're going, but doesn't this mean that buffers will stay > > allocated forever ? If an application uses a camera for some time and > > then stops it, memory won't be released until the application > > terminates. That's trading an issue for another one, and which one is > > worse really depends on the use case. > > > > There are (at least) two ways to address this. The simplest one would be > > to fire a timer at stop() time, and free buffers when it elapses. The > > timer would be cancelled if the camera is restarted first. > > > > The second option is to make this controllable by the application. We > > could hook it up to the Camera::release() call for instance, adding a > > new pipeline handler operation to handle it. release() may not be the > > best option though, maybe we need a new cleanup function. Or maybe an > > argument to stop(), to tell if the camera is expected to be restarted > > soon ? I haven't really thought about the pros and cons of the different > > options, I'm just brainstorming here. > > Yes, I do see a possible issue here with holding onto buffers for longer than > expected. My preferred option would be to have a Camera::release() call > that explicitly requests the pipeline handler to remove all buffer allocations. > That way, the application controls the intended behavior if it chooses to, but > the pipeline handler keeps buffers allocated otherwise. The downside of tying this to Camera::release() is that the application will need to let go of exclusive ownership of the camera to free the buffers. Someone else could then acquire() it. It's probably a corner case, but I'd prefer not hardcoding this limitation in the API. I'm also wondering if adding a hint flag to stop() to tell if streaming will soon tbe restarted could also have other use cases. A pipeline handler could avoid powering down hardware for instance, to decrease the time needed when restarting (although with recent drivers PM is handled using runtime PM in the sensor driver, with auto-suspend, so the sensor itself won't benefit much). While brainstorming on this topic, would it be useful to have a mean to allocate internal buffers large enough for different resolutions, and use them in different capture cycles instead of reallocating ? > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 3781e4e0e3c4..8bb9fc429912 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -189,6 +189,11 @@ public: > > > { > > > } > > > > > > + ~RPiCameraData() > > > + { > > > + freeBuffers(); > > > + } > > > + > > > void freeBuffers(); > > > void frameStarted(uint32_t sequence); > > > > > > @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > > RPiCameraData *data = cameraData(camera); > > > int ret; > > > > > > - /* Start by resetting the Unicam and ISP stream states. */ > > > + /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */ > > > + data->freeBuffers(); > > > for (auto const stream : data->streams_) > > > stream->reset(); > > > > > > @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > > > > > /* Stop the IPA. */ > > > data->ipa_->stop(); > > > - > > > - data->freeBuffers(); > > > } > > > > > > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
Hi Laurent, On Tue, 15 Mar 2022 at 08:03, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Mon, Mar 14, 2022 at 11:13:44AM +0000, Naushir Patuck wrote: > > On Sun, 13 Mar 2022 at 16:02, Laurent Pinchart wrote: > > > On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via > > > libcamera-devel wrote: > > > > Currently, all framebuffer allocations get freed and cleared on a > stop in > > > > PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is > then called > > > > without an intermediate PipelineHandlerRPi::configure(), it will > re-allocate and > > > > prepare all the buffers again, which is unnecessary. > > > > > > > > Fix this by not freeing the buffer in > PipelineHandlerRPi::stopDevice(), but > > > > insted doing it in PipelineHandlerRPi::configure(), as the buffers > might have > > > > to be resized. > > > > > > I see where you're going, but doesn't this mean that buffers will stay > > > allocated forever ? If an application uses a camera for some time and > > > then stops it, memory won't be released until the application > > > terminates. That's trading an issue for another one, and which one is > > > worse really depends on the use case. > > > > > > There are (at least) two ways to address this. The simplest one would > be > > > to fire a timer at stop() time, and free buffers when it elapses. The > > > timer would be cancelled if the camera is restarted first. > > > > > > The second option is to make this controllable by the application. We > > > could hook it up to the Camera::release() call for instance, adding a > > > new pipeline handler operation to handle it. release() may not be the > > > best option though, maybe we need a new cleanup function. Or maybe an > > > argument to stop(), to tell if the camera is expected to be restarted > > > soon ? I haven't really thought about the pros and cons of the > different > > > options, I'm just brainstorming here. > > > > Yes, I do see a possible issue here with holding onto buffers for longer > than > > expected. My preferred option would be to have a Camera::release() call > > that explicitly requests the pipeline handler to remove all buffer > allocations. > > That way, the application controls the intended behavior if it chooses > to, but > > the pipeline handler keeps buffers allocated otherwise. > > The downside of tying this to Camera::release() is that the application > will need to let go of exclusive ownership of the camera to free the > buffers. Someone else could then acquire() it. It's probably a corner > case, but I'd prefer not hardcoding this limitation in the API. > I agree, this may not be the best approach then. > > I'm also wondering if adding a hint flag to stop() to tell if streaming > will soon tbe restarted could also have other use cases. A pipeline > handler could avoid powering down hardware for instance, to decrease the > time needed when restarting (although with recent drivers PM is handled > using runtime PM in the sensor driver, with auto-suspend, so the sensor > itself won't benefit much). > I'm actually coming round to the idea that the pipeline handler has an internal timeout that is used to free the buffers. Partly because I need a timeout functionality for something entirely unrelated :) But it does remove the decision from the application side. I'll prototype something along these lines to see how it works. > > While brainstorming on this topic, would it be useful to have a mean to > allocate internal buffers large enough for different resolutions, and > use them in different capture cycles instead of reallocating ? > It would certainly help, but is the extra complexity worth the savings on allocation time, I can't say... Regards, Naush > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++--- > > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 3781e4e0e3c4..8bb9fc429912 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -189,6 +189,11 @@ public: > > > > { > > > > } > > > > > > > > + ~RPiCameraData() > > > > + { > > > > + freeBuffers(); > > > > + } > > > > + > > > > void freeBuffers(); > > > > void frameStarted(uint32_t sequence); > > > > > > > > @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera > *camera, CameraConfiguration *config) > > > > RPiCameraData *data = cameraData(camera); > > > > int ret; > > > > > > > > - /* Start by resetting the Unicam and ISP stream states. */ > > > > + /* Start by freeing all buffers and resetting the Unicam and > ISP stream states. */ > > > > + data->freeBuffers(); > > > > for (auto const stream : data->streams_) > > > > stream->reset(); > > > > > > > > @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera > *camera) > > > > > > > > /* Stop the IPA. */ > > > > data->ipa_->stop(); > > > > - > > > > - data->freeBuffers(); > > > > } > > > > > > > > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request > *request) > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 3781e4e0e3c4..8bb9fc429912 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -189,6 +189,11 @@ public: { } + ~RPiCameraData() + { + freeBuffers(); + } + void freeBuffers(); void frameStarted(uint32_t sequence); @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) RPiCameraData *data = cameraData(camera); int ret; - /* Start by resetting the Unicam and ISP stream states. */ + /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */ + data->freeBuffers(); for (auto const stream : data->streams_) stream->reset(); @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) /* Stop the IPA. */ data->ipa_->stop(); - - data->freeBuffers(); } int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
Currently, all framebuffer allocations get freed and cleared on a stop in PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then called without an intermediate PipelineHandlerRPi::configure(), it will re-allocate and prepare all the buffers again, which is unnecessary. Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), but insted doing it in PipelineHandlerRPi::configure(), as the buffers might have to be resized. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)