Re: [PATCH v2 3/4] drm/panel: visionox-vtdr6130: Modularize panel config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/panel: shared global state mutation in DSC config
- [Medium] drm/panel: ignores num_modes and drops all but first display mode
- [Low] drm/panel: unused bpc field in panel descriptor
--

commit 9e3a1ea8fd3c45699472995a794b6b527ce2a3ff
Author: Aaron Kling <webgeek1234@gmail.com>

drm/panel: visionox-vtdr6130: Modularize panel config
    
The vtdr6130 is a DDIC, a common chip that is used by multiple panels.
Prepare the driver for more panels.

> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> index b66d6c7128c75..278f5acc2d661 100644
> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
[ ... ]
> @@ -200,24 +213,48 @@ static int visionox_vtdr6130_unprepare(struct drm_panel *panel)
[ ... ]
> +static struct visionox_vtdr6130_desc visionox_vtdr6130_panel_desc = {
> +	.modes = visionox_vtdr6130_modes,
> +	.num_modes = ARRAY_SIZE(visionox_vtdr6130_modes),
> +	.bpc = 8,

[Severity: Low]
Does this code forget to propagate the bpc field? It looks like this field
is initialized here but never assigned to connector->display_info.bpc in the
visionox_vtdr6130_get_modes() callback.

> +	.lanes = 4,
> +	.format = MIPI_DSI_FMT_RGB888,
> +	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET |
> +		      MIPI_DSI_CLOCK_NON_CONTINUOUS,
> +	.init_sequence = visionox_vtdr6130_on,
> +	.dsc = {
> +		.dsc_version_major = 0x1,
> +		.dsc_version_minor = 0x2,
> +		.slice_height = 40,
> +		.slice_width = 540,
> +		.slice_count = 2,
> +		.bits_per_component = 8,
> +		.bits_per_pixel = 8 << 4,
> +		.block_pred_enable = true,
> +	},
>  };
>  
>  static int visionox_vtdr6130_get_modes(struct drm_panel *panel,
>  				       struct drm_connector *connector)
>  {
> +	struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel);
>  	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_duplicate(connector->dev, &visionox_vtdr6130_mode);
> +	mode = drm_mode_duplicate(connector->dev, ctx->desc->modes);

[Severity: Medium]
Will this silently drop display modes if the panel has more than one?
Calling drm_mode_duplicate() directly on ctx->desc->modes without a loop
ignores ctx->desc->num_modes, which means only the first mode in the array
gets registered.

>  	if (!mode)
>  		return -ENOMEM;
>  
[ ... ]
> @@ -321,17 +358,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi)
>  		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
>  				     "Failed to get reset-gpios\n");
>  
> +	ctx->desc = (struct visionox_vtdr6130_desc *)of_device_get_match_data(dev);

[Severity: High]
Does casting away const from of_device_get_match_data() create a risk of
global state corruption?

By embedding the dsc configuration in a static global struct and pointing
dsi->dsc to it:

	dsi->dsc = &ctx->desc->dsc;

it gets passed to the DSI host controller. Later, the
drm_dsc_compute_rc_parameters() function mutates dsi->dsc in-place.
Because it's a shared global struct, this can lead to data races and
corruption if multiple panels exist.

Should the dsc config be copied into per-instance data instead, and the
descriptor be made static const?

> +	if (!ctx->desc)
> +		return -ENODEV;
> +
>  	ctx->dsi = dsi;
>  	mipi_dsi_set_drvdata(dsi, ctx);
>  
> -	ctx->dsc.dsc_version_major = 0x1;
> -	ctx->dsc.dsc_version_minor = 0x2;
> -	ctx->dsc.slice_height = 40;
> -	ctx->dsc.slice_width = 540;
> -	ctx->dsc.slice_count = 2;
> -	ctx->dsc.bits_per_component = 8;
> -	ctx->dsc.bits_per_pixel = 8 << 4;
> -	ctx->dsc.block_pred_enable = true;
> -
> -	dsi->dsc = &ctx->dsc;
> -	dsi->lanes = 4;
> -	dsi->format = MIPI_DSI_FMT_RGB888;
> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET |
> -			  MIPI_DSI_CLOCK_NON_CONTINUOUS;
> +	dsi->dsc = &ctx->desc->dsc;
> +	dsi->lanes = ctx->desc->lanes;
> +	dsi->format = ctx->desc->format;
> +	dsi->mode_flags = ctx->desc->mode_flags;
>  	ctx->panel.prepare_prev_first = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-rp6-panel-v2-0-19c425e523de@gmail.com?part=3