[v4,27/49] ipa: libipa: agc: Use minimum line length

27700 diff mbox series

`IPACameraSensorInfo::minLineLength` already contains the minimum
line length, taking the output format and minimum horizontal blanking
into account.

Furthermore, the `CameraSensor` implementations already set the horizontal
blanking to the minimum when initializing the sensor.

And finally the line duration is already calculated with the minimum
line length.

So use the minimum for the frame duration calculations as well.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Hi Barnabás,

Quoting Barnabás Pőcze (2026-08-10 12:38:23)
> `IPACameraSensorInfo::minLineLength` already contains the minimum
> line length, taking the output format and minimum horizontal blanking
> into account.
> 
> Furthermore, the `CameraSensor` implementations already set the horizontal
> blanking to the minimum when initializing the sensor.
> 
> And finally the line duration is already calculated with the minimum
> line length.
> 
> So use the minimum for the frame duration calculations as well.

This is actually a big thing. It might be worth noting that this
effectively changes the FrameDurationLimits (especially the lower end).

I wonder why it was implemented that way in first place. Looking at the
original commit message it was stated that "...with the minimum reported
horizontal blanking..." but instead the default blanking was used for
the calculations. So either it was a bug in first place, then we should
maybe add

Fixes: 1d8fb3175936 ("ipa: rkisp1: add FrameDurationLimits control")

or it was done that way on purpose and we don't know the reason.

Given that the sensor is initialized with min hblank, I can't see how it
fails. Nevertheless I think we should move such a change to the end of
the series (after migrating the rkisp1) to be able to do easy a-b
testing in case we hit a unknown regression.

Anyhow:
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Best regards,
Stefan

> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/libipa/agc.cpp | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 5fcfc6a9ba..3c95071259 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -250,10 +250,11 @@ int AgcAlgorithm::init(const ValueNode &tuningData)
>  int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>                             const ConfigurationParams &config)
>  {
> +       const uint32_t lineLength = config.sensorInfo.minLineLength;
> +
>         session = {};
>         session.autoAllowed = config.autoAllowed;
> -       session.lineDuration =
> -               config.sensorInfo.minLineLength * 1.0s / config.sensorInfo.pixelRate;
> +       session.lineDuration = lineLength * 1.0s / config.sensorInfo.pixelRate;
>         session.sensor.outputSize = config.sensorInfo.outputSize;
>  
>         const double lineDurationUs = session.lineDuration.get<std::micro>();
> @@ -286,9 +287,6 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>          * The frame length is computed assuming a fixed line length combined
>          * with the vertical frame sizes.
>          */
> -       const ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;
> -       uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> -       uint32_t lineLength = config.sensorInfo.outputSize.width + hblank;
>  
>         const ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;
>         std::array<uint32_t, 3> frameHeights{
> -- 
> 2.55.0
>

2026. 08. 12. 7:30 keltezéssel, Stefan Klug írta:
> Hi Barnabás,
> 
> Quoting Barnabás Pőcze (2026-08-10 12:38:23)
>> `IPACameraSensorInfo::minLineLength` already contains the minimum
>> line length, taking the output format and minimum horizontal blanking
>> into account.
>>
>> Furthermore, the `CameraSensor` implementations already set the horizontal
>> blanking to the minimum when initializing the sensor.
>>
>> And finally the line duration is already calculated with the minimum
>> line length.
>>
>> So use the minimum for the frame duration calculations as well.
> 
> This is actually a big thing. It might be worth noting that this
> effectively changes the FrameDurationLimits (especially the lower end).
> 
> I wonder why it was implemented that way in first place. Looking at the
> original commit message it was stated that "...with the minimum reported
> horizontal blanking..." but instead the default blanking was used for
> the calculations. So either it was a bug in first place, then we should
> maybe add
> 
> Fixes: 1d8fb3175936 ("ipa: rkisp1: add FrameDurationLimits control")
> 
> or it was done that way on purpose and we don't know the reason.

Since `lineDuration` uses `IPACameraSensorInfo::minLineLength`, it seems
in any case more consistent to use that instead of querying the hblank
and doing the same calculation as `CameraSensor::sensorInfo()` but with
slightly different numbers.


> 
> Given that the sensor is initialized with min hblank, I can't see how it
> fails. Nevertheless I think we should move such a change to the end of
> the series (after migrating the rkisp1) to be able to do easy a-b
> testing in case we hit a unknown regression.

To be honest I'm not entirely happy with this, still. If the hblank control
is read-only, then it cannot be modified, so shouldn't `IPACameraSensorInfo`
provide the current line length and shouldn't that be used? (Or maybe v4l2
enforces that read-only controls have the same min/max/def/current, in
which case, there is no issue?)


> 
> Anyhow:
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> Best regards,
> Stefan
> 
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc.cpp | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
>> index 5fcfc6a9ba..3c95071259 100644
>> --- a/src/ipa/libipa/agc.cpp
>> +++ b/src/ipa/libipa/agc.cpp
>> @@ -250,10 +250,11 @@ int AgcAlgorithm::init(const ValueNode &tuningData)
>>   int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>                              const ConfigurationParams &config)
>>   {
>> +       const uint32_t lineLength = config.sensorInfo.minLineLength;
>> +
>>          session = {};
>>          session.autoAllowed = config.autoAllowed;
>> -       session.lineDuration =
>> -               config.sensorInfo.minLineLength * 1.0s / config.sensorInfo.pixelRate;
>> +       session.lineDuration = lineLength * 1.0s / config.sensorInfo.pixelRate;
>>          session.sensor.outputSize = config.sensorInfo.outputSize;
>>   
>>          const double lineDurationUs = session.lineDuration.get<std::micro>();
>> @@ -286,9 +287,6 @@ int AgcAlgorithm::configure(agc::Session &session, agc::ActiveState &state,
>>           * The frame length is computed assuming a fixed line length combined
>>           * with the vertical frame sizes.
>>           */
>> -       const ControlInfo &v4l2HBlank = config.sensorControls.find(V4L2_CID_HBLANK)->second;
>> -       uint32_t hblank = v4l2HBlank.def().get<int32_t>();
>> -       uint32_t lineLength = config.sensorInfo.outputSize.width + hblank;
>>   
>>          const ControlInfo &v4l2VBlank = config.sensorControls.find(V4L2_CID_VBLANK)->second;
>>          std::array<uint32_t, 3> frameHeights{
>> -- 
>> 2.55.0
>>