Message ID | 20250905-vd55g1_support-v1-1-545d39f280b2@foss.st.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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 >
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 >>
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
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
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
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 = {
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(+)