Re: [PATCH v4 1/6] media: synopsys: Fix IPI using hardcoded datatype

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] If userspace starts streaming without explicitly calling `VIDIOC_SUBDEV_S_FMT`, the driver will use an incorrect hardware datatype due to a mismatch between the probe-time pointer and the V4L2 default state.
- [High] Caching the active format in a device-global pointer (`csi2->formats`) during `set_fmt` circumvents the V4L2 subdev state API, causing state corruption and hardware misconfiguration when multiple streams are used.
- [Medium] The patch confusingly repurposes an array pointer (`csi2->formats`) into a single active state pointer, while retaining its paired array-bounds variable.
--

commit ac0f1e2d8d5a5aefaf04a50efcbfbc4c7693090d
Author: Guoniu Zhou <guoniu.zhou@oss.nxp.com>

media: synopsys: Fix IPI using hardcoded datatype

This commit updates the set_fmt callback to reflect the negotiated active
format by storing it in csi2->formats. This ensures the IPI configuration
matches the runtime datatype instead of the probe-time default.

> diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> index 02eb4a6cafade..0b80e84983f9e 100644
> --- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> +++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> @@ -311,7 +311,7 @@ dw_mipi_csi2rx_find_format(struct dw_mipi_csi2rx_device *csi2, u32 mbus_code)
>  	WARN_ON(csi2->formats_num == 0);
>  
>  	for (unsigned int i = 0; i < csi2->formats_num; i++) {
> -		const struct dw_mipi_csi2rx_format *format = &csi2->formats[i];
> +		const struct dw_mipi_csi2rx_format *format = &formats[i];

Does this repurpose the csi2->formats array pointer to store a single active
format, while leaving csi2->formats_num as the array length?

This decouples the pointer from its length variable. Iterating over a global
formats array instead of the instance fields might also bypass device
encapsulation and create a semantic mismatch.

[ ... ]

> @@ -470,6 +470,17 @@ static int dw_mipi_csi2rx_set_fmt(struct v4l2_subdev *sd,
>  
>  	*src = *sink;
>  
> +	/* Store the CSIS format descriptor for active formats. */
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +		csi2->formats = fmt ? :
> +			dw_mipi_csi2rx_find_format(csi2, default_format.code);

Does caching the active format in a device-global pointer (csi2->formats)
circumvent the V4L2 subdev state API?

If multiple streams are configured via V4L2_SUBDEV_FL_STREAMS, it appears this
single pointer would be repeatedly overwritten, storing only the format of the
last configured stream.

Could this create a regression when the streams are enabled? The hardware would
be programmed with this last format, which could cause issues for any streams
that negotiated a different format. Would it be better to retrieve the active
format dynamically from sd->active_state via v4l2_subdev_state_get_format()
during stream enablement?

Additionally, does this introduce a regression if userspace starts streaming
without explicitly calling VIDIOC_SUBDEV_S_FMT?

During probe, csi2->formats is initialized to the base of the formats array,
pointing to the first element (MEDIA_BUS_FMT_YUYV8_1X16). The V4L2 subdev
active state is initialized to default_format (MEDIA_BUS_FMT_SRGGB10_1X10).

If set_fmt(ACTIVE) is never called, csi2->formats is never updated. The
hardware would then be configured for YUV422_8B instead of the expected RAW10,
causing a mismatch between the active V4L2 state and the actual hardware
configuration.

> +		if (!csi2->formats) {
> +			dev_err(csi2->dev, "Failed to find valid format\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-csi2_imx95-v4-0-84ea4bb78a88@oss.nxp.com?part=1