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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The V4L2 crop rectangles for the newly added 6MP and 8MP modes exceed the static V4L2 crop bounds returned by `hi846_get_selection()` and contradict the hardware register configurations.
--

commit ba8b377e2d211a8774704df370bcbc25d095a60c
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Add 6MP and 8MP modes support

This patch adds 6MP and 8MP modes to maximize sensor utilization on the
hi846 sensor. These modes require 4-lane MIPI CSI-2 and use register
sequences extracted from the downstream Windows driver.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index c77b34a5e1662..12df1dc96163e 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_3264x2448_config[] = {
> +	{HI846_REG_MODE_SELECT,			0x0000},
> +	{HI846_REG_Y_ODD_INC_FOBP,		0x1111},
> +	{HI846_REG_Y_ODD_INC_VACT,		0x1111},
> +	{HI846_REG_Y_ADDR_START_VACT_H,		0x0040},
> +	{HI846_REG_Y_ADDR_END_VACT_H,		0x09cf},
> +	{HI846_REG_UNKNOWN_005C,		0x2101},
> +	{HI846_REG_FLL,				0x09de},
> +	{HI846_REG_LLP,				0x0ed8},
> +	{HI846_REG_BINNING_MODE,		0x0022},
> +	{HI846_REG_HBIN_MODE,			0x0000},
> +	{HI846_REG_UNKNOWN_0A24,		0x0000},
> +	{HI846_REG_X_START_H,			0x0000},

Does this register configuration contradict the crop configuration below? 

HI846_REG_X_START_H is configured to 0x0000, meaning the hardware reads
from column 0. However, the crop left offset is set to 0x46 (70). Exposing
a left crop offset to userspace while the hardware is programmed to read
from 0 seems contradictory.

> +	{HI846_REG_X_OUTPUT_SIZE_H,		0x0cc0},
> +	{HI846_REG_Y_OUTPUT_SIZE_H,		0x0990},

[ ... ]

> @@ -1128,9 +1228,59 @@ static const struct hi846_mode supported_modes[] = {
>  			.width = 1632 * 2,
>  			.height = 1224 * 2,
>  		},
> -	}
> +	},

[ ... ]

> +	{
> +		.width = 3264,
> +		.height = 2448,
> +		.pll_cfg_4lane = 0x4124, /* HI846_REG_PLL_CFG_MIPI2_H */
> +		.fps = 30,
> +		.frame_len = 2526,
> +		.llp = HI846_LINE_LENGTH,
> +		.reg_list_config = {
> +			.num_of_regs = ARRAY_SIZE(mode_3264x2448_config),
> +			.regs = mode_3264x2448_config,
> +		},
> +		.reg_list_2lane = {
> +			.num_of_regs = 0,
> +		},
> +		.reg_list_4lane = {
> +			.num_of_regs = ARRAY_SIZE(mode_3264x2448_mipi_4lane),
> +			.regs = mode_3264x2448_mipi_4lane,
> +		},
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x40,
> +			.width = 3264,
> +			.height = 2448,

Could this crop configuration break userspace boundary checks? 

In hi846_get_selection(), V4L2_SEL_TGT_CROP_BOUNDS unconditionally returns a 
hardcoded rectangle:

static int hi846_get_selection(...)
{
...
	case V4L2_SEL_TGT_CROP_BOUNDS:
	case V4L2_SEL_TGT_NATIVE_SIZE:
		sel->r.top = 0;
		sel->r.left = 0;
		sel->r.width = 3264;
		sel->r.height = 2448;
		return 0;
...
}

With the newly introduced 3264x2448 mode defining its active crop rectangle
with .left = 70 and .top = 64, the right boundary becomes 3334 and the bottom 
boundary becomes 2512. 

These values strictly exceed the crop bounds. According to the V4L2
specification, any valid crop rectangle must fit completely inside the crop
bounds. Returning a crop bound that is physically smaller than the allowed 
crop geometries breaks boundary checks in applications like libcamera.

> +		},
> +	},
>  };

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