[2/2] ipa: rpi: cam-helper: Support IMX676 through IMX678 helper

28451 diff mbox series

Sony IMX676 is also a Starvis2 sensor, with same gain model and
integration time constraints as IMX678. Add support for it through the
existing IMX678 camera helper.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 1 +
 1 file changed, 1 insertion(+)

Quoting Jai Luthra (2026-10-14 10:07:15)
> Sony IMX676 is also a Starvis2 sensor, with same gain model and
> integration time constraints as IMX678. Add support for it through the
> existing IMX678 camera helper.

I wonder if we'd end up calling these the 'starvis2' helper or such. But
this is better than duplicating and copying identical files I think.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> index 5be1fa795370..794751acbfd1 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> @@ -56,5 +56,6 @@ CamHelper *create()
>  }
>  
>  RegisterCamHelper reg("imx678", &create);
> +RegisterCamHelper reg676("imx676", &create);
>  
>  } /* namespace */
> 
> -- 
> 2.55.0
>

Hi Jai and Kieran

On Wed, 14 Oct 2026 at 10:16, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Jai Luthra (2026-10-14 10:07:15)
> > Sony IMX676 is also a Starvis2 sensor, with same gain model and
> > integration time constraints as IMX678. Add support for it through the
> > existing IMX678 camera helper.
>
> I wonder if we'd end up calling these the 'starvis2' helper or such. But
> this is better than duplicating and copying identical files I think.

Tangentially related, but it's probably worth moving imx662 (also
Starvis2) under this helper rather than it being currently lumped with
imx290 (Starvis1). The two families do have different
frameIntegrationDiff values.

Doing so would also avoid parts of
https://lists.libcamera.org/pipermail/libcamera-devel/2026-August/060990.html
that David submitted impacting imx290.
I'll dig out my imx678 to see if it too is unhappy with a
frameIntegrationDiff of 4 in HCG mode.

  Dave

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > index 5be1fa795370..794751acbfd1 100644
> > --- a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > @@ -56,5 +56,6 @@ CamHelper *create()
> >  }
> >
> >  RegisterCamHelper reg("imx678", &create);
> > +RegisterCamHelper reg676("imx676", &create);
> >
> >  } /* namespace */
> >
> > --
> > 2.55.0
> >

Hi Dave

Quoting Dave Stevenson (2026-10-14 16:43:57)
> Hi Jai and Kieran
> 
> On Wed, 14 Oct 2026 at 10:16, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Jai Luthra (2026-10-14 10:07:15)
> > > Sony IMX676 is also a Starvis2 sensor, with same gain model and
> > > integration time constraints as IMX678. Add support for it through the
> > > existing IMX678 camera helper.
> >
> > I wonder if we'd end up calling these the 'starvis2' helper or such. But
> > this is better than duplicating and copying identical files I think.
> 
> Tangentially related, but it's probably worth moving imx662 (also
> Starvis2) under this helper rather than it being currently lumped with
> imx290 (Starvis1). The two families do have different
> frameIntegrationDiff values.
> 
> Doing so would also avoid parts of
> https://lists.libcamera.org/pipermail/libcamera-devel/2026-August/060990.html
> that David submitted impacting imx290.
> I'll dig out my imx678 to see if it too is unhappy with a
> frameIntegrationDiff of 4 in HCG mode.
> 

Argh, reading your comment I double checked the shutter and exposure
sections for IMX676 in the SRM, and it seems to be slightly different from
both IMX662/IMX678. I don't know how I missed that before, I remember going
through the datasheets :-(

The SHR0 register should be from 8 to N - 2 with 2H step size. As opposed
to 1H steps from 4 to N -1 for the other two starvis2 sensors (IMX662,
IMX678) that I can access documentation for. I don't yet know if HCG mode
will effect it even more.

I'll paramererize the frameIntegrationDiff value here, similar to how
IMX662 parameterizes it today in cam_helper_imx290.cpp. I guess I'll need
to do the same for the exposure control in the driver.

But coming back to your question, I still think we should lump all starvis2
sensor camera helpers into one file here similar to the driver, even if the
values are a bit different. I know once we parameterize it might make sense
to even support starvis1 or 3 in the same helper class, but I'd rather
start off with the sensors lying on my desk, and leave the rest for
tomorrow ;-)

Thanks,
  Jai

>   Dave
> 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > >
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > >  src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > index 5be1fa795370..794751acbfd1 100644
> > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > @@ -56,5 +56,6 @@ CamHelper *create()
> > >  }
> > >
> > >  RegisterCamHelper reg("imx678", &create);
> > > +RegisterCamHelper reg676("imx676", &create);
> > >
> > >  } /* namespace */
> > >
> > > --
> > > 2.55.0
> > >

Hi Jai

On Wed, 14 Oct 2026 at 12:50, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
>
> Hi Dave
>
> Quoting Dave Stevenson (2026-10-14 16:43:57)
> > Hi Jai and Kieran
> >
> > On Wed, 14 Oct 2026 at 10:16, Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting Jai Luthra (2026-10-14 10:07:15)
> > > > Sony IMX676 is also a Starvis2 sensor, with same gain model and
> > > > integration time constraints as IMX678. Add support for it through the
> > > > existing IMX678 camera helper.
> > >
> > > I wonder if we'd end up calling these the 'starvis2' helper or such. But
> > > this is better than duplicating and copying identical files I think.
> >
> > Tangentially related, but it's probably worth moving imx662 (also
> > Starvis2) under this helper rather than it being currently lumped with
> > imx290 (Starvis1). The two families do have different
> > frameIntegrationDiff values.
> >
> > Doing so would also avoid parts of
> > https://lists.libcamera.org/pipermail/libcamera-devel/2026-August/060990.html
> > that David submitted impacting imx290.
> > I'll dig out my imx678 to see if it too is unhappy with a
> > frameIntegrationDiff of 4 in HCG mode.
> >
>
> Argh, reading your comment I double checked the shutter and exposure
> sections for IMX676 in the SRM, and it seems to be slightly different from
> both IMX662/IMX678. I don't know how I missed that before, I remember going
> through the datasheets :-(
>
> The SHR0 register should be from 8 to N - 2 with 2H step size. As opposed
> to 1H steps from 4 to N -1 for the other two starvis2 sensors (IMX662,
> IMX678) that I can access documentation for. I don't yet know if HCG mode
> will effect it even more.

Drat, you got me to check the others, and IMX585 is also "8 to (Number
of lines per frame - 2)"  in multiples of 2.
So we both missed it, and I need to fix it in the driver for IMX585.

IMX662, IMX675, IMX832, and IMX678 all confirmed to be "4 to N -1".

> I'll paramererize the frameIntegrationDiff value here, similar to how
> IMX662 parameterizes it today in cam_helper_imx290.cpp. I guess I'll need
> to do the same for the exposure control in the driver.
>
> But coming back to your question, I still think we should lump all starvis2
> sensor camera helpers into one file here similar to the driver, even if the
> values are a bit different. I know once we parameterize it might make sense
> to even support starvis1 or 3 in the same helper class, but I'd rather
> start off with the sensors lying on my desk, and leave the rest for
> tomorrow ;-)

Agreed.
For now keep the 1080p Starvis1 sensors under cam_helper_imx290, and
all Starvis2 sensors under cam_helper_imx678, potentially with a
rename of imx678 to starvis2 if someone fancies making the patch.

Looking at IMX335 and IMX415 as the other Starvis1 sensors I have,
they all differ.
IMX290: 4 to N -1
IMX335: All pixel scan : 9 to (N - 1)
  Horizontal/Vertical 2/2-line binning scan : 17 to (N - 1)
IMX415: 8 to (N - 4)
So those make sense to stay as independent helpers. I haven't checked
that their respective drivers impose the correct limits.

  Dave

> Thanks,
>   Jai
>
> >   Dave
> >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > >
> > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > > ---
> > > >  src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > > index 5be1fa795370..794751acbfd1 100644
> > > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > > @@ -56,5 +56,6 @@ CamHelper *create()
> > > >  }
> > > >
> > > >  RegisterCamHelper reg("imx678", &create);
> > > > +RegisterCamHelper reg676("imx676", &create);
> > > >
> > > >  } /* namespace */
> > > >
> > > > --
> > > > 2.55.0
> > > >