| Message ID | 20210907194107.803730-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Sep 07, 2021 at 09:40:51PM +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 uses 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. On a side note, I missed reviewing the changes in cio2.cpp, doesn't that belong to ipu3.cpp instead ? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ > 1 file changed, 23 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..291338288685 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); We support a single raw stream only, is this change needed ? > } else { > yuvCount++; > maxYuvSize.expandTo(cfg.size); > @@ -269,24 +278,20 @@ 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). > + * The output YUV streams will be limited in size to the > + * maximum frame size requested for the RAW stream, if present. This could be reflowed. > * > - * 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. > + * If no raw stream is requested generate a size as large as the maximum > + * requested YUV size aligned to the ImgU constraints and bound by the > + * sensor's maximum resolution. See > + * https://bugs.libcamera.org/show_bug.cgi?id=32 > */ > - if (!yuvCount) > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > - else > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > + if (rawSize.isNull()) > + rawSize = maxYuvSize.alignedUpTo(40, 540) Is alignedUpTo() the right function ? It will round up the height to a multiple of 540, that doesn't sound right. > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > + IMGU_OUTPUT_HEIGHT_MARGIN) Same here. > + .boundedTo(data_->cio2_.sensor()->resolution()); > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > if (!cio2Configuration_.pixelFormat.isValid()) > return Invalid; >
Hi Jacopo, Thank you for the patch. (Resending with Umang's correct e-mail address now) On Tue, Sep 07, 2021 at 09:40:51PM +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 uses 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. On a side note, I missed reviewing the changes in cio2.cpp, doesn't that belong to ipu3.cpp instead ? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ > 1 file changed, 23 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c287bf86e79a..291338288685 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); We support a single raw stream only, is this change needed ? > } else { > yuvCount++; > maxYuvSize.expandTo(cfg.size); > @@ -269,24 +278,20 @@ 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). > + * The output YUV streams will be limited in size to the > + * maximum frame size requested for the RAW stream, if present. This could be reflowed. > * > - * 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. > + * If no raw stream is requested generate a size as large as the maximum > + * requested YUV size aligned to the ImgU constraints and bound by the > + * sensor's maximum resolution. See > + * https://bugs.libcamera.org/show_bug.cgi?id=32 > */ > - if (!yuvCount) > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > - else > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > + if (rawSize.isNull()) > + rawSize = maxYuvSize.alignedUpTo(40, 540) Is alignedUpTo() the right function ? It will round up the height to a multiple of 540, that doesn't sound right. > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > + IMGU_OUTPUT_HEIGHT_MARGIN) Same here. > + .boundedTo(data_->cio2_.sensor()->resolution()); > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > if (!cio2Configuration_.pixelFormat.isValid()) > return Invalid; >
Sorry I forgot to reply to this one On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > (Resending with Umang's correct e-mail address now) > > On Tue, Sep 07, 2021 at 09:40:51PM +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 uses 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. > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that > belong to ipu3.cpp instead ? > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ > > 1 file changed, 23 insertions(+), 18 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index c287bf86e79a..291338288685 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); > > We support a single raw stream only, is this change needed ? > It makes the two branches look the same > > } else { > > yuvCount++; > > maxYuvSize.expandTo(cfg.size); > > @@ -269,24 +278,20 @@ 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). > > + * The output YUV streams will be limited in size to the > > + * maximum frame size requested for the RAW stream, if present. > > This could be reflowed. > > > * > > - * 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. > > + * If no raw stream is requested generate a size as large as the maximum > > + * requested YUV size aligned to the ImgU constraints and bound by the > > + * sensor's maximum resolution. See > > + * https://bugs.libcamera.org/show_bug.cgi?id=32 > > */ > > - if (!yuvCount) > > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > - else > > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > > + if (rawSize.isNull()) > > + rawSize = maxYuvSize.alignedUpTo(40, 540) > > Is alignedUpTo() the right function ? It will round up the height to a > multiple of 540, that doesn't sound right. > No it should probably be expandedTo() > > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > > + IMGU_OUTPUT_HEIGHT_MARGIN) > > Same here. > While I guess this one is correct as we need to align up to the margins > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > if (!cio2Configuration_.pixelFormat.isValid()) > > return Invalid; > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote: > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > (Resending with Umang's correct e-mail address now) > > > > On Tue, Sep 07, 2021 at 09:40:51PM +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 uses 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. > > > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that > > belong to ipu3.cpp instead ? > > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ? > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ > > > 1 file changed, 23 insertions(+), 18 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index c287bf86e79a..291338288685 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); > > > > We support a single raw stream only, is this change needed ? > > It makes the two branches look the same > > > > } else { > > > yuvCount++; > > > maxYuvSize.expandTo(cfg.size); > > > @@ -269,24 +278,20 @@ 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). > > > + * The output YUV streams will be limited in size to the > > > + * maximum frame size requested for the RAW stream, if present. > > > > This could be reflowed. > > > > > * > > > - * 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. > > > + * If no raw stream is requested generate a size as large as the maximum > > > + * requested YUV size aligned to the ImgU constraints and bound by the > > > + * sensor's maximum resolution. See > > > + * https://bugs.libcamera.org/show_bug.cgi?id=32 > > > */ > > > - if (!yuvCount) > > > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > - else > > > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > > > + if (rawSize.isNull()) > > > + rawSize = maxYuvSize.alignedUpTo(40, 540) > > > > Is alignedUpTo() the right function ? It will round up the height to a > > multiple of 540, that doesn't sound right. > > > > No it should probably be expandedTo() > > > > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > > > + IMGU_OUTPUT_HEIGHT_MARGIN) > > > > Same here. > > While I guess this one is correct as we need to align up to the > margins Are margins alignments, or a number of pixels to be added because they will be cropped by the processing blocks ? > > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > return Invalid; > > >
Hi Laurent, On Mon, Oct 11, 2021 at 05:08:26PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > > On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote: > > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > (Resending with Umang's correct e-mail address now) > > > > > > On Tue, Sep 07, 2021 at 09:40:51PM +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 uses 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. > > > > > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that > > > belong to ipu3.cpp instead ? > > > > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ > > I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ? > Are you suggesting to move them here ? > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > --- > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ > > > > 1 file changed, 23 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > index c287bf86e79a..291338288685 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); > > > > > > We support a single raw stream only, is this change needed ? > > > > It makes the two branches look the same > > > > > > } else { > > > > yuvCount++; > > > > maxYuvSize.expandTo(cfg.size); > > > > @@ -269,24 +278,20 @@ 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). > > > > + * The output YUV streams will be limited in size to the > > > > + * maximum frame size requested for the RAW stream, if present. > > > > > > This could be reflowed. > > > > > > > * > > > > - * 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. > > > > + * If no raw stream is requested generate a size as large as the maximum > > > > + * requested YUV size aligned to the ImgU constraints and bound by the > > > > + * sensor's maximum resolution. See > > > > + * https://bugs.libcamera.org/show_bug.cgi?id=32 > > > > */ > > > > - if (!yuvCount) > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > > - else > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > > > > + if (rawSize.isNull()) > > > > + rawSize = maxYuvSize.alignedUpTo(40, 540) > > > > > > Is alignedUpTo() the right function ? It will round up the height to a > > > multiple of 540, that doesn't sound right. > > > > > > > No it should probably be expandedTo() > > > > > > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > > > > + IMGU_OUTPUT_HEIGHT_MARGIN) > > > > > > Same here. > > > > While I guess this one is correct as we need to align up to the > > margins > > Are margins alignments, or a number of pixels to be added because they > will be cropped by the processing blocks ? > I wish I knew. We never had any real understanding of how to get from a raw size to a suitable output size and what are the requirements of the ImgU in terms of processing margins. This one works, I would rather not touch it (I don't like operating in such conservative way because of a lack of understanding, but I have no other options here). Thanks j > > > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > > > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > > return Invalid; > > > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Oct 11, 2021 at 05:00:40PM +0200, Jacopo Mondi wrote: > On Mon, Oct 11, 2021 at 05:08:26PM +0300, Laurent Pinchart wrote: > > On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote: > > > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote: > > > > Hi Jacopo, > > > > > > > > Thank you for the patch. > > > > > > > > (Resending with Umang's correct e-mail address now) > > > > > > > > On Tue, Sep 07, 2021 at 09:40:51PM +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 uses 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. > > > > > > > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that > > > > belong to ipu3.cpp instead ? > > > > > > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat > > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ > > > > I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ? > > Are you suggesting to move them here ? Yes, but not as part of this series. I wanted your opinion on whether this would be a good idea or not. CIO2Device::getSensorFormat() implements constraints coming from the ImgU, which seems to be a bit of a layering violation to me. > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > --- > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ > > > > > 1 file changed, 23 insertions(+), 18 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > index c287bf86e79a..291338288685 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); > > > > > > > > We support a single raw stream only, is this change needed ? > > > > > > It makes the two branches look the same > > > > > > > > } else { > > > > > yuvCount++; > > > > > maxYuvSize.expandTo(cfg.size); > > > > > @@ -269,24 +278,20 @@ 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). > > > > > + * The output YUV streams will be limited in size to the > > > > > + * maximum frame size requested for the RAW stream, if present. > > > > > > > > This could be reflowed. > > > > > > > > > * > > > > > - * 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. > > > > > + * If no raw stream is requested generate a size as large as the maximum > > > > > + * requested YUV size aligned to the ImgU constraints and bound by the > > > > > + * sensor's maximum resolution. See > > > > > + * https://bugs.libcamera.org/show_bug.cgi?id=32 > > > > > */ > > > > > - if (!yuvCount) > > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > > > - else > > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > > > > > + if (rawSize.isNull()) > > > > > + rawSize = maxYuvSize.alignedUpTo(40, 540) > > > > > > > > Is alignedUpTo() the right function ? It will round up the height to a > > > > multiple of 540, that doesn't sound right. > > > > > > > > > > No it should probably be expandedTo() > > > > > > > > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > > > > > + IMGU_OUTPUT_HEIGHT_MARGIN) > > > > > > > > Same here. > > > > > > While I guess this one is correct as we need to align up to the > > > margins > > > > Are margins alignments, or a number of pixels to be added because they > > will be cropped by the processing blocks ? > > I wish I knew. > > We never had any real understanding of how to get from a raw size to a > suitable output size and what are the requirements of the ImgU in > terms of processing margins. > > This one works, I would rather not touch it But this is new code, right ? Do you mean you'd rather not alter the behaviour of the existing implementation in the libcamera master branch, or the behaviour of this patch ? > (I don't like operating in > such conservative way because of a lack of understanding, but I have > no other options here). I still have this on my todo list, not to prove you wrong as such, but because it bothers me too :-) > > > > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > > > > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > > > return Invalid; > > > > >
On Mon, Oct 11, 2021 at 06:20:41PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Oct 11, 2021 at 05:00:40PM +0200, Jacopo Mondi wrote: > > On Mon, Oct 11, 2021 at 05:08:26PM +0300, Laurent Pinchart wrote: > > > On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote: > > > > On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote: > > > > > Hi Jacopo, > > > > > > > > > > Thank you for the patch. > > > > > > > > > > (Resending with Umang's correct e-mail address now) > > > > > > > > > > On Tue, Sep 07, 2021 at 09:40:51PM +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 uses 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. > > > > > > > > > > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that > > > > > belong to ipu3.cpp instead ? > > > > > > > > $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ > > > > > > I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ? > > > > Are you suggesting to move them here ? > > Yes, but not as part of this series. I wanted your opinion on whether > this would be a good idea or not. CIO2Device::getSensorFormat() > implements constraints coming from the ImgU, which seems to be a bit of > a layering violation to me. > mmmm, dunno... it is conceptually a requirement of the ImgU, but the whole sensor handling is done by the CIO2Device class and hidden from the rest (something I was never really found of). So in both cases we violates a layer. Not sure to be honest > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > --- > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ > > > > > > 1 file changed, 23 insertions(+), 18 deletions(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > > index c287bf86e79a..291338288685 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); > > > > > > > > > > We support a single raw stream only, is this change needed ? > > > > > > > > It makes the two branches look the same > > > > > > > > > > } else { > > > > > > yuvCount++; > > > > > > maxYuvSize.expandTo(cfg.size); > > > > > > @@ -269,24 +278,20 @@ 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). > > > > > > + * The output YUV streams will be limited in size to the > > > > > > + * maximum frame size requested for the RAW stream, if present. > > > > > > > > > > This could be reflowed. > > > > > > > > > > > * > > > > > > - * 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. > > > > > > + * If no raw stream is requested generate a size as large as the maximum > > > > > > + * requested YUV size aligned to the ImgU constraints and bound by the > > > > > > + * sensor's maximum resolution. See > > > > > > + * https://bugs.libcamera.org/show_bug.cgi?id=32 > > > > > > */ > > > > > > - if (!yuvCount) > > > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > > > > - else > > > > > > - cio2Configuration_ = data_->cio2_.generateConfiguration({}); > > > > > > + if (rawSize.isNull()) > > > > > > + rawSize = maxYuvSize.alignedUpTo(40, 540) > > > > > > > > > > Is alignedUpTo() the right function ? It will round up the height to a > > > > > multiple of 540, that doesn't sound right. > > > > > > > > > > > > > No it should probably be expandedTo() > > > > > > > > > > + .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > > > > > > + IMGU_OUTPUT_HEIGHT_MARGIN) > > > > > > > > > > Same here. > > > > > > > > While I guess this one is correct as we need to align up to the > > > > margins > > > > > > Are margins alignments, or a number of pixels to be added because they > > > will be cropped by the processing blocks ? > > > > I wish I knew. > > > > We never had any real understanding of how to get from a raw size to a > > suitable output size and what are the requirements of the ImgU in > > terms of processing margins. > > > > This one works, I would rather not touch it > > But this is new code, right ? Do you mean you'd rather not alter the > behaviour of the existing implementation in the libcamera master branch, > or the behaviour of this patch ? > Correct, and now that I look at how margins are used I could take a bet and use instead the defines we have for alignment ? static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64; static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; To find out if moving the alignement from 32 to 4 casues any issue. > > (I don't like operating in > > such conservative way because of a lack of understanding, but I have > > no other options here). > > I still have this on my todo list, not to prove you wrong as such, but > because it bothers me too :-) > I understand :) We've been dragging this debt for too long, I concur. Thanks j > > > > > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > > > > > + cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > > > > return Invalid; > > > > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index c287bf86e79a..291338288685 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,20 @@ 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). + * The output YUV streams will be limited in size to the + * maximum frame size requested for the RAW stream, if present. * - * 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. + * If no raw stream is requested generate a size as large as the maximum + * requested YUV size aligned to the ImgU constraints and bound by the + * sensor's maximum resolution. See + * https://bugs.libcamera.org/show_bug.cgi?id=32 */ - 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 uses 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: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------ 1 file changed, 23 insertions(+), 18 deletions(-)