| Message ID | 20241120085753.1993436-3-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote: > In the current code, the input stage of the image resizer is used to > apply a crop to keep the aspect ratio in cases where the requested > output aspect ratio differs from the one of the selected sensor mode. On > the imx8mp the resizer hardware is not capable of cropping (for > reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the > linux kernel v6.10). > > Therefore apply the necessary cropping on the output of the ISP (on the > image stabilization block). The cropping code on the image resizer > doesn't need modifications as the requested crop gets ignored by the > kernel. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++ > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++ > 2 files changed, 21 insertions(+) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 5ffcfbce56be..9d36554cec6e 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (!isRaw_) > format.code = MEDIA_BUS_FMT_YUYV8_2X8; > > + /* > + * On devices without DUAL_CROP (like the imx8mp) cropping needs to be > + * done on the ISP/IS output. > + */ > + if (media_->hwRevision() == RKISP1_V_IMX8MP) { > + const auto &cfg = config->at(0); > + Size ispCrop = format.size.boundedToAspectRatio(cfg.size) > + .alignedUpTo(2, 2); > + rect = ispCrop.centeredTo(Rectangle(format.size).center()); > + if (ispCrop != format.size) > + LOG(RkISP1, Info) << "ISP output needs to be cropped to " > + << rect; > + format.size = ispCrop; > + } > + > LOG(RkISP1, Debug) > << "Configuring ISP output pad with " << format > << " crop " << rect; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 236d05af7a2f..0651de464907 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config, > /* > * Crop on the resizer input to maintain FOV before downscaling. > * > + * Note that this does not apply on the imx8mp, where the cropping needs > + * to be done on the ImageStabilizer output and therefore is configured > + * before this stage. For simplicity we still set the crop. This gets > + * ignored by the kernel driver because the hardware is missing the > + * capability. > + * > * \todo The alignment to a multiple of 2 pixels is required but may > * change the aspect ratio very slightly. A more advanced algorithm to > * compute the resizer input crop rectangle is needed, and it should > -- > 2.43.0 >
Hi Stefan On Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote: > In the current code, the input stage of the image resizer is used to > apply a crop to keep the aspect ratio in cases where the requested > output aspect ratio differs from the one of the selected sensor mode. On > the imx8mp the resizer hardware is not capable of cropping (for > reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the > linux kernel v6.10). > > Therefore apply the necessary cropping on the output of the ISP (on the > image stabilization block). The cropping code on the image resizer > doesn't need modifications as the requested crop gets ignored by the > kernel. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++ > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++ > 2 files changed, 21 insertions(+) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 5ffcfbce56be..9d36554cec6e 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (!isRaw_) > format.code = MEDIA_BUS_FMT_YUYV8_2X8; > > + /* > + * On devices without DUAL_CROP (like the imx8mp) cropping needs to be > + * done on the ISP/IS output. > + */ > + if (media_->hwRevision() == RKISP1_V_IMX8MP) { I wish we had a better way to identify the single ISP features instead of relying on the whole model.. > + const auto &cfg = config->at(0); This will only work on (!DUAL_CROP && single_stream) which is the imx8mp case. I would add a comment to clarify we can assume config->at(0) because of this > + Size ispCrop = format.size.boundedToAspectRatio(cfg.size) > + .alignedUpTo(2, 2); Please align Size ispCrop = format.size.boundedToAspectRatio(cfg.size) .alignedUpTo(2, 2); > + rect = ispCrop.centeredTo(Rectangle(format.size).center()); > + if (ispCrop != format.size) > + LOG(RkISP1, Info) << "ISP output needs to be cropped to " > + << rect; > + format.size = ispCrop; > + } > + > LOG(RkISP1, Debug) > << "Configuring ISP output pad with " << format > << " crop " << rect; Unrelated, but we have LOG(RkISP1, Debug) << "ISP input pad configured with " << format << " crop " << rect; LOG(RkISP1, Debug) << "Configuring ISP output pad with " << format << " crop " << rect; It would be nice to align the two messages. Also, unrelated, but do you know why we were already setting both crop and format on the source pad with the same sizes ? Setting a format source pad propagates its sizes to the crop rectangle. Is this a better safe then sorry ? > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 236d05af7a2f..0651de464907 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config, > /* > * Crop on the resizer input to maintain FOV before downscaling. > * > + * Note that this does not apply on the imx8mp, where the cropping needs I would say, as you did already * Note that this does not apply to devices without DUAL_CROP * support (like imx8mp) , where the cropping needs > + * to be done on the ImageStabilizer output and therefore is configured What about re-stating that the IS block is configured through the ISP source pad crop rectangle ? * to be done on the ImageStabilizer block on the ISP source pad and * therefore is configured > + * before this stage. For simplicity we still set the crop. This gets > + * ignored by the kernel driver because the hardware is missing the > + * capability. > + * nits apart Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > * \todo The alignment to a multiple of 2 pixels is required but may > * change the aspect ratio very slightly. A more advanced algorithm to > * compute the resizer input crop rectangle is needed, and it should the driver aligns to 2 anyway, so this todo is kind of moot maybe > -- > 2.43.0 >
Hi Jacopo, Thank you for the review. On Thu, Nov 21, 2024 at 10:20:14AM +0100, Jacopo Mondi wrote: > Hi Stefan > > On Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote: > > In the current code, the input stage of the image resizer is used to > > apply a crop to keep the aspect ratio in cases where the requested > > output aspect ratio differs from the one of the selected sensor mode. On > > the imx8mp the resizer hardware is not capable of cropping (for > > reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the > > linux kernel v6.10). > > > > Therefore apply the necessary cropping on the output of the ISP (on the > > image stabilization block). The cropping code on the image resizer > > doesn't need modifications as the requested crop gets ignored by the > > kernel. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++ > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 5ffcfbce56be..9d36554cec6e 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > if (!isRaw_) > > format.code = MEDIA_BUS_FMT_YUYV8_2X8; > > > > + /* > > + * On devices without DUAL_CROP (like the imx8mp) cropping needs to be > > + * done on the ISP/IS output. > > + */ > > + if (media_->hwRevision() == RKISP1_V_IMX8MP) { > > I wish we had a better way to identify the single ISP features instead > of relying on the whole model.. mee too :-) > > > + const auto &cfg = config->at(0); > > This will only work on (!DUAL_CROP && single_stream) which is the > imx8mp case. > > I would add a comment to clarify we can assume config->at(0) because > of this > > > + Size ispCrop = format.size.boundedToAspectRatio(cfg.size) > > + .alignedUpTo(2, 2); > > Please align Thanks for spotting. Will do. > > Size ispCrop = format.size.boundedToAspectRatio(cfg.size) > .alignedUpTo(2, 2); > > > + rect = ispCrop.centeredTo(Rectangle(format.size).center()); > > + if (ispCrop != format.size) > > + LOG(RkISP1, Info) << "ISP output needs to be cropped to " > > + << rect; > > + format.size = ispCrop; > > + } > > + > > LOG(RkISP1, Debug) > > << "Configuring ISP output pad with " << format > > << " crop " << rect; > > Unrelated, but we have > > LOG(RkISP1, Debug) > << "ISP input pad configured with " << format > << " crop " << rect; > > > LOG(RkISP1, Debug) > << "Configuring ISP output pad with " << format > << " crop " << rect; > > It would be nice to align the two messages. I don't really get what you mean here. On the output pad there are two messages: Configuring ISP output pad with... and ISP output pad configured with ... Which seems reasonable, if we expect the kernel to adjust the TGT_CROP. Or do you mean that we should add the Configuring ISP input pad with... message? > > Also, unrelated, but do you know why we were already setting both crop > and format on the source pad with the same sizes ? Setting a format source > pad propagates its sizes to the crop rectangle. Is this a better safe > then sorry ? It wasn't me :-). No I really don't know. Either there was a driver which didn't do that properly or it is really a better safe than sorry case. > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > index 236d05af7a2f..0651de464907 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config, > > /* > > * Crop on the resizer input to maintain FOV before downscaling. > > * > > + * Note that this does not apply on the imx8mp, where the cropping needs > > I would say, as you did already Maybe a bit over verbose. > > * Note that this does not apply to devices without DUAL_CROP > * support (like imx8mp) , where the cropping needs > > > + * to be done on the ImageStabilizer output and therefore is configured > > What about re-stating that the IS block is configured through the ISP > source pad crop rectangle ? Yes I can improve that comment. > > * to be done on the ImageStabilizer block on the ISP source pad and > * therefore is configured > > > + * before this stage. For simplicity we still set the crop. This gets > > + * ignored by the kernel driver because the hardware is missing the > > + * capability. > > + * > > nits apart > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Thank you! Cheers, Stefan > Thanks > j > > > * \todo The alignment to a multiple of 2 pixels is required but may > > * change the aspect ratio very slightly. A more advanced algorithm to > > * compute the resizer input crop rectangle is needed, and it should > > the driver aligns to 2 anyway, so this todo is kind of moot maybe > > -- > > 2.43.0 > >
Hi Stefan On Thu, Nov 21, 2024 at 03:03:27PM +0100, Stefan Klug wrote: > Hi Jacopo, > > Thank you for the review. > > On Thu, Nov 21, 2024 at 10:20:14AM +0100, Jacopo Mondi wrote: > > Hi Stefan > > > > On Wed, Nov 20, 2024 at 09:57:41AM +0100, Stefan Klug wrote: > > > In the current code, the input stage of the image resizer is used to > > > apply a crop to keep the aspect ratio in cases where the requested > > > output aspect ratio differs from the one of the selected sensor mode. On > > > the imx8mp the resizer hardware is not capable of cropping (for > > > reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the > > > linux kernel v6.10). > > > > > > Therefore apply the necessary cropping on the output of the ISP (on the > > > image stabilization block). The cropping code on the image resizer > > > doesn't need modifications as the requested crop gets ignored by the > > > kernel. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++ > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++ > > > 2 files changed, 21 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 5ffcfbce56be..9d36554cec6e 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > if (!isRaw_) > > > format.code = MEDIA_BUS_FMT_YUYV8_2X8; > > > > > > + /* > > > + * On devices without DUAL_CROP (like the imx8mp) cropping needs to be > > > + * done on the ISP/IS output. > > > + */ > > > + if (media_->hwRevision() == RKISP1_V_IMX8MP) { > > > > I wish we had a better way to identify the single ISP features instead > > of relying on the whole model.. > > mee too :-) > > > > > > + const auto &cfg = config->at(0); > > > > This will only work on (!DUAL_CROP && single_stream) which is the > > imx8mp case. > > > > I would add a comment to clarify we can assume config->at(0) because > > of this > > > > > + Size ispCrop = format.size.boundedToAspectRatio(cfg.size) > > > + .alignedUpTo(2, 2); > > > > Please align > > Thanks for spotting. Will do. > > > > > Size ispCrop = format.size.boundedToAspectRatio(cfg.size) > > .alignedUpTo(2, 2); > > > > > + rect = ispCrop.centeredTo(Rectangle(format.size).center()); > > > + if (ispCrop != format.size) > > > + LOG(RkISP1, Info) << "ISP output needs to be cropped to " > > > + << rect; > > > + format.size = ispCrop; > > > + } > > > + > > > LOG(RkISP1, Debug) > > > << "Configuring ISP output pad with " << format > > > << " crop " << rect; > > > > Unrelated, but we have > > > > LOG(RkISP1, Debug) > > << "ISP input pad configured with " << format > > << " crop " << rect; > > > > > > LOG(RkISP1, Debug) > > << "Configuring ISP output pad with " << format > > << " crop " << rect; > > > > It would be nice to align the two messages. > > I don't really get what you mean here. On the output pad there are two > messages: > > Configuring ISP output pad with... and > ISP output pad configured with ... Sorry, I didn't notice the second one, I thought we only had Configuring ISP input pad with... and ISP output pad configured with ... which I was suggesting to align to the same style. Please ignore this comment :) > > Which seems reasonable, if we expect the kernel to adjust the TGT_CROP. > Or do you mean that we should add the > > Configuring ISP input pad with... > > message? > > > > > Also, unrelated, but do you know why we were already setting both crop > > and format on the source pad with the same sizes ? Setting a format source > > pad propagates its sizes to the crop rectangle. Is this a better safe > > then sorry ? > > It wasn't me :-). No I really don't know. Either there was a driver > which didn't do that properly or it is really a better safe than sorry > case. > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > index 236d05af7a2f..0651de464907 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config, > > > /* > > > * Crop on the resizer input to maintain FOV before downscaling. > > > * > > > + * Note that this does not apply on the imx8mp, where the cropping needs > > > > I would say, as you did already > > Maybe a bit over verbose. > > > > > * Note that this does not apply to devices without DUAL_CROP > > * support (like imx8mp) , where the cropping needs > > > > > + * to be done on the ImageStabilizer output and therefore is configured > > > > What about re-stating that the IS block is configured through the ISP > > source pad crop rectangle ? > > Yes I can improve that comment. > > > > > * to be done on the ImageStabilizer block on the ISP source pad and > > * therefore is configured > > > > > + * before this stage. For simplicity we still set the crop. This gets > > > + * ignored by the kernel driver because the hardware is missing the > > > + * capability. > > > + * > > > > nits apart > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thank you! > > Cheers, > Stefan > > > Thanks > > j > > > > > * \todo The alignment to a multiple of 2 pixels is required but may > > > * change the aspect ratio very slightly. A more advanced algorithm to > > > * compute the resizer input crop rectangle is needed, and it should > > > > the driver aligns to 2 anyway, so this todo is kind of moot maybe > > > -- > > > 2.43.0 > > >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 5ffcfbce56be..9d36554cec6e 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -811,6 +811,21 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) if (!isRaw_) format.code = MEDIA_BUS_FMT_YUYV8_2X8; + /* + * On devices without DUAL_CROP (like the imx8mp) cropping needs to be + * done on the ISP/IS output. + */ + if (media_->hwRevision() == RKISP1_V_IMX8MP) { + const auto &cfg = config->at(0); + Size ispCrop = format.size.boundedToAspectRatio(cfg.size) + .alignedUpTo(2, 2); + rect = ispCrop.centeredTo(Rectangle(format.size).center()); + if (ispCrop != format.size) + LOG(RkISP1, Info) << "ISP output needs to be cropped to " + << rect; + format.size = ispCrop; + } + LOG(RkISP1, Debug) << "Configuring ISP output pad with " << format << " crop " << rect; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index 236d05af7a2f..0651de464907 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -417,6 +417,12 @@ int RkISP1Path::configure(const StreamConfiguration &config, /* * Crop on the resizer input to maintain FOV before downscaling. * + * Note that this does not apply on the imx8mp, where the cropping needs + * to be done on the ImageStabilizer output and therefore is configured + * before this stage. For simplicity we still set the crop. This gets + * ignored by the kernel driver because the hardware is missing the + * capability. + * * \todo The alignment to a multiple of 2 pixels is required but may * change the aspect ratio very slightly. A more advanced algorithm to * compute the resizer input crop rectangle is needed, and it should
In the current code, the input stage of the image resizer is used to apply a crop to keep the aspect ratio in cases where the requested output aspect ratio differs from the one of the selected sensor mode. On the imx8mp the resizer hardware is not capable of cropping (for reference see also rkisp1-resizer.c:rkisp1_rsz_set_sink_crop() in the linux kernel v6.10). Therefore apply the necessary cropping on the output of the ISP (on the image stabilization block). The cropping code on the image resizer doesn't need modifications as the requested crop gets ignored by the kernel. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++++ src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++++++ 2 files changed, 21 insertions(+)