[v4,18/20] pipeline: rkisp1: Fix config validation when dewarper is used

22364 diff mbox series

When the dewarper is used, config->validate() needs to take the
restrictions of the dewarper into account. Add the corresponding checks.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

---

Changes in v4:
- Small fixes in comments
- Collected tags
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 +++++++++++++++++++++---
 1 file changed, 41 insertions(+), 5 deletions(-)

Hi Stefan

On Mon, Dec 16, 2024 at 04:40:58PM +0100, Stefan Klug wrote:
> When the dewarper is used, config->validate() needs to take the
> restrictions of the dewarper into account. Add the corresponding checks.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
>
> Changes in v4:
> - Small fixes in comments
> - Collected tags
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 +++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 55b839e76d06..432a01694913 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -497,6 +497,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
>
>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
> +	const PipelineHandlerRkISP1 *pipe = data_->pipe();
>  	const CameraSensor *sensor = data_->sensor_.get();
>  	unsigned int pathCount = data_->selfPath_ ? 2 : 1;
>  	Status status;
> @@ -553,6 +554,18 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		}
>  	}
>
> +	bool useDewarper = false;
> +	if (pipe->dewarper_) {
> +		/*
> +		 * Platforms with dewarper support, such as i.MX8MP, support
> +		 * only a single stream. We can inspect config_[0] only here.
> +		 */
> +		bool isRaw = PixelFormatInfo::info(config_[0].pixelFormat).colourEncoding ==
> +			     PixelFormatInfo::ColourEncodingRAW;
> +		if (!isRaw)
> +			useDewarper = true;
> +	}
> +
>  	/*
>  	 * If there are more than one stream in the configuration figure out the
>  	 * order to evaluate the streams. The first stream has the highest
> @@ -565,12 +578,31 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  	if (config_.size() == 2 && fitsAllPaths(config_[0]))
>  		std::reverse(order.begin(), order.end());
>
> +	/*
> +	 * Validate the configuration against the desired path and, if the
> +	 * platform supports it, the dewarper.
> +	 */
>  	auto validateConfig = [&](StreamConfiguration &cfg, RkISP1Path *path,
>  				  Stream *stream, Status expectedStatus) {
>  		StreamConfiguration tryCfg = cfg;
> -		if (path->validate(sensor, sensorConfig, &tryCfg) != expectedStatus)
> +
> +		Status ret = path->validate(sensor, sensorConfig, &tryCfg);
> +		if (ret == Invalid)
> +			return false;
> +
> +		if (!useDewarper &&
> +		    (expectedStatus == Valid && ret == Adjusted))
>  			return false;
>
> +		if (useDewarper) {
> +			bool adjusted;
> +
> +			pipe->dewarper_->validateOutput(&tryCfg, &adjusted,
> +							Converter::Alignment::Down);
> +			if (expectedStatus == Valid && adjusted)
> +				return false;
> +		}
> +
>  		cfg = tryCfg;
>  		cfg.setStream(stream);
>  		return true;
> @@ -820,6 +852,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	const PixelFormat &streamFormat = config->at(0).pixelFormat;
>  	const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
>  	isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> +	useDewarper_ = dewarper_ && !isRaw_;

This should probably be moved to
 pipeline: rkisp1: Enable the dewarper unconditionally

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Thanks
  j

>
>  	/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
>  	if (!isRaw_)
> @@ -832,8 +865,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	if (media_->hwRevision() == RKISP1_V_IMX8MP) {
>  		/* imx8mp has only a single path. */
>  		const auto &cfg = config->at(0);
> -		Size ispCrop = format.size.boundedToAspectRatio(cfg.size)
> -					  .alignedUpTo(2, 2);
> +		Size ispCrop = format.size.boundedToAspectRatio(cfg.size);
> +		if (useDewarper_)
> +			ispCrop = dewarper_->adjustInputSize(cfg.pixelFormat,
> +							     ispCrop);
> +		else
> +			ispCrop.alignUpTo(2, 2);
> +
>  		outputCrop = ispCrop.centeredTo(Rectangle(format.size).center());
>  		format.size = ispCrop;
>  	}
> @@ -875,8 +913,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  				if (ret)
>  					return ret;
>
> -				useDewarper_ = true;
> -
>  				/*
>  				 * Calculate the crop rectangle of the data
>  				 * flowing into the dewarper in sensor
> --
> 2.43.0
>

Hi Jacopo,

Thank you for the review. 

On Mon, Dec 16, 2024 at 07:35:41PM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Mon, Dec 16, 2024 at 04:40:58PM +0100, Stefan Klug wrote:
> > When the dewarper is used, config->validate() needs to take the
> > restrictions of the dewarper into account. Add the corresponding checks.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> >
> > Changes in v4:
> > - Small fixes in comments
> > - Collected tags
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 +++++++++++++++++++++---
> >  1 file changed, 41 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 55b839e76d06..432a01694913 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -497,6 +497,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> >
> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  {
> > +	const PipelineHandlerRkISP1 *pipe = data_->pipe();
> >  	const CameraSensor *sensor = data_->sensor_.get();
> >  	unsigned int pathCount = data_->selfPath_ ? 2 : 1;
> >  	Status status;
> > @@ -553,6 +554,18 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  		}
> >  	}
> >
> > +	bool useDewarper = false;
> > +	if (pipe->dewarper_) {
> > +		/*
> > +		 * Platforms with dewarper support, such as i.MX8MP, support
> > +		 * only a single stream. We can inspect config_[0] only here.
> > +		 */
> > +		bool isRaw = PixelFormatInfo::info(config_[0].pixelFormat).colourEncoding ==
> > +			     PixelFormatInfo::ColourEncodingRAW;
> > +		if (!isRaw)
> > +			useDewarper = true;
> > +	}
> > +
> >  	/*
> >  	 * If there are more than one stream in the configuration figure out the
> >  	 * order to evaluate the streams. The first stream has the highest
> > @@ -565,12 +578,31 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  	if (config_.size() == 2 && fitsAllPaths(config_[0]))
> >  		std::reverse(order.begin(), order.end());
> >
> > +	/*
> > +	 * Validate the configuration against the desired path and, if the
> > +	 * platform supports it, the dewarper.
> > +	 */
> >  	auto validateConfig = [&](StreamConfiguration &cfg, RkISP1Path *path,
> >  				  Stream *stream, Status expectedStatus) {
> >  		StreamConfiguration tryCfg = cfg;
> > -		if (path->validate(sensor, sensorConfig, &tryCfg) != expectedStatus)
> > +
> > +		Status ret = path->validate(sensor, sensorConfig, &tryCfg);
> > +		if (ret == Invalid)
> > +			return false;
> > +
> > +		if (!useDewarper &&
> > +		    (expectedStatus == Valid && ret == Adjusted))
> >  			return false;
> >
> > +		if (useDewarper) {
> > +			bool adjusted;
> > +
> > +			pipe->dewarper_->validateOutput(&tryCfg, &adjusted,
> > +							Converter::Alignment::Down);
> > +			if (expectedStatus == Valid && adjusted)
> > +				return false;
> > +		}
> > +
> >  		cfg = tryCfg;
> >  		cfg.setStream(stream);
> >  		return true;
> > @@ -820,6 +852,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  	const PixelFormat &streamFormat = config->at(0).pixelFormat;
> >  	const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
> >  	isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> > +	useDewarper_ = dewarper_ && !isRaw_;
> 
> This should probably be moved to
>  pipeline: rkisp1: Enable the dewarper unconditionally

As mentioned on patch 15/20 you didn't like it in that patch (which
makes sense), so I moved it here where it's actually needed.

> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Thanks
>   j

Thank you.

Stefan

> 
> >
> >  	/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
> >  	if (!isRaw_)
> > @@ -832,8 +865,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  	if (media_->hwRevision() == RKISP1_V_IMX8MP) {
> >  		/* imx8mp has only a single path. */
> >  		const auto &cfg = config->at(0);
> > -		Size ispCrop = format.size.boundedToAspectRatio(cfg.size)
> > -					  .alignedUpTo(2, 2);
> > +		Size ispCrop = format.size.boundedToAspectRatio(cfg.size);
> > +		if (useDewarper_)
> > +			ispCrop = dewarper_->adjustInputSize(cfg.pixelFormat,
> > +							     ispCrop);
> > +		else
> > +			ispCrop.alignUpTo(2, 2);
> > +
> >  		outputCrop = ispCrop.centeredTo(Rectangle(format.size).center());
> >  		format.size = ispCrop;
> >  	}
> > @@ -875,8 +913,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  				if (ret)
> >  					return ret;
> >
> > -				useDewarper_ = true;
> > -
> >  				/*
> >  				 * Calculate the crop rectangle of the data
> >  				 * flowing into the dewarper in sensor
> > --
> > 2.43.0
> >