Re: [PATCH v6 2/2] drm: panel: add support for the Renesas R63419 based dual-DSI video mode Display Panels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The MIPI DCS initialization sequence in `renesas_r63419_on()` is inverted, sending 'Set Display On' before 'Exit Sleep Mode'.
- [High] Missing device link between the panel and the secondary DSI host creates a Use-After-Free risk.
- [High] The panel is published to the DRM subsystem before its DSI device properties are initialized and attached.
- [Medium] Asymmetrical DRM panel callbacks: DCS ON logic is in `.prepare` while DCS OFF logic is in `.disable`.
--

commit 2dc8ee5a664e9e795b047b17cd7126c4bd9d27d8
Author: KancyJoe <kancy2333@outlook.com>

drm: panel: add support for the Renesas R63419 based dual-DSI video mode Display Panels
    
Implement support for the Renesas 63419 based dual-DSI video mode
Display Panels found in the Ayaneo gaming handled devices.

> diff --git a/drivers/gpu/drm/panel/panel-renesas-r63419.c b/drivers/gpu/drm/panel/panel-renesas-r63419.c
> new file mode 100644
> index 0000000000000..dba156f8427cd
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-renesas-r63419.c

[ ... ]

> +static int renesas_r63419_on(struct renesas_r63419_panel *ctx)
> +{
> +	struct mipi_dsi_multi_context dsi_ctx = { 0 };
> +
> +	dsi_link_switch(ctx, &dsi_ctx, 0);
> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +	dsi_link_switch(ctx, &dsi_ctx, 1);
> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 150);
> +
> +	dsi_link_switch(ctx, &dsi_ctx, 0);
> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +	dsi_link_switch(ctx, &dsi_ctx, 1);
> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 50);

[Severity: High]
Does this send the MIPI DCS commands in the correct order?

The MIPI DCS specification requires a display module to be taken out of sleep
mode (0x11 command) and allowed to wake up before the display on command
(0x29) is sent. 

Here the display on command is sent while the panel is still in sleep mode,
which typically results in the command being ignored by the display
controller.

[ ... ]

> +static const struct drm_panel_funcs renesas_r63419_panel_funcs = {
> +	.disable = renesas_r63419_disable,
> +	.prepare = renesas_r63419_prepare,
> +	.unprepare = renesas_r63419_unprepare,
> +	.get_modes = renesas_r63419_get_modes,
> +	.get_orientation = renesas_r63419_get_orientation,
> +};

[Severity: Medium]
Should the DCS ON/OFF logic be symmetrical in these callbacks?

The driver executes its DCS turn-on sequence inside .prepare via
renesas_r63419_on(), but assigns its DCS turn-off sequence to the .disable
callback.

If the DRM core aborts display initialization after .prepare but before
.enable would logically occur, it will call .unprepare. However, .unprepare
only powers off the regulators and skips the DCS OFF sequence.

[ ... ]

> +	/* Register the second DSI device */
> +	ctx->dsi[1] = devm_mipi_dsi_device_register_full(dev, dsi1_host, &info);
> +	if (IS_ERR(ctx->dsi[1]))
> +		return dev_err_probe(dev, PTR_ERR(ctx->dsi[1]),
> +				     "Failed to register second DSI device\n");

[Severity: High]
Is a device link needed between the panel and the secondary DSI host?

The driver registers a secondary DSI device on a remote host, but it does
not establish a device link (e.g., via device_link_add()) to enforce
consumer-supplier unbind ordering.

If the secondary DSI host is unbound independently, the panel driver will
not be unbound along with it, leaving ctx->dsi[1] pointing to a freed or
unregistered host. Subsequent DRM atomic operations would dereference this
dangling pointer.

> +
> +	ctx->dsi[0] = dsi;
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +
> +	/* Get panel orientation */
> +	ret = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get panel orientation\n");
> +
> +	ctx->panel.prepare_prev_first = true;
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get backlight\n");
> +
> +	ret = devm_drm_panel_add(dev, &ctx->panel);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add panel\n");
> +
> +	/* Configure and attach both DSI devices */
> +	for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) {
> +		ctx->dsi[i]->lanes = ctx->desc->lanes;
> +		ctx->dsi[i]->format = ctx->desc->format;
> +		ctx->dsi[i]->mode_flags = ctx->desc->mode_flags;
> +
> +		ret = devm_mipi_dsi_attach(dev, ctx->dsi[i]);

[Severity: High]
Could the panel be probed by the DRM subsystem before it is fully initialized?

devm_drm_panel_add() adds the panel to the global DRM panel list, making it
visible to other drivers. However, critical DSI device properties like
lanes and format are set and devm_mipi_dsi_attach() is executed after this
call.

A concurrent display controller probing the panel could find it and attempt
to send DCS commands before the DSI physical links are attached.

> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to attach DSI device %d\n", i);
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-topic-sm8650-ayaneo-pocket-s2-r63419-v6-0-16edddda9951@linaro.org?part=2