[RFC,v1,1/2] libipa: camera_sensor_helper: Add imx708
diff mbox series

Message ID 20251021080651.401753-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [RFC,v1,1/2] libipa: camera_sensor_helper: Add imx708
Related show

Commit Message

Barnabás Pőcze Oct. 21, 2025, 8:06 a.m. UTC
From: Daniel Scally <dan.scally@ideasonboard.com>

The imx708 sensor driver has long been available, especially in raspberry pi
kernels; and the raspberry pi ipa modules had the corresponding helper classes
since 952ef94ed78d71 in 2023. The camera sensor properties database also has
an entry for it, but the camera sensor helper classes are missing from the
common libipa component.

So add camera sensor helper classes for all four variants of the sensor
(wide, noir). The gain calculation matches that in the raspberry pi ipa.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
[Add variants, rewrite commit message.]
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/camera_sensor_helper.cpp | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Kieran Bingham Oct. 21, 2025, 9:22 a.m. UTC | #1
Quoting Barnabás Pőcze (2025-10-21 09:06:50)
> From: Daniel Scally <dan.scally@ideasonboard.com>
> 
> The imx708 sensor driver has long been available, especially in raspberry pi
> kernels; and the raspberry pi ipa modules had the corresponding helper classes
> since 952ef94ed78d71 in 2023. The camera sensor properties database also has
> an entry for it, but the camera sensor helper classes are missing from the
> common libipa component.
> 
> So add camera sensor helper classes for all four variants of the sensor
> (wide, noir). The gain calculation matches that in the raspberry pi ipa.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> [Add variants, rewrite commit message.]
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index ef3bd0d62..829743a6d 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -642,6 +642,31 @@ public:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
>  
> +class CameraSensorHelperImx708 : public CameraSensorHelper
> +{
> +public:
> +       CameraSensorHelperImx708()
> +       {
> +               gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
> +       }
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx708", CameraSensorHelperImx708)

Do we have to duplicate these? or can this just be:
> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708)
> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708)
> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708)

I think these strings are 'RPi specific' - and the reality here is we
need to expose how we identify modules.

Isaac has been working on this recently - to propose a new string
control.

I suspect the IMX708 could be updated to be able to use something like
this as well so we can always report this as an IMX708 - but account for
the module differences through the identifier.

--
Kieran


> +
> +class CameraSensorHelperImx708Wide : public CameraSensorHelperImx708
> +{
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708Wide)
> +
> +class CameraSensorHelperImx708NoIR : public CameraSensorHelperImx708
> +{
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708NoIR)
> +
> +class CameraSensorHelperImx708WideNoIR : public CameraSensorHelperImx708
> +{
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708WideNoIR)
> +
>  class CameraSensorHelperOv2685 : public CameraSensorHelper
>  {
>  public:
> -- 
> 2.51.1.dirty
>
Barnabás Pőcze Oct. 21, 2025, 10:05 a.m. UTC | #2
Hi

2025. 10. 21. 11:22 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-10-21 09:06:50)
>> From: Daniel Scally <dan.scally@ideasonboard.com>
>>
>> The imx708 sensor driver has long been available, especially in raspberry pi
>> kernels; and the raspberry pi ipa modules had the corresponding helper classes
>> since 952ef94ed78d71 in 2023. The camera sensor properties database also has
>> an entry for it, but the camera sensor helper classes are missing from the
>> common libipa component.
>>
>> So add camera sensor helper classes for all four variants of the sensor
>> (wide, noir). The gain calculation matches that in the raspberry pi ipa.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> [Add variants, rewrite commit message.]
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/camera_sensor_helper.cpp | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>> index ef3bd0d62..829743a6d 100644
>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>> @@ -642,6 +642,31 @@ public:
>>   };
>>   REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
>>   
>> +class CameraSensorHelperImx708 : public CameraSensorHelper
>> +{
>> +public:
>> +       CameraSensorHelperImx708()
>> +       {
>> +               gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
>> +       }
>> +};
>> +REGISTER_CAMERA_SENSOR_HELPER("imx708", CameraSensorHelperImx708)
> 
> Do we have to duplicate these? or can this just be:
>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708)
>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708)
>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708)

Yes because the variable name is derived from `CameraSensorHelperImx708`, so
they would be the same, leading to redefinition errors.


> 
> I think these strings are 'RPi specific' - and the reality here is we
> need to expose how we identify modules.
> 
> Isaac has been working on this recently - to propose a new string
> control.
> 
> I suspect the IMX708 could be updated to be able to use something like
> this as well so we can always report this as an IMX708 - but account for
> the module differences through the identifier.

Do you think it's fine to merge as is (with the rpi specific names) to support the "de facto"
driver in the rpi kernels, or should we wait for a proper mainline kernel solution?


Regards,
Barnabás Pőcze

> 
> --
> Kieran
> 
> 
>> +
>> +class CameraSensorHelperImx708Wide : public CameraSensorHelperImx708
>> +{
>> +};
>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708Wide)
>> +
>> +class CameraSensorHelperImx708NoIR : public CameraSensorHelperImx708
>> +{
>> +};
>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708NoIR)
>> +
>> +class CameraSensorHelperImx708WideNoIR : public CameraSensorHelperImx708
>> +{
>> +};
>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708WideNoIR)
>> +
>>   class CameraSensorHelperOv2685 : public CameraSensorHelper
>>   {
>>   public:
>> -- 
>> 2.51.1.dirty
>>
Naushir Patuck Oct. 21, 2025, 11:25 a.m. UTC | #3
Hi,

On Tue, 21 Oct 2025 at 11:05, Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> wrote:
>
> Hi
>
> 2025. 10. 21. 11:22 keltezéssel, Kieran Bingham írta:
> > Quoting Barnabás Pőcze (2025-10-21 09:06:50)
> >> From: Daniel Scally <dan.scally@ideasonboard.com>
> >>
> >> The imx708 sensor driver has long been available, especially in raspberry pi
> >> kernels; and the raspberry pi ipa modules had the corresponding helper classes
> >> since 952ef94ed78d71 in 2023. The camera sensor properties database also has
> >> an entry for it, but the camera sensor helper classes are missing from the
> >> common libipa component.
> >>
> >> So add camera sensor helper classes for all four variants of the sensor
> >> (wide, noir). The gain calculation matches that in the raspberry pi ipa.
> >>
> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> [Add variants, rewrite commit message.]
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   src/ipa/libipa/camera_sensor_helper.cpp | 25 +++++++++++++++++++++++++
> >>   1 file changed, 25 insertions(+)
> >>
> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> >> index ef3bd0d62..829743a6d 100644
> >> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> >> @@ -642,6 +642,31 @@ public:
> >>   };
> >>   REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
> >>
> >> +class CameraSensorHelperImx708 : public CameraSensorHelper
> >> +{
> >> +public:
> >> +       CameraSensorHelperImx708()
> >> +       {
> >> +               gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
> >> +       }
> >> +};
> >> +REGISTER_CAMERA_SENSOR_HELPER("imx708", CameraSensorHelperImx708)
> >
> > Do we have to duplicate these? or can this just be:
> >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708)
> >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708)
> >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708)
>
> Yes because the variable name is derived from `CameraSensorHelperImx708`, so
> they would be the same, leading to redefinition errors.
>
>
> >
> > I think these strings are 'RPi specific' - and the reality here is we
> > need to expose how we identify modules.
> >
> > Isaac has been working on this recently - to propose a new string
> > control.
> >
> > I suspect the IMX708 could be updated to be able to use something like
> > this as well so we can always report this as an IMX708 - but account for
> > the module differences through the identifier.
>
> Do you think it's fine to merge as is (with the rpi specific names) to support the "de facto"
> driver in the rpi kernels, or should we wait for a proper mainline kernel solution?

The changes in this series were initially submitted when we added
support for IMX708.  They were not meraged because (as Kieran
mentioned) this is an RPi specific discovery/naming convention.  I'm
happy to add this if it's useful to others, or move to a more standard
scheme which sounds like Dan is aiming to do?

Regards,
Naush


>
>
> Regards,
> Barnabás Pőcze
>
> >
> > --
> > Kieran
> >
> >
> >> +
> >> +class CameraSensorHelperImx708Wide : public CameraSensorHelperImx708
> >> +{
> >> +};
> >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708Wide)
> >> +
> >> +class CameraSensorHelperImx708NoIR : public CameraSensorHelperImx708
> >> +{
> >> +};
> >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708NoIR)
> >> +
> >> +class CameraSensorHelperImx708WideNoIR : public CameraSensorHelperImx708
> >> +{
> >> +};
> >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708WideNoIR)
> >> +
> >>   class CameraSensorHelperOv2685 : public CameraSensorHelper
> >>   {
> >>   public:
> >> --
> >> 2.51.1.dirty
> >>
>
Kieran Bingham Oct. 21, 2025, 11:50 a.m. UTC | #4
Quoting Naushir Patuck (2025-10-21 12:25:06)
> Hi,
> 
> On Tue, 21 Oct 2025 at 11:05, Barnabás Pőcze
> <barnabas.pocze@ideasonboard.com> wrote:
> >
> > Hi
> >
> > 2025. 10. 21. 11:22 keltezéssel, Kieran Bingham írta:
> > > Quoting Barnabás Pőcze (2025-10-21 09:06:50)
> > >> From: Daniel Scally <dan.scally@ideasonboard.com>
> > >>
> > >> The imx708 sensor driver has long been available, especially in raspberry pi
> > >> kernels; and the raspberry pi ipa modules had the corresponding helper classes
> > >> since 952ef94ed78d71 in 2023. The camera sensor properties database also has
> > >> an entry for it, but the camera sensor helper classes are missing from the
> > >> common libipa component.
> > >>
> > >> So add camera sensor helper classes for all four variants of the sensor
> > >> (wide, noir). The gain calculation matches that in the raspberry pi ipa.
> > >>
> > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > >> [Add variants, rewrite commit message.]
> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > >> ---
> > >>   src/ipa/libipa/camera_sensor_helper.cpp | 25 +++++++++++++++++++++++++
> > >>   1 file changed, 25 insertions(+)
> > >>
> > >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > >> index ef3bd0d62..829743a6d 100644
> > >> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > >> @@ -642,6 +642,31 @@ public:
> > >>   };
> > >>   REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
> > >>
> > >> +class CameraSensorHelperImx708 : public CameraSensorHelper
> > >> +{
> > >> +public:
> > >> +       CameraSensorHelperImx708()
> > >> +       {
> > >> +               gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
> > >> +       }
> > >> +};
> > >> +REGISTER_CAMERA_SENSOR_HELPER("imx708", CameraSensorHelperImx708)

At the very least, this part could/should already be merged.

> > >
> > > Do we have to duplicate these? or can this just be:
> > >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708)
> > >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708)
> > >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708)
> >
> > Yes because the variable name is derived from `CameraSensorHelperImx708`, so
> > they would be the same, leading to redefinition errors.
> >
> >
> > >
> > > I think these strings are 'RPi specific' - and the reality here is we
> > > need to expose how we identify modules.
> > >
> > > Isaac has been working on this recently - to propose a new string
> > > control.
> > >
> > > I suspect the IMX708 could be updated to be able to use something like
> > > this as well so we can always report this as an IMX708 - but account for
> > > the module differences through the identifier.
> >
> > Do you think it's fine to merge as is (with the rpi specific names) to support the "de facto"
> > driver in the rpi kernels, or should we wait for a proper mainline kernel solution?
> 
> The changes in this series were initially submitted when we added
> support for IMX708.  They were not meraged because (as Kieran
> mentioned) this is an RPi specific discovery/naming convention.  I'm
> happy to add this if it's useful to others, or move to a more standard
> scheme which sounds like Dan is aiming to do?

I think we need to push on how to get an upstream solution, rather than
continue to try to workaround things.


We need module identifiers in so many places - not just imx708 ... so
lets give this topic a kick.


Isaac, could you post your proposed module identifier control to the
linux-media mailing list please?


--
Kieran

> 
> Regards,
> Naush
> 
> 
> >
> >
> > Regards,
> > Barnabás Pőcze
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > >> +
> > >> +class CameraSensorHelperImx708Wide : public CameraSensorHelperImx708
> > >> +{
> > >> +};
> > >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708Wide)
> > >> +
> > >> +class CameraSensorHelperImx708NoIR : public CameraSensorHelperImx708
> > >> +{
> > >> +};
> > >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708NoIR)
> > >> +
> > >> +class CameraSensorHelperImx708WideNoIR : public CameraSensorHelperImx708
> > >> +{
> > >> +};
> > >> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708WideNoIR)
> > >> +
> > >>   class CameraSensorHelperOv2685 : public CameraSensorHelper
> > >>   {
> > >>   public:
> > >> --
> > >> 2.51.1.dirty
> > >>
> >
Barnabás Pőcze Oct. 21, 2025, 11:55 a.m. UTC | #5
Hi

2025. 10. 21. 13:25 keltezéssel, Naushir Patuck írta:
> Hi,
> 
> On Tue, 21 Oct 2025 at 11:05, Barnabás Pőcze
> <barnabas.pocze@ideasonboard.com> wrote:
>>
>> Hi
>>
>> 2025. 10. 21. 11:22 keltezéssel, Kieran Bingham írta:
>>> Quoting Barnabás Pőcze (2025-10-21 09:06:50)
>>>> From: Daniel Scally <dan.scally@ideasonboard.com>
>>>>
>>>> The imx708 sensor driver has long been available, especially in raspberry pi
>>>> kernels; and the raspberry pi ipa modules had the corresponding helper classes
>>>> since 952ef94ed78d71 in 2023. The camera sensor properties database also has
>>>> an entry for it, but the camera sensor helper classes are missing from the
>>>> common libipa component.
>>>>
>>>> So add camera sensor helper classes for all four variants of the sensor
>>>> (wide, noir). The gain calculation matches that in the raspberry pi ipa.
>>>>
>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>> [Add variants, rewrite commit message.]
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    src/ipa/libipa/camera_sensor_helper.cpp | 25 +++++++++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>>>> index ef3bd0d62..829743a6d 100644
>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>>>> @@ -642,6 +642,31 @@ public:
>>>>    };
>>>>    REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
>>>>
>>>> +class CameraSensorHelperImx708 : public CameraSensorHelper
>>>> +{
>>>> +public:
>>>> +       CameraSensorHelperImx708()
>>>> +       {
>>>> +               gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
>>>> +       }
>>>> +};
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708", CameraSensorHelperImx708)
>>>
>>> Do we have to duplicate these? or can this just be:
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708)
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708)
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708)
>>
>> Yes because the variable name is derived from `CameraSensorHelperImx708`, so
>> they would be the same, leading to redefinition errors.
>>
>>
>>>
>>> I think these strings are 'RPi specific' - and the reality here is we
>>> need to expose how we identify modules.
>>>
>>> Isaac has been working on this recently - to propose a new string
>>> control.
>>>
>>> I suspect the IMX708 could be updated to be able to use something like
>>> this as well so we can always report this as an IMX708 - but account for
>>> the module differences through the identifier.
>>
>> Do you think it's fine to merge as is (with the rpi specific names) to support the "de facto"
>> driver in the rpi kernels, or should we wait for a proper mainline kernel solution?
> 
> The changes in this series were initially submitted when we added
> support for IMX708.  They were not meraged because (as Kieran
> mentioned) this is an RPi specific discovery/naming convention.  I'm
> happy to add this if it's useful to others, or move to a more standard
> scheme which sounds like Dan is aiming to do?

I assume you mean https://patchwork.libcamera.org/patch/18161/ ?

In any case, I suppose merging the helper for the base model ("imx708") for now
should not be contentious, right? (@Kieran ?)

Somewhat unrelated, out of curiosity, I see only a single version of the kernel driver
from 2023: https://lore.kernel.org/linux-media/20230124150546.12876-1-naush@raspberrypi.com/
are there any updates that I haven't found?


> 
> Regards,
> Naush
> 
> 
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>>
>>> --
>>> Kieran
>>>
>>>
>>>> +
>>>> +class CameraSensorHelperImx708Wide : public CameraSensorHelperImx708
>>>> +{
>>>> +};
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708Wide)
>>>> +
>>>> +class CameraSensorHelperImx708NoIR : public CameraSensorHelperImx708
>>>> +{
>>>> +};
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708NoIR)
>>>> +
>>>> +class CameraSensorHelperImx708WideNoIR : public CameraSensorHelperImx708
>>>> +{
>>>> +};
>>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708WideNoIR)
>>>> +
>>>>    class CameraSensorHelperOv2685 : public CameraSensorHelper
>>>>    {
>>>>    public:
>>>> --
>>>> 2.51.1.dirty
>>>>
>>
Naushir Patuck Oct. 21, 2025, 12:06 p.m. UTC | #6
Hi Barnabás,

On Tue, 21 Oct 2025 at 12:55, Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> wrote:
>
> Hi
>
> 2025. 10. 21. 13:25 keltezéssel, Naushir Patuck írta:
> > Hi,
> >
> > On Tue, 21 Oct 2025 at 11:05, Barnabás Pőcze
> > <barnabas.pocze@ideasonboard.com> wrote:
> >>
> >> Hi
> >>
> >> 2025. 10. 21. 11:22 keltezéssel, Kieran Bingham írta:
> >>> Quoting Barnabás Pőcze (2025-10-21 09:06:50)
> >>>> From: Daniel Scally <dan.scally@ideasonboard.com>
> >>>>
> >>>> The imx708 sensor driver has long been available, especially in raspberry pi
> >>>> kernels; and the raspberry pi ipa modules had the corresponding helper classes
> >>>> since 952ef94ed78d71 in 2023. The camera sensor properties database also has
> >>>> an entry for it, but the camera sensor helper classes are missing from the
> >>>> common libipa component.
> >>>>
> >>>> So add camera sensor helper classes for all four variants of the sensor
> >>>> (wide, noir). The gain calculation matches that in the raspberry pi ipa.
> >>>>
> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >>>> [Add variants, rewrite commit message.]
> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>> ---
> >>>>    src/ipa/libipa/camera_sensor_helper.cpp | 25 +++++++++++++++++++++++++
> >>>>    1 file changed, 25 insertions(+)
> >>>>
> >>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> >>>> index ef3bd0d62..829743a6d 100644
> >>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> >>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> >>>> @@ -642,6 +642,31 @@ public:
> >>>>    };
> >>>>    REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
> >>>>
> >>>> +class CameraSensorHelperImx708 : public CameraSensorHelper
> >>>> +{
> >>>> +public:
> >>>> +       CameraSensorHelperImx708()
> >>>> +       {
> >>>> +               gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
> >>>> +       }
> >>>> +};
> >>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708", CameraSensorHelperImx708)
> >>>
> >>> Do we have to duplicate these? or can this just be:
> >>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708)
> >>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708)
> >>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708)
> >>
> >> Yes because the variable name is derived from `CameraSensorHelperImx708`, so
> >> they would be the same, leading to redefinition errors.
> >>
> >>
> >>>
> >>> I think these strings are 'RPi specific' - and the reality here is we
> >>> need to expose how we identify modules.
> >>>
> >>> Isaac has been working on this recently - to propose a new string
> >>> control.
> >>>
> >>> I suspect the IMX708 could be updated to be able to use something like
> >>> this as well so we can always report this as an IMX708 - but account for
> >>> the module differences through the identifier.
> >>
> >> Do you think it's fine to merge as is (with the rpi specific names) to support the "de facto"
> >> driver in the rpi kernels, or should we wait for a proper mainline kernel solution?
> >
> > The changes in this series were initially submitted when we added
> > support for IMX708.  They were not meraged because (as Kieran
> > mentioned) this is an RPi specific discovery/naming convention.  I'm
> > happy to add this if it's useful to others, or move to a more standard
> > scheme which sounds like Dan is aiming to do?
>
> I assume you mean https://patchwork.libcamera.org/patch/18161/ ?

Yes, that looks to be the one.

>
> In any case, I suppose merging the helper for the base model ("imx708") for now
> should not be contentious, right? (@Kieran ?)
>
> Somewhat unrelated, out of curiosity, I see only a single version of the kernel driver
> from 2023: https://lore.kernel.org/linux-media/20230124150546.12876-1-naush@raspberrypi.com/
> are there any updates that I haven't found?

We made the decision to hold off submitting any updated patches for
the IMX708 driver until we had full streams/metadata support
(essential for the PDAF and HDR metadata).  In hindsight that was the
wrong call given the time it's taken (taking!) to get that
functionality merged ;-)

As soon as that work lands, I'll re-submit the driver with full
functionality for upstreaming.

Naush

>
>
> >
> > Regards,
> > Naush
> >
> >
> >>
> >>
> >> Regards,
> >> Barnabás Pőcze
> >>
> >>>
> >>> --
> >>> Kieran
> >>>
> >>>
> >>>> +
> >>>> +class CameraSensorHelperImx708Wide : public CameraSensorHelperImx708
> >>>> +{
> >>>> +};
> >>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708Wide)
> >>>> +
> >>>> +class CameraSensorHelperImx708NoIR : public CameraSensorHelperImx708
> >>>> +{
> >>>> +};
> >>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708NoIR)
> >>>> +
> >>>> +class CameraSensorHelperImx708WideNoIR : public CameraSensorHelperImx708
> >>>> +{
> >>>> +};
> >>>> +REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708WideNoIR)
> >>>> +
> >>>>    class CameraSensorHelperOv2685 : public CameraSensorHelper
> >>>>    {
> >>>>    public:
> >>>> --
> >>>> 2.51.1.dirty
> >>>>
> >>
>

Patch
diff mbox series

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index ef3bd0d62..829743a6d 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -642,6 +642,31 @@  public:
 };
 REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
 
+class CameraSensorHelperImx708 : public CameraSensorHelper
+{
+public:
+	CameraSensorHelperImx708()
+	{
+		gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
+	}
+};
+REGISTER_CAMERA_SENSOR_HELPER("imx708", CameraSensorHelperImx708)
+
+class CameraSensorHelperImx708Wide : public CameraSensorHelperImx708
+{
+};
+REGISTER_CAMERA_SENSOR_HELPER("imx708_wide", CameraSensorHelperImx708Wide)
+
+class CameraSensorHelperImx708NoIR : public CameraSensorHelperImx708
+{
+};
+REGISTER_CAMERA_SENSOR_HELPER("imx708_noir", CameraSensorHelperImx708NoIR)
+
+class CameraSensorHelperImx708WideNoIR : public CameraSensorHelperImx708
+{
+};
+REGISTER_CAMERA_SENSOR_HELPER("imx708_wide_noir", CameraSensorHelperImx708WideNoIR)
+
 class CameraSensorHelperOv2685 : public CameraSensorHelper
 {
 public: