| Message ID | 20260729154046.207568-1-robert.mader@collabora.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert, out of curiosity, why an RFC and not a proper patch ? On Wed, Jul 29, 2026 at 05:40:46PM +0200, Robert Mader wrote: > These sensors have existing sensor helpers and black level values in the > corresponding tuning files. Lift them up so they can be used by other corresponding tuning files "in the RaspberryPi IPA". > IPAs, most importantly the softISP. "Lift them up in libipa so they can also be used by other platforms" Not sure why softISP is "most importantly" here :) > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index cc16e25123d9..139e7dd646ee 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -595,6 +595,7 @@ class CameraSensorHelperImx296 : public CameraSensorHelper > public: > CameraSensorHelperImx296() > { > + blackLevel_ = 3840; > gain_ = AnalogueGainExp{ 1.0, expGainDb(0.1) }; > } > }; > @@ -634,6 +635,7 @@ class CameraSensorHelperImx415 : public CameraSensorHelper > public: > CameraSensorHelperImx415() > { > + blackLevel_ = 3840; the bu0569 version has a different value here. (3200) We're not able to distinguish camera modules apart, so I guess using the plain 415's value is correct > gain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) }; > } > }; > @@ -649,6 +651,7 @@ class CameraSensorHelperImx477 : public CameraSensorHelper > public: > CameraSensorHelperImx477() > { > + blackLevel_ = 4096; > gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 }; > } > }; > @@ -779,6 +782,7 @@ class CameraSensorHelperOv5647 : public CameraSensorHelper > public: > CameraSensorHelperOv5647() > { > + blackLevel_ = 1024; > gain_ = AnalogueGainLinear{ 1, 0, 0, 16 }; > } > }; > @@ -822,6 +826,7 @@ class CameraSensorHelperOv64a40 : public CameraSensorHelper > public: > CameraSensorHelperOv64a40() > { > + blackLevel_ = 4096; > gain_ = AnalogueGainLinear{ 1, 0, 0, 128 }; > } Compared the values with the pips and vc4 entries and they're all good, thanks Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > }; > -- > 2.55.0 >
Hi Jakobo, thanks for the review! On 31.07.26 11:08, Jacopo Mondi wrote: > Hi Robert, > > out of curiosity, why an RFC and not a proper patch ? Just because I copied the values without direct source - i.e. no comments à la "From datasheet:" - and am not sure if that's acceptable. If there's no resistance / if other people are also ok with that I'll resend as proper patch. > > On Wed, Jul 29, 2026 at 05:40:46PM +0200, Robert Mader wrote: >> These sensors have existing sensor helpers and black level values in the >> corresponding tuning files. Lift them up so they can be used by other > corresponding tuning files "in the RaspberryPi IPA". > > >> IPAs, most importantly the softISP. > "Lift them up in libipa so they can also be used by other platforms" > > Not sure why softISP is "most importantly" here :) True, just wanted to express that that's what *I* am currently mostly interested in :P Will reword. Regards >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- >> src/ipa/libipa/camera_sensor_helper.cpp | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp >> index cc16e25123d9..139e7dd646ee 100644 >> --- a/src/ipa/libipa/camera_sensor_helper.cpp >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp >> @@ -595,6 +595,7 @@ class CameraSensorHelperImx296 : public CameraSensorHelper >> public: >> CameraSensorHelperImx296() >> { >> + blackLevel_ = 3840; >> gain_ = AnalogueGainExp{ 1.0, expGainDb(0.1) }; >> } >> }; >> @@ -634,6 +635,7 @@ class CameraSensorHelperImx415 : public CameraSensorHelper >> public: >> CameraSensorHelperImx415() >> { >> + blackLevel_ = 3840; > the bu0569 version has a different value here. (3200) > > We're not able to distinguish camera modules apart, so I guess using > the plain 415's value is correct > >> gain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) }; >> } >> }; >> @@ -649,6 +651,7 @@ class CameraSensorHelperImx477 : public CameraSensorHelper >> public: >> CameraSensorHelperImx477() >> { >> + blackLevel_ = 4096; >> gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 }; >> } >> }; >> @@ -779,6 +782,7 @@ class CameraSensorHelperOv5647 : public CameraSensorHelper >> public: >> CameraSensorHelperOv5647() >> { >> + blackLevel_ = 1024; >> gain_ = AnalogueGainLinear{ 1, 0, 0, 16 }; >> } >> }; >> @@ -822,6 +826,7 @@ class CameraSensorHelperOv64a40 : public CameraSensorHelper >> public: >> CameraSensorHelperOv64a40() >> { >> + blackLevel_ = 4096; >> gain_ = AnalogueGainLinear{ 1, 0, 0, 128 }; >> } > Compared the values with the pips and vc4 entries and they're all > good, thanks > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> }; >> -- >> 2.55.0 >>
(CC'ing Naush and David) On Fri, Jul 31, 2026 at 11:08:04AM +0200, Jacopo Mondi wrote: > Hi Robert, > > out of curiosity, why an RFC and not a proper patch ? > > On Wed, Jul 29, 2026 at 05:40:46PM +0200, Robert Mader wrote: > > These sensors have existing sensor helpers and black level values in the > > corresponding tuning files. Lift them up so they can be used by other > > corresponding tuning files "in the RaspberryPi IPA". > > > IPAs, most importantly the softISP. > > "Lift them up in libipa so they can also be used by other platforms" > > Not sure why softISP is "most importantly" here :) > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index cc16e25123d9..139e7dd646ee 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -595,6 +595,7 @@ class CameraSensorHelperImx296 : public CameraSensorHelper > > public: > > CameraSensorHelperImx296() > > { > > + blackLevel_ = 3840; > > gain_ = AnalogueGainExp{ 1.0, expGainDb(0.1) }; > > } > > }; > > @@ -634,6 +635,7 @@ class CameraSensorHelperImx415 : public CameraSensorHelper > > public: > > CameraSensorHelperImx415() > > { > > + blackLevel_ = 3840; > > the bu0569 version has a different value here. (3200) > > We're not able to distinguish camera modules apart, so I guess using > the plain 415's value is correct Looking at drivers/media/i2c/imx415.c: #define IMX415_BLKLEVEL CCI_REG16_LE(0x30e2) #define IMX415_BLKLEVEL_DEFAULT 50 It's a RAW10 sensor, and 50 << (16 - 10) = 3200. I think that's the value we should use. David, Naush, could this be a mistake in your imx415.yaml tuning files ? The rest looks good, I've validated against existing drivers and documentation. With this addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > gain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) }; > > } > > }; > > @@ -649,6 +651,7 @@ class CameraSensorHelperImx477 : public CameraSensorHelper > > public: > > CameraSensorHelperImx477() > > { > > + blackLevel_ = 4096; > > gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 }; > > } > > }; > > @@ -779,6 +782,7 @@ class CameraSensorHelperOv5647 : public CameraSensorHelper > > public: > > CameraSensorHelperOv5647() > > { > > + blackLevel_ = 1024; > > gain_ = AnalogueGainLinear{ 1, 0, 0, 16 }; > > } > > }; > > @@ -822,6 +826,7 @@ class CameraSensorHelperOv64a40 : public CameraSensorHelper > > public: > > CameraSensorHelperOv64a40() > > { > > + blackLevel_ = 4096; > > gain_ = AnalogueGainLinear{ 1, 0, 0, 128 }; > > } > > Compared the values with the pips and vc4 entries and they're all > good, thanks > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > };
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index cc16e25123d9..139e7dd646ee 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -595,6 +595,7 @@ class CameraSensorHelperImx296 : public CameraSensorHelper public: CameraSensorHelperImx296() { + blackLevel_ = 3840; gain_ = AnalogueGainExp{ 1.0, expGainDb(0.1) }; } }; @@ -634,6 +635,7 @@ class CameraSensorHelperImx415 : public CameraSensorHelper public: CameraSensorHelperImx415() { + blackLevel_ = 3840; gain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) }; } }; @@ -649,6 +651,7 @@ class CameraSensorHelperImx477 : public CameraSensorHelper public: CameraSensorHelperImx477() { + blackLevel_ = 4096; gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 }; } }; @@ -779,6 +782,7 @@ class CameraSensorHelperOv5647 : public CameraSensorHelper public: CameraSensorHelperOv5647() { + blackLevel_ = 1024; gain_ = AnalogueGainLinear{ 1, 0, 0, 16 }; } }; @@ -822,6 +826,7 @@ class CameraSensorHelperOv64a40 : public CameraSensorHelper public: CameraSensorHelperOv64a40() { + blackLevel_ = 4096; gain_ = AnalogueGainLinear{ 1, 0, 0, 128 }; } };
These sensors have existing sensor helpers and black level values in the corresponding tuning files. Lift them up so they can be used by other IPAs, most importantly the softISP. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 5 +++++ 1 file changed, 5 insertions(+)