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

[Jacopo Mondi]

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 at 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 at raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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
> > >