[PATCH 2/7] libcamera: rkisp1: Keep aspect ratio on imx8mp

[Stefan Klug]

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 at 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 at 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
> >