| Message ID | 20230118085953.7027-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:46) > Look for OptionalStream flag in the hints field of the Unicam Image > StreamConfiguration structure. If this flag is not set, it guarantees that the > application will provide buffers for Unicam Image, so override the > minUnicamBuffers and minTotalUnicamBuffers config parameters in the following > way: > > - If startup drop frames are required, allocate at least 1 internal buffer. > - If no startup drop frames are required, do not allocate any internal buffers. > As highlighted somewhere else, the only part I hadn't considered when suggesting this way around is the raw sensor mode configuration. But that might still be ok. And probably means we really need a helper or something to improve how we handle configuring the sensor modes/sizes separately from the output stream, as here we would have to make sure that configuring a RAW stream when the application doesn't provide buffers will have to report it's an OptionalStream in the hint.... > All other buffer allocations remain unchanged. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++++++----- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8d325c7a18c5..6ed4cc2c7ba7 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1448,12 +1448,33 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > int PipelineHandlerRPi::prepareBuffers(Camera *camera) > { > RPiCameraData *data = cameraData(camera); > - unsigned int numRawBuffers = 0; > + unsigned int minUnicamBuffers = data->config_.minUnicamBuffers, > + minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers, > + numRawBuffers = 0; > int ret; > > for (Stream *s : camera->streams()) { > - if (isRaw(s->configuration().pixelFormat)) { > + if (s == &data->unicam_[Unicam::Image]) { > numRawBuffers = s->configuration().bufferCount; > + /* > + * If the application provides a guarantees that Unicam > + * image buffers will always be provided for the RAW stream > + * in a Request, we need: > + * - at least 1 internal Unicam buffer to handle startup frame drops, > + * - no internal Unicam buffers if there are no startup frame drops. > + */ > + if (!(s->configuration().hints & > + StreamConfiguration::Hint::OptionalStream)) { > + if (data->dropFrameCount_) { > + minUnicamBuffers = std::max(1u, minUnicamBuffers); > + minTotalUnicamBuffers = > + std::max(minUnicamBuffers, minTotalUnicamBuffers); > + } else { > + minUnicamBuffers = 0; > + minTotalUnicamBuffers = 0; > + } > + } > + > break; > } > } > @@ -1465,7 +1486,6 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > * For Unicam, allocate a minimum number of buffers for internal > * use as we want to avoid any frame drops. > */ > - const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; > if (stream == &data->unicam_[Unicam::Image]) { > /* > * If an application has configured a RAW stream, allocate > @@ -1473,8 +1493,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > * we have at least minUnicamBuffers of internal buffers > * to use to minimise frame drops. > */ > - numBuffers = std::max<int>(data->config_.minUnicamBuffers, > - minBuffers - numRawBuffers); > + numBuffers = std::max<int>(minUnicamBuffers, > + minTotalUnicamBuffers - numRawBuffers); > } else if (stream == &data->isp_[Isp::Input]) { > /* > * ISP input buffers are imported from Unicam, so follow > @@ -1482,15 +1502,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > * available. > */ > numBuffers = numRawBuffers + > - std::max<int>(data->config_.minUnicamBuffers, > - minBuffers - numRawBuffers); > + std::max<int>(minUnicamBuffers, > + minTotalUnicamBuffers - numRawBuffers); > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > /* > * Embedded data buffers are (currently) for internal use, > * so allocate the minimum required to avoid frame drops. > */ > - numBuffers = minBuffers; > + numBuffers = data->config_.minTotalUnicamBuffers; > } else { > /* > * Since the ISP runs synchronous with the IPA and requests, > -- > 2.25.1 >
Hello, On Fri, Jan 20, 2023 at 10:53:22AM +0000, Kieran Bingham via libcamera-devel wrote: > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:46) > > Look for OptionalStream flag in the hints field of the Unicam Image > > StreamConfiguration structure. If this flag is not set, it guarantees that the > > application will provide buffers for Unicam Image, so override the > > minUnicamBuffers and minTotalUnicamBuffers config parameters in the following > > way: > > > > - If startup drop frames are required, allocate at least 1 internal buffer. > > - If no startup drop frames are required, do not allocate any internal buffers. > > As highlighted somewhere else, the only part I hadn't considered when > suggesting this way around is the raw sensor mode configuration. But > that might still be ok. > > And probably means we really need a helper or something to improve how > we handle configuring the sensor modes/sizes separately from the output > stream, as here we would have to make sure that configuring a RAW stream > when the application doesn't provide buffers will have to report it's an > OptionalStream in the hint.... > > > All other buffer allocations remain unchanged. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++++++----- > > 1 file changed, 28 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 8d325c7a18c5..6ed4cc2c7ba7 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1448,12 +1448,33 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > > int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > { > > RPiCameraData *data = cameraData(camera); > > - unsigned int numRawBuffers = 0; > > + unsigned int minUnicamBuffers = data->config_.minUnicamBuffers, > > + minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers, > > + numRawBuffers = 0; One variable per line please. > > int ret; > > > > for (Stream *s : camera->streams()) { > > - if (isRaw(s->configuration().pixelFormat)) { > > + if (s == &data->unicam_[Unicam::Image]) { Is this a functional change or do you expect it to preserve the existing behaviour ? I'm wondering how it affects use cases with YUV sensors. > > numRawBuffers = s->configuration().bufferCount; > > + /* > > + * If the application provides a guarantees that Unicam > > + * image buffers will always be provided for the RAW stream > > + * in a Request, we need: > > + * - at least 1 internal Unicam buffer to handle startup frame drops, > > + * - no internal Unicam buffers if there are no startup frame drops. > > + */ > > + if (!(s->configuration().hints & > > + StreamConfiguration::Hint::OptionalStream)) { > > + if (data->dropFrameCount_) { > > + minUnicamBuffers = std::max(1u, minUnicamBuffers); > > + minTotalUnicamBuffers = > > + std::max(minUnicamBuffers, minTotalUnicamBuffers); > > + } else { > > + minUnicamBuffers = 0; > > + minTotalUnicamBuffers = 0; > > + } Why do you override the minimums from the configuration only when not dropping startup frames ? I'm not saying this is wrong, but I think better documentation is needed (likely in the example configuration file in patch 07/12) to explain the behaviour and how the configuration parameters influence it. > > + } > > + > > break; > > } > > } > > @@ -1465,7 +1486,6 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > * For Unicam, allocate a minimum number of buffers for internal > > * use as we want to avoid any frame drops. > > */ > > - const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; > > if (stream == &data->unicam_[Unicam::Image]) { > > /* > > * If an application has configured a RAW stream, allocate > > @@ -1473,8 +1493,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > * we have at least minUnicamBuffers of internal buffers > > * to use to minimise frame drops. > > */ > > - numBuffers = std::max<int>(data->config_.minUnicamBuffers, > > - minBuffers - numRawBuffers); > > + numBuffers = std::max<int>(minUnicamBuffers, > > + minTotalUnicamBuffers - numRawBuffers); > > } else if (stream == &data->isp_[Isp::Input]) { > > /* > > * ISP input buffers are imported from Unicam, so follow > > @@ -1482,15 +1502,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > * available. > > */ > > numBuffers = numRawBuffers + > > - std::max<int>(data->config_.minUnicamBuffers, > > - minBuffers - numRawBuffers); > > + std::max<int>(minUnicamBuffers, > > + minTotalUnicamBuffers - numRawBuffers); > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > > /* > > * Embedded data buffers are (currently) for internal use, > > * so allocate the minimum required to avoid frame drops. > > */ > > - numBuffers = minBuffers; > > + numBuffers = data->config_.minTotalUnicamBuffers; > > } else { > > /* > > * Since the ISP runs synchronous with the IPA and requests,
Hi Laurent, Thank you for your feedback. On Sun, 22 Jan 2023 at 21:57, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Fri, Jan 20, 2023 at 10:53:22AM +0000, Kieran Bingham via libcamera-devel wrote: > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:46) > > > Look for OptionalStream flag in the hints field of the Unicam Image > > > StreamConfiguration structure. If this flag is not set, it guarantees that the > > > application will provide buffers for Unicam Image, so override the > > > minUnicamBuffers and minTotalUnicamBuffers config parameters in the following > > > way: > > > > > > - If startup drop frames are required, allocate at least 1 internal buffer. > > > - If no startup drop frames are required, do not allocate any internal buffers. > > > > As highlighted somewhere else, the only part I hadn't considered when > > suggesting this way around is the raw sensor mode configuration. But > > that might still be ok. > > > > And probably means we really need a helper or something to improve how > > we handle configuring the sensor modes/sizes separately from the output > > stream, as here we would have to make sure that configuring a RAW stream > > when the application doesn't provide buffers will have to report it's an > > OptionalStream in the hint.... > > > > > All other buffer allocations remain unchanged. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++++++----- > > > 1 file changed, 28 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 8d325c7a18c5..6ed4cc2c7ba7 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1448,12 +1448,33 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > > > int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > > { > > > RPiCameraData *data = cameraData(camera); > > > - unsigned int numRawBuffers = 0; > > > + unsigned int minUnicamBuffers = data->config_.minUnicamBuffers, > > > + minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers, > > > + numRawBuffers = 0; > > One variable per line please. Ack. > > > > int ret; > > > > > > for (Stream *s : camera->streams()) { > > > - if (isRaw(s->configuration().pixelFormat)) { > > > + if (s == &data->unicam_[Unicam::Image]) { > > Is this a functional change or do you expect it to preserve the existing > behaviour ? I'm wondering how it affects use cases with YUV sensors Since we don't support YUV sensors in the pipeline handler (just yet), the behavior here is identical. The reason for the change is to be more explicit with the stream identification, which becomes important when we add a test for Isp::Output0 in a subsequent change. . > > > > numRawBuffers = s->configuration().bufferCount; > > > + /* > > > + * If the application provides a guarantees that Unicam > > > + * image buffers will always be provided for the RAW stream > > > + * in a Request, we need: > > > + * - at least 1 internal Unicam buffer to handle startup frame drops, > > > + * - no internal Unicam buffers if there are no startup frame drops. > > > + */ > > > + if (!(s->configuration().hints & > > > + StreamConfiguration::Hint::OptionalStream)) { > > > + if (data->dropFrameCount_) { > > > + minUnicamBuffers = std::max(1u, minUnicamBuffers); > > > + minTotalUnicamBuffers = > > > + std::max(minUnicamBuffers, minTotalUnicamBuffers); > > > + } else { > > > + minUnicamBuffers = 0; > > > + minTotalUnicamBuffers = 0; > > > + } > > Why do you override the minimums from the configuration only when not > dropping startup frames ? I'm not saying this is wrong, but I think > better documentation is needed (likely in the example configuration file > in patch 07/12) to explain the behaviour and how the configuration > parameters influence it. The overrides are needed to ensure we can correctly drop frames by using at least one internal Unicam buffer. I will add a note the explain this behavior in the config file. Regards, Naush > > > > + } > > > + > > > break; > > > } > > > } > > > @@ -1465,7 +1486,6 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > > * For Unicam, allocate a minimum number of buffers for internal > > > * use as we want to avoid any frame drops. > > > */ > > > - const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; > > > if (stream == &data->unicam_[Unicam::Image]) { > > > /* > > > * If an application has configured a RAW stream, allocate > > > @@ -1473,8 +1493,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > > * we have at least minUnicamBuffers of internal buffers > > > * to use to minimise frame drops. > > > */ > > > - numBuffers = std::max<int>(data->config_.minUnicamBuffers, > > > - minBuffers - numRawBuffers); > > > + numBuffers = std::max<int>(minUnicamBuffers, > > > + minTotalUnicamBuffers - numRawBuffers); > > > } else if (stream == &data->isp_[Isp::Input]) { > > > /* > > > * ISP input buffers are imported from Unicam, so follow > > > @@ -1482,15 +1502,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > > * available. > > > */ > > > numBuffers = numRawBuffers + > > > - std::max<int>(data->config_.minUnicamBuffers, > > > - minBuffers - numRawBuffers); > > > + std::max<int>(minUnicamBuffers, > > > + minTotalUnicamBuffers - numRawBuffers); > > > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > > > /* > > > * Embedded data buffers are (currently) for internal use, > > > * so allocate the minimum required to avoid frame drops. > > > */ > > > - numBuffers = minBuffers; > > > + numBuffers = data->config_.minTotalUnicamBuffers; > > > } else { > > > /* > > > * Since the ISP runs synchronous with the IPA and requests, > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 8d325c7a18c5..6ed4cc2c7ba7 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1448,12 +1448,33 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) int PipelineHandlerRPi::prepareBuffers(Camera *camera) { RPiCameraData *data = cameraData(camera); - unsigned int numRawBuffers = 0; + unsigned int minUnicamBuffers = data->config_.minUnicamBuffers, + minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers, + numRawBuffers = 0; int ret; for (Stream *s : camera->streams()) { - if (isRaw(s->configuration().pixelFormat)) { + if (s == &data->unicam_[Unicam::Image]) { numRawBuffers = s->configuration().bufferCount; + /* + * If the application provides a guarantees that Unicam + * image buffers will always be provided for the RAW stream + * in a Request, we need: + * - at least 1 internal Unicam buffer to handle startup frame drops, + * - no internal Unicam buffers if there are no startup frame drops. + */ + if (!(s->configuration().hints & + StreamConfiguration::Hint::OptionalStream)) { + if (data->dropFrameCount_) { + minUnicamBuffers = std::max(1u, minUnicamBuffers); + minTotalUnicamBuffers = + std::max(minUnicamBuffers, minTotalUnicamBuffers); + } else { + minUnicamBuffers = 0; + minTotalUnicamBuffers = 0; + } + } + break; } } @@ -1465,7 +1486,6 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) * For Unicam, allocate a minimum number of buffers for internal * use as we want to avoid any frame drops. */ - const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; if (stream == &data->unicam_[Unicam::Image]) { /* * If an application has configured a RAW stream, allocate @@ -1473,8 +1493,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) * we have at least minUnicamBuffers of internal buffers * to use to minimise frame drops. */ - numBuffers = std::max<int>(data->config_.minUnicamBuffers, - minBuffers - numRawBuffers); + numBuffers = std::max<int>(minUnicamBuffers, + minTotalUnicamBuffers - numRawBuffers); } else if (stream == &data->isp_[Isp::Input]) { /* * ISP input buffers are imported from Unicam, so follow @@ -1482,15 +1502,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) * available. */ numBuffers = numRawBuffers + - std::max<int>(data->config_.minUnicamBuffers, - minBuffers - numRawBuffers); + std::max<int>(minUnicamBuffers, + minTotalUnicamBuffers - numRawBuffers); } else if (stream == &data->unicam_[Unicam::Embedded]) { /* * Embedded data buffers are (currently) for internal use, * so allocate the minimum required to avoid frame drops. */ - numBuffers = minBuffers; + numBuffers = data->config_.minTotalUnicamBuffers; } else { /* * Since the ISP runs synchronous with the IPA and requests,
Look for OptionalStream flag in the hints field of the Unicam Image StreamConfiguration structure. If this flag is not set, it guarantees that the application will provide buffers for Unicam Image, so override the minUnicamBuffers and minTotalUnicamBuffers config parameters in the following way: - If startup drop frames are required, allocate at least 1 internal buffer. - If no startup drop frames are required, do not allocate any internal buffers. All other buffer allocations remain unchanged. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-)