| Message ID | 20210827120757.110615-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Fri, Aug 27, 2021 at 02:07:42PM +0200, Jacopo Mondi wrote: > As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor > full frame size") the current implementation of the IPU3 pipeline > handler always use the sensor resolution as the ImgU input frame size in s/use/uses > order to work around an issue with the ImgU configuration procedure. > > Now that the frame selection policy has been modified in the CIO2Device > class implementation to comply with the requirements of the ImgU > configuration script we can remove the workaround and select the most > opportune sensor size to feed the ImgU with. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++-------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..b321c94e9cb0 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > status = Adjusted; > } > > - /* Validate the requested stream configuration */ > + /* > + * Validate the requested stream configuration and select the sensor > + * format by collecting the maximum RAW stream width and height and > + * picking the closest larger match. > + * > + * If no RAW stream is requested use the one of the largest YUV stream, > + * plus margin pixels for the IF and BDS rectangle to downscale. > + * > + * \todo Clarify the IF and BDS margins requirements. > + */ > unsigned int rawCount = 0; > unsigned int yuvCount = 0; > Size maxYuvSize; > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > rawCount++; > - rawSize = cfg.size; > + rawSize.expandTo(cfg.size); > } else { > yuvCount++; > maxYuvSize.expandTo(cfg.size); > @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > /* > * Generate raw configuration from CIO2. > * > - * \todo The image sensor frame size should be selected to optimize > - * operations based on the sizes of the requested streams. However such > - * a selection makes the pipeline configuration procedure fail for small > - * resolutions (for example: 640x480 with OV5670) and causes the capture > - * operations to stall for some stream size combinations (see the > - * commit message of the patch that introduced this comment for more > - * failure examples). > - * > - * Until the sensor frame size calculation criteria are clarified, when > - * capturing from ImgU always use the largest possible size which > - * guarantees better results at the expense of the frame rate and CSI-2 > - * bus bandwidth. When only a raw stream is requested use the requested > - * size instead, as the ImgU is not involved. > + * The output YUV streams will be limited in size to the > + * maximum frame size requested for the RAW stream. > */ > - if (!yuvCount) > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > - else > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > + if (rawSize.isNull()) > + rawSize = maxYuvSize.alignedUpTo(40, 540) > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > + IMGU_OUTPUT_HEIGHT_MARGIN) > + .boundedTo(data_->cio2_.sensor()->resolution()); > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > if (!cio2Configuration_.pixelFormat.isValid()) > return Invalid; > > -- > 2.32.0 >
Hi Jacopo On 8/27/21 5:37 PM, Jacopo Mondi wrote: > As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor > full frame size") the current implementation of the IPU3 pipeline > handler always use the sensor resolution as the ImgU input frame size in > order to work around an issue with the ImgU configuration procedure. > > Now that the frame selection policy has been modified in the CIO2Device > class implementation to comply with the requirements of the ImgU > configuration script we can remove the workaround and select the most > opportune sensor size to feed the ImgU with. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++-------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..b321c94e9cb0 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > status = Adjusted; > } > > - /* Validate the requested stream configuration */ > + /* > + * Validate the requested stream configuration and select the sensor > + * format by collecting the maximum RAW stream width and height and > + * picking the closest larger match. > + * > + * If no RAW stream is requested use the one of the largest YUV stream, > + * plus margin pixels for the IF and BDS rectangle to downscale. > + * > + * \todo Clarify the IF and BDS margins requirements. > + */ > unsigned int rawCount = 0; > unsigned int yuvCount = 0; > Size maxYuvSize; > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > rawCount++; > - rawSize = cfg.size; > + rawSize.expandTo(cfg.size); > } else { > yuvCount++; > maxYuvSize.expandTo(cfg.size); > @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > /* > * Generate raw configuration from CIO2. > * > - * \todo The image sensor frame size should be selected to optimize > - * operations based on the sizes of the requested streams. However such > - * a selection makes the pipeline configuration procedure fail for small > - * resolutions (for example: 640x480 with OV5670) and causes the capture > - * operations to stall for some stream size combinations (see the > - * commit message of the patch that introduced this comment for more > - * failure examples). > - * > - * Until the sensor frame size calculation criteria are clarified, when > - * capturing from ImgU always use the largest possible size which > - * guarantees better results at the expense of the frame rate and CSI-2 > - * bus bandwidth. When only a raw stream is requested use the requested > - * size instead, as the ImgU is not involved. > + * The output YUV streams will be limited in size to the > + * maximum frame size requested for the RAW stream. > */ > - if (!yuvCount) > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > - else > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > + if (rawSize.isNull()) > + rawSize = maxYuvSize.alignedUpTo(40, 540) > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > + IMGU_OUTPUT_HEIGHT_MARGIN) > + .boundedTo(data_->cio2_.sensor()->resolution()); > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > if (!cio2Configuration_.pixelFormat.isValid()) > return Invalid; >
Hi Jacopo, On 27/08/2021 14:07, Jacopo Mondi wrote: > As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor > full frame size") the current implementation of the IPU3 pipeline > handler always use the sensor resolution as the ImgU input frame size in > order to work around an issue with the ImgU configuration procedure. > > Now that the frame selection policy has been modified in the CIO2Device > class implementation to comply with the requirements of the ImgU > configuration script we can remove the workaround and select the most > opportune sensor size to feed the ImgU with. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++-------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..b321c94e9cb0 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > status = Adjusted; > } > > - /* Validate the requested stream configuration */ > + /* > + * Validate the requested stream configuration and select the sensor > + * format by collecting the maximum RAW stream width and height and > + * picking the closest larger match. > + * > + * If no RAW stream is requested use the one of the largest YUV stream, > + * plus margin pixels for the IF and BDS rectangle to downscale. > + * > + * \todo Clarify the IF and BDS margins requirements. > + */ > unsigned int rawCount = 0; > unsigned int yuvCount = 0; > Size maxYuvSize; > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > rawCount++; > - rawSize = cfg.size; > + rawSize.expandTo(cfg.size); > } else { > yuvCount++; > maxYuvSize.expandTo(cfg.size); > @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > /* > * Generate raw configuration from CIO2. > * > - * \todo The image sensor frame size should be selected to optimize > - * operations based on the sizes of the requested streams. However such > - * a selection makes the pipeline configuration procedure fail for small > - * resolutions (for example: 640x480 with OV5670) and causes the capture > - * operations to stall for some stream size combinations (see the > - * commit message of the patch that introduced this comment for more > - * failure examples). > - * > - * Until the sensor frame size calculation criteria are clarified, when > - * capturing from ImgU always use the largest possible size which > - * guarantees better results at the expense of the frame rate and CSI-2 > - * bus bandwidth. When only a raw stream is requested use the requested > - * size instead, as the ImgU is not involved. > + * The output YUV streams will be limited in size to the > + * maximum frame size requested for the RAW stream. > */ > - if (!yuvCount) > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > - else > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > + if (rawSize.isNull()) > + rawSize = maxYuvSize.alignedUpTo(40, 540) > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > + IMGU_OUTPUT_HEIGHT_MARGIN) We have mixed magical values and defined ones... What are 40 and 540 exactly ? > + .boundedTo(data_->cio2_.sensor()->resolution()); > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > if (!cio2Configuration_.pixelFormat.isValid()) > return Invalid; > >
Hello Jean-Michel On Fri, Sep 03, 2021 at 09:00:37AM +0200, Jean-Michel Hautbois wrote: > Hi Jacopo, > > On 27/08/2021 14:07, Jacopo Mondi wrote: > > As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor > > full frame size") the current implementation of the IPU3 pipeline > > handler always use the sensor resolution as the ImgU input frame size in > > order to work around an issue with the ImgU configuration procedure. > > > > Now that the frame selection policy has been modified in the CIO2Device > > class implementation to comply with the requirements of the ImgU > > configuration script we can remove the workaround and select the most > > opportune sensor size to feed the ImgU with. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++-------------- > > 1 file changed, 19 insertions(+), 19 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index c287bf86e79a..b321c94e9cb0 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > status = Adjusted; > > } > > > > - /* Validate the requested stream configuration */ > > + /* > > + * Validate the requested stream configuration and select the sensor > > + * format by collecting the maximum RAW stream width and height and > > + * picking the closest larger match. > > + * > > + * If no RAW stream is requested use the one of the largest YUV stream, > > + * plus margin pixels for the IF and BDS rectangle to downscale. > > + * > > + * \todo Clarify the IF and BDS margins requirements. > > + */ > > unsigned int rawCount = 0; > > unsigned int yuvCount = 0; > > Size maxYuvSize; > > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > > rawCount++; > > - rawSize = cfg.size; > > + rawSize.expandTo(cfg.size); > > } else { > > yuvCount++; > > maxYuvSize.expandTo(cfg.size); > > @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > /* > > * Generate raw configuration from CIO2. > > * > > - * \todo The image sensor frame size should be selected to optimize > > - * operations based on the sizes of the requested streams. However such > > - * a selection makes the pipeline configuration procedure fail for small > > - * resolutions (for example: 640x480 with OV5670) and causes the capture > > - * operations to stall for some stream size combinations (see the > > - * commit message of the patch that introduced this comment for more > > - * failure examples). > > - * > > - * Until the sensor frame size calculation criteria are clarified, when > > - * capturing from ImgU always use the largest possible size which > > - * guarantees better results at the expense of the frame rate and CSI-2 > > - * bus bandwidth. When only a raw stream is requested use the requested > > - * size instead, as the ImgU is not involved. > > + * The output YUV streams will be limited in size to the > > + * maximum frame size requested for the RAW stream. > > */ > > - if (!yuvCount) > > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > - else > > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > > + if (rawSize.isNull()) > > + rawSize = maxYuvSize.alignedUpTo(40, 540) > > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > > + IMGU_OUTPUT_HEIGHT_MARGIN) > > We have mixed magical values and defined ones... What are 40 and 540 > exactly ? > You are right, 40 and 540 are magical :) They are defined in imgu.cpp as static constexpr unsigned int IF_CROP_MAX_W = 40; static constexpr unsigned int IF_CROP_MAX_H = 540; And represent the IF rectangle crop limits. As the ImgU configuration procedure is iterative, it tries to to find a suitable configuration by testing one after the other IF rectangles scaled down by a known factor, up to the maximum scaling limits expressed above. /* Populate the configurations vector by scaling width and height. */ unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W); unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H); unsigned int minIfWidth = in.width - IF_CROP_MAX_W; unsigned int minIfHeight = in.height - IF_CROP_MAX_H; while (ifWidth >= minIfWidth) { while (ifHeight >= minIfHeight) { Size iif{ ifWidth, ifHeight }; calculateBDS(pipe, iif, gdc, sf); ifHeight -= IF_ALIGN_H; } ifWidth -= IF_ALIGN_W; } /* Repeat search by scaling width first. */ ifWidth = utils::alignUp(in.width, IF_ALIGN_W); ifHeight = utils::alignUp(in.height, IF_ALIGN_H); minIfWidth = in.width - IF_CROP_MAX_W; minIfHeight = in.height - IF_CROP_MAX_H; while (ifHeight >= minIfHeight) { /* * \todo This procedure is probably broken: * https://github.com/intel/intel-ipu3-pipecfg/issues/2 */ while (ifWidth >= minIfWidth) { Size iif{ ifWidth, ifHeight }; calculateBDS(pipe, iif, gdc, sf); ifWidth -= IF_ALIGN_W; } ifHeight -= IF_ALIGN_H; } For this reason, we refuse input sizes smaller than the IF scaling limits /* * \todo Filter out all resolutions < IF_CROP_MAX. * See https://bugs.libcamera.org/show_bug.cgi?id=32 */ if (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) { LOG(IPU3, Error) << "Input resolution " << in.toString() << " not supported"; return {}; } And so here in ipu3.cpp we try to find a raw size from the sensor to feed the ImgU with larger than [40, 450]. I'll move those definitions to imgu.h and use a more expressive variable instead of the raw numbers. Thanks j > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > if (!cio2Configuration_.pixelFormat.isValid()) > > return Invalid; > > > >
On 03/09/2021 10:43, Jacopo Mondi wrote: > Hello Jean-Michel > > On Fri, Sep 03, 2021 at 09:00:37AM +0200, Jean-Michel Hautbois wrote: >> Hi Jacopo, >> >> On 27/08/2021 14:07, Jacopo Mondi wrote: >>> As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor >>> full frame size") the current implementation of the IPU3 pipeline >>> handler always use the sensor resolution as the ImgU input frame size in >>> order to work around an issue with the ImgU configuration procedure. >>> >>> Now that the frame selection policy has been modified in the CIO2Device >>> class implementation to comply with the requirements of the ImgU >>> configuration script we can remove the workaround and select the most >>> opportune sensor size to feed the ImgU with. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++-------------- >>> 1 file changed, 19 insertions(+), 19 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >>> index c287bf86e79a..b321c94e9cb0 100644 >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>> @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >>> status = Adjusted; >>> } >>> >>> - /* Validate the requested stream configuration */ >>> + /* >>> + * Validate the requested stream configuration and select the sensor >>> + * format by collecting the maximum RAW stream width and height and >>> + * picking the closest larger match. >>> + * >>> + * If no RAW stream is requested use the one of the largest YUV stream, >>> + * plus margin pixels for the IF and BDS rectangle to downscale. >>> + * >>> + * \todo Clarify the IF and BDS margins requirements. >>> + */ >>> unsigned int rawCount = 0; >>> unsigned int yuvCount = 0; >>> Size maxYuvSize; >>> @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >>> >>> if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { >>> rawCount++; >>> - rawSize = cfg.size; >>> + rawSize.expandTo(cfg.size); >>> } else { >>> yuvCount++; >>> maxYuvSize.expandTo(cfg.size); >>> @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >>> /* >>> * Generate raw configuration from CIO2. >>> * >>> - * \todo The image sensor frame size should be selected to optimize >>> - * operations based on the sizes of the requested streams. However such >>> - * a selection makes the pipeline configuration procedure fail for small >>> - * resolutions (for example: 640x480 with OV5670) and causes the capture >>> - * operations to stall for some stream size combinations (see the >>> - * commit message of the patch that introduced this comment for more >>> - * failure examples). >>> - * >>> - * Until the sensor frame size calculation criteria are clarified, when >>> - * capturing from ImgU always use the largest possible size which >>> - * guarantees better results at the expense of the frame rate and CSI-2 >>> - * bus bandwidth. When only a raw stream is requested use the requested >>> - * size instead, as the ImgU is not involved. >>> + * The output YUV streams will be limited in size to the >>> + * maximum frame size requested for the RAW stream. >>> */ >>> - if (!yuvCount) >>> - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); >>> - else >>> - cio2Configuration_ = data_->cio2_.generateConfiguration({}); >>> + if (rawSize.isNull()) >>> + rawSize = maxYuvSize.alignedUpTo(40, 540) >>> + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, >>> + IMGU_OUTPUT_HEIGHT_MARGIN) >> >> We have mixed magical values and defined ones... What are 40 and 540 >> exactly ? >> > > You are right, 40 and 540 are magical :) > > They are defined in imgu.cpp as > > static constexpr unsigned int IF_CROP_MAX_W = 40; > static constexpr unsigned int IF_CROP_MAX_H = 540; > > And represent the IF rectangle crop limits. > > As the ImgU configuration procedure is iterative, it tries to to find > a suitable configuration by testing one after the other IF rectangles > scaled down by a known factor, up to the maximum scaling limits > expressed above. > > /* Populate the configurations vector by scaling width and height. */ > unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W); > unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H); > unsigned int minIfWidth = in.width - IF_CROP_MAX_W; > unsigned int minIfHeight = in.height - IF_CROP_MAX_H; > while (ifWidth >= minIfWidth) { > while (ifHeight >= minIfHeight) { > Size iif{ ifWidth, ifHeight }; > calculateBDS(pipe, iif, gdc, sf); > ifHeight -= IF_ALIGN_H; > } > > ifWidth -= IF_ALIGN_W; > } > > /* Repeat search by scaling width first. */ > ifWidth = utils::alignUp(in.width, IF_ALIGN_W); > ifHeight = utils::alignUp(in.height, IF_ALIGN_H); > minIfWidth = in.width - IF_CROP_MAX_W; > minIfHeight = in.height - IF_CROP_MAX_H; > while (ifHeight >= minIfHeight) { > /* > * \todo This procedure is probably broken: > * https://github.com/intel/intel-ipu3-pipecfg/issues/2 > */ > while (ifWidth >= minIfWidth) { > Size iif{ ifWidth, ifHeight }; > calculateBDS(pipe, iif, gdc, sf); > ifWidth -= IF_ALIGN_W; > } > > ifHeight -= IF_ALIGN_H; > } > > For this reason, we refuse input sizes smaller than the IF scaling > limits > > /* > * \todo Filter out all resolutions < IF_CROP_MAX. > * See https://bugs.libcamera.org/show_bug.cgi?id=32 > */ > if (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) { > LOG(IPU3, Error) << "Input resolution " << in.toString() > << " not supported"; > return {}; > } > > And so here in ipu3.cpp we try to find a raw size from the sensor to > feed the ImgU with larger than [40, 450]. > > I'll move those definitions to imgu.h and use a more expressive > variable instead of the raw numbers. Thank you ! Could you also add a reference to the bug for reference maybe ? > > Thanks > j > >>> + .boundedTo(data_->cio2_.sensor()->resolution()); >>> + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); >>> if (!cio2Configuration_.pixelFormat.isValid()) >>> return Invalid; >>> >>>
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index c287bf86e79a..b321c94e9cb0 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() status = Adjusted; } - /* Validate the requested stream configuration */ + /* + * Validate the requested stream configuration and select the sensor + * format by collecting the maximum RAW stream width and height and + * picking the closest larger match. + * + * If no RAW stream is requested use the one of the largest YUV stream, + * plus margin pixels for the IF and BDS rectangle to downscale. + * + * \todo Clarify the IF and BDS margins requirements. + */ unsigned int rawCount = 0; unsigned int yuvCount = 0; Size maxYuvSize; @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { rawCount++; - rawSize = cfg.size; + rawSize.expandTo(cfg.size); } else { yuvCount++; maxYuvSize.expandTo(cfg.size); @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() /* * Generate raw configuration from CIO2. * - * \todo The image sensor frame size should be selected to optimize - * operations based on the sizes of the requested streams. However such - * a selection makes the pipeline configuration procedure fail for small - * resolutions (for example: 640x480 with OV5670) and causes the capture - * operations to stall for some stream size combinations (see the - * commit message of the patch that introduced this comment for more - * failure examples). - * - * Until the sensor frame size calculation criteria are clarified, when - * capturing from ImgU always use the largest possible size which - * guarantees better results at the expense of the frame rate and CSI-2 - * bus bandwidth. When only a raw stream is requested use the requested - * size instead, as the ImgU is not involved. + * The output YUV streams will be limited in size to the + * maximum frame size requested for the RAW stream. */ - if (!yuvCount) - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); - else - cio2Configuration_ = data_->cio2_.generateConfiguration({}); + if (rawSize.isNull()) + rawSize = maxYuvSize.alignedUpTo(40, 540) + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, + IMGU_OUTPUT_HEIGHT_MARGIN) + .boundedTo(data_->cio2_.sensor()->resolution()); + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); if (!cio2Configuration_.pixelFormat.isValid()) return Invalid;
As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame size") the current implementation of the IPU3 pipeline handler always use the sensor resolution as the ImgU input frame size in order to work around an issue with the ImgU configuration procedure. Now that the frame selection policy has been modified in the CIO2Device class implementation to comply with the requirements of the ImgU configuration script we can remove the workaround and select the most opportune sensor size to feed the ImgU with. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++-------------- 1 file changed, 19 insertions(+), 19 deletions(-)