| Message ID | 20220317140827.1835029-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch! Perhaps I'm coming a bit late to this particular discussion, but this makes me a bit nervous. I'm worried about deallocations/allocations happening at unpredictable moments relative to other things in the system. Could there be a risk of creating more intermittent and hard-to-reproduce failures? One example is the Pi 3 where I think we know that CMA fragmentation is already a problem. I wonder if I would rather be able to control when/whether buffers get freed? But it does make the API more involved and leaves an application with more to think about. Possibly I'm just worrying too much? Thanks! David On Thu, 17 Mar 2022 at 14:08, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Start a 5 second timer when stopDevice() is called, and if it times out, > free all internally allocated buffers. The timer is cleared when start() is > subsequently called. > > This avoids the pipeline handler from holding onto internal buffers for long > periods of time due to application inactivity. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index d3212b193ced..32b282b4bdbd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -15,6 +15,7 @@ > #include <utility> > > #include <libcamera/base/shared_fd.h> > +#include <libcamera/base/timer.h> > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > @@ -45,6 +46,8 @@ > #include "dma_heaps.h" > #include "rpi_stream.h" > > +using namespace std::literals::chrono_literals; > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(RPI) > @@ -288,6 +291,9 @@ public: > /* Has this camera been reconfigured? */ > bool reallocate_; > > + /* Timer to free internal buffers once stopDevice() has been called. */ > + Timer stopTimer_; > + > private: > void checkRequestCompleted(); > void fillRequestMetadata(const ControlList &bufferControls, > @@ -991,6 +997,10 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > RPiCameraData *data = cameraData(camera); > int ret; > > + /* Disable the stop timeout waiting to release buffers after a stop(). */ > + data->stopTimer_.stop(); > + data->stopTimer_.timeout.disconnect(); > + > for (auto const stream : data->streams_) > stream->resetBuffers(); > > @@ -1071,6 +1081,13 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > /* Stop the IPA. */ > data->ipa_->stop(); > + > + /* > + * Start a 5 second timer once the camera is stopped, and on a timeout, > + * release all internally allocated buffers. > + */ > + data->stopTimer_.timeout.connect(data, &RPiCameraData::freeBuffers); > + data->stopTimer_.start(5s); > } > > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) > -- > 2.25.1 >
Hi David, On Thu, 17 Mar 2022 at 14:32, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for the patch! > > Perhaps I'm coming a bit late to this particular discussion, but this > makes me a bit nervous. I'm worried about deallocations/allocations > happening at unpredictable moments relative to other things in the > system. Could there be a risk of creating more intermittent and > hard-to-reproduce failures? > > One example is the Pi 3 where I think we know that CMA fragmentation > is already a problem. I wonder if I would rather be able to control > when/whether buffers get freed? But it does make the API more involved > and leaves an application with more to think about. > Yes, this is a distinct possibility. I'm fairly confident this will not be an issue with libcamera-apps, as the timeout will never hit. However, picamera2 can certainly invoke the timeout and cause issues. Hmmm.... not sure which way to go here.... perhaps an application flag would indeed be better. Naush > > Possibly I'm just worrying too much? > > Thanks! > David > > On Thu, 17 Mar 2022 at 14:08, Naushir Patuck via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > Start a 5 second timer when stopDevice() is called, and if it times out, > > free all internally allocated buffers. The timer is cleared when start() > is > > subsequently called. > > > > This avoids the pipeline handler from holding onto internal buffers for > long > > periods of time due to application inactivity. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index d3212b193ced..32b282b4bdbd 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -15,6 +15,7 @@ > > #include <utility> > > > > #include <libcamera/base/shared_fd.h> > > +#include <libcamera/base/timer.h> > > #include <libcamera/base/utils.h> > > > > #include <libcamera/camera.h> > > @@ -45,6 +46,8 @@ > > #include "dma_heaps.h" > > #include "rpi_stream.h" > > > > +using namespace std::literals::chrono_literals; > > + > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(RPI) > > @@ -288,6 +291,9 @@ public: > > /* Has this camera been reconfigured? */ > > bool reallocate_; > > > > + /* Timer to free internal buffers once stopDevice() has been > called. */ > > + Timer stopTimer_; > > + > > private: > > void checkRequestCompleted(); > > void fillRequestMetadata(const ControlList &bufferControls, > > @@ -991,6 +997,10 @@ int PipelineHandlerRPi::start(Camera *camera, const > ControlList *controls) > > RPiCameraData *data = cameraData(camera); > > int ret; > > > > + /* Disable the stop timeout waiting to release buffers after a > stop(). */ > > + data->stopTimer_.stop(); > > + data->stopTimer_.timeout.disconnect(); > > + > > for (auto const stream : data->streams_) > > stream->resetBuffers(); > > > > @@ -1071,6 +1081,13 @@ void PipelineHandlerRPi::stopDevice(Camera > *camera) > > > > /* Stop the IPA. */ > > data->ipa_->stop(); > > + > > + /* > > + * Start a 5 second timer once the camera is stopped, and on a > timeout, > > + * release all internally allocated buffers. > > + */ > > + data->stopTimer_.timeout.connect(data, > &RPiCameraData::freeBuffers); > > + data->stopTimer_.start(5s); > > } > > > > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request > *request) > > -- > > 2.25.1 > > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index d3212b193ced..32b282b4bdbd 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -15,6 +15,7 @@ #include <utility> #include <libcamera/base/shared_fd.h> +#include <libcamera/base/timer.h> #include <libcamera/base/utils.h> #include <libcamera/camera.h> @@ -45,6 +46,8 @@ #include "dma_heaps.h" #include "rpi_stream.h" +using namespace std::literals::chrono_literals; + namespace libcamera { LOG_DEFINE_CATEGORY(RPI) @@ -288,6 +291,9 @@ public: /* Has this camera been reconfigured? */ bool reallocate_; + /* Timer to free internal buffers once stopDevice() has been called. */ + Timer stopTimer_; + private: void checkRequestCompleted(); void fillRequestMetadata(const ControlList &bufferControls, @@ -991,6 +997,10 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) RPiCameraData *data = cameraData(camera); int ret; + /* Disable the stop timeout waiting to release buffers after a stop(). */ + data->stopTimer_.stop(); + data->stopTimer_.timeout.disconnect(); + for (auto const stream : data->streams_) stream->resetBuffers(); @@ -1071,6 +1081,13 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) /* Stop the IPA. */ data->ipa_->stop(); + + /* + * Start a 5 second timer once the camera is stopped, and on a timeout, + * release all internally allocated buffers. + */ + data->stopTimer_.timeout.connect(data, &RPiCameraData::freeBuffers); + data->stopTimer_.start(5s); } int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
Start a 5 second timer when stopDevice() is called, and if it times out, free all internally allocated buffers. The timer is cleared when start() is subsequently called. This avoids the pipeline handler from holding onto internal buffers for long periods of time due to application inactivity. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)