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

[Jacopo Mondi]

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

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