Re: [PATCH v6 10/12] media: microchip-isc: use weighted averages for Grey World AWB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new histogram loop incorrectly uses 1-based indexing, skipping bin 0 and corrupting AWB black-level offsets.
- [Medium] The minimum pixel count guard is not applied to the black-level offset and stretch gain calculations.
--

commit e4a1f6aa73ea293f51168aa52cc58f18a48000f7
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>
Subject: media: microchip-isc: use weighted averages for Grey World AWB

This commit replaces pixel-count averages with intensity-weighted averages for
Grey World AWB estimation to prevent overexposures from dominating the estimate.
It also resets per-channel histogram statistics in isc_reset_awb_ctrls() and
skips the darkest and brightest 2% of histogram entries as outliers.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index 3a941757906ae..6ebbb8adea452 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
[ ... ]
> @@ -1283,44 +1304,98 @@ static void isc_hist_count(struct isc_device *isc, u32 *min, u32 *max)
>  	regmap_bulk_read(regmap, ISC_HIS_ENTRY + isc->offsets.his_entry,
>  			 hist_entry, HIST_ENTRIES);
[ ... ]
> +	/* Find effective minimum (skip dark noise) */
> +	*min = 1;
>  	for (i = 1; i < HIST_ENTRIES; i++) {
> -		if (*hist_entry && !*min)
> +		cumulative += hist_entry[i];

[Severity: High]
Does this loop unintentionally skip hist_entry[0]?

Since the loop starts at index 1 and uses literal array indexing
(hist_entry[i]) instead of the previous pointer increment (*hist_entry++),
bin 0 appears to be completely omitted when finding the effective minimum.

> +		if (cumulative > dark_threshold) {
>  			*min = i;
> -		if (*hist_entry)
> +			break;
> +		}
> +	}
[ ... ]
> +	/* Calculate both pixel count and weighted average for useful range */
> +	*hist_count = 0;
> +	weighted_sum = 0;
> +
> +	for (i = *min; i <= *max; i++) {
> +		pixel_count = hist_entry[i];
> +		*hist_count += pixel_count;
> +		weighted_sum += (u64)i * pixel_count;
> +	}

[Severity: High]
Because *min is forced to be at least 1 above, this loop also skips bin 0
from *hist_count and ctrls->total_pixels.

For mostly dark scenes, could this report an artificially low total pixel
count, improperly triggering the min_pixels guard in isc_wb_update() and
erroneously disabling AWB gain?

>  static void isc_wb_update(struct isc_ctrls *ctrls)
>  {
[ ... ]
> +	/*
> +	 * Require a minimum pixel count for both black-level offset and
> +	 * grey-world gain: 1/64 of the frame area, which equals ~6.25% of
> +	 * one Bayer channel's expected pixel count.  This scales with sensor
> +	 * resolution and prevents noise-dominated histograms (from very small
> +	 * crops or a nearly-empty frame) from producing wild corrections.
> +	 * A floor of 64 ensures the guard is non-zero for tiny crops.
> +	 */
> +	frame_pixels = isc->fmt.fmt.pix.width * isc->fmt.fmt.pix.height;
> +	min_pixels = frame_pixels ? max(frame_pixels >> 6, 64u) : 64u;
> +
>  	for (c = ISC_HIS_CFG_MODE_GR; c <= ISC_HIS_CFG_MODE_B; c++) {
> +		u32 hist_min = ctrls->hist_minmax[c][HIST_MIN_INDEX];
> +		u32 hist_max = ctrls->hist_minmax[c][HIST_MAX_INDEX];
> +		u32 channel_avg = ctrls->channel_avg[c];
> +		u32 total_pixels = ctrls->total_pixels[c];
> +
>  		/*
> -		 * the color offset is the minimum value of the histogram.
> -		 * we stretch this color to the full range by substracting
> +		 * the color offset is the minimum value of the histogram.
> +		 * we stretch this color to the full range by substracting
>  		 * this value from the color component.
>  		 */
> -		offset[c] = ctrls->hist_minmax[c][HIST_MIN_INDEX];
> +		offset[c] = ctrls->hist_minmax[c][HIST_MIN_INDEX];
>  		/*
>  		 * The offset is always at least 1. If the offset is 1, we do
>  		 * not need to adjust it, so our result must be zero.
>  		 * the offset is computed in a histogram on 9 bits (0..512)
>  		 * but the offset in register is based on
>  		 * 12 bits pipeline (0..4096).
>  		 * we need to shift with the 3 bits that the histogram is
>  		 * ignoring
>  		 */
> -		ctrls->offset[c] = (offset[c] - 1) << 3;
> +		ctrls->offset[c] = (offset[c] - 1) << 3;

[Severity: High]
Since offset[c] (which is *min) is now a 0-based array index but the loop
above forced it to be >= 1, does subtracting 1 here corrupt the offset?

For example, if hist_entry[1] exceeds the threshold, *min is 1. The formula
(1 - 1) << 3 yields 0 instead of the expected 8 for bin 1, potentially
under-compensating the black level persistently.

>  		/*
>  		 * the offset is then taken and converted to 2's complements,
>  		 * and must be negative, as we subtract this value from the
>  		 * color components
>  		 */
>  		ctrls->offset[c] = -ctrls->offset[c];
>  
>  		/*
> -		 * the stretch gain is the total number of histogram bins
> -		 * divided by the actual range of color component (Max - Min)
> -		 * If we compute gain like this, the actual color component
> -		 * will be stretched to the full histogram.
> -		 * We need to shift 9 bits for precision, we have 9 bits for
> -		 * decimals
> +		 * Stretch gain: scale the histogram range [hist_min, hist_max]
> +		 * to the full 512-bin span.  Result is in Q9 fixed-point
> +		 * (1.0 = 512).
>  		 */
> -		s_gain[c] = (HIST_ENTRIES << 9) /
> -			(ctrls->hist_minmax[c][HIST_MAX_INDEX] -
> -			ctrls->hist_minmax[c][HIST_MIN_INDEX] + 1);
> +		s_gain = (HIST_ENTRIES << 9) / (hist_max - hist_min + 1);
>  
>  		/*
> -		 * Now we have to compute the gain w.r.t. the average.
> -		 * Add/lose gain to the component towards the average.
> -		 * If it happens that the component is zero, use the
> -		 * fixed point value : 1.0 gain.
> +		 * Grey-world gain: scale each channel towards the green
> +		 * average.  Require a minimum pixel count so noise-dominated
> +		 * channels do not produce wild corrections.
>  		 */
> -		if (hist_count[c])
> -			gw_gain[c] = div_u64(avg << 9, hist_count[c]);
> +		if (channel_avg > 0 && total_pixels >= min_pixels)
> +			gw_gain = div64_u64((avg << 9), channel_avg);
>  		else
> -			gw_gain[c] = 1 << 9;
> +			gw_gain = 1 << 9;

[Severity: Medium]
The comment earlier explicitly states the min_pixels guard is required for
"both black-level offset and grey-world gain", but the condition here is
only applied to gw_gain.

Because offset[c] and s_gain are computed unconditionally above, will nearly
empty histograms still compute s_gain and offset from noise bins? This seems
like it could apply wildly incorrect black-level offsets and up to a 16x
stretch multiplier to the noise, contradicting the intent of the guard.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-microchip-isc-fixes-v6-0-8c3d7474a768@microchip.com?part=10