Re: [PATCH v5 4/5] media: hi846: Add 6MP and 8MP modes support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional link frequency validation fails probe on 2-lane hardware for modes with dummy frequencies.
- [High] Dynamic link frequency calculation breaks backward compatibility with existing device trees.
- [Medium] The active crop rectangles for the new 6MP and 8MP modes exceed the sensor's native crop bounds.
- [Low] The patch author failed to address reviewer feedback requesting the use of separated fields for PLL configuration.

Pre-existing issues:
- [High] The default active mode lacks a 4-lane register configuration, leading to missing MIPI setup and streaming failures on 4-lane hardware.
--

--- Patch [4]: [PATCH v5 4/5] media: hi846: Add 6MP and 8MP modes support ---
commit 3cb5bb77cc3eceb92db3158280dae51e0e69e655
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Add 6MP and 8MP modes support
    
Hi846 is an 8MP sensor, but the upstream driver has only supported 2MP
mode for years. This patch adds 6MP and 8MP modes to maximize sensor
utilization.

Note that these modes require 4-lane MIPI CSI-2, as the downstream
driver only exposes 2MP, 6MP, and 8MP configurations in 4-lane
operation on the target device. The register sequences are extracted
from the downstream Windows driver.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index df7ea3ecf0d82..0fc27545eec8b 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1028,6 +1028,106 @@ static const struct hi846_reg mode_1632x1224_mipi_4lane[] = {
[ ... ]
> +static const struct hi846_reg mode_3264x1836_mipi_4lane[] = {
[ ... ]
> +	{HI846_REG_UNKNOWN_095A,		0x9240},
> +	{HI846_REG_PLL_CFG_MIPI2_H,		0x4124},

[Severity: Low]
Does this still use raw register values instead of separated fields?

The v5 changelog states: "Use separated fields instead of raw register values
for PLL cfg (Sakari)". 

However, this newly added array defines the PLL configuration using a raw 16-bit
payload (0x4124) rather than using field preparation macros (FIELD_PREP) or
separated fields.

Was this reviewer feedback missed?

> +	{HI846_REG_TG_ENABLE,			0x0100},
> +};
> +
[ ... ]
> +static const struct hi846_reg mode_3264x2448_mipi_4lane[] = {
[ ... ]
> +	{HI846_REG_UNKNOWN_095A,		0x9240},
> +	{HI846_REG_PLL_CFG_MIPI2_H,		0x4124},

[Severity: Low]
Does this still use raw register values instead of separated fields?

As mentioned above, the v5 changelog indicates this should use separated fields
rather than a raw 16-bit payload.

> +	{HI846_REG_TG_ENABLE,			0x0100},
> +};
> +
[ ... ]
> @@ -1136,7 +1236,59 @@ static const struct hi846_mode supported_modes[] = {
>  			.width = 1632 * 2,
>  			.height = 1224 * 2,
>  		},
> -	}

[Severity: High]
This isn't a bug introduced by this patch, but does the default active mode
lack a 4-lane register configuration, leading to missing MIPI setup?

During probe, the driver sets the default format in hi846->cur_mode to
&supported_modes[0] (640x480). This mode has an empty 4-lane MIPI configuration.

If a userspace application running on 4-lane hardware starts streaming without
explicitly setting a supported format via VIDIOC_S_FMT, hi846_start_streaming()
will attempt to apply the empty 4-lane register list. 

Will this silently leave the sensor's MIPI interface unconfigured, resulting in
capture hangs or deadlocks?

> +	},
> +	{
> +		.width = 3264,
> +		.height = 1836,
> +		.mipi_clk_div_2lane = 1, /* dummy */

[Severity: High]
Does this dummy frequency break probe on 2-lane hardware?

During probe, hi846_check_link_freqs() iterates over all elements in
supported_modes, calculating their link frequencies and verifying they exist
in the device tree endpoint.

Since this dummy frequency is not present in the 2-lane device tree's
link-frequencies property, will the check unconditionally fail and abort driver
probe, breaking 2-lane hardware configurations?

> +		.mipi_clk_div_4lane = 2,

[Severity: High]
Does this dynamic link frequency calculation break backward compatibility with
existing device trees?

Prior to this series, the driver hardcoded acceptable link frequencies (80MHz
and 200MHz), and existing device trees list these specific values in their
link-frequencies property. 

Since patch 2 ("media: hi846: Fix link frequency handling") replaced these
hardcoded values with a dynamic calculation based on mclk and lane count,
calculating it here yields 187.5MHz and 375MHz when driven by the previously
standard 25MHz external clock. 

Because hi846_check_link_freqs() requires the exact calculated frequencies to
exist in the device tree, will probe fail on existing platforms using the legacy
mandated values, violating Device Tree backward compatibility rules?

> +		.fps = 30,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x172,
> +			.width = 3264,
> +			.height = 1836,
> +		},

[Severity: Medium]
Do these active crop rectangles exceed the sensor's native crop bounds?

The V4L2 selection API strictly requires that any active crop rectangle must be
fully contained within the native bounds (V4L2_SEL_TGT_CROP_BOUNDS), which are
hardcoded to 3264x2448 in hi846_get_selection().

However, this crop rectangle is defined with a width of 3264 and a left offset
of 0x46 (70), positioning the rightmost edge at X = 3334, which exceeds the
3264 width limit.

Can this API violation cause v4l2-compliance test failures and confuse
userspace applications?

> +	},
> +	{
> +		.width = 3264,
> +		.height = 2448,
> +		.mipi_clk_div_2lane = 1, /* dummy */
> +		.mipi_clk_div_4lane = 2,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x40,
> +			.width = 3264,
> +			.height = 2448,
> +		},

[Severity: Medium]
Similarly, does this crop rectangle exceed the sensor's native bounds?

With a top offset of 0x40 and height of 2448, the bottom edge is at Y = 2512,
which exceeds the 2448 native height limit, in addition to exceeding the width
limit as seen in the previous mode.

> +	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712115012.91600-1-mitltlatltl@gmail.com?part=4