Re: [PATCH v5 3/5] media: hi846: Fix modes handling for different lane cases

[sashiko-bot@kernel.org]

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

New issues:
- [High] `hi846_check_link_freqs()` iterates over all modes without filtering them by the active MIPI lanes, demanding bogus link frequencies for unsupported configurations.

Pre-existing issues:
- [High] The sensor's default active mode is unconditionally initialized to an unsupported mode for 4-lane MIPI configurations during probe.
--

--- Patch [3]: [PATCH v5 3/5] media: hi846: Fix modes handling for different lane cases ---
commit 110c167f519b3b39914f2d9ef7ed2f39cf8cf0d1
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Fix modes handling for different lane cases

This patch fixes an issue where v4l2_find_nearest_size() could return
an unsupported 640x480 mode on 4-lane configurations. It introduces
filter_by_mipi_lanes() to filter modes based on MIPI lanes when finding
the nearest size and enumerating frame sizes.

Link: https://lore.kernel.org/linux-media/OmTXoHZJTSGePymL9I-1Cw@puri.sm

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 6bcd862aee301..df7ea3ecf0d82 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1688,6 +1688,16 @@ static int __maybe_unused hi846_resume(struct device *dev)
>  	return hi846_power_on(hi846);
>  }
>  
> +static bool filter_by_mipi_lanes(const void *array, size_t index,
> +				 const void *context)
> +{
> +	const struct hi846_mode *mode = array;
> +	const struct hi846 *hi846 = context;
> +
> +	return (hi846->nr_lanes == 2 && mode->reg_list_2lane.num_of_regs) ||
> +	       (hi846->nr_lanes == 4 && mode->reg_list_4lane.num_of_regs);
> +}
> +

[Severity: High]
Since we are filtering unsupported modes by lanes here, do we also need to
apply this filter in hi846_check_link_freqs()?

Currently, hi846_check_link_freqs() iterates over all modes without filtering
by active MIPI lanes:

drivers/media/i2c/hi846.c:hi846_check_link_freqs() {
    ...
	for (i = 0; i < freqs_count; i++) {
		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);
		for (j = 0; j < ep->nr_of_link_frequencies; j++)
    ...
}

This demands link frequencies for unsupported configurations. For a 4-lane
setup, it will calculate a link frequency for the unsupported 640x480 mode.
If this frequency is correctly omitted from the device tree's
link-frequencies property, the probe will fail with -EINVAL, making the
driver unusable on 4-lane hardware.

[Severity: High]
This is a pre-existing issue, but the default mode initialized in
hi846_probe() might be unsupported on 4-lane configurations:

drivers/media/i2c/hi846.c:hi846_probe() {
    ...
	hi846->cur_mode = &supported_modes[0];
    ...
}

The first mode in supported_modes is the 640x480 mode, which has
reg_list_4lane.num_of_regs = 0. When userspace starts streaming without
explicitly changing the format, hi846_start_streaming() will execute zero
MIPI configuration register writes for 4-lane setups:

drivers/media/i2c/hi846.c:hi846_start_streaming() {
    ...
	else
		ret = hi846_write_reg_list(hi846,
					   &hi846->cur_mode->reg_list_4lane);
    ...
}

This leaves the sensor in a non-functional state. Should the probe function
initialize cur_mode to a supported mode using the new filtering logic?

[ ... ]

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