[2/2] ipa: rpi: Add cam_helper for Sony IMX678

26932 diff mbox series

From: will whang <will@willwhang.com>

This adds a basic cam helper for Sony IMX678, taken from Will Whang's
github.

Link: https://github.com/will127534/libcamera/commit/619459e7c70306ba18476
Signed-off-by: will whang <will@willwhang.com>
[fix copyright notice, comments and whitespace]
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 69 ++++++++++++++++++++++++++++
 src/ipa/rpi/cam_helper/meson.build           |  1 +
 2 files changed, 70 insertions(+)

On Thu, Jun 18, 2026 at 05:18:40PM +0530, Jai Luthra wrote:
> From: will whang <will@willwhang.com>
> 
> This adds a basic cam helper for Sony IMX678, taken from Will Whang's
> github.
> 
> Link: https://github.com/will127534/libcamera/commit/619459e7c70306ba18476
> Signed-off-by: will whang <will@willwhang.com>
> [fix copyright notice, comments and whitespace]
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 69 ++++++++++++++++++++++++++++
>  src/ipa/rpi/cam_helper/meson.build           |  1 +
>  2 files changed, 70 insertions(+)
> 
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> new file mode 100644
> index 000000000..43fc6ae54
> --- /dev/null
> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2026, Will Whang
> + *
> + * cam_helper_imx678.cpp - camera information for Sony IMX678 sensor
> + */
> +
> +#include <assert.h>
> +
> +#include "cam_helper.h"
> +#include "math.h"
> +using namespace RPiController;
> +
> +class CamHelperImx678 : public CamHelper
> +{
> +public:
> +	CamHelperImx678();
> +	uint32_t gainCode(double gain) const override;
> +	double gain(uint32_t gainCode) const override;
> +	unsigned int hideFramesStartup() const override;
> +	unsigned int hideFramesModeSwitch() const override;
> +
> +private:
> +	/*
> +	 * Smallest difference between the frame length and integration time,
> +	 * in units of lines.
> +	 */
> +	static constexpr int frameIntegrationDiff = 4;
> +};
> +
> +/*
> + * IMX678 doesn't output metadata, so we have to use the "unicam parser" which
> + * works by counting frames.

I assume this is a software limitation of the existing driver. It's fine
for the time being.

> + */
> +
> +CamHelperImx678::CamHelperImx678()
> +	: CamHelper({}, frameIntegrationDiff)
> +{
> +}
> +
> +uint32_t CamHelperImx678::gainCode(double gain) const
> +{
> +	int code = 66.6667 * log10(gain);
> +	return std::max(0, std::min(code, 0xf0));
> +}
> +
> +double CamHelperImx678::gain(uint32_t gainCode) const
> +{
> +	return pow(10, 0.015 * gainCode);
> +}
> +
> +unsigned int CamHelperImx678::hideFramesStartup() const
> +{
> +	/* On startup, we seem to get 1 bad frame. */
> +	return 1;

Have you tested that ? How "bad" is the frame ?

> +}
> +
> +unsigned int CamHelperImx678::hideFramesModeSwitch() const
> +{
> +	/* After a mode switch, we seem to get 1 bad frame. */
> +	return 1;
> +}
> +
> +static CamHelper *create()
> +{
> +	return new CamHelperImx678();
> +}
> +
> +static RegisterCamHelper reg("imx678", &create);
> diff --git a/src/ipa/rpi/cam_helper/meson.build b/src/ipa/rpi/cam_helper/meson.build
> index 87b6a3600..eabd55dce 100644
> --- a/src/ipa/rpi/cam_helper/meson.build
> +++ b/src/ipa/rpi/cam_helper/meson.build
> @@ -10,6 +10,7 @@ rpi_ipa_cam_helper_sources = files([
>      'cam_helper_imx415.cpp',
>      'cam_helper_imx477.cpp',
>      'cam_helper_imx519.cpp',
> +    'cam_helper_imx678.cpp',
>      'cam_helper_imx708.cpp',
>      'cam_helper_ov64a40.cpp',
>      'cam_helper_ov7251.cpp',

Hi Laurent,

Thanks for the review.

Quoting Laurent Pinchart (2026-06-18 18:21:12)
> On Thu, Jun 18, 2026 at 05:18:40PM +0530, Jai Luthra wrote:
> > From: will whang <will@willwhang.com>
> > 
> > This adds a basic cam helper for Sony IMX678, taken from Will Whang's
> > github.
> > 
> > Link: https://github.com/will127534/libcamera/commit/619459e7c70306ba18476
> > Signed-off-by: will whang <will@willwhang.com>
> > [fix copyright notice, comments and whitespace]
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 69 ++++++++++++++++++++++++++++
> >  src/ipa/rpi/cam_helper/meson.build           |  1 +
> >  2 files changed, 70 insertions(+)
> > 
> > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > new file mode 100644
> > index 000000000..43fc6ae54
> > --- /dev/null
> > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > @@ -0,0 +1,69 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2026, Will Whang
> > + *
> > + * cam_helper_imx678.cpp - camera information for Sony IMX678 sensor
> > + */
> > +
> > +#include <assert.h>
> > +
> > +#include "cam_helper.h"
> > +#include "math.h"
> > +using namespace RPiController;
> > +
> > +class CamHelperImx678 : public CamHelper
> > +{
> > +public:
> > +     CamHelperImx678();
> > +     uint32_t gainCode(double gain) const override;
> > +     double gain(uint32_t gainCode) const override;
> > +     unsigned int hideFramesStartup() const override;
> > +     unsigned int hideFramesModeSwitch() const override;
> > +
> > +private:
> > +     /*
> > +      * Smallest difference between the frame length and integration time,
> > +      * in units of lines.
> > +      */
> > +     static constexpr int frameIntegrationDiff = 4;
> > +};
> > +
> > +/*
> > + * IMX678 doesn't output metadata, so we have to use the "unicam parser" which
> > + * works by counting frames.
> 
> I assume this is a software limitation of the existing driver. It's fine
> for the time being.
> 

Indeed, I'll reword the comment in v2 to make it clearer.

> > + */
> > +
> > +CamHelperImx678::CamHelperImx678()
> > +     : CamHelper({}, frameIntegrationDiff)
> > +{
> > +}
> > +
> > +uint32_t CamHelperImx678::gainCode(double gain) const
> > +{
> > +     int code = 66.6667 * log10(gain);
> > +     return std::max(0, std::min(code, 0xf0));
> > +}
> > +
> > +double CamHelperImx678::gain(uint32_t gainCode) const
> > +{
> > +     return pow(10, 0.015 * gainCode);
> > +}
> > +
> > +unsigned int CamHelperImx678::hideFramesStartup() const
> > +{
> > +     /* On startup, we seem to get 1 bad frame. */
> > +     return 1;
> 
> Have you tested that ? How "bad" is the frame ?
> 

Sorry I didn't test that before, doing it now I don't see any issues when
this (and the below function) are returning 0.

It's possible something got fixed in the startup sequence compared to the
driver Will was using.

Will update these in v2.

> > +}
> > +
> > +unsigned int CamHelperImx678::hideFramesModeSwitch() const
> > +{
> > +     /* After a mode switch, we seem to get 1 bad frame. */
> > +     return 1;
> > +}
> > +
> > +static CamHelper *create()
> > +{
> > +     return new CamHelperImx678();
> > +}
> > +
> > +static RegisterCamHelper reg("imx678", &create);
> > diff --git a/src/ipa/rpi/cam_helper/meson.build b/src/ipa/rpi/cam_helper/meson.build
> > index 87b6a3600..eabd55dce 100644
> > --- a/src/ipa/rpi/cam_helper/meson.build
> > +++ b/src/ipa/rpi/cam_helper/meson.build
> > @@ -10,6 +10,7 @@ rpi_ipa_cam_helper_sources = files([
> >      'cam_helper_imx415.cpp',
> >      'cam_helper_imx477.cpp',
> >      'cam_helper_imx519.cpp',
> > +    'cam_helper_imx678.cpp',
> >      'cam_helper_imx708.cpp',
> >      'cam_helper_ov64a40.cpp',
> >      'cam_helper_ov7251.cpp',
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Thanks,
Jai

Quoting Jai Luthra (2026-06-18 14:56:06)
> Hi Laurent,
> 
> Thanks for the review.
> 
> Quoting Laurent Pinchart (2026-06-18 18:21:12)
> > On Thu, Jun 18, 2026 at 05:18:40PM +0530, Jai Luthra wrote:
> > > From: will whang <will@willwhang.com>
> > > 
> > > This adds a basic cam helper for Sony IMX678, taken from Will Whang's
> > > github.

I don't think "taken from Will Whangs' github is the focus point of the
commit message. Especially as you've kept Will as the author so suddenly
this sounds like "I took this from my repo".

When you respin, could you make this about the content of the patch
please?

Keeping the link reference below is still a good idea though.

> > > 
> > > Link: https://github.com/will127534/libcamera/commit/619459e7c70306ba18476
> > > Signed-off-by: will whang <will@willwhang.com>
> > > [fix copyright notice, comments and whitespace]
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > >  src/ipa/rpi/cam_helper/cam_helper_imx678.cpp | 69 ++++++++++++++++++++++++++++
> > >  src/ipa/rpi/cam_helper/meson.build           |  1 +
> > >  2 files changed, 70 insertions(+)
> > > 
> > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > new file mode 100644
> > > index 000000000..43fc6ae54
> > > --- /dev/null
> > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx678.cpp
> > > @@ -0,0 +1,69 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +/*
> > > + * Copyright (C) 2026, Will Whang
> > > + *
> > > + * cam_helper_imx678.cpp - camera information for Sony IMX678 sensor
> > > + */
> > > +
> > > +#include <assert.h>
> > > +
> > > +#include "cam_helper.h"
> > > +#include "math.h"
> > > +using namespace RPiController;
> > > +
> > > +class CamHelperImx678 : public CamHelper
> > > +{
> > > +public:
> > > +     CamHelperImx678();
> > > +     uint32_t gainCode(double gain) const override;
> > > +     double gain(uint32_t gainCode) const override;
> > > +     unsigned int hideFramesStartup() const override;
> > > +     unsigned int hideFramesModeSwitch() const override;
> > > +
> > > +private:
> > > +     /*
> > > +      * Smallest difference between the frame length and integration time,
> > > +      * in units of lines.
> > > +      */
> > > +     static constexpr int frameIntegrationDiff = 4;
> > > +};
> > > +
> > > +/*
> > > + * IMX678 doesn't output metadata, so we have to use the "unicam parser" which
> > > + * works by counting frames.
> > 
> > I assume this is a software limitation of the existing driver. It's fine
> > for the time being.
> > 
> 
> Indeed, I'll reword the comment in v2 to make it clearer.
> 
> > > + */
> > > +
> > > +CamHelperImx678::CamHelperImx678()
> > > +     : CamHelper({}, frameIntegrationDiff)
> > > +{
> > > +}
> > > +
> > > +uint32_t CamHelperImx678::gainCode(double gain) const
> > > +{
> > > +     int code = 66.6667 * log10(gain);
> > > +     return std::max(0, std::min(code, 0xf0));
> > > +}
> > > +
> > > +double CamHelperImx678::gain(uint32_t gainCode) const
> > > +{
> > > +     return pow(10, 0.015 * gainCode);
> > > +}
> > > +
> > > +unsigned int CamHelperImx678::hideFramesStartup() const
> > > +{
> > > +     /* On startup, we seem to get 1 bad frame. */
> > > +     return 1;
> > 
> > Have you tested that ? How "bad" is the frame ?
> > 
> 
> Sorry I didn't test that before, doing it now I don't see any issues when
> this (and the below function) are returning 0.
> 
> It's possible something got fixed in the startup sequence compared to the
> driver Will was using.
> 
> Will update these in v2.

I think everyone copy/pastes an existing helper so the values that
aren't tested are simply propogated.

Kieran

> 
> > > +}
> > > +
> > > +unsigned int CamHelperImx678::hideFramesModeSwitch() const
> > > +{
> > > +     /* After a mode switch, we seem to get 1 bad frame. */
> > > +     return 1;
> > > +}
> > > +
> > > +static CamHelper *create()
> > > +{
> > > +     return new CamHelperImx678();
> > > +}
> > > +
> > > +static RegisterCamHelper reg("imx678", &create);
> > > diff --git a/src/ipa/rpi/cam_helper/meson.build b/src/ipa/rpi/cam_helper/meson.build
> > > index 87b6a3600..eabd55dce 100644
> > > --- a/src/ipa/rpi/cam_helper/meson.build
> > > +++ b/src/ipa/rpi/cam_helper/meson.build
> > > @@ -10,6 +10,7 @@ rpi_ipa_cam_helper_sources = files([
> > >      'cam_helper_imx415.cpp',
> > >      'cam_helper_imx477.cpp',
> > >      'cam_helper_imx519.cpp',
> > > +    'cam_helper_imx678.cpp',
> > >      'cam_helper_imx708.cpp',
> > >      'cam_helper_ov64a40.cpp',
> > >      'cam_helper_ov7251.cpp',
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> Thanks,
> Jai