[v2,3/7] pipeline: rpi: Pass crop rectangle as a parameter to platformSetIspCrop()
diff mbox series

Message ID 20240930141415.8857-4-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Add controls::rpi::ScalerCrops
Related show

Commit Message

Naushir Patuck Sept. 30, 2024, 2:14 p.m. UTC 
This will be required when we program separate crop values to each ISP
output in a future commit.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +-
 src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-
 src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---
 3 files changed, 6 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi Oct. 1, 2024, 6:25 a.m. UTC | #1 
Hi Naush

On Mon, Sep 30, 2024 at 03:14:11PM 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@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +-
>  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 11f1bfd4a5da..2de6111bacfd 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -1314,7 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)
>
>  		if (ispCrop != ispCrop_) {
>  			ispCrop_ = ispCrop;
> -			platformSetIspCrop();
> +			platformSetIspCrop(ispCrop);
>  		}
>  	}
>  }
> 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);

setSelection() can modify the rectangle received as argument, if that
happens, this won't be reflected on the argument ?

>  	}
>
>  	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
>
Naushir Patuck Oct. 1, 2024, 11:07 a.m. UTC | #2 
Hi Jacopo,

On Tue, 1 Oct 2024 at 07:25, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Mon, Sep 30, 2024 at 03:14:11PM 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@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +-
> >  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-
> >  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---
> >  3 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 11f1bfd4a5da..2de6111bacfd 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -1314,7 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> >
> >               if (ispCrop != ispCrop_) {
> >                       ispCrop_ = ispCrop;
> > -                     platformSetIspCrop();
> > +                     platformSetIspCrop(ispCrop);
> >               }
> >       }
> >  }
> > 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);
>
> setSelection() can modify the rectangle received as argument, if that
> happens, this won't be reflected on the argument ?

This cannot happen on the VC4 platform - we allow arbitrary crops, and
the device driver will never modify the user requested value.

Regards,
Naush

>
> >       }
> >
> >       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
> >
Jacopo Mondi Oct. 1, 2024, 11:28 a.m. UTC | #3 
Hi Naush

On Tue, Oct 01, 2024 at 12:07:06PM GMT, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Tue, 1 Oct 2024 at 07:25, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Mon, Sep 30, 2024 at 03:14:11PM 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@raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +-
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-
> > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---
> > >  3 files changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 11f1bfd4a5da..2de6111bacfd 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -1314,7 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> > >
> > >               if (ispCrop != ispCrop_) {
> > >                       ispCrop_ = ispCrop;
> > > -                     platformSetIspCrop();
> > > +                     platformSetIspCrop(ispCrop);
> > >               }
> > >       }
> > >  }
> > > 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);
> >
> > setSelection() can modify the rectangle received as argument, if that
> > happens, this won't be reflected on the argument ?
>
> This cannot happen on the VC4 platform - we allow arbitrary crops, and
> the device driver will never modify the user requested value.
>

Is this the same for pisp ?

> Regards,
> Naush
>
> >
> > >       }
> > >
> > >       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
> > >
Naushir Patuck Oct. 1, 2024, 12:06 p.m. UTC | #4 
On Tue, 1 Oct 2024 at 12:28, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Tue, Oct 01, 2024 at 12:07:06PM GMT, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> > On Tue, 1 Oct 2024 at 07:25, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hi Naush
> > >
> > > On Mon, Sep 30, 2024 at 03:14:11PM 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@raspberrypi.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +-
> > > >  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-
> > > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---
> > > >  3 files changed, 6 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > index 11f1bfd4a5da..2de6111bacfd 100644
> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > @@ -1314,7 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> > > >
> > > >               if (ispCrop != ispCrop_) {
> > > >                       ispCrop_ = ispCrop;
> > > > -                     platformSetIspCrop();
> > > > +                     platformSetIspCrop(ispCrop);
> > > >               }
> > > >       }
> > > >  }
> > > > 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);
> > >
> > > setSelection() can modify the rectangle received as argument, if that
> > > happens, this won't be reflected on the argument ?
> >
> > This cannot happen on the VC4 platform - we allow arbitrary crops, and
> > the device driver will never modify the user requested value.
> >
>
> Is this the same for pisp ?

pisp can also support entirely arbitrary crops - but we don't use
setSelection there.  It's all done through the config buffer.

Naush


>
> > Regards,
> > Naush
> >
> > >
> > > >       }
> > > >
> > > >       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
> > > >
Jacopo Mondi Oct. 1, 2024, 6:15 p.m. UTC | #5 
Hi Naush

On Tue, Oct 01, 2024 at 01:06:40PM GMT, Naushir Patuck wrote:
> On Tue, 1 Oct 2024 at 12:28, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Tue, Oct 01, 2024 at 12:07:06PM GMT, Naushir Patuck wrote:
> > > Hi Jacopo,
> > >
> > > On Tue, 1 Oct 2024 at 07:25, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> > > >
> > > > Hi Naush
> > > >
> > > > On Mon, Sep 30, 2024 at 03:14:11PM 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@raspberrypi.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +-
> > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-
> > > > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---
> > > > >  3 files changed, 6 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > index 11f1bfd4a5da..2de6111bacfd 100644
> > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > @@ -1314,7 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> > > > >
> > > > >               if (ispCrop != ispCrop_) {
> > > > >                       ispCrop_ = ispCrop;
> > > > > -                     platformSetIspCrop();
> > > > > +                     platformSetIspCrop(ispCrop);
> > > > >               }
> > > > >       }
> > > > >  }
> > > > > 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);
> > > >
> > > > setSelection() can modify the rectangle received as argument, if that
> > > > happens, this won't be reflected on the argument ?
> > >
> > > This cannot happen on the VC4 platform - we allow arbitrary crops, and
> > > the device driver will never modify the user requested value.
> > >
> >
> > Is this the same for pisp ?
>
> pisp can also support entirely arbitrary crops - but we don't use
> setSelection there.  It's all done through the config buffer.

Fine then
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
> Naush
>
>
> >
> > > Regards,
> > > Naush
> > >
> > > >
> > > > >       }
> > > > >
> > > > >       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
> > > > >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 11f1bfd4a5da..2de6111bacfd 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -1314,7 +1314,7 @@  void CameraData::applyScalerCrop(const ControlList &controls)
 
 		if (ispCrop != ispCrop_) {
 			ispCrop_ = ispCrop;
-			platformSetIspCrop();
+			platformSetIspCrop(ispCrop);
 		}
 	}
 }
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;
 }