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

[Naushir Patuck]

Hi Jacopo,

Thanks for the feedback.

On Wed, 28 Aug 2024 at 10:31, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> 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 ?

This is not a problem.  Practically the vc4 ISP driver never changes
the crop rectangle, so there is no need to store it back into
ispCrop_.

Regards,
Naush

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