| Message ID | 20221014131846.27169-8-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch! On Fri, 14 Oct 2022 at 14:19, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Add a new pipeline config parameter "disable_startup_frame_drops" to disable > any startup drop frames, overriding the IPA request. > > When this parameter is set, it allows the pipeline handler to run with no > internally allocated Unicam buffers ("min_unicam_buffers"). > > Add a validation to ensure if "disable_startup_frame_drops" is false, at least > one internal Unicam buffer is allocated, possibly overriding the > "min_unicam_buffers" parameter. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/data/default.json | 7 +++++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 15 ++++++++++++++- > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json > index a7ea735c87f4..707414bcc5c5 100644 > --- a/src/libcamera/pipeline/raspberrypi/data/default.json > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json > @@ -5,7 +5,7 @@ > "pipeline_handler": > { > # The minimum number of internal buffers to be allocated for Unicam. > - # This value must be greater than 0, but less than or equal to min_total_unicam_buffers. > + # This value must less than or equal to min_total_unicam_buffers. > "min_unicam_buffers": 2, > > # The minimum total (internal + external) buffer count used for Unicam. > @@ -15,6 +15,9 @@ > "min_total_unicam_buffers": 4, > > # The number of internal buffers used for ISP Output0. > - "num_output0_buffers": 1 > + "num_output0_buffers": 1, > + > + # Override any request from the IPA to drop a number of startup frames. > + "disable_startup_frame_drops": false > } > } > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 2aba0430c02e..135948d82f41 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -300,6 +300,7 @@ public: > unsigned int minUnicamBuffers; > unsigned int minTotalUnicamBuffers; > unsigned int numOutput0Buffers; > + bool disableStartupFrameDrops; > }; > > Config config_; > @@ -1044,7 +1045,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > data->setSensorControls(startConfig.controls); > > /* Configure the number of dropped frames required on startup. */ > - data->dropFrameCount_ = startConfig.dropFrameCount; > + data->dropFrameCount_ = data->config_.disableStartupFrameDrops ? 0 : startConfig.dropFrameCount; Might this warrant an INFO message, just in case folks are confused by the changing AGC/AWB when things start? Or maybe that would be annoying, for example if an application wants to run like this and handles the startup frames for itself? Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > > for (auto const stream : data->streams_) > stream->resetBuffers(); > @@ -1430,6 +1431,7 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) > .minUnicamBuffers = 2, > .minTotalUnicamBuffers = 4, > .numOutput0Buffers = 1, > + .disableStartupFrameDrops = false, > }; > > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); > @@ -1464,6 +1466,8 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) > phConfig["min_total_unicam_buffers"].get<unsigned int>(config.minTotalUnicamBuffers); > config.numOutput0Buffers = > phConfig["num_output0_buffers"].get<unsigned int>(config.numOutput0Buffers); > + config.disableStartupFrameDrops = > + phConfig["disable_startup_frame_drops"].get<bool>(config.disableStartupFrameDrops); > > if (config.minTotalUnicamBuffers < config.minUnicamBuffers || config.minTotalUnicamBuffers < 1) { > LOG(RPI, Error) << "Invalid Unicam buffer configuration used!"; > @@ -1543,6 +1547,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > */ > numBuffers = std::max<int>(data->config_.minUnicamBuffers, > minBuffers - numRawBuffers); > + > + if (numBuffers == 0 && data->dropFrameCount_) { > + LOG(RPI, Warning) > + << "Configured with no Unicam buffers," > + " but the IPA requested startup frame drops." > + " Increasing to one buffer."; > + numBuffers = 1; > + } > + > data->numUnicamBuffers = numBuffers; > } else if (stream == &data->isp_[Isp::Input]) { > /* > -- > 2.25.1 >
Hi David, Thank you for your feedback. On Tue, 1 Nov 2022 at 12:12, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for the patch! > > On Fri, 14 Oct 2022 at 14:19, Naushir Patuck via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > Add a new pipeline config parameter "disable_startup_frame_drops" to > disable > > any startup drop frames, overriding the IPA request. > > > > When this parameter is set, it allows the pipeline handler to run with no > > internally allocated Unicam buffers ("min_unicam_buffers"). > > > > Add a validation to ensure if "disable_startup_frame_drops" is false, at > least > > one internal Unicam buffer is allocated, possibly overriding the > > "min_unicam_buffers" parameter. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/data/default.json | 7 +++++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 ++++++++++++++- > > 2 files changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json > b/src/libcamera/pipeline/raspberrypi/data/default.json > > index a7ea735c87f4..707414bcc5c5 100644 > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json > > @@ -5,7 +5,7 @@ > > "pipeline_handler": > > { > > # The minimum number of internal buffers to be > allocated for Unicam. > > - # This value must be greater than 0, but less than or > equal to min_total_unicam_buffers. > > + # This value must less than or equal to > min_total_unicam_buffers. > > "min_unicam_buffers": 2, > > > > # The minimum total (internal + external) buffer count > used for Unicam. > > @@ -15,6 +15,9 @@ > > "min_total_unicam_buffers": 4, > > > > # The number of internal buffers used for ISP Output0. > > - "num_output0_buffers": 1 > > + "num_output0_buffers": 1, > > + > > + # Override any request from the IPA to drop a number of > startup frames. > > + "disable_startup_frame_drops": false > > } > > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 2aba0430c02e..135948d82f41 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -300,6 +300,7 @@ public: > > unsigned int minUnicamBuffers; > > unsigned int minTotalUnicamBuffers; > > unsigned int numOutput0Buffers; > > + bool disableStartupFrameDrops; > > }; > > > > Config config_; > > @@ -1044,7 +1045,7 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > data->setSensorControls(startConfig.controls); > > > > /* Configure the number of dropped frames required on startup. */ > > - data->dropFrameCount_ = startConfig.dropFrameCount; > > + data->dropFrameCount_ = data->config_.disableStartupFrameDrops ? > 0 : startConfig.dropFrameCount; > > Might this warrant an INFO message, just in case folks are confused by > the changing AGC/AWB when things start? Or maybe that would be > annoying, for example if an application wants to run like this and > handles the startup frames for itself? > There are already DEBUG level messages for dropping convergence frames, do you think that is enough? Regards, Naush > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David > > > > > for (auto const stream : data->streams_) > > stream->resetBuffers(); > > @@ -1430,6 +1431,7 @@ int > PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) > > .minUnicamBuffers = 2, > > .minTotalUnicamBuffers = 4, > > .numOutput0Buffers = 1, > > + .disableStartupFrameDrops = false, > > }; > > > > char const *configFromEnv = > utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); > > @@ -1464,6 +1466,8 @@ int > PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) > > phConfig["min_total_unicam_buffers"].get<unsigned > int>(config.minTotalUnicamBuffers); > > config.numOutput0Buffers = > > phConfig["num_output0_buffers"].get<unsigned > int>(config.numOutput0Buffers); > > + config.disableStartupFrameDrops = > > + > phConfig["disable_startup_frame_drops"].get<bool>(config.disableStartupFrameDrops); > > > > if (config.minTotalUnicamBuffers < config.minUnicamBuffers || > config.minTotalUnicamBuffers < 1) { > > LOG(RPI, Error) << "Invalid Unicam buffer configuration > used!"; > > @@ -1543,6 +1547,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera > *camera) > > */ > > numBuffers = > std::max<int>(data->config_.minUnicamBuffers, > > minBuffers - > numRawBuffers); > > + > > + if (numBuffers == 0 && data->dropFrameCount_) { > > + LOG(RPI, Warning) > > + << "Configured with no Unicam > buffers," > > + " but the IPA requested > startup frame drops." > > + " Increasing to one buffer."; > > + numBuffers = 1; > > + } > > + > > data->numUnicamBuffers = numBuffers; > > } else if (stream == &data->isp_[Isp::Input]) { > > /* > > -- > > 2.25.1 > > >
diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json index a7ea735c87f4..707414bcc5c5 100644 --- a/src/libcamera/pipeline/raspberrypi/data/default.json +++ b/src/libcamera/pipeline/raspberrypi/data/default.json @@ -5,7 +5,7 @@ "pipeline_handler": { # The minimum number of internal buffers to be allocated for Unicam. - # This value must be greater than 0, but less than or equal to min_total_unicam_buffers. + # This value must less than or equal to min_total_unicam_buffers. "min_unicam_buffers": 2, # The minimum total (internal + external) buffer count used for Unicam. @@ -15,6 +15,9 @@ "min_total_unicam_buffers": 4, # The number of internal buffers used for ISP Output0. - "num_output0_buffers": 1 + "num_output0_buffers": 1, + + # Override any request from the IPA to drop a number of startup frames. + "disable_startup_frame_drops": false } } diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 2aba0430c02e..135948d82f41 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -300,6 +300,7 @@ public: unsigned int minUnicamBuffers; unsigned int minTotalUnicamBuffers; unsigned int numOutput0Buffers; + bool disableStartupFrameDrops; }; Config config_; @@ -1044,7 +1045,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) data->setSensorControls(startConfig.controls); /* Configure the number of dropped frames required on startup. */ - data->dropFrameCount_ = startConfig.dropFrameCount; + data->dropFrameCount_ = data->config_.disableStartupFrameDrops ? 0 : startConfig.dropFrameCount; for (auto const stream : data->streams_) stream->resetBuffers(); @@ -1430,6 +1431,7 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) .minUnicamBuffers = 2, .minTotalUnicamBuffers = 4, .numOutput0Buffers = 1, + .disableStartupFrameDrops = false, }; char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); @@ -1464,6 +1466,8 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) phConfig["min_total_unicam_buffers"].get<unsigned int>(config.minTotalUnicamBuffers); config.numOutput0Buffers = phConfig["num_output0_buffers"].get<unsigned int>(config.numOutput0Buffers); + config.disableStartupFrameDrops = + phConfig["disable_startup_frame_drops"].get<bool>(config.disableStartupFrameDrops); if (config.minTotalUnicamBuffers < config.minUnicamBuffers || config.minTotalUnicamBuffers < 1) { LOG(RPI, Error) << "Invalid Unicam buffer configuration used!"; @@ -1543,6 +1547,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) */ numBuffers = std::max<int>(data->config_.minUnicamBuffers, minBuffers - numRawBuffers); + + if (numBuffers == 0 && data->dropFrameCount_) { + LOG(RPI, Warning) + << "Configured with no Unicam buffers," + " but the IPA requested startup frame drops." + " Increasing to one buffer."; + numBuffers = 1; + } + data->numUnicamBuffers = numBuffers; } else if (stream == &data->isp_[Isp::Input]) { /*
Add a new pipeline config parameter "disable_startup_frame_drops" to disable any startup drop frames, overriding the IPA request. When this parameter is set, it allows the pipeline handler to run with no internally allocated Unicam buffers ("min_unicam_buffers"). Add a validation to ensure if "disable_startup_frame_drops" is false, at least one internal Unicam buffer is allocated, possibly overriding the "min_unicam_buffers" parameter. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/data/default.json | 7 +++++-- .../pipeline/raspberrypi/raspberrypi.cpp | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-)