| Message ID | 20210705160120.86313-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jean-Michel, Thank you for the patch. On Mon, Jul 05, 2021 at 06:01:20PM +0200, Jean-Michel Hautbois wrote: > Based on datasheet, the gain multiplier is 128 and not 256 (the > fraction bits are the 7 lower bits). Fix it. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Change-Id: I87e4b078f65c68da005a3ad4f1216256b8487df7 You know about this already :-) > --- > src/ipa/libipa/camera_sensor_helper.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 23335ed6..cd0a19bb 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -294,7 +294,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper > public: > CameraSensorHelperOv5670() > { > - analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; > + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 }; Interestingly, the OV5670 supports two different gain models at the hardware level, with a register bit to select which model to use. Bit 2 in register 0x3503 selects the "real gain format" (linear) when set to 0, and the "sensor gain format" (exponential) when set to 1. I really wonder why. I've had a look at the driver, and quite interestingly, it selects the linear gain model in all cases except for the 2560x1440 mode, where the exponential gain model is used. Register 0x3503 is set to 0x04 towards the beginning of the mode-specific register arrays for all modes, and then set to 0x00 towards the end, except for 2560x1440 where the 0x00 entry is missing. https://lore.kernel.org/linux-media/YOay2tIJxpBzYKzW@pendragon.ideasonboard.com/ I believe that's a driver bug, and this patch looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > }; > REGISTER_CAMERA_SENSOR_HELPER("ov5670", CameraSensorHelperOv5670)
Hi Laurent, On 08/07/2021 10:13, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Mon, Jul 05, 2021 at 06:01:20PM +0200, Jean-Michel Hautbois wrote: >> Based on datasheet, the gain multiplier is 128 and not 256 (the >> fraction bits are the 7 lower bits). Fix it. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> Change-Id: I87e4b078f65c68da005a3ad4f1216256b8487df7 > > You know about this already :-) Indeed :-). >> --- >> src/ipa/libipa/camera_sensor_helper.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp >> index 23335ed6..cd0a19bb 100644 >> --- a/src/ipa/libipa/camera_sensor_helper.cpp >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp >> @@ -294,7 +294,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper >> public: >> CameraSensorHelperOv5670() >> { >> - analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; >> + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 }; > > Interestingly, the OV5670 supports two different gain models at the > hardware level, with a register bit to select which model to use. Bit 2 > in register 0x3503 selects the "real gain format" (linear) when set to > 0, and the "sensor gain format" (exponential) when set to 1. I really > wonder why. > > I've had a look at the driver, and quite interestingly, it selects the > linear gain model in all cases except for the 2560x1440 mode, where the > exponential gain model is used. Register 0x3503 is set to 0x04 towards > the beginning of the mode-specific register arrays for all modes, and > then set to 0x00 towards the end, except for 2560x1440 where the 0x00 > entry is missing. > > https://lore.kernel.org/linux-media/YOay2tIJxpBzYKzW@pendragon.ideasonboard.com/ Nice catch ! I wonder if other OV* drivers may have the same issue... > I believe that's a driver bug, and this patch looks good to me. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> } >> }; >> REGISTER_CAMERA_SENSOR_HELPER("ov5670", CameraSensorHelperOv5670) >
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index 23335ed6..cd0a19bb 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -294,7 +294,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper public: CameraSensorHelperOv5670() { - analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 }; } }; REGISTER_CAMERA_SENSOR_HELPER("ov5670", CameraSensorHelperOv5670)
Based on datasheet, the gain multiplier is 128 and not 256 (the fraction bits are the 7 lower bits). Fix it. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Change-Id: I87e4b078f65c68da005a3ad4f1216256b8487df7 --- src/ipa/libipa/camera_sensor_helper.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)