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 <[email protected]> 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/[email protected]?part=3
