ipa: rpi: vd56g3: Fix frameIntegrationDiff value
diff mbox series

Message ID 20251028084650.13043-1-benjamin.mugnier@foss.st.com
State New
Headers show
Series
  • ipa: rpi: vd56g3: Fix frameIntegrationDiff value
Related show

Commit Message

Benjamin Mugnier Oct. 28, 2025, 8:46 a.m. UTC
In the vd56g3 user manual :

  MAX_EXPOSURE_COARSE = FRAME_LENGTH − EXP_COARSE_INTG_MARGIN − 7
  EXP_COARSE_INTG_MARGIN >= 68

Therefore, frameIntegrationDiff is EXP_COARSE_INTG_MARGIN + 7, equals
75. This value is coherent with the VD56G3_EXPOSURE_MARGIN in the kernel
driver source code.

Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jacopo Mondi Oct. 28, 2025, 8:58 a.m. UTC | #1
Hi Benjamin

On Tue, Oct 28, 2025 at 09:46:50AM +0100, Benjamin Mugnier wrote:
> In the vd56g3 user manual :
>
>   MAX_EXPOSURE_COARSE = FRAME_LENGTH − EXP_COARSE_INTG_MARGIN − 7
>   EXP_COARSE_INTG_MARGIN >= 68
>
> Therefore, frameIntegrationDiff is EXP_COARSE_INTG_MARGIN + 7, equals
> 75. This value is coherent with the VD56G3_EXPOSURE_MARGIN in the kernel
> driver source code.
>
> Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

Thanks for the fix

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> index 140aabd802ad..8a58bc746711 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> @@ -23,7 +23,7 @@ private:
>  	 * Smallest difference between the frame length and integration time,
>  	 * in units of lines.
>  	 */
> -	static constexpr int frameIntegrationDiff = 61;
> +	static constexpr int frameIntegrationDiff = 75;
>  };
>
>  CamHelperVd56g3::CamHelperVd56g3()
> --
> 2.25.1
>
Kieran Bingham Oct. 28, 2025, 11:04 a.m. UTC | #2
Quoting Benjamin Mugnier (2025-10-28 08:46:50)
> In the vd56g3 user manual :
> 
>   MAX_EXPOSURE_COARSE = FRAME_LENGTH − EXP_COARSE_INTG_MARGIN − 7
>   EXP_COARSE_INTG_MARGIN >= 68
> 
> Therefore, frameIntegrationDiff is EXP_COARSE_INTG_MARGIN + 7, equals
> 75. This value is coherent with the VD56G3_EXPOSURE_MARGIN in the kernel
> driver source code.
> 
> Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> index 140aabd802ad..8a58bc746711 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> @@ -23,7 +23,7 @@ private:
>          * Smallest difference between the frame length and integration time,
>          * in units of lines.
>          */
> -       static constexpr int frameIntegrationDiff = 61;
> +       static constexpr int frameIntegrationDiff = 75;
>  };
>  
>  CamHelperVd56g3::CamHelperVd56g3()
> -- 
> 2.25.1
>
Naushir Patuck Oct. 28, 2025, 3:03 p.m. UTC | #3
Hi Benjamin,

On Tue, 28 Oct 2025 at 08:47, Benjamin Mugnier
<benjamin.mugnier@foss.st.com> wrote:
>
> In the vd56g3 user manual :
>
>   MAX_EXPOSURE_COARSE = FRAME_LENGTH − EXP_COARSE_INTG_MARGIN − 7
>   EXP_COARSE_INTG_MARGIN >= 68
>
> Therefore, frameIntegrationDiff is EXP_COARSE_INTG_MARGIN + 7, equals
> 75. This value is coherent with the VD56G3_EXPOSURE_MARGIN in the kernel
> driver source code.
>
> Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

I'm a bit confused why the datasheet would not fold the 7 lines into
EXP_COARSE_INTG_MARGIN?
But since these numbers are correct according to the datasheet,

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> index 140aabd802ad..8a58bc746711 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
> @@ -23,7 +23,7 @@ private:
>          * Smallest difference between the frame length and integration time,
>          * in units of lines.
>          */
> -       static constexpr int frameIntegrationDiff = 61;
> +       static constexpr int frameIntegrationDiff = 75;
>  };
>
>  CamHelperVd56g3::CamHelperVd56g3()
> --
> 2.25.1
>
Benjamin Mugnier Oct. 28, 2025, 3:23 p.m. UTC | #4
Hi Naush,

On 10/28/25 16:03, Naushir Patuck wrote:
> Hi Benjamin,
> 
> On Tue, 28 Oct 2025 at 08:47, Benjamin Mugnier
> <benjamin.mugnier@foss.st.com> wrote:
>>
>> In the vd56g3 user manual :
>>
>>   MAX_EXPOSURE_COARSE = FRAME_LENGTH − EXP_COARSE_INTG_MARGIN − 7
>>   EXP_COARSE_INTG_MARGIN >= 68
>>
>> Therefore, frameIntegrationDiff is EXP_COARSE_INTG_MARGIN + 7, equals
>> 75. This value is coherent with the VD56G3_EXPOSURE_MARGIN in the kernel
>> driver source code.
>>
>> Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> 
> I'm a bit confused why the datasheet would not fold the 7 lines into
> EXP_COARSE_INTG_MARGIN?
> But since these numbers are correct according to the datasheet,

I had the same surprise. Documentation writers choose not to merge the 7
into EXP_COARSE_INTG_MARGIN because in the firmware the sensor is
effectively stopped for 7 lines, which differs from the integration time
per se.

Having the 7 defined behind a constant may help the reading
  MAX_EXPOSURE_COARSE = FRAME_LENGTH − EXP_COARSE_INTG_MARGIN −
SENSOR_STOP_LINES

With SENSOR_STOP_LINES = 7. But we're talking about very specific stuff.

> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> 
>> ---
>>  src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
>> index 140aabd802ad..8a58bc746711 100644
>> --- a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
>> +++ b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
>> @@ -23,7 +23,7 @@ private:
>>          * Smallest difference between the frame length and integration time,
>>          * in units of lines.
>>          */
>> -       static constexpr int frameIntegrationDiff = 61;
>> +       static constexpr int frameIntegrationDiff = 75;
>>  };
>>
>>  CamHelperVd56g3::CamHelperVd56g3()
>> --
>> 2.25.1
>>

Patch
diff mbox series

diff --git a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
index 140aabd802ad..8a58bc746711 100644
--- a/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
+++ b/src/ipa/rpi/cam_helper/cam_helper_vd56g3.cpp
@@ -23,7 +23,7 @@  private:
 	 * Smallest difference between the frame length and integration time,
 	 * in units of lines.
 	 */
-	static constexpr int frameIntegrationDiff = 61;
+	static constexpr int frameIntegrationDiff = 75;
 };
 
 CamHelperVd56g3::CamHelperVd56g3()