[PATCH v1 3/7] pipeline: rpi: Pass crop rectangle as a parameter to platformSetIspCrop()

[Jacopo Mondi]

Hi Naush

On Thu, Aug 08, 2024 at 11:23:42AM GMT, Naushir Patuck wrote:
> This will be required when we program separate crop values to each ISP
> output in a future commit.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 3 +--
>  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 5322fd798a36..2de6111bacfd 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -1314,8 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)
>
>  		if (ispCrop != ispCrop_) {
>  			ispCrop_ = ispCrop;
> -			platformSetIspCrop();
> -
> +			platformSetIspCrop(ispCrop);

I see this introducing a potential issue ? Before this change
platformSetIspCrop() operated on ispCrop_

   isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);

With the side effect that ispCrop_ was adjusted to whatever the v4l2
video device has effectively applied.

Now you're going through a temporary variable

+               Rectangle crop = ispCrop;
+               isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);

Meaning ispCrop_ will not be updated.

Is this an issue in your opinion ? Will it change in the next patches ?

>  		}
>  	}
>  }
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 5161c16e518f..d65b695c30b5 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -83,7 +83,7 @@ public:
>
>  	Rectangle scaleIspCrop(const Rectangle &ispCrop) const;
>  	void applyScalerCrop(const ControlList &controls);
> -	virtual void platformSetIspCrop() = 0;
> +	virtual void platformSetIspCrop(const Rectangle &ispCrop) = 0;
>
>  	void cameraTimeout();
>  	void frameStarted(uint32_t sequence);
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index e5b6ef2b37cd..0ea032293bc9 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -109,9 +109,10 @@ public:
>  	Config config_;
>
>  private:
> -	void platformSetIspCrop() override
> +	void platformSetIspCrop(const Rectangle &ispCrop) override
>  	{
> -		isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);
> +		Rectangle crop = ispCrop;
> +		isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>  	}
>
>  	int platformConfigure(const RPi::RPiCameraConfiguration *rpiConfig) override;
> @@ -707,7 +708,7 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
>  	Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
>  	ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
>
> -	platformSetIspCrop();
> +	platformSetIspCrop(ispCrop_);
>
>  	return 0;
>  }
> --
> 2.34.1
>