[PATCH v6 00/12] media: microchip-isc: fixes and enhancements

[PATCH v6 00/12] media: microchip-isc: fixes and enhancements

[Balakrishnan Sambath]

Bug fixes and feature additions for the Microchip ISC/XISC driver.

Fixes:
 - SBGGR10 Bayer pattern was mapped incorrectly (red/blue swap)
 - WB register fields corrupted by sign extension
 - Race between histogram IRQ and stream stop
 - PM runtime reference leak in AWB work handler

Features:
 - Driver documentation
 - Gamma 1.8/2.4 preset curves
 - Hue/saturation controls for SAMA7G5
 - Grey World AWB with EMA smoothing

Split from v1 per review. Histogram statistics support is being sent
as a separate follow-up series.

Tested on SAMA7G5-EK with IMX219 (RAW10 Bayer capture, AWB, controls
verified across multiple pixel formats and resolutions up to 3264x2464).

Based on v6.19 (e9ec05addd1a).

v1: https://lore.kernel.org/linux-media/20251009155251.102472-1-balamanikandan.gunasundar@microchip.com/
v2: https://lore.kernel.org/linux-media/20260512154339.210444-1-balakrishnan.s@microchip.com/
v3: https://lore.kernel.org/linux-media/20260513071742.97263-1-balakrishnan.s@microchip.com/
v4: https://lore.kernel.org/linux-media/20260518-balki-isc-series1-v4-v4-0-97f189185b7e@microchip.com/
v5: https://lore.kernel.org/linux-media/20260527-microchip-isc-fixes-v5-0-b0ec1bb954be@microchip.com/

v6:
 - Picked up Eugen's Reviewed-by on the SBGGR10 fix
 - Rename isc_format_has_chroma() to isc_format_is_yuv() and
   isc_update_cbc_ctrl_activity() to isc_update_cbhs_ctrls()
 - Store the CBHS control pointers in struct isc_device and use them
   instead of v4l2_ctrl_find()
 - Drop a stale comment in isc_s_ctrl()
 - Fold gain_smooth[] back into gain[]
 - Drop DPC_DPCENABLE from the SAMA7G5 pipeline mask, no format sets it
 - Fix the DPC black level BPS comment
 - Fix the documented pipeline order (CSC before CBHS) and contrast
   default

v5:
 - Picked up Eugen's Reviewed-by on the PM runtime leak fix.
   The SBGGR10 fix is extended to SAMA5D2 in v5, so dropped his
   Reviewed-by from that patch pending re-review.
 - Fix SAMA7G5 pipeline mask: CBHS_ENABLE, not CBC_ENABLE
 - Per-platform gamma_default. v4 used 1 for both SoCs, which picks
   the wrong curve on SAMA5D2 (1/2.2 is at index 2 there, index 1
   on SAMA7G5)
 - Fix V4L2_CID_SATURATION range to 0..127 (Q4); update docs to match
 - Initialise hue/saturation at probe to avoid a grayscale first frame
 - Reset histogram stats and gain_smooth in isc_reset_awb_ctrls() so
   AWB does not consume stale state from a previous stream
 - Reword WB masking subject; expand commit bodies on the feature
   patches

v4:
 - Drop gamma LUT controls and CC matrix V4L2 controls patches (move
   to parameter buffer follow-up per Sakari's review)
 - Drop AWB enable pipeline reset patch (cleanup for the dropped
   controls, will return with the follow-up series)
 - Update documentation patch to remove references to dropped controls
 - Rebase on v6.19

v3:
 - Fix bisect failures (regmap declaration, gamma LUT macro ordering)
 - Fix Fixes: tag (use mainline commit, not staging)
 - Add Co-developed-by trailers for Balamanikandan Gunasundar

v2:
 - Split series (histogram stats moved to a separate follow-up series)
 - Reorder: bug fixes first, then features
 - Commit message cleanups
 - Rebase on v6.19-rc8

Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>

---
Changes in v6:
- Picked up Eugen's Reviewed-by on the SBGGR10 fix
- Rename isc_format_has_chroma() to isc_format_is_yuv() and
  isc_update_cbc_ctrl_activity() to isc_update_cbhs_ctrls()
- Store the CBHS control pointers in struct isc_device, use them
  instead of v4l2_ctrl_find()
- Drop a stale comment in isc_s_ctrl()
- Fold gain_smooth[] back into gain[]
- Drop DPC_DPCENABLE from the SAMA7G5 pipeline mask, no format sets it
- Fix the DPC black level BPS comment
- Fix the documented pipeline order (CSC before CBHS) and contrast default
- Link to v5: https://lore.kernel.org/r/20260527-microchip-isc-fixes-v5-0-b0ec1bb954be@microchip.com

---
Balakrishnan Sambath (12):
      media: microchip-isc: fix SBGGR10 Bayer pattern
      media: microchip-isc: fix WB offset and gain register field masking
      media: microchip-isc: fix race condition on stream stop
      media: microchip-isc: fix PM runtime leak in AWB work handler
      media: microchip-isc: add driver documentation
      media: microchip-isc: set SAM9X7 maximum resolution to 2560x1920
      media: microchip-isc: configure DPC and pipeline for SAMA7G5
      media: microchip-isc: add gamma 1.8 and 2.4 correction curves
      media: microchip-isc: add SAMA7G5 hue and saturation controls
      media: microchip-isc: use weighted averages for Grey World AWB
      media: microchip-isc: smooth AWB gains with EMA filter
      media: microchip-isc: scale DPC black level to sensor bit depth

 .../userspace-api/media/drivers/index.rst          |   1 +
 .../userspace-api/media/drivers/microchip-isc.rst  |  69 +++++
 MAINTAINERS                                        |   1 +
 .../media/platform/microchip/microchip-isc-base.c  | 319 ++++++++++++++++-----
 .../media/platform/microchip/microchip-isc-regs.h  |  11 +-
 drivers/media/platform/microchip/microchip-isc.h   |  12 +-
 .../platform/microchip/microchip-sama5d2-isc.c     |   9 +-
 .../platform/microchip/microchip-sama7g5-isc.c     | 105 +++++--
 8 files changed, 424 insertions(+), 103 deletions(-)
---
base-commit: 05f7e89ab9731565d8a62e3b5d1ec206485eeb0b
change-id: 20260525-microchip-isc-fixes-a2cdef7c3d6e

Best regards,
-- 
Balakrishnan Sambath <balakrishnan.s@microchip.com>

[PATCH v6 01/12] media: microchip-isc: fix SBGGR10 Bayer pattern

[Balakrishnan Sambath]

SBGGR10 was mapped to ISC_BAY_CFG_RGRG instead of ISC_BAY_CFG_BGBG,
causing red/blue channel swap.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
Reviewed-by: Eugen Hristev <ehristev@kernel.org>
---
 drivers/media/platform/microchip/microchip-sama5d2-isc.c | 2 +-
 drivers/media/platform/microchip/microchip-sama7g5-isc.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 66d3d7891991..0ddff1e7b0a0 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -147,7 +147,7 @@ static struct isc_format sama5d2_formats_list[] = {
 		.fourcc		= V4L2_PIX_FMT_SBGGR10,
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR10_1X10,
 		.pfe_cfg0_bps	= ISC_PFG_CFG0_BPS_TEN,
-		.cfa_baycfg	= ISC_BAY_CFG_RGRG,
+		.cfa_baycfg	= ISC_BAY_CFG_BGBG,
 	},
 	{
 		.fourcc		= V4L2_PIX_FMT_SGBRG10,
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index b0302dfc3278..ca23e8adecbd 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -156,7 +156,7 @@ static struct isc_format sama7g5_formats_list[] = {
 		.fourcc		= V4L2_PIX_FMT_SBGGR10,
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR10_1X10,
 		.pfe_cfg0_bps	= ISC_PFG_CFG0_BPS_TEN,
-		.cfa_baycfg	= ISC_BAY_CFG_RGRG,
+		.cfa_baycfg	= ISC_BAY_CFG_BGBG,
 	},
 	{
 		.fourcc		= V4L2_PIX_FMT_SGBRG10,

-- 
2.34.1

[PATCH v6 02/12] media: microchip-isc: fix WB offset and gain register field masking

[Balakrishnan Sambath]

ISC_WB_O_* and ISC_WB_G_* pack two 13-bit fields per register. Sign
extension from negative offsets corrupts the upper field. Mask both
fields to 13 bits before packing.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 .../media/platform/microchip/microchip-isc-base.c   | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index a7cdc743fda7..45b94f1e89d8 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -61,18 +61,23 @@ static inline void isc_update_awb_ctrls(struct isc_device *isc)
 
 	/* In here we set our actual hw pipeline config */
 
+	/*
+	 * Mask offset fields to 13 bits. Sign extension of negative s32
+	 * values would otherwise corrupt the adjacent field.
+	 */
 	regmap_write(isc->regmap, ISC_WB_O_RGR,
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_R])) |
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_GR]) << 16));
+		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
+		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
 	regmap_write(isc->regmap, ISC_WB_O_BGB,
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_B])) |
-		     ((ctrls->offset[ISC_HIS_CFG_MODE_GB]) << 16));
+		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
+		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
+	/* Gains are 13-bit unsigned fields [12:0] and [28:16] */
 	regmap_write(isc->regmap, ISC_WB_G_RGR,
-		     ctrls->gain[ISC_HIS_CFG_MODE_R] |
-		     (ctrls->gain[ISC_HIS_CFG_MODE_GR] << 16));
+		     (ctrls->gain[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
+		     ((ctrls->gain[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
 	regmap_write(isc->regmap, ISC_WB_G_BGB,
-		     ctrls->gain[ISC_HIS_CFG_MODE_B] |
-		     (ctrls->gain[ISC_HIS_CFG_MODE_GB] << 16));
+		     (ctrls->gain[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
+		     ((ctrls->gain[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
 }
 
 static inline void isc_reset_awb_ctrls(struct isc_device *isc)

-- 
2.34.1

Re: [PATCH v6 02/12] media: microchip-isc: fix WB offset and gain register field masking

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsafe hardware register access and data race in AWB control handler
--

commit d6eb051b7d19bbd8b11d5cebc7ab4ed5ebef07c8
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: fix WB offset and gain register field masking

ISC_WB_O_* and ISC_WB_G_* pack two 13-bit fields per register. Sign
extension from negative offsets corrupts the upper field. Mask both
fields to 13 bits before packing.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index a7cdc743fda71..45b94f1e89d86 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -61,18 +61,23 @@ static inline void isc_update_awb_ctrls(struct isc_device *isc)
>  
>  	/* In here we set our actual hw pipeline config */
>  
> +	/*
> +	 * Mask offset fields to 13 bits. Sign extension of negative s32
> +	 * values would otherwise corrupt the adjacent field.
> +	 */
>  	regmap_write(isc->regmap, ISC_WB_O_RGR,
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_R])) |
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_GR]) << 16));
> +		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
> +		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
>  	regmap_write(isc->regmap, ISC_WB_O_BGB,
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_B])) |
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_GB]) << 16));
> +		     ((u32)ctrls->offset[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
> +		     (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
> +	/* Gains are 13-bit unsigned fields [12:0] and [28:16] */
>  	regmap_write(isc->regmap, ISC_WB_G_RGR,
> -		     ctrls->gain[ISC_HIS_CFG_MODE_R] |
> -		     (ctrls->gain[ISC_HIS_CFG_MODE_GR] << 16));
> +		     (ctrls->gain[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
> +		     ((ctrls->gain[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
>  	regmap_write(isc->regmap, ISC_WB_G_BGB,
> -		     ctrls->gain[ISC_HIS_CFG_MODE_B] |
> -		     (ctrls->gain[ISC_HIS_CFG_MODE_GB] << 16));
> +		     (ctrls->gain[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
> +		     ((ctrls->gain[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at how this function
is called, can this cause a bus fault if the device is suspended?

When userspace modifies an AWB control, isc_s_awb_ctrl() is executed and
calls isc_update_awb_ctrls() unconditionally:

drivers/media/platform/microchip/microchip-isc-base.c:isc_s_awb_ctrl() {
        ...
        if (ctrl->cluster[ISC_CTRL_GB_OFF]->is_new)
                ctrls->offset[ISC_HIS_CFG_MODE_GB] = isc->gb_off_ctrl->val;

        isc_update_awb_ctrls(isc);

        mutex_lock(&isc->awb_mutex);
        ...
}

This happens outside of any vb2_is_streaming() check and without pm runtime
protection like pm_runtime_resume_and_get(), which means we might access
MMIO registers while the AHB clock is disabled.

Additionally, this call is made without holding the awb_lock. Does this
create a data race with isc_awb_work() that would allow concurrent register
updates to tear the hardware pipeline configuration?

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

[PATCH v6 03/12] media: microchip-isc: fix race condition on stream stop

[Balakrishnan Sambath]

Disable histogram and drain AWB work queue before releasing DMA
buffers to prevent use-after-free if histogram IRQ fires during
stream stop.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 45b94f1e89d8..b19c5a63b4bd 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -427,6 +427,14 @@ static void isc_stop_streaming(struct vb2_queue *vq)
 
 	mutex_unlock(&isc->awb_mutex);
 
+	/*
+	 * Disable the histogram so the ISR stops firing HISREQ, then drain
+	 * any work that was already queued before returning.  This must happen
+	 * after releasing awb_mutex because isc_awb_work also takes it.
+	 */
+	isc_set_histogram(isc, false);
+	cancel_work_sync(&isc->awb_work);
+
 	/* Disable DMA interrupt */
 	regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
 
@@ -1519,10 +1527,17 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
 		}
 		mutex_unlock(&isc->awb_mutex);
 
-		/* if we have autowhitebalance on, start histogram procedure */
+		/*
+		 * If AWB auto mode is requested and we are streaming RAW,
+		 * start the histogram procedure, but only if it is not
+		 * already running. Repeated enable requests would reset
+		 * hist_id, preventing the 4-channel Bayer cycle from
+		 * completing.
+		 */
 		if (ctrls->awb == ISC_WB_AUTO &&
 		    vb2_is_streaming(&isc->vb2_vidq) &&
-		    ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code))
+		    ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code) &&
+		    ctrls->hist_stat != HIST_ENABLED)
 			isc_set_histogram(isc, true);
 
 		/*

-- 
2.34.1

Re: [PATCH v6 03/12] media: microchip-isc: fix race condition on stream stop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-after-destroy in `isc_async_unbind()` due to incorrect teardown ordering.
- [High] Missing histogram disable and workqueue cancellation in the `isc_start_streaming` error path leads to a kernel panic.
- [High] Missing IRQ synchronization in `isc_stop_streaming()` leads to a synchronous external abort.
--

commit 9cf229b57997c74f4db26f1539c0b73c428dd501
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: fix race condition on stream stop

Disable histogram and drain AWB work queue before releasing DMA
buffers to prevent use-after-free if histogram IRQ fires during
stream stop.

Cc: stable@vger.kernel.org

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index 45b94f1e89d86..b19c5a63b4bdc 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -427,6 +427,14 @@ static void isc_stop_streaming(struct vb2_queue *vq)
>  
>  	mutex_unlock(&isc->awb_mutex);
>  
> +	/*
> +	 * Disable the histogram so the ISR stops firing HISREQ, then drain
> +	 * any work that was already queued before returning.  This must happen
> +	 * after releasing awb_mutex because isc_awb_work also takes it.
> +	 */
> +	isc_set_histogram(isc, false);
> +	cancel_work_sync(&isc->awb_work);
> +
>  	/* Disable DMA interrupt */
>  	regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
>  
>  	pm_runtime_put_sync(isc->dev);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that device power is
synchronously disabled here without waiting for in-flight IRQs to finish
executing.

Because the interrupt is registered via devm_request_irq() without a call to
synchronize_irq() here, microchip_isc_interrupt() may already be executing on
another CPU when the clocks are disabled.

The ISR performs a regmap_read() on ISC_INTSR, which will crash the system
with a bus fault/external abort when attempting to read the unclocked
memory-mapped registers. Could we add a call to synchronize_irq() before
shutting down power?

[ ... ]

While reviewing this file, I noticed a couple of other pre-existing issues:

drivers/media/platform/microchip/microchip-isc-base.c:isc_start_streaming() {
...
	ret = isc_configure(isc);
	if (unlikely(ret))
		goto err_configure;
...
err_configure:
	pm_runtime_put_sync(isc->dev);
...
}

[Severity: High]
This is a pre-existing issue, but the error path fails to call
isc_set_histogram(isc, false) and cancel_work_sync(), leaving the hardware
enabled and the workqueue vulnerable to accessing unpowered registers.

If isc_configure() succeeds (enabling the histogram hardware and its
ISC_INT_HISDONE interrupt) but a subsequent step times out, the error path
calls pm_runtime_put_sync() to power down the device clocks. It fails to
disable the histogram interrupt or cancel isc->awb_work.

The hardware can trigger the interrupt before or during shutdown, scheduling
the work. When isc_awb_work() runs, it bypasses the enabled check and calls
isc_hist_count(), which performs a regmap_bulk_read() on unpowered registers.
This causes a synchronous external abort (bus fault) and crashes the system.
Should this error path be updated to clean up the histogram state and
workqueue?


drivers/media/platform/microchip/microchip-isc-base.c:isc_async_unbind() {
...
	mutex_destroy(&isc->awb_mutex);
	cancel_work_sync(&isc->awb_work);
...
}

[Severity: High]
This is a pre-existing issue, but the mutex is incorrectly destroyed before
the asynchronous work using it is definitively flushed and cancelled.

The isc_awb_work handler fundamentally relies on isc->awb_mutex to serialize
its execution. If the work is running or scheduled when unbind is triggered,
the worker thread will attempt to lock a destroyed/poisoned mutex. Would it be
safer to call cancel_work_sync() before mutex_destroy()?

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

[PATCH v6 04/12] media: microchip-isc: fix PM runtime leak in AWB work handler

[Balakrishnan Sambath]

Early return when streaming stops skips pm_runtime_put_sync(),
leaking the reference and preventing runtime suspend.

Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
Cc: stable@vger.kernel.org
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
Reviewed-by: Eugen Hristev <ehristev@kernel.org>
---
 drivers/media/platform/microchip/microchip-isc-base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index b19c5a63b4bd..f61a5d5a3e04 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -1429,7 +1429,7 @@ static void isc_awb_work(struct work_struct *w)
 	/* streaming is not active anymore */
 	if (isc->stop) {
 		mutex_unlock(&isc->awb_mutex);
-		return;
+		goto out_pm_put;
 	}
 
 	isc_update_profile(isc);
@@ -1440,6 +1440,7 @@ static void isc_awb_work(struct work_struct *w)
 	if (ctrls->awb)
 		regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
 
+out_pm_put:
 	pm_runtime_put_sync(isc->dev);
 }
 

-- 
2.34.1

Re: [PATCH v6 04/12] media: microchip-isc: fix PM runtime leak in AWB work handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] In `isc_s_awb_ctrl()`, `isc_update_awb_ctrls()` and `isc_update_profile()` can access unclocked memory-mapped registers, causing an external abort.
- [High] In `isc_async_unbind()`, `mutex_destroy(&isc->awb_mutex)` is called before synchronously cancelling `awb_work`, leading to use-after-free or invalid lock operations.
- [Medium] The identical PM runtime leak fixed in microchip-isc-base.c still exists in the equivalent atmel-isc-base.c staging driver.
- [Medium] Data race on internal AWB state and unprotected hardware register updates in `isc_s_awb_ctrl`.
- [Low] Unconditional assignment of `dev_pm_ops` pointers generates dead code when PM is disabled.
--

commit a240fe3354d5d26bf1f38d9f8646ecfe72dfa0ae
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

    media: microchip-isc: fix PM runtime leak in AWB work handler
    
    Early return when streaming stops skips pm_runtime_put_sync(),
    leaking the reference and preventing runtime suspend.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index b19c5a63b4bdc..f61a5d5a3e045 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -1429,7 +1429,7 @@ static void isc_awb_work(struct work_struct *w)
>  	/* streaming is not active anymore */
>  	if (isc->stop) {
>  		mutex_unlock(&isc->awb_mutex);
> -		return;
> +		goto out_pm_put;
>  	}
>  
>  	isc_update_profile(isc);
> @@ -1440,6 +1440,7 @@ static void isc_awb_work(struct work_struct *w)
>  	if (ctrls->awb)
>  		regmap_write(regmap, ISC_CTRLEN, ISC_CTRL_HISREQ);
>  
> +out_pm_put:
>  	pm_runtime_put_sync(isc->dev);
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but does this identical PM runtime
leak still exist in the equivalent atmel-isc-base.c staging driver?

The staging driver (drivers/staging/media/deprecated/atmel/atmel-isc-base.c)
contains a copy of isc_awb_work() with the same early return bug:

drivers/staging/media/deprecated/atmel/atmel-isc-base.c:isc_awb_work() {
    ...
	if (isc->stop) {
		mutex_unlock(&isc->awb_mutex);
		return;
	}
    ...
}

Should the staging driver be patched as well to prevent a resource leak there?

[Severity: High]
This is a pre-existing issue, but can isc_s_awb_ctrl() and
isc_update_profile() access unclocked memory-mapped registers, causing an
external abort?

isc_s_awb_ctrl() executes independently of streaming state and without
pm_runtime protection. When called while the hardware is runtime suspended,
isc_update_awb_ctrls() will perform an MMIO regmap_write() to unclocked
hardware registers:

drivers/media/platform/microchip/microchip-isc-base.c:isc_s_awb_ctrl() {
    ...
    isc_update_awb_ctrls(isc);
    ...
}

On many ARM architectures, accessing unclocked registers causes an external
abort which immediately panics the kernel.

Furthermore, if called concurrently during teardown, isc_stop_streaming()
releases awb_mutex and drops the PM reference via pm_runtime_put_sync()
before returning. Since videobuf2 only clears q->streaming = 0 after
stop_streaming returns, vb2_is_streaming() can evaluate to true while the
hardware is runtime suspended.

This triggers an additional call to isc_update_profile() which polls
unclocked hardware indefinitely.

[Severity: High]
This isn't a bug introduced by this patch, but does isc_async_unbind() destroy
awb_mutex before synchronously cancelling the awb_work, leading to an invalid
lock operation or use-after-free?

Looking at isc_async_unbind():

drivers/media/platform/microchip/microchip-isc-base.c:isc_async_unbind() {
    ...
    mutex_destroy(&isc->awb_mutex);
    cancel_work_sync(&isc->awb_work);
    ...
}

cancel_work_sync() blocks until the pending isc_awb_work completes. While it
waits, the work handler executes and attempts to lock isc->awb_mutex, which
has already been destroyed. The identical bug appears to exist in the staging
driver's isc_async_unbind() as well.

[Severity: Medium]
This is a pre-existing issue, but does isc_s_awb_ctrl() have a data race and
unprotected hardware register updates?

The V4L2 ioctl handler isc_s_awb_ctrl() modifies internal AWB controls
(ctrls->gain, ctrls->offset, ctrls->awb) holding the V4L2 lock, while the work
handler isc_awb_work() (via isc_wb_update()) reads and modifies these exact
fields locklessly:

drivers/media/platform/microchip/microchip-isc-base.c:isc_wb_update() {
    ...
    ctrls->gain[c] = (3 * ctrls->gain[c] + gain) / 4;
    ...
}

In addition to corrupting the values, isc_s_awb_ctrl() unconditionally calls
isc_update_awb_ctrls() to write to hardware registers without holding
isc->awb_lock. The work handler explicitly documents that awb_lock is strictly
required to prevent the DMA Done IRQ from latching a partially updated state.

Can this omission result in incorrect white balance configurations taking effect
for a frame?

[Severity: Low]
This isn't a bug introduced by this patch, but are the dev_pm_ops structures
assigned unconditionally in the platform drivers, generating dead code when PM
is disabled?

In microchip-sama5d2-isc.c and microchip-sama7g5-isc.c:

drivers/media/platform/microchip/microchip-sama5d2-isc.c:microchip_isc_driver {
    ...
    .pm = &microchip_isc_dev_pm_ops,
    ...
}

This violates the PM subsystem rule which requires the pm_ptr() wrapper to
ensure the compiler can optimize away the structures when CONFIG_PM is
disabled. Without it, the callbacks and the structures generate dead code.

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

[PATCH v6 05/12] media: microchip-isc: add driver documentation

[Balakrishnan Sambath]

Document the driver topology, supported formats, controls, the AWB
algorithm, the gamma table layout, and the Microchip-specific custom
controls exposed via atmel-isc-media.h.

Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 .../userspace-api/media/drivers/index.rst          |  1 +
 .../userspace-api/media/drivers/microchip-isc.rst  | 69 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 3 files changed, 71 insertions(+)

diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
index 02967c9b18d6..65ef6ba3523e 100644
--- a/Documentation/userspace-api/media/drivers/index.rst
+++ b/Documentation/userspace-api/media/drivers/index.rst
@@ -34,6 +34,7 @@ For more details see the file COPYING in the source distribution of Linux.
 	imx-uapi
 	mali-c55
 	max2175
+	microchip-isc
 	npcm-video
 	omap3isp-uapi
 	thp7312
diff --git a/Documentation/userspace-api/media/drivers/microchip-isc.rst b/Documentation/userspace-api/media/drivers/microchip-isc.rst
new file mode 100644
index 000000000000..69c7672e122a
--- /dev/null
+++ b/Documentation/userspace-api/media/drivers/microchip-isc.rst
@@ -0,0 +1,69 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Microchip ISC/XISC Driver
+=========================
+
+The Image Sensor Controller (ISC) on SAMA5D2 and eXtended ISC (XISC) on
+SAMA7G5/SAM9X7 provide camera capture with hardware image processing.
+
+Supported Hardware
+------------------
+
+==========  ==========  ==============  ================  ===============
+SoC         Controller  Max Resolution  Interface         Hue/Saturation
+==========  ==========  ==============  ================  ===============
+SAMA5D2     ISC         2592x1944       12-bit parallel   No
+SAMA7G5     XISC        3264x2464       12-bit + CSI-2    Yes
+SAM9X7      XISC        2560x1920       12-bit + CSI-2    Yes
+==========  ==========  ==============  ================  ===============
+
+SAM9X7 shares the XISC pipeline with SAMA7G5 but has a smaller internal
+line buffer, limiting horizontal resolution to 2560 pixels.
+
+Controls
+--------
+
+Standard V4L2 controls:
+
+* ``V4L2_CID_BRIGHTNESS``: -1024..1023, default 0
+* ``V4L2_CID_CONTRAST``: -2048..2047. Default differs per SoC:
+  SAMA7G5/SAM9X7 use 16, SAMA5D2 uses 256.
+* ``V4L2_CID_GAMMA``: 0..2 selects a preset curve. Indices differ
+  per SoC: SAMA7G5/SAM9X7 use 0=1/2.4, 1=1/2.2 (default), 2=1/1.8;
+  SAMA5D2 uses 0=1/1.8, 1=1/2.0, 2=1/2.2 (default).
+* ``V4L2_CID_AUTO_WHITE_BALANCE``: Enable kernel Grey World AWB
+* ``V4L2_CID_DO_WHITE_BALANCE``: Trigger one-shot AWB
+
+SAMA7G5/SAM9X7 add:
+
+* ``V4L2_CID_HUE``: -180..180 degrees
+* ``V4L2_CID_SATURATION``: 0..127, default 16 (Q4 fixed-point, 16 = 1.0x)
+
+Custom controls (defined in ``atmel-isc-media.h``):
+
+* ``ISC_CID_R_GAIN``, ``ISC_CID_B_GAIN``, ``ISC_CID_GR_GAIN``,
+  ``ISC_CID_GB_GAIN``: WB gains, 0..8191, Q2.9 (512 = 1.0x)
+* ``ISC_CID_R_OFFSET``, ``ISC_CID_B_OFFSET``, ``ISC_CID_GR_OFFSET``,
+  ``ISC_CID_GB_OFFSET``: WB offsets, -4096..4095
+
+Pipeline
+--------
+
+Pipeline modules: DPC -> WB -> CFA -> CC -> GAM -> CSC -> CBHS/CBC -> SUB
+
+* DPC: Defective Pixel Correction (XISC only), black level subtraction
+  to sensor bit depth, green disparity correction
+* WB: White Balance gains/offsets
+* CFA: Color Filter Array interpolation (demosaic)
+* CC: Color Correction matrix
+* GAM: Gamma correction (preset)
+* CSC: Color Space Conversion (RGB to YCbCr)
+* CBHS: Contrast/Brightness/Hue/Saturation (XISC only), operates on YCbCr
+* CBC: Contrast/Brightness (ISC only), operates on YCbCr
+* SUB: Chroma subsampling (4:2:2, 4:2:0)
+
+Pipeline usage depends on input and output formats:
+
+* Raw Bayer input, RGB output: DPC, WB, CFA, CC, GAM
+* Raw Bayer input, YUV output: Full pipeline including CSC, CBHS/CBC, SUB
+* Non-RAW input (YUV/RGB sensor): Pipeline bypassed
diff --git a/MAINTAINERS b/MAINTAINERS
index e08767323763..d4aa7e86e2bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17057,6 +17057,7 @@ L:	linux-media@vger.kernel.org
 S:	Supported
 F:	Documentation/devicetree/bindings/media/atmel,isc.yaml
 F:	Documentation/devicetree/bindings/media/microchip,xisc.yaml
+F:	Documentation/userspace-api/media/drivers/microchip-isc.rst
 F:	drivers/media/platform/microchip/microchip-isc*
 F:	drivers/media/platform/microchip/microchip-sama*-isc*
 F:	drivers/staging/media/deprecated/atmel/atmel-isc*

-- 
2.34.1

Re: [PATCH v6 05/12] media: microchip-isc: add driver documentation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The newly added documentation incorrectly directs userspace developers to `atmel-isc-media.h`, which is not an exported UAPI header.
- [Low] Mismatch between documented and implemented minimum value for White Balance offset controls.
--

commit 71e9f4b43ec565af48fe0deff5792741daea5496
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: add driver documentation

This commit adds UAPI documentation for the Microchip ISC and XISC drivers.
It covers driver topology, supported formats, controls, the AWB algorithm,
the gamma table layout, and custom controls.

> diff --git a/Documentation/userspace-api/media/drivers/microchip-isc.rst b/Documentation/userspace-api/media/drivers/microchip-isc.rst
> new file mode 100644
> index 0000000000000..69c7672e122a9
> --- /dev/null
> +++ b/Documentation/userspace-api/media/drivers/microchip-isc.rst

[ ... ]

> +SAMA7G5/SAM9X7 add:
> +
> +* ``V4L2_CID_HUE``: -180..180 degrees
> +* ``V4L2_CID_SATURATION``: 0..127, default 16 (Q4 fixed-point, 16 = 1.0x)
> +
> +Custom controls (defined in ``atmel-isc-media.h``):

[Severity: Low]
Will this correctly guide userspace application developers?

The documentation points users to atmel-isc-media.h for custom control
definitions. However, because atmel-isc-media.h is located in include/linux/
rather than include/uapi/linux/, it is an internal kernel header and will
not be exported by make headers_install.

> +
> +* ``ISC_CID_R_GAIN``, ``ISC_CID_B_GAIN``, ``ISC_CID_GR_GAIN``,
> +  ``ISC_CID_GB_GAIN``: WB gains, 0..8191, Q2.9 (512 = 1.0x)
> +* ``ISC_CID_R_OFFSET``, ``ISC_CID_B_OFFSET``, ``ISC_CID_GR_OFFSET``,
> +  ``ISC_CID_GB_OFFSET``: WB offsets, -4096..4095

[Severity: Low]
Does the minimum value for the WB offset controls match the driver
implementation?

The documentation states the range is -4096 to 4095, but looking at
drivers/media/platform/microchip/microchip-isc-base.c, the ISC_CTRL_OFF
macro sets the .min value to -4095:

drivers/media/platform/microchip/microchip-isc-base.c:
#define ISC_CTRL_OFF(_id, _name) \
    ...
    .min = -4095, \
    ...

Could this be updated so the documentation matches the off-by-one difference
in the source?

> +
> +Pipeline
> +--------

[ ... ]

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

[PATCH v6 06/12] media: microchip-isc: set SAM9X7 maximum resolution to 2560x1920

[Balakrishnan Sambath]

SAM9X7 XISC uses the same image processing pipeline as SAMA7G5 but has
a smaller internal line buffer. The reduced RAM constrains the maximum
horizontal resolution to 2560 pixels (compared to 3264 on SAMA7G5),
resulting in a maximum capture resolution of 2560x1920.

Co-developed-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-sama7g5-isc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index ca23e8adecbd..4119cfe12cdf 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -55,6 +55,9 @@
 #define ISC_SAMA7G5_MAX_SUPPORT_WIDTH   3264
 #define ISC_SAMA7G5_MAX_SUPPORT_HEIGHT  2464
 
+#define ISC_SAM9X7_MAX_SUPPORT_WIDTH    2560
+#define ISC_SAM9X7_MAX_SUPPORT_HEIGHT   1920
+
 #define ISC_SAMA7G5_PIPELINE \
 	(WB_ENABLE | CFA_ENABLE | CC_ENABLE | GAM_ENABLES | CSC_ENABLE | \
 	CBC_ENABLE | SUB422_ENABLE | SUB420_ENABLE)
@@ -432,8 +435,13 @@ static int microchip_xisc_probe(struct platform_device *pdev)
 	isc->gamma_table = isc_sama7g5_gamma_table;
 	isc->gamma_max = 0;
 
-	isc->max_width = ISC_SAMA7G5_MAX_SUPPORT_WIDTH;
-	isc->max_height = ISC_SAMA7G5_MAX_SUPPORT_HEIGHT;
+	if (of_machine_is_compatible("microchip,sam9x7")) {
+		isc->max_width = ISC_SAM9X7_MAX_SUPPORT_WIDTH;
+		isc->max_height = ISC_SAM9X7_MAX_SUPPORT_HEIGHT;
+	} else {
+		isc->max_width = ISC_SAMA7G5_MAX_SUPPORT_WIDTH;
+		isc->max_height = ISC_SAMA7G5_MAX_SUPPORT_HEIGHT;
+	}
 
 	isc->config_dpc = isc_sama7g5_config_dpc;
 	isc->config_csc = isc_sama7g5_config_csc;

-- 
2.34.1

Re: [PATCH v6 06/12] media: microchip-isc: set SAM9X7 maximum resolution to 2560x1920

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The driver checks the machine root compatible string to infer device-specific hardware parameters instead of using a device match table.

Pre-existing issues:
- [High] The driver saves OF endpoint nodes into a persistent list without acquiring a reference, leading to a use-after-free and double-free during probe.
--

commit c91e2880e308dc3e7795189aa50b13e996251309
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: set SAM9X7 maximum resolution to 2560x1920

This patch updates the microchip-sama7g5-isc driver to support the SAM9X7
SoC. It reduces the maximum horizontal and vertical resolutions due to
smaller internal line buffer constraints on the SAM9X7 compared to the
SAMA7G5.

> diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> index ca23e8adecbdc..4119cfe12cdf2 100644
> --- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> @@ -432,8 +435,13 @@ static int microchip_xisc_probe(struct platform_device *pdev)
>  	isc->gamma_table = isc_sama7g5_gamma_table;
>  	isc->gamma_max = 0;
>  
> -	isc->max_width = ISC_SAMA7G5_MAX_SUPPORT_WIDTH;
> -	isc->max_height = ISC_SAMA7G5_MAX_SUPPORT_HEIGHT;
> +	if (of_machine_is_compatible("microchip,sam9x7")) {

[Severity: Medium]
Is checking the machine root compatible string the best approach here?

Typically, a device driver relies on its own device node's compatible
string to infer device-specific hardware parameters, rather than the root
machine's compatible string.

Could this be implemented by introducing a new compatible string for the
SAM9X7 XISC (e.g., microchip,sam9x7-isc) to the driver's of_match_table,
and associating the specific resolution limits using
of_device_get_match_data()?

> +		isc->max_width = ISC_SAM9X7_MAX_SUPPORT_WIDTH;
> +		isc->max_height = ISC_SAM9X7_MAX_SUPPORT_HEIGHT;
> +	} else {
> +		isc->max_width = ISC_SAMA7G5_MAX_SUPPORT_WIDTH;
> +		isc->max_height = ISC_SAMA7G5_MAX_SUPPORT_HEIGHT;
> +	}
>  
>  	isc->config_dpc = isc_sama7g5_config_dpc;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the probe
sequence, I noticed a potential reference counting issue with the endpoint
nodes.

In xisc_parse_dt(), the endpoint node is assigned without taking a
reference:

xisc_parse_dt() {
    ...
    for_each_endpoint_of_node(np, epn) {
        ...
        subdev_entity->epn = epn;
    ...
}

Since for_each_endpoint_of_node() drops the reference on the current epn
when it advances, subdev_entity->epn might point to a node whose
reference count has already been decremented.

Later in microchip_xisc_probe(), this endpoint is accessed and explicitly
freed:

microchip_xisc_probe() {
    ...
    list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
        struct fwnode_handle *fwnode =
            of_fwnode_handle(subdev_entity->epn);
        ...
        of_node_put(subdev_entity->epn);
    ...
}

Could this lead to a use-after-free when registering the async remote,
and a double-free when of_node_put() is called? Should the assignment
in xisc_parse_dt() use of_node_get(epn)?

A similar omission also appears to be present in the SAMA5D2 driver
variant in isc_parse_dt().

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

[PATCH v6 07/12] media: microchip-isc: configure DPC and pipeline for SAMA7G5

[Balakrishnan Sambath]

Enable DPC black level and green disparity correction for raw Bayer
to RGB conversion. Bypass the pipeline for raw Bayer output so
software ISPs (libcamera) receive unmodified sensor data.

Rename CBC_ENABLE to CBHS_ENABLE to match the SAMA7G5 block name;
SAMA5D2's CBC sub-block uses the same bitmap bit.

Co-developed-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/media/platform/microchip/microchip-isc-base.c    | 15 ++++++---------
 drivers/media/platform/microchip/microchip-isc.h         |  2 +-
 drivers/media/platform/microchip/microchip-sama5d2-isc.c |  2 +-
 drivers/media/platform/microchip/microchip-sama7g5-isc.c |  5 +++--
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index f61a5d5a3e04..ff920019fe37 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -800,7 +800,7 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
 		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
 			isc->try_config.bits_pipeline = CFA_ENABLE |
 				WB_ENABLE | GAM_ENABLES | DPC_BLCENABLE |
-				CC_ENABLE;
+				DPC_GDCENABLE | CC_ENABLE;
 		} else {
 			isc->try_config.bits_pipeline = 0x0;
 		}
@@ -810,7 +810,7 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
 		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
 			isc->try_config.bits_pipeline = CFA_ENABLE |
 				CSC_ENABLE | GAM_ENABLES | WB_ENABLE |
-				SUB420_ENABLE | SUB422_ENABLE | CBC_ENABLE |
+				SUB420_ENABLE | SUB422_ENABLE | CBHS_ENABLE |
 				DPC_BLCENABLE;
 		} else {
 			isc->try_config.bits_pipeline = 0x0;
@@ -821,7 +821,7 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
 		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
 			isc->try_config.bits_pipeline = CFA_ENABLE |
 				CSC_ENABLE | WB_ENABLE | GAM_ENABLES |
-				SUB422_ENABLE | CBC_ENABLE | DPC_BLCENABLE;
+				SUB422_ENABLE | CBHS_ENABLE | DPC_BLCENABLE;
 		} else {
 			isc->try_config.bits_pipeline = 0x0;
 		}
@@ -833,7 +833,7 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
 		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
 			isc->try_config.bits_pipeline = CFA_ENABLE |
 				CSC_ENABLE | WB_ENABLE | GAM_ENABLES |
-				SUB422_ENABLE | CBC_ENABLE | DPC_BLCENABLE;
+				SUB422_ENABLE | CBHS_ENABLE | DPC_BLCENABLE;
 		} else {
 			isc->try_config.bits_pipeline = 0x0;
 		}
@@ -844,16 +844,13 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
 		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
 			isc->try_config.bits_pipeline = CFA_ENABLE |
 				CSC_ENABLE | WB_ENABLE | GAM_ENABLES |
-				CBC_ENABLE | DPC_BLCENABLE;
+				CBHS_ENABLE | DPC_BLCENABLE;
 		} else {
 			isc->try_config.bits_pipeline = 0x0;
 		}
 		break;
 	default:
-		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code))
-			isc->try_config.bits_pipeline = WB_ENABLE | DPC_BLCENABLE;
-		else
-			isc->try_config.bits_pipeline = 0x0;
+		isc->try_config.bits_pipeline = 0x0;
 	}
 
 	/* Tune the pipeline to product specific */
diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
index ad4e98a1dd8f..a943b072f6be 100644
--- a/drivers/media/platform/microchip/microchip-isc.h
+++ b/drivers/media/platform/microchip/microchip-isc.h
@@ -88,7 +88,7 @@ struct isc_format {
 #define GAM_RENABLE	BIT(9)
 #define VHXS_ENABLE	BIT(10)
 #define CSC_ENABLE	BIT(11)
-#define CBC_ENABLE	BIT(12)
+#define CBHS_ENABLE	BIT(12)
 #define SUB422_ENABLE	BIT(13)
 #define SUB420_ENABLE	BIT(14)
 
diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 0ddff1e7b0a0..71609a93358a 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -54,7 +54,7 @@
 
 #define ISC_SAMA5D2_PIPELINE \
 	(WB_ENABLE | CFA_ENABLE | CC_ENABLE | GAM_ENABLES | CSC_ENABLE | \
-	CBC_ENABLE | SUB422_ENABLE | SUB420_ENABLE)
+	CBHS_ENABLE | SUB422_ENABLE | SUB420_ENABLE)
 
 /* This is a list of the formats that the ISC can *output* */
 static const struct isc_format sama5d2_controller_formats[] = {
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index 4119cfe12cdf..287fc76da479 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -59,8 +59,9 @@
 #define ISC_SAM9X7_MAX_SUPPORT_HEIGHT   1920
 
 #define ISC_SAMA7G5_PIPELINE \
-	(WB_ENABLE | CFA_ENABLE | CC_ENABLE | GAM_ENABLES | CSC_ENABLE | \
-	CBC_ENABLE | SUB422_ENABLE | SUB420_ENABLE)
+	(DPC_GDCENABLE | DPC_BLCENABLE | \
+	 WB_ENABLE | CFA_ENABLE | CC_ENABLE | GAM_ENABLES | CSC_ENABLE | \
+	 CBHS_ENABLE | SUB422_ENABLE | SUB420_ENABLE)
 
 /* This is a list of the formats that the ISC can *output* */
 static const struct isc_format sama7g5_controller_formats[] = {

-- 
2.34.1

Re: [PATCH v6 07/12] media: microchip-isc: configure DPC and pipeline for SAMA7G5

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Green Disparity Correction (DPC_GDCENABLE) is omitted for YUV and GREY output formats.
--

commit 4ad55e55f4026d00b689214d6dbbe7f59a62bf02
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: configure DPC and pipeline for SAMA7G5

Enable DPC black level and green disparity correction for raw Bayer
to RGB conversion. Bypass the pipeline for raw Bayer output so
software ISPs (libcamera) receive unmodified sensor data.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index f61a5d5a3e045..ff920019fe371 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -800,7 +800,7 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
>  		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
>  			isc->try_config.bits_pipeline = CFA_ENABLE |
>  				WB_ENABLE | GAM_ENABLES | DPC_BLCENABLE |
> -				CC_ENABLE;
> +				DPC_GDCENABLE | CC_ENABLE;
>  		} else {
>  			isc->try_config.bits_pipeline = 0x0;
>  		}

[ ... ]

> @@ -810,7 +810,7 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
>  		if (ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
>  			isc->try_config.bits_pipeline = CFA_ENABLE |
>  				CSC_ENABLE | GAM_ENABLES | WB_ENABLE |
> -				SUB420_ENABLE | SUB422_ENABLE | CBC_ENABLE |
> +				SUB420_ENABLE | SUB422_ENABLE | CBHS_ENABLE |
>  				DPC_BLCENABLE;

[Severity: Medium]
Should DPC_GDCENABLE also be included here and for the other YUV and GREY
formats in this switch statement?

Since these formats also originate from raw Bayer data and pass through the CFA
(Color Filter Array) block, would they also require the same pre-demosaicing
green disparity correction that was added for the RGB formats above?

>  		} else {
>  			isc->try_config.bits_pipeline = 0x0;
>  		}

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

[PATCH v6 08/12] media: microchip-isc: add gamma 1.8 and 2.4 correction curves

[Balakrishnan Sambath]

Display profiles for older macOS content (gamma 1.8) and HDR
pipelines (gamma 2.4) need curves not covered by the existing sRGB
2.2 default. Add the two extra curves to the SAMA7G5 table so
userspace can pick a curve matching the target display profile.

The two SoCs put gamma 1/2.2 at different indices in their tables
(SAMA5D2 at index 2, SAMA7G5 at index 1), so introduce a
gamma_default field on struct isc_device and let each platform set
it. The SAMA5D2 default of index 2 matches the historical behaviour.

Co-developed-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 .../media/platform/microchip/microchip-isc-base.c  |  2 +-
 drivers/media/platform/microchip/microchip-isc.h   |  1 +
 .../platform/microchip/microchip-sama5d2-isc.c     |  2 +
 .../platform/microchip/microchip-sama7g5-isc.c     | 56 ++++++++++++++++------
 4 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index ff920019fe37..04187127070d 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -1648,7 +1648,7 @@ static int isc_ctrl_init(struct isc_device *isc)
 
 	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -1024, 1023, 1, 0);
 	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, isc->gamma_max, 1,
-			  isc->gamma_max);
+			  isc->gamma_default);
 	isc->awb_ctrl = v4l2_ctrl_new_std(hdl, &isc_awb_ops,
 					  V4L2_CID_AUTO_WHITE_BALANCE,
 					  0, 1, 1, 1);
diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
index a943b072f6be..2282ef7dd596 100644
--- a/drivers/media/platform/microchip/microchip-isc.h
+++ b/drivers/media/platform/microchip/microchip-isc.h
@@ -342,6 +342,7 @@ struct isc_device {
 	/* pointer to the defined gamma table */
 	const u32	(*gamma_table)[GAMMA_ENTRIES];
 	u32		gamma_max;
+	u32		gamma_default;
 
 	u32		max_width;
 	u32		max_height;
diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 71609a93358a..9fa8413c74c7 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -442,6 +442,8 @@ static int microchip_isc_probe(struct platform_device *pdev)
 
 	isc->gamma_table = isc_sama5d2_gamma_table;
 	isc->gamma_max = 2;
+	/* Index 2 in the SAMA5D2 table is gamma 1/2.2 (sRGB). */
+	isc->gamma_default = 2;
 
 	isc->max_width = ISC_SAMA5D2_MAX_SUPPORT_WIDTH;
 	isc->max_height = ISC_SAMA5D2_MAX_SUPPORT_HEIGHT;
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index 287fc76da479..ac21fe1dade0 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -320,21 +320,47 @@ static void isc_sama7g5_adapt_pipeline(struct isc_device *isc)
 	isc->try_config.bits_pipeline &= ISC_SAMA7G5_PIPELINE;
 }
 
-/* Gamma table with gamma 1/2.2 */
+/* Gamma tables with gamma values 0.42, 0.45(Default), 0.56 */
 static const u32 isc_sama7g5_gamma_table[][GAMMA_ENTRIES] = {
-	/* index 0 --> gamma bipartite */
+	/* index 0 --> gamma bipartite 1/2.4(=0.42) */
 	{
-	      0x980,  0x4c0320,  0x650260,  0x7801e0,  0x8701a0,  0x940180,
-	   0xa00160,  0xab0120,  0xb40120,  0xbd0120,  0xc60100,  0xce0100,
-	   0xd600e0,  0xdd00e0,  0xe400e0,  0xeb00c0,  0xf100c0,  0xf700c0,
-	   0xfd00c0, 0x10300a0, 0x10800c0, 0x10e00a0, 0x11300a0, 0x11800a0,
-	  0x11d00a0, 0x12200a0, 0x12700a0, 0x12c0080, 0x13000a0, 0x1350080,
-	  0x13900a0, 0x13e0080, 0x1420076, 0x17d0062, 0x1ae0054, 0x1d8004a,
-	  0x1fd0044, 0x21f003e, 0x23e003a, 0x25b0036, 0x2760032, 0x28f0030,
-	  0x2a7002e, 0x2be002c, 0x2d4002c, 0x2ea0028, 0x2fe0028, 0x3120026,
-	  0x3250024, 0x3370024, 0x3490022, 0x35a0022, 0x36b0020, 0x37b0020,
-	  0x38b0020, 0x39b001e, 0x3aa001e, 0x3b9001c, 0x3c7001c, 0x3d5001c,
-	  0x3e3001c, 0x3f1001c, 0x3ff001a, 0x40c001a },
+	      0x940,  0x4b0310,  0x630250,  0x7601d0,  0x840190,  0x910170,
+	   0x9d0150,  0xa80110,  0xb10110,  0xba0110,  0xc300f0,  0xcb00f0,
+	   0xd300e0,  0xda00e0,  0xe100c0,  0xe800c0,  0xee00c0,  0xf400c0,
+	   0xfa00a0, 0x10000a0, 0x10500a0, 0x10b00a0, 0x11000a0, 0x11500a0,
+	  0x11a0080, 0x11f0080, 0x1240080, 0x1290080, 0x12e0080, 0x1330070,
+	  0x1380070, 0x13c0070, 0x1410070, 0x17a0060, 0x1aa0052, 0x1d40046,
+	  0x1f90042, 0x21b003c, 0x23a0038, 0x2570034, 0x2720030, 0x28b002e,
+	  0x2a3002c, 0x2ba002a, 0x2d0002a, 0x2e60028, 0x2fa0026, 0x30e0026,
+	  0x3210024, 0x3330022, 0x3450022, 0x3560020, 0x3670020, 0x3770020,
+	  0x387001e, 0x396001e, 0x3a5001c, 0x3b3001c, 0x3c1001c, 0x3cf001a,
+	  0x3dd001a, 0x3eb0018, 0x3f90018, 0x4070016 },
+	/* index 1 --> gamma bipartite 1/2.2(=0.45) */
+	{
+	     0x980,  0x4c0320,  0x650260,  0x7801e0,  0x8701a0,  0x940180,
+	  0xa00160,  0xab0120,  0xb40120,  0xbd0120,  0xc60100,  0xce0100,
+	  0xd600e0,  0xdd00e0,  0xe400e0,  0xeb00c0,  0xf100c0,  0xf700c0,
+	  0xfd00c0, 0x10300a0, 0x10800c0, 0x10e00a0, 0x11300a0, 0x11800a0,
+	 0x11d00a0, 0x12200a0, 0x12700a0, 0x12c0080, 0x13000a0, 0x1350080,
+	 0x13900a0, 0x13e0080, 0x1420076, 0x17d0062, 0x1ae0054, 0x1d8004a,
+	 0x1fd0044, 0x21f003e, 0x23e003a, 0x25b0036, 0x2760032, 0x28f0030,
+	 0x2a7002e, 0x2be002c, 0x2d4002c, 0x2ea0028, 0x2fe0028, 0x3120026,
+	 0x3250024, 0x3370024, 0x3490022, 0x35a0022, 0x36b0020, 0x37b0020,
+	 0x38b0020, 0x39b001e, 0x3aa001e, 0x3b9001c, 0x3c7001c, 0x3d5001c,
+	 0x3e3001c, 0x3f1001c, 0x3ff001a, 0x40c001a },
+	/* index 2 --> gamma bipartite 1/1.8(=0.56) */
+	{
+	      0xa62,  0x4f0350,  0x680280,  0x7e0200,  0x8d01c0,  0x9a01a0,
+	   0xa50180,  0xb00140,  0xb90140,  0xc20120,  0xcb0120,  0xd30100,
+	   0xdb0100,  0xe300e0,  0xea00e0,  0xf100e0,  0xf700c0,  0xfd00c0,
+	  0x10300c0, 0x10900a0, 0x10e00a0, 0x11400a0, 0x11900a0, 0x11e00a0,
+	  0x12300a0, 0x12800a0, 0x12d0080, 0x1320080, 0x1370080, 0x13c0080,
+	  0x1410080, 0x1460080, 0x14a0070, 0x1830060, 0x1b40052, 0x1df0048,
+	  0x2040042, 0x2250040, 0x2440038, 0x2600036, 0x27b0032, 0x2940030,
+	  0x2ac002e, 0x2c4002c, 0x2da002a, 0x2f0002a, 0x3050028, 0x3190026,
+	  0x32c0026, 0x33e0024, 0x3500024, 0x3610022, 0x3720020, 0x3820020,
+	  0x3920020, 0x3a2001e, 0x3b1001e, 0x3c0001c, 0x3ce001c, 0x3dc001c,
+	  0x3ea001a, 0x3f8001a, 0x4060018, 0x4130018 },
 };
 
 static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
@@ -434,7 +460,9 @@ static int microchip_xisc_probe(struct platform_device *pdev)
 	}
 
 	isc->gamma_table = isc_sama7g5_gamma_table;
-	isc->gamma_max = 0;
+	isc->gamma_max = 2;
+	/* Index 1 in the SAMA7G5 table is gamma 1/2.2 (sRGB). */
+	isc->gamma_default = 1;
 
 	if (of_machine_is_compatible("microchip,sam9x7")) {
 		isc->max_width = ISC_SAM9X7_MAX_SUPPORT_WIDTH;

-- 
2.34.1

[PATCH v6 09/12] media: microchip-isc: add SAMA7G5 hue and saturation controls

[Balakrishnan Sambath]

The CBHS (Contrast/Brightness/Hue/Saturation) block on SAMA7G5
operates in YCbCr space; expose hue and saturation as V4L2 controls.
Hue and saturation act on chroma, so they are active only when the
output format is YUV. The SAMA5D2 has only the CBC block (no
hue/saturation), so the controls are gated on a new has_cbhs flag.

Saturation uses the Q4 fixed-point range 0..127 with default 16
(1.0x) directly matching the CBHS_SAT register field. The control
state is initialised to neutral at probe so the first config_cbc()
write after streaming starts does not produce a grayscale image.

Co-developed-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 .../media/platform/microchip/microchip-isc-base.c  | 70 +++++++++++++++++++++-
 .../media/platform/microchip/microchip-isc-regs.h  | 11 ++--
 drivers/media/platform/microchip/microchip-isc.h   |  7 +++
 .../platform/microchip/microchip-sama5d2-isc.c     |  3 +-
 .../platform/microchip/microchip-sama7g5-isc.c     |  9 +--
 5 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 04187127070d..3a941757906a 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -859,6 +859,46 @@ static int isc_try_configure_pipeline(struct isc_device *isc)
 	return 0;
 }
 
+static bool isc_format_is_yuv(u32 fourcc)
+{
+	switch (fourcc) {
+	case V4L2_PIX_FMT_YUV420:
+	case V4L2_PIX_FMT_YUV422P:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:
+		return true;
+	default:
+		return false;
+	}
+}
+
+/*
+ * isc_update_cbhs_ctrls() - Activate/deactivate CBHS controls
+ *
+ * Called from isc_set_fmt(), isc_link_validate(), and isc_ctrl_init().
+ * At isc_ctrl_init() time isc->config.bits_pipeline is zero (no format
+ * has been negotiated yet), so all CBHS controls are initially marked
+ * inactive.  They become active once a format that includes CBHS in the
+ * pipeline is configured via VIDIOC_S_FMT or link validation.  Hue and
+ * saturation operate in YCbCr space, so they activate only when the
+ * output format is YUV.
+ */
+static void isc_update_cbhs_ctrls(struct isc_device *isc)
+{
+	bool cbhs_active = isc->config.bits_pipeline & CBHS_ENABLE;
+	bool chroma_active = cbhs_active && isc_format_is_yuv(isc->config.fourcc);
+
+	if (isc->brightness_ctrl)
+		v4l2_ctrl_activate(isc->brightness_ctrl, cbhs_active);
+	if (isc->contrast_ctrl)
+		v4l2_ctrl_activate(isc->contrast_ctrl, cbhs_active);
+	if (isc->hue_ctrl)
+		v4l2_ctrl_activate(isc->hue_ctrl, chroma_active);
+	if (isc->saturation_ctrl)
+		v4l2_ctrl_activate(isc->saturation_ctrl, chroma_active);
+}
+
 static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f)
 {
 	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
@@ -902,6 +942,7 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
 	/* make the try configuration active */
 	isc->config = isc->try_config;
 	isc->fmt = isc->try_fmt;
+	isc_update_cbhs_ctrls(isc);
 
 	dev_dbg(isc->dev, "ISC set_fmt to %.4s @%dx%d\n",
 		(char *)&f->fmt.pix.pixelformat,
@@ -989,6 +1030,7 @@ static int isc_link_validate(struct media_link *link)
 		return ret;
 
 	isc->config = isc->try_config;
+	isc_update_cbhs_ctrls(isc);
 
 	dev_dbg(isc->dev, "New ISC configuration in place\n");
 
@@ -1457,6 +1499,14 @@ static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_CONTRAST:
 		ctrls->contrast = ctrl->val & ISC_CBC_CONTRAST_MASK;
 		break;
+	case V4L2_CID_HUE:
+		if (isc->has_cbhs)
+			ctrls->hue = ctrl->val & ISC_CBHS_HUE_MASK;
+		break;
+	case V4L2_CID_SATURATION:
+		if (isc->has_cbhs)
+			ctrls->saturation = ctrl->val & ISC_CBHS_SAT_MASK;
+		break;
 	case V4L2_CID_GAMMA:
 		ctrls->gamma_index = ctrl->val;
 		break;
@@ -1646,7 +1696,24 @@ static int isc_ctrl_init(struct isc_device *isc)
 
 	ctrls->brightness = 0;
 
-	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -1024, 1023, 1, 0);
+	isc->brightness_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS,
+						 -1024, 1023, 1, 0);
+	if (isc->has_cbhs) {
+		/*
+		 * CBHS_HUE is a signed 9-bit value in degrees.
+		 * CBHS_SAT is Q4 unsigned 7-bit, 16 = 1.0x.
+		 * Initialize the kernel-side state to neutral here so the
+		 * first config_cbc() call after streaming starts does not
+		 * write zero (grayscale) to the hardware.
+		 */
+		ctrls->hue = 0;
+		ctrls->saturation = 16;
+		isc->hue_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HUE,
+						  -180, 180, 1, 0);
+		isc->saturation_ctrl = v4l2_ctrl_new_std(hdl, ops,
+							 V4L2_CID_SATURATION,
+							 0, 127, 1, 16);
+	}
 	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, isc->gamma_max, 1,
 			  isc->gamma_default);
 	isc->awb_ctrl = v4l2_ctrl_new_std(hdl, &isc_awb_ops,
@@ -1665,6 +1732,7 @@ static int isc_ctrl_init(struct isc_device *isc)
 	}
 
 	v4l2_ctrl_activate(isc->do_wb_ctrl, false);
+	isc_update_cbhs_ctrls(isc);
 
 	isc->r_gain_ctrl = v4l2_ctrl_new_custom(hdl, &isc_r_gain_ctrl, NULL);
 	isc->b_gain_ctrl = v4l2_ctrl_new_custom(hdl, &isc_b_gain_ctrl, NULL);
diff --git a/drivers/media/platform/microchip/microchip-isc-regs.h b/drivers/media/platform/microchip/microchip-isc-regs.h
index e77e1d9a1db8..7f5c2e50e74b 100644
--- a/drivers/media/platform/microchip/microchip-isc-regs.h
+++ b/drivers/media/platform/microchip/microchip-isc-regs.h
@@ -268,10 +268,13 @@
 #define ISC_CBC_CONTRAST	0x000003c0
 #define ISC_CBC_CONTRAST_MASK	GENMASK(11, 0)
 
-/* Hue Register */
-#define ISC_CBCHS_HUE	0x4e0
-/* Saturation Register */
-#define ISC_CBCHS_SAT	0x4e4
+/* Hue Register: signed 9-bit two's complement, covers -180 to +180 degrees */
+#define ISC_CBHS_HUE		0x4e0
+#define ISC_CBHS_HUE_MASK	GENMASK(8, 0)
+
+/* Saturation Register: unsigned Q4 fixed-point (1.0 = 16, V4L2 range 0..127) */
+#define ISC_CBHS_SAT		0x4e4
+#define ISC_CBHS_SAT_MASK	GENMASK(6, 0)
 
 /* Offset for SUB422 register specific to sama5d2 product */
 #define ISC_SAMA5D2_SUB422_OFFSET	0
diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
index 2282ef7dd596..1ecefe990f00 100644
--- a/drivers/media/platform/microchip/microchip-isc.h
+++ b/drivers/media/platform/microchip/microchip-isc.h
@@ -139,6 +139,8 @@ struct isc_ctrls {
 
 	u32 brightness;
 	u32 contrast;
+	u32 hue;
+	u32 saturation;
 	u8 gamma_index;
 #define ISC_WB_NONE	0
 #define ISC_WB_AUTO	1
@@ -336,6 +338,10 @@ struct isc_device {
 		struct v4l2_ctrl	*b_off_ctrl;
 		struct v4l2_ctrl	*gr_off_ctrl;
 		struct v4l2_ctrl	*gb_off_ctrl;
+		struct v4l2_ctrl	*brightness_ctrl;
+		struct v4l2_ctrl	*contrast_ctrl;
+		struct v4l2_ctrl	*hue_ctrl;
+		struct v4l2_ctrl	*saturation_ctrl;
 	};
 
 #define GAMMA_ENTRIES	64
@@ -343,6 +349,7 @@ struct isc_device {
 	const u32	(*gamma_table)[GAMMA_ENTRIES];
 	u32		gamma_max;
 	u32		gamma_default;
+	bool		has_cbhs;
 
 	u32		max_width;
 	u32		max_height;
diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 9fa8413c74c7..4b8279fba560 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -264,7 +264,8 @@ static void isc_sama5d2_config_ctrls(struct isc_device *isc,
 
 	ctrls->contrast = 256;
 
-	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -2048, 2047, 1, 256);
+	isc->contrast_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST,
+					       -2048, 2047, 1, 256);
 }
 
 static void isc_sama5d2_config_dpc(struct isc_device *isc)
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index ac21fe1dade0..e6ccdd465805 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -257,9 +257,8 @@ static void isc_sama7g5_config_cbc(struct isc_device *isc)
 	/* Configure what is set via v4l2 ctrls */
 	regmap_write(regmap, ISC_CBC_BRIGHT + isc->offsets.cbc, isc->ctrls.brightness);
 	regmap_write(regmap, ISC_CBC_CONTRAST + isc->offsets.cbc, isc->ctrls.contrast);
-	/* Configure Hue and Saturation as neutral midpoint */
-	regmap_write(regmap, ISC_CBCHS_HUE, 0);
-	regmap_write(regmap, ISC_CBCHS_SAT, (1 << 4));
+	regmap_write(regmap, ISC_CBHS_HUE, isc->ctrls.hue);
+	regmap_write(regmap, ISC_CBHS_SAT, isc->ctrls.saturation);
 }
 
 static void isc_sama7g5_config_cc(struct isc_device *isc)
@@ -283,7 +282,8 @@ static void isc_sama7g5_config_ctrls(struct isc_device *isc,
 
 	ctrls->contrast = 16;
 
-	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -2048, 2047, 1, 16);
+	isc->contrast_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST,
+					       -2048, 2047, 1, 16);
 }
 
 static void isc_sama7g5_config_dpc(struct isc_device *isc)
@@ -463,6 +463,7 @@ static int microchip_xisc_probe(struct platform_device *pdev)
 	isc->gamma_max = 2;
 	/* Index 1 in the SAMA7G5 table is gamma 1/2.2 (sRGB). */
 	isc->gamma_default = 1;
+	isc->has_cbhs = true;
 
 	if (of_machine_is_compatible("microchip,sam9x7")) {
 		isc->max_width = ISC_SAM9X7_MAX_SUPPORT_WIDTH;

-- 
2.34.1

Re: [PATCH v6 09/12] media: microchip-isc: add SAMA7G5 hue and saturation controls

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] V4L2 controls are improperly gated due to deferred pipeline configuration, causing incorrect active states and state desynchronization when inactive controls are modified.

Pre-existing issues:
- [High] `mutex_destroy(&isc->awb_mutex)` is called on an uninitialized or already destroyed mutex during async error and unbind paths.
--

commit a918c53cb3531019eb23300da7b6bcbdebef4df4
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: add SAMA7G5 hue and saturation controls

This commit exposes hue and saturation as V4L2 controls for the SAMA7G5
CBHS block. The controls are gated based on the has_cbhs flag and are
initialized to neutral values at probe.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -859,6 +859,46 @@ static int isc_try_configure_pipeline(struct isc_device *isc)

[ ... ]

> +static void isc_update_cbhs_ctrls(struct isc_device *isc)
> +{
> +	bool cbhs_active = isc->config.bits_pipeline & CBHS_ENABLE;
> +	bool chroma_active = cbhs_active && isc_format_is_yuv(isc->config.fourcc);
> +
> +	if (isc->brightness_ctrl)
> +		v4l2_ctrl_activate(isc->brightness_ctrl, cbhs_active);
> +	if (isc->contrast_ctrl)
> +		v4l2_ctrl_activate(isc->contrast_ctrl, cbhs_active);
> +	if (isc->hue_ctrl)
> +		v4l2_ctrl_activate(isc->hue_ctrl, chroma_active);
> +	if (isc->saturation_ctrl)
> +		v4l2_ctrl_activate(isc->saturation_ctrl, chroma_active);
> +}

[ ... ]

> @@ -902,6 +942,7 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
>  	/* make the try configuration active */
>  	isc->config = isc->try_config;
>  	isc->fmt = isc->try_fmt;
> +	isc_update_cbhs_ctrls(isc);

[Severity: Medium]
Does this leave the CBHS controls incorrectly deactivated before streaming
starts?

Because isc->config.bits_pipeline is only updated during link validation
(VIDIOC_STREAMON), it appears bits_pipeline will be zero or stale here
in isc_set_fmt().

If the controls remain incorrectly inactive, and userspace tries to set
them, isc_s_ctrl() might quietly reject the change:

drivers/media/platform/microchip/microchip-isc-base.c:isc_s_ctrl() {
    ...
    if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
        return 0;
    ...
}

Will this silently ignore the update while the V4L2 core caches the new
value, causing the driver's internal hardware state to permanently diverge
from the V4L2 framework's cached state?


[Severity: High]
This is a pre-existing issue, but can the async error handling path cause
a double-destroy or uninitialized-destroy panic?

If an early operation such as v4l2_device_register_subdev_nodes() fails
in isc_async_complete(), it returns an error before initializing
isc->awb_mutex. The V4L2 async core handles this failure by unbinding the
subdevices, which triggers isc_async_unbind().

isc_async_unbind() unconditionally calls:

drivers/media/platform/microchip/microchip-isc-base.c:isc_async_unbind() {
    ...
    mutex_destroy(&isc->awb_mutex);
    ...
}

Could this lead to a kernel oops on an uninitialized mutex if
CONFIG_DEBUG_MUTEXES is enabled? Additionally, if isc_async_complete()
fails after initialization, it explicitly destroys the mutex in its error
block, causing isc_async_unbind() to destroy it a second time.

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

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

[Balakrishnan Sambath]

Bright highlights dominate the unweighted pixel-count average and
bias grey-world estimates toward overexposed regions. Replace pixel
counts with intensity-weighted averages and add 2% outlier rejection
at the histogram tails so saturated highlights and the noise floor
do not dominate the gain calculation.

Also reset the new per-channel histogram statistics in
isc_reset_awb_ctrls() so a fresh streaming session does not feed
the first AWB cycle with stale values from a previous session.

Co-developed-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 .../media/platform/microchip/microchip-isc-base.c  | 180 +++++++++++++++------
 drivers/media/platform/microchip/microchip-isc.h   |   2 +
 2 files changed, 134 insertions(+), 48 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 3a941757906a..6ebbb8adea45 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -39,6 +39,12 @@
 	(((mbus_code) == MEDIA_BUS_FMT_Y10_1X10) | \
 	(((mbus_code) == MEDIA_BUS_FMT_Y8_1X8)))
 
+/* 4.0 in Q9 fixed-point: cap grey-world correction at 4x. */
+#define ISC_AWB_GW_GAIN_MAX	(4u << 9)
+
+/* Outlier rejection: skip darkest/brightest 2% of histogram. */
+#define ISC_AWB_OUTLIER_DIV	50
+
 static inline void isc_update_v4l2_ctrls(struct isc_device *isc)
 {
 	struct isc_ctrls *ctrls = &isc->ctrls;
@@ -82,14 +88,24 @@ static inline void isc_update_awb_ctrls(struct isc_device *isc)
 
 static inline void isc_reset_awb_ctrls(struct isc_device *isc)
 {
+	struct isc_ctrls *ctrls = &isc->ctrls;
 	unsigned int c;
 
 	for (c = ISC_HIS_CFG_MODE_GR; c <= ISC_HIS_CFG_MODE_B; c++) {
 		/* gains have a fixed point at 9 decimals */
-		isc->ctrls.gain[c] = 1 << 9;
+		ctrls->gain[c] = 1 << 9;
 		/* offsets are in 2's complements */
-		isc->ctrls.offset[c] = 0;
+		ctrls->offset[c] = 0;
 	}
+
+	/*
+	 * Reset histogram statistics so the first AWB cycle of a new
+	 * streaming session does not feed isc_wb_update with stale
+	 * values left over from a previous session.
+	 */
+	memset(ctrls->channel_avg, 0, sizeof(ctrls->channel_avg));
+	memset(ctrls->total_pixels, 0, sizeof(ctrls->total_pixels));
+	memset(ctrls->hist_minmax, 0, sizeof(ctrls->hist_minmax));
 }
 
 static int isc_queue_setup(struct vb2_queue *vq,
@@ -1276,6 +1292,11 @@ static void isc_hist_count(struct isc_device *isc, u32 *min, u32 *max)
 	u32 *hist_count = &ctrls->hist_count[ctrls->hist_id];
 	u32 *hist_entry = &ctrls->hist_entry[0];
 	u32 i;
+	u32 total_pixels;
+	u32 dark_threshold, bright_threshold;
+	u32 cumulative;
+	u64 weighted_sum;
+	u32 pixel_count;
 
 	*min = 0;
 	*max = HIST_ENTRIES;
@@ -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);
 
-	*hist_count = 0;
-	/*
-	 * we deliberately ignore the end of the histogram,
-	 * the most white pixels
-	 */
+	/* Calculate total pixels */
+	total_pixels = 0;
+	for (i = 0; i < HIST_ENTRIES; i++)
+		total_pixels += hist_entry[i];
+
+	/* Handle empty histogram case */
+	if (total_pixels == 0) {
+		*hist_count = 0;
+		ctrls->channel_avg[ctrls->hist_id] = 256; /* Default middle value */
+		ctrls->total_pixels[ctrls->hist_id] = 0;
+		*min = 1;
+		*max = HIST_ENTRIES - 1;
+		dev_dbg(isc->dev,
+			"isc wb: no pixels in histogram for channel %u\n",
+			ctrls->hist_id);
+		return;
+	}
+
+	/* Outlier rejection: skip darkest/brightest 2% of histogram */
+	dark_threshold = total_pixels / ISC_AWB_OUTLIER_DIV;
+	bright_threshold = total_pixels / ISC_AWB_OUTLIER_DIV;
+	cumulative = 0;
+
+	/* Find effective minimum (skip dark noise) */
+	*min = 1;
 	for (i = 1; i < HIST_ENTRIES; i++) {
-		if (*hist_entry && !*min)
+		cumulative += hist_entry[i];
+		if (cumulative > dark_threshold) {
 			*min = i;
-		if (*hist_entry)
+			break;
+		}
+	}
+
+	/* Find effective maximum (skip bright saturation) */
+	cumulative = 0;
+	*max = HIST_ENTRIES - 1;
+	for (i = HIST_ENTRIES - 1; i > *min; i--) {
+		cumulative += hist_entry[i];
+		if (cumulative > bright_threshold) {
 			*max = i;
-		*hist_count += i * (*hist_entry++);
+			break;
+		}
 	}
 
-	if (!*min)
-		*min = 1;
+	/* Ensure reasonable range */
+	if (*max <= *min) {
+		*min = HIST_ENTRIES / 4;
+		*max = (HIST_ENTRIES * 3) / 4;
+	}
+
+	/* 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;
+	}
 
-	dev_dbg(isc->dev, "isc wb: hist_id %u, hist_count %u",
-		ctrls->hist_id, *hist_count);
+	/* Store total useful pixels for this channel */
+	ctrls->total_pixels[ctrls->hist_id] = *hist_count;
+
+	if (*hist_count > 0)
+		ctrls->channel_avg[ctrls->hist_id] =
+			div64_u64(weighted_sum, *hist_count);
+	else
+		ctrls->channel_avg[ctrls->hist_id] = 256;
+
+	dev_dbg(isc->dev,
+		"isc wb: hist_id %u, avg %u, count %u, range [%u,%u], total %u\n",
+		ctrls->hist_id, ctrls->channel_avg[ctrls->hist_id],
+		*hist_count, *min, *max, total_pixels);
 }
 
 static void isc_wb_update(struct isc_ctrls *ctrls)
 {
 	struct isc_device *isc = container_of(ctrls, struct isc_device, ctrls);
-	u32 *hist_count = &ctrls->hist_count[0];
 	u32 c, offset[4];
 	u64 avg = 0;
-	/* We compute two gains, stretch gain and grey world gain */
-	u32 s_gain[4], gw_gain[4];
+	u32 gain, gw_gain, s_gain;
+	u32 min_pixels;
+	u32 frame_pixels;
 
 	/*
 	 * According to Grey World, we need to set gains for R/B to normalize
 	 * them towards the green channel.
-	 * Thus we want to keep Green as fixed and adjust only Red/Blue
-	 * Compute the average of the both green channels first
+	 * Thus we want to keep Green as fixed and adjust only Red/Blue.
+	 * Compute the average of the both green channels first.
 	 */
-	avg = (u64)hist_count[ISC_HIS_CFG_MODE_GR] +
-		(u64)hist_count[ISC_HIS_CFG_MODE_GB];
-	avg >>= 1;
+	avg = (ctrls->channel_avg[ISC_HIS_CFG_MODE_GR] +
+		ctrls->channel_avg[ISC_HIS_CFG_MODE_GB]) >> 1;
 
 	dev_dbg(isc->dev, "isc wb: green components average %llu\n", avg);
 
@@ -1328,7 +1403,23 @@ static void isc_wb_update(struct isc_ctrls *ctrls)
 	if (!avg)
 		return;
 
+	/*
+	 * 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
@@ -1354,40 +1445,33 @@ static void isc_wb_update(struct isc_ctrls *ctrls)
 		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;
 
-		dev_dbg(isc->dev,
-			"isc wb: component %d, s_gain %u, gw_gain %u\n",
-			c, s_gain[c], gw_gain[c]);
-		/* multiply both gains and adjust for decimals */
-		ctrls->gain[c] = s_gain[c] * gw_gain[c];
-		ctrls->gain[c] >>= 9;
+		/* Cap grey-world correction at 4x to avoid over-amplification. */
+		gw_gain = min_t(u32, gw_gain, ISC_AWB_GW_GAIN_MAX);
 
-		/* make sure we are not out of range */
-		ctrls->gain[c] = clamp_val(ctrls->gain[c], 0, GENMASK(12, 0));
+		/* Combine stretch and grey-world gains; result stays in Q9. */
+		gain = (s_gain * gw_gain) >> 9;
 
-		dev_dbg(isc->dev, "isc wb: component %d, final gain %u\n",
-			c, ctrls->gain[c]);
+		ctrls->gain[c] = clamp_val(gain, 0, GENMASK(12, 0));
+
+		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);
 	}
 }
 
diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
index 1ecefe990f00..d7241af3804f 100644
--- a/drivers/media/platform/microchip/microchip-isc.h
+++ b/drivers/media/platform/microchip/microchip-isc.h
@@ -158,6 +158,8 @@ struct isc_ctrls {
 #define HIST_MIN_INDEX		0
 #define HIST_MAX_INDEX		1
 	u32 hist_minmax[HIST_BAYER][2];
+	u32 channel_avg[HIST_BAYER];
+	u32 total_pixels[HIST_BAYER];
 };
 
 #define ISC_PIPE_LINE_NODE_NUM	15

-- 
2.34.1

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

[sashiko-bot]

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

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

[Balakrishnan Sambath]

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 | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index 6ebbb8adea45..621d90afe730 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -1467,11 +1467,20 @@ 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:
+		 *   gain = (3 * gain_old + gain_new) / 4   (alpha = 0.25)
+		 * Clamp to the hardware register width to prevent unbounded
+		 * accumulation under degenerate (near-empty histogram) inputs.
+		 */
+		ctrls->gain[c] = (3 * ctrls->gain[c] + gain) / 4;
+		ctrls->gain[c] = min_t(u32, ctrls->gain[c], GENMASK(12, 0));
 
 		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\n",
+			c, hist_min, channel_avg, s_gain, gw_gain,
+			ctrls->gain[c]);
 	}
 }
 

-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The raw calculated `gain` is not clamped before being fed into the exponential moving average (EMA) filter, causing severe and prolonged overexposure artifacts under degenerate conditions.
- [High] The unconditionally applied EMA filter breaks the 'Do White Balance' (ISC_WB_ONETIME) feature, requiring users to trigger it multiple times to reach the correct balance.
- [Medium] The exponential moving average (EMA) filter introduces a truncation bias because integer division truncates towards zero.
--

commit 8392fdc05942f24c94ecfeddbdfc0822a7cde3b3
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>
Date:   Wed Jun 3 06:58:54 2026 +0000

media: microchip-isc: smooth AWB gains with EMA filter

This patch applies an exponential moving average (alpha=0.25) to the automatic
white balance (AWB) gains in the microchip-isc driver. This aims to reduce
per-frame flicker caused by sensor noise.

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index 6ebbb8adea452..621d90afe730c 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -1467,11 +1467,20 @@ 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:
> +		 *   gain = (3 * gain_old + gain_new) / 4   (alpha = 0.25)
> +		 * Clamp to the hardware register width to prevent unbounded
> +		 * accumulation under degenerate (near-empty histogram) inputs.
> +		 */
> +		ctrls->gain[c] = (3 * ctrls->gain[c] + gain) / 4;

[Severity: High]
Does feeding the unbounded raw gain into the EMA filter before clamping cause
prolonged overexposure artifacts under degenerate conditions?

Under degenerate inputs (like a near-empty histogram where hist_max ==
hist_min), the raw calculated gain can reach up to 524,288. Because this
unbounded value is fed into the EMA filter before clamping, (3 * gain_old +
524288) / 4 evaluates to over 131,000, which is then clamped to the maximum
value 8,191.

When normal frames resume, the state is pinned at 8,191 and the EMA makes it
decay slowly, taking around 15 to 20 frames to return to normal values. Before
this patch, the unbounded gain was directly clamped, causing only a 1-frame
flash.

[Severity: High]
Could unconditionally applying the EMA filter here break the 'Do White Balance'
(ISC_WB_ONETIME) feature?

The ISC_WB_ONETIME feature performs exactly one 4-channel Bayer histogram cycle
to compute and apply white balance gains, then sets ctrls->awb = ISC_WB_NONE
and stops requesting histograms:

drivers/media/platform/microchip/microchip-isc-base.c:isc_awb_work() {
    ...
    if (ctrls->awb == ISC_WB_ONETIME) {
        dev_info(isc->dev,
                 "Completed one time white-balance adjustment.\n");
        /* update the v4l2 controls values */
        isc_update_v4l2_ctrls(isc);
        ctrls->awb = ISC_WB_NONE;
    }
    ...
}

By unconditionally applying the EMA filter here, this single adjustment cycle
only updates the gains by 25% of the required delta towards the target.

The one-time AWB fails to converge to the correct values, requiring users to
trigger the V4L2_CID_DO_WHITE_BALANCE ioctl multiple times to asymptotically
approach the correct balance.

[Severity: Medium]
Will this integer division introduce a truncation bias?

Because integer division truncates towards zero, when the target gain is
slightly above the current value (by 1 to 3 units), the fractional addition is
truncated and the value never increases. For example, if the current gain is
512 and the target is 515, (1536 + 515) / 4 = 512, so it never moves.

Conversely, downward trends track effortlessly. This creates a permanent
asymmetrical bias where the smoothed gain settles below the true mathematical
average. Symmetric rounding (3 * ctrls->gain[c] + gain + 2) / 4 might resolve
this.

> +		ctrls->gain[c] = min_t(u32, ctrls->gain[c], GENMASK(12, 0));
>  
>  		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\n",
> +			c, hist_min, channel_avg, s_gain, gw_gain,
> +			ctrls->gain[c]);
>  	}
>  }

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

[PATCH v6 12/12] media: microchip-isc: scale DPC black level to sensor bit depth

[Balakrishnan Sambath]

The DPC_BLCFG black level register expects counts in the sensor's
native bit depth. The previous fixed 10-bit value (64 counts) under-
corrects 12-bit sensors and over-corrects 8-bit ones, producing an
incorrect black point. Scale the nominal 10-bit value to match the
8/10/12-bit sensor bus width derived from pfe_cfg0_bps.

Co-developed-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 .../media/platform/microchip/microchip-sama7g5-isc.c  | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index e6ccdd465805..a57bc022aeb6 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -26,6 +26,7 @@
  * HIS: Histogram module performs statistic counters on the frames
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
@@ -290,9 +291,25 @@ static void isc_sama7g5_config_dpc(struct isc_device *isc)
 {
 	u32 bay_cfg = isc->config.sd_format->cfa_baycfg;
 	struct regmap *regmap = isc->regmap;
+	u32 bps, bloff;
+
+	/*
+	 * Scale the nominal 10-bit black level offset (64 counts) to the
+	 * actual sensor bus width.
+	 * ISC_PFE_CFG0_BPS encodes 12 - bit_depth in bits[30:28]:
+	 *   BPS_EIGHT  = 4  ->  8-bit  -> bloff = 64 >> 2 = 16
+	 *   BPS_TEN    = 2  -> 10-bit  -> bloff = 64
+	 *   BPS_TWELVE = 0  -> 12-bit  -> bloff = min(64 << 2, 255) = 255
+	 * The BLOFF hardware field is 8-bit so values are clamped to 255.
+	 */
+	bps = FIELD_GET(ISC_PFE_CFG0_BPS_MASK, isc->config.sd_format->pfe_cfg0_bps);
+	if (bps >= 2)
+		bloff = 64u >> (bps - 2);
+	else
+		bloff = min(64u << (2 - bps), 255u);
 
 	regmap_update_bits(regmap, ISC_DPC_CFG, ISC_DPC_CFG_BLOFF_MASK,
-			   (64 << ISC_DPC_CFG_BLOFF_SHIFT));
+			   (bloff << ISC_DPC_CFG_BLOFF_SHIFT));
 	regmap_update_bits(regmap, ISC_DPC_CFG, ISC_DPC_CFG_BAYCFG_MASK,
 			   (bay_cfg << ISC_DPC_CFG_BAYCFG_SHIFT));
 }

-- 
2.34.1