Re: [PATCH v5 11/12] media: microchip-isc: smooth AWB gains with EMA filter

[<Balakrishnan.S@microchip.com>]

Hi Eugen,

On 29/05/26 1:50 am, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 5/27/26 14:07, Balakrishnan Sambath wrote:
>> Apply exponential moving average (alpha=0.25) to reduce per-frame
>> flicker from sensor noise.
>>
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/media/platform/microchip/microchip-isc-base.c | 19 ++++++++++++++++---
>>   drivers/media/platform/microchip/microchip-isc.h      |  1 +
>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
>> index a2719830d39b..d07ea2fa33c6 100644
>> --- a/drivers/media/platform/microchip/microchip-isc-base.c
>> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
>> @@ -94,6 +94,7 @@ static inline void isc_reset_awb_ctrls(struct isc_device *isc)
>>        for (c = ISC_HIS_CFG_MODE_GR; c <= ISC_HIS_CFG_MODE_B; c++) {
>>                /* gains have a fixed point at 9 decimals */
>>                ctrls->gain[c] = 1 << 9;
>> +             ctrls->gain_smooth[c] = 1 << 9;
>>                /* offsets are in 2's complements */
>>                ctrls->offset[c] = 0;
>>        }
>> @@ -1477,11 +1478,23 @@ static void isc_wb_update(struct isc_ctrls *ctrls)
>>                /* Combine stretch and grey-world gains; result stays in Q9. */
>>                gain = (s_gain * gw_gain) >> 9;
>>
>> -             ctrls->gain[c] = clamp_val(gain, 0, GENMASK(12, 0));
>> +             /*
>> +              * Smooth gain updates with an exponential weighted average
>> +              * to suppress per-frame flicker:
>> +              *   smooth[n] = (3 * smooth[n-1] + gain) / 4
>> +              * Clamp to the hardware register width to prevent unbounded
>> +              * accumulation under degenerate (near-empty histogram) inputs.
>> +              */
>> +             ctrls->gain_smooth[c] = (3 * ctrls->gain_smooth[c] + gain) / 4;
>> +             ctrls->gain_smooth[c] = min_t(u32, ctrls->gain_smooth[c],
>> +                                           GENMASK(12, 0));
>> +
>> +             ctrls->gain[c] = ctrls->gain_smooth[c];
> 
> If now 'gain' becomes 'gain_smooth' , what is the purpose of still
> having 'gain' at all ?
> Does it make sense to just recompute gain in the new way ?

I had kept gain_smooth[] separate so that isc_s_awb_ctrl, which
also writes ctrls->gain[], would not touch the EMA state.

Going back through the cluster setup, v4l2_ctrl_auto_cluster()
grabs the slave gain controls while AWB is in AUTO mode. The
v4l2 framework actually rejects user writes to those slaves before they
reach isc_s_awb_ctrl which I overlooked, so the case I had in mind 
cannot happen.

Thanks for the valuable insight.

Will collapse gain_smooth[] into gain[] in v6:

     ctrls->gain[c] = (3 * ctrls->gain[c] + gain) / 4;
     ctrls->gain[c] = min_t(u32, ctrls->gain[c], GENMASK(12, 0));

Best Regards,
Balakrishnan

> 
> 
>>
>>                dev_dbg(isc->dev,
>> -                     "isc wb: c=%u black=%u avg=%u s_gain=%u gw_gain=%u gain=%u",
>> -                     c, hist_min, channel_avg, s_gain, gw_gain, gain);
>> +                     "isc wb: c=%u black=%u avg=%u s_gain=%u gw_gain=%u gain=%u smooth=%u\n",
>> +                     c, hist_min, channel_avg, s_gain, gw_gain, gain,
>> +                     ctrls->gain_smooth[c]);
>>        }
>>   }
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
>> index 45168c62e3bc..0ae9b4e8f32d 100644
>> --- a/drivers/media/platform/microchip/microchip-isc.h
>> +++ b/drivers/media/platform/microchip/microchip-isc.h
>> @@ -149,6 +149,7 @@ struct isc_ctrls {
>>
>>        /* one for each component : GR, R, GB, B */
>>        u32 gain[HIST_BAYER];
>> +     u32 gain_smooth[HIST_BAYER];
>>        s32 offset[HIST_BAYER];
>>
>>        u32 hist_entry[HIST_ENTRIES];
>>
>