[1/4] libcamera: camera_sensor_properties: Add vd55g1 camera sensor
diff mbox series

Message ID 20250905-vd55g1_support-v1-1-545d39f280b2@foss.st.com
State Superseded
Headers show
Series
  • Add vd55g1 support for rpi and libipa
Related show

Commit Message

Benjamin Mugnier Sept. 5, 2025, 9:08 a.m. UTC
Add unit cell size from the 'pixel size' element in the datasheet.
Delays are set to 2 in case a setting is entered at the very end of the
N frame, the N+1 frame will miss it and only the N+2 frame will use this
new setting.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 src/libcamera/sensor/camera_sensor_properties.cpp | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Kieran Bingham Sept. 16, 2025, 1:36 p.m. UTC | #1
Quoting Benjamin Mugnier (2025-09-05 10:08:22)
> Add unit cell size from the 'pixel size' element in the datasheet.
> Delays are set to 2 in case a setting is entered at the very end of the
> N frame, the N+1 frame will miss it and only the N+2 frame will use this
> new setting.

I'm not sure that's the meaning of the delay values. I don't think
they're supposed to be increased just incase ...

 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  src/libcamera/sensor/camera_sensor_properties.cpp | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> index f2da8205372baabca58416e2c0f9da64e722fe02..09f60391fddba1738a5e2409703f023c9ba1655c 100644
> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> @@ -461,6 +461,23 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>                         },
>                         .sensorDelays = { },
>                 } },
> +               { "vd55g1", {
> +                       .unitCellSize = { 2160, 2160 },
> +                       .testPatternModes = {
> +                               { controls::draft::TestPatternModeOff, 0 },
> +                               { controls::draft::TestPatternModePn9, 2},
> +                               /*
> +                                * No corresponding test pattern mode for:
> +                                * 1: "Diagonal Gray Scale"
> +                                */
> +                       },
> +                       .sensorDelays = {
> +                               .exposureDelay = 2,
> +                               .gainDelay = 2,
> +                               .vblankDelay = 2,
> +                               .hblankDelay = 2
> +                       },
> +               } },
>                 { "vd56g3", {
>                         .unitCellSize = { 2610, 2610 },
>                         .testPatternModes = {
> 
> -- 
> 2.25.1
>
Benjamin Mugnier Sept. 17, 2025, 8:57 a.m. UTC | #2
Hi Kieran

On 9/16/25 15:36, Kieran Bingham wrote:
> Quoting Benjamin Mugnier (2025-09-05 10:08:22)
>> Add unit cell size from the 'pixel size' element in the datasheet.
>> Delays are set to 2 in case a setting is entered at the very end of the
>> N frame, the N+1 frame will miss it and only the N+2 frame will use this
>> new setting.
> 
> I'm not sure that's the meaning of the delay values. I don't think
> they're supposed to be increased just incase ...
> 

Ah, maybe this paragraph is not precise enough. Let me try again.

These delay values are the maximum number of frames required for the
sensor to reflect the parameter change in its acquisition.

If you set a parameter during frame N, usually the new parameter will be
taken into account at frame N+1, resulting in a delay of 1. Now, there
is a case where if you set a parameter at the very end of frame N, it
will be too late to be applied at frame N+1 (which will keep frame N's
parameters), and will effectively be taken into account at frame N+2,
hence the delays set to 2. These delays are the exact delays required to
acknowledge this worst-case scenario.

From my understanding, these values are correct, and by reading other
sensors' parameters in camera_sensor_properties.cpp, this seems pretty
standard.

Hope this helps. This is the same paragraph as the vd56g3 one that got
accepted as is. But maybe I should take this opportunity to rephrase it?

>  
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>  src/libcamera/sensor/camera_sensor_properties.cpp | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
>> index f2da8205372baabca58416e2c0f9da64e722fe02..09f60391fddba1738a5e2409703f023c9ba1655c 100644
>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
>> @@ -461,6 +461,23 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>                         },
>>                         .sensorDelays = { },
>>                 } },
>> +               { "vd55g1", {
>> +                       .unitCellSize = { 2160, 2160 },
>> +                       .testPatternModes = {
>> +                               { controls::draft::TestPatternModeOff, 0 },
>> +                               { controls::draft::TestPatternModePn9, 2},
>> +                               /*
>> +                                * No corresponding test pattern mode for:
>> +                                * 1: "Diagonal Gray Scale"
>> +                                */
>> +                       },
>> +                       .sensorDelays = {
>> +                               .exposureDelay = 2,
>> +                               .gainDelay = 2,
>> +                               .vblankDelay = 2,
>> +                               .hblankDelay = 2
>> +                       },
>> +               } },
>>                 { "vd56g3", {
>>                         .unitCellSize = { 2610, 2610 },
>>                         .testPatternModes = {
>>
>> -- 
>> 2.25.1
>>
Kieran Bingham Sept. 17, 2025, 10:15 a.m. UTC | #3
Quoting Benjamin Mugnier (2025-09-17 09:57:12)
> Hi Kieran
> 
> On 9/16/25 15:36, Kieran Bingham wrote:
> > Quoting Benjamin Mugnier (2025-09-05 10:08:22)
> >> Add unit cell size from the 'pixel size' element in the datasheet.
> >> Delays are set to 2 in case a setting is entered at the very end of the
> >> N frame, the N+1 frame will miss it and only the N+2 frame will use this
> >> new setting.
> > 
> > I'm not sure that's the meaning of the delay values. I don't think
> > they're supposed to be increased just incase ...
> > 
> 
> Ah, maybe this paragraph is not precise enough. Let me try again.
> 
> These delay values are the maximum number of frames required for the
> sensor to reflect the parameter change in its acquisition.
> 
> If you set a parameter during frame N, usually the new parameter will be
> taken into account at frame N+1, resulting in a delay of 1. Now, there
> is a case where if you set a parameter at the very end of frame N, it
> will be too late to be applied at frame N+1 (which will keep frame N's
> parameters), and will effectively be taken into account at frame N+2,
> hence the delays set to 2. These delays are the exact delays required to
> acknowledge this worst-case scenario.

that's the bit I'm concerned about. I don't think we take the worst case
scenario in these values - I think we take the best case because that's
what 99% of the occasions will be.

Ideally we need to know from the sensor metadata which values were
applied on a frame - but because we don't have that universally (or at
all yet) we use the delays specified here to have our best guess as to
what gain and exposure are *really* applied in an image frame.

We shouldn't increase by one just to be sure. And to be more clear -
what I'm really curious about is the fact that you have the delay for
both gain and exposure set the same.

Can you clarify/confirm that both the gain and exposure are applied on
the same frame when set together ?

> From my understanding, these values are correct, and by reading other
> sensors' parameters in camera_sensor_properties.cpp, this seems pretty
> standard.

Yes, but it's the wording that caught my eye indeed. It sounds like
you're adding an extra '1' to the numbers 'just in case' but that would
be wrong.

These should be 'precise' so they're either right or wrong - we can't
just set them to 3 for instance. That would be likely to introduce
oscillations in the AEGC.

> Hope this helps. This is the same paragraph as the vd56g3 one that got
> accepted as is. But maybe I should take this opportunity to rephrase it?

Wording aside I want to know what the sensor is really doing ;-)

--
Kieran

> 
> >  
> >> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> >> ---
> >>  src/libcamera/sensor/camera_sensor_properties.cpp | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> >> index f2da8205372baabca58416e2c0f9da64e722fe02..09f60391fddba1738a5e2409703f023c9ba1655c 100644
> >> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> >> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> >> @@ -461,6 +461,23 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >>                         },
> >>                         .sensorDelays = { },
> >>                 } },
> >> +               { "vd55g1", {
> >> +                       .unitCellSize = { 2160, 2160 },
> >> +                       .testPatternModes = {
> >> +                               { controls::draft::TestPatternModeOff, 0 },
> >> +                               { controls::draft::TestPatternModePn9, 2},
> >> +                               /*
> >> +                                * No corresponding test pattern mode for:
> >> +                                * 1: "Diagonal Gray Scale"
> >> +                                */
> >> +                       },
> >> +                       .sensorDelays = {
> >> +                               .exposureDelay = 2,
> >> +                               .gainDelay = 2,
> >> +                               .vblankDelay = 2,
> >> +                               .hblankDelay = 2
> >> +                       },
> >> +               } },
> >>                 { "vd56g3", {
> >>                         .unitCellSize = { 2610, 2610 },
> >>                         .testPatternModes = {
> >>
> >> -- 
> >> 2.25.1
> >>
> 
> -- 
> Regards,
> Benjamin
Benjamin Mugnier Sept. 17, 2025, 1:23 p.m. UTC | #4
On 9/17/25 12:15, Kieran Bingham wrote:
> Quoting Benjamin Mugnier (2025-09-17 09:57:12)
>> Hi Kieran
>>
>> On 9/16/25 15:36, Kieran Bingham wrote:
>>> Quoting Benjamin Mugnier (2025-09-05 10:08:22)
>>>> Add unit cell size from the 'pixel size' element in the datasheet.
>>>> Delays are set to 2 in case a setting is entered at the very end of the
>>>> N frame, the N+1 frame will miss it and only the N+2 frame will use this
>>>> new setting.
>>>
>>> I'm not sure that's the meaning of the delay values. I don't think
>>> they're supposed to be increased just incase ...
>>>
>>
>> Ah, maybe this paragraph is not precise enough. Let me try again.
>>
>> These delay values are the maximum number of frames required for the
>> sensor to reflect the parameter change in its acquisition.
>>
>> If you set a parameter during frame N, usually the new parameter will be
>> taken into account at frame N+1, resulting in a delay of 1. Now, there
>> is a case where if you set a parameter at the very end of frame N, it
>> will be too late to be applied at frame N+1 (which will keep frame N's
>> parameters), and will effectively be taken into account at frame N+2,
>> hence the delays set to 2. These delays are the exact delays required to
>> acknowledge this worst-case scenario.
> 
> that's the bit I'm concerned about. I don't think we take the worst case
> scenario in these values - I think we take the best case because that's
> what 99% of the occasions will be.
> 
> Ideally we need to know from the sensor metadata which values were
> applied on a frame - but because we don't have that universally (or at
> all yet) we use the delays specified here to have our best guess as to
> what gain and exposure are *really* applied in an image frame.
> 
> We shouldn't increase by one just to be sure. And to be more clear -
> what I'm really curious about is the fact that you have the delay for
> both gain and exposure set the same.
> 
> Can you clarify/confirm that both the gain and exposure are applied on
> the same frame when set together ?
> 

Yes, they are. It doesn't matter which combination of parameters is set.

>> From my understanding, these values are correct, and by reading other
>> sensors' parameters in camera_sensor_properties.cpp, this seems pretty
>> standard.
> 
> Yes, but it's the wording that caught my eye indeed. It sounds like
> you're adding an extra '1' to the numbers 'just in case' but that would
> be wrong.
> 
> These should be 'precise' so they're either right or wrong - we can't
> just set them to 3 for instance. That would be likely to introduce
> oscillations in the AEGC.
> 

I just discussed with the team in charge. My assumptions were wrong and
the vd55g1 is a bit specific. It buffers parameters and *always* applies
them at N+2 for consistency, avoiding the best-case/worst-case scenarios
I described above.

So I confirm '2' is the correct value. I'll rewrite the commit message
for v2. Thank you for pointing this.


Now I wonder what values does libcamera expects as values for a sensor
that doesn't have this buffer mechanism ? As you correctly pointed out
it could either be 1 or 2 depending on when the parameter is applied.

>> Hope this helps. This is the same paragraph as the vd56g3 one that got
>> accepted as is. But maybe I should take this opportunity to rephrase it?
> 
> Wording aside I want to know what the sensor is really doing ;-)
> 
> --
> Kieran
> 
>>
>>>  
>>>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>> ---
>>>>  src/libcamera/sensor/camera_sensor_properties.cpp | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
>>>> index f2da8205372baabca58416e2c0f9da64e722fe02..09f60391fddba1738a5e2409703f023c9ba1655c 100644
>>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
>>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
>>>> @@ -461,6 +461,23 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>>>                         },
>>>>                         .sensorDelays = { },
>>>>                 } },
>>>> +               { "vd55g1", {
>>>> +                       .unitCellSize = { 2160, 2160 },
>>>> +                       .testPatternModes = {
>>>> +                               { controls::draft::TestPatternModeOff, 0 },
>>>> +                               { controls::draft::TestPatternModePn9, 2},
>>>> +                               /*
>>>> +                                * No corresponding test pattern mode for:
>>>> +                                * 1: "Diagonal Gray Scale"
>>>> +                                */
>>>> +                       },
>>>> +                       .sensorDelays = {
>>>> +                               .exposureDelay = 2,
>>>> +                               .gainDelay = 2,
>>>> +                               .vblankDelay = 2,
>>>> +                               .hblankDelay = 2
>>>> +                       },
>>>> +               } },
>>>>                 { "vd56g3", {
>>>>                         .unitCellSize = { 2610, 2610 },
>>>>                         .testPatternModes = {
>>>>
>>>> -- 
>>>> 2.25.1
>>>>
>>
>> -- 
>> Regards,
>> Benjamin
Kieran Bingham Sept. 17, 2025, 2 p.m. UTC | #5
Quoting Benjamin Mugnier (2025-09-17 14:23:51)
> 
> 
> On 9/17/25 12:15, Kieran Bingham wrote:
> > Quoting Benjamin Mugnier (2025-09-17 09:57:12)
> >> Hi Kieran
> >>
> >> On 9/16/25 15:36, Kieran Bingham wrote:
> >>> Quoting Benjamin Mugnier (2025-09-05 10:08:22)
> >>>> Add unit cell size from the 'pixel size' element in the datasheet.
> >>>> Delays are set to 2 in case a setting is entered at the very end of the
> >>>> N frame, the N+1 frame will miss it and only the N+2 frame will use this
> >>>> new setting.
> >>>
> >>> I'm not sure that's the meaning of the delay values. I don't think
> >>> they're supposed to be increased just incase ...
> >>>
> >>
> >> Ah, maybe this paragraph is not precise enough. Let me try again.
> >>
> >> These delay values are the maximum number of frames required for the
> >> sensor to reflect the parameter change in its acquisition.
> >>
> >> If you set a parameter during frame N, usually the new parameter will be
> >> taken into account at frame N+1, resulting in a delay of 1. Now, there
> >> is a case where if you set a parameter at the very end of frame N, it
> >> will be too late to be applied at frame N+1 (which will keep frame N's
> >> parameters), and will effectively be taken into account at frame N+2,
> >> hence the delays set to 2. These delays are the exact delays required to
> >> acknowledge this worst-case scenario.
> > 
> > that's the bit I'm concerned about. I don't think we take the worst case
> > scenario in these values - I think we take the best case because that's
> > what 99% of the occasions will be.
> > 
> > Ideally we need to know from the sensor metadata which values were
> > applied on a frame - but because we don't have that universally (or at
> > all yet) we use the delays specified here to have our best guess as to
> > what gain and exposure are *really* applied in an image frame.
> > 
> > We shouldn't increase by one just to be sure. And to be more clear -
> > what I'm really curious about is the fact that you have the delay for
> > both gain and exposure set the same.
> > 
> > Can you clarify/confirm that both the gain and exposure are applied on
> > the same frame when set together ?
> > 
> 
> Yes, they are. It doesn't matter which combination of parameters is set.
> 
> >> From my understanding, these values are correct, and by reading other
> >> sensors' parameters in camera_sensor_properties.cpp, this seems pretty
> >> standard.
> > 
> > Yes, but it's the wording that caught my eye indeed. It sounds like
> > you're adding an extra '1' to the numbers 'just in case' but that would
> > be wrong.
> > 
> > These should be 'precise' so they're either right or wrong - we can't
> > just set them to 3 for instance. That would be likely to introduce
> > oscillations in the AEGC.
> > 
> 
> I just discussed with the team in charge. My assumptions were wrong and
> the vd55g1 is a bit specific. It buffers parameters and *always* applies
> them at N+2 for consistency, avoiding the best-case/worst-case scenarios
> I described above.
> 
> So I confirm '2' is the correct value. I'll rewrite the commit message
> for v2. Thank you for pointing this.
> 
> 
> Now I wonder what values does libcamera expects as values for a sensor
> that doesn't have this buffer mechanism ? As you correctly pointed out
> it could either be 1 or 2 depending on when the parameter is applied.

Indeed, but libcamera uses the frame start signal to apply controls to
aim to apply the updates 'as soon as possible' and avoid this issue.

Of course - not being specifically realtime - this isn't guaranteed ...

--
Kieran

> 
> >> Hope this helps. This is the same paragraph as the vd56g3 one that got
> >> accepted as is. But maybe I should take this opportunity to rephrase it?
> > 
> > Wording aside I want to know what the sensor is really doing ;-)
> > 
> > --
> > Kieran
> > 
> >>
> >>>  
> >>>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> >>>> ---
> >>>>  src/libcamera/sensor/camera_sensor_properties.cpp | 17 +++++++++++++++++
> >>>>  1 file changed, 17 insertions(+)
> >>>>
> >>>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> >>>> index f2da8205372baabca58416e2c0f9da64e722fe02..09f60391fddba1738a5e2409703f023c9ba1655c 100644
> >>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> >>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> >>>> @@ -461,6 +461,23 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >>>>                         },
> >>>>                         .sensorDelays = { },
> >>>>                 } },
> >>>> +               { "vd55g1", {
> >>>> +                       .unitCellSize = { 2160, 2160 },
> >>>> +                       .testPatternModes = {
> >>>> +                               { controls::draft::TestPatternModeOff, 0 },
> >>>> +                               { controls::draft::TestPatternModePn9, 2},
> >>>> +                               /*
> >>>> +                                * No corresponding test pattern mode for:
> >>>> +                                * 1: "Diagonal Gray Scale"
> >>>> +                                */
> >>>> +                       },
> >>>> +                       .sensorDelays = {
> >>>> +                               .exposureDelay = 2,
> >>>> +                               .gainDelay = 2,
> >>>> +                               .vblankDelay = 2,
> >>>> +                               .hblankDelay = 2
> >>>> +                       },
> >>>> +               } },
> >>>>                 { "vd56g3", {
> >>>>                         .unitCellSize = { 2610, 2610 },
> >>>>                         .testPatternModes = {
> >>>>
> >>>> -- 
> >>>> 2.25.1
> >>>>
> >>
> >> -- 
> >> Regards,
> >> Benjamin
> 
> -- 
> Regards,
> Benjamin

Patch
diff mbox series

diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
index f2da8205372baabca58416e2c0f9da64e722fe02..09f60391fddba1738a5e2409703f023c9ba1655c 100644
--- a/src/libcamera/sensor/camera_sensor_properties.cpp
+++ b/src/libcamera/sensor/camera_sensor_properties.cpp
@@ -461,6 +461,23 @@  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
 			},
 			.sensorDelays = { },
 		} },
+		{ "vd55g1", {
+			.unitCellSize = { 2160, 2160 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModePn9, 2},
+				/*
+				 * No corresponding test pattern mode for:
+				 * 1: "Diagonal Gray Scale"
+				 */
+			},
+			.sensorDelays = {
+				.exposureDelay = 2,
+				.gainDelay = 2,
+				.vblankDelay = 2,
+				.hblankDelay = 2
+			},
+		} },
 		{ "vd56g3", {
 			.unitCellSize = { 2610, 2610 },
 			.testPatternModes = {