Re: [PATCH 2/2] drm/panel: Add BOE BF068MWM-TD0 panel driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] State corruption due to un-restored MIPI_DSI_MODE_LPM flag on error path
- [High] Uninitialized core DSC parameters result in malformed PPS payload
- [High] Concurrent unprotected bitwise modification of dsi->mode_flags
- [High] DCS commands sent to unpowered hardware due to missing state check
- [Medium] Resource leak on failure to setup DCS PPS or compression mode
- [Medium] Race condition exposing incomplete panel configuration during probe
- [Low] Missing Kconfig dependency for DRM_DISPLAY_DSC_HELPER
--

commit f349a000202648026372e12253353f6e49977419
Author: Alexander Koskovich <akoskovich@pm.me>
Subject: drm/panel: Add BOE BF068MWM-TD0 panel driver

Add the panel driver for BOE BF068MWM-TD0 support found in the Nothing
Phone (3a).

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index d592f4f4b939a..1e257657ef9ec 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
[ ... ]
> @@ -56,6 +56,17 @@ config DRM_PANEL_BOE_BF060Y8M_AJ0
>  	  uses 24 bit RGB per pixel. It provides a MIPI DSI interface to
>  	  the host and backlight is controlled through DSI commands.
>  
> +config DRM_PANEL_BOE_BF068MWM_TD0
> +	tristate "BOE BF068MWM-TD0 panel"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE

[Severity: Low]
Since the driver invokes drm_dsc_pps_payload_pack(), should this config
select DRM_DISPLAY_DSC_HELPER to prevent potential build failures when the
DSC helper subsystem isn't enabled?

> +	help
> +	  Say Y here if you want to enable support for BOE BF068MWM-TD0
> +	  6.77" AMOLED modules. The panel has a 1080x2392 resolution and
> +	  uses 30 bit RGB per pixel. It provides a MIPI DSI interface to
> +	  the host and backlight is controlled through DSI commands.
> +
>  config DRM_PANEL_BOE_HIMAX8279D
>  	tristate "Boe Himax8279d panel"
>  	depends on OF
> diff --git a/drivers/gpu/drm/panel/panel-boe-bf068mwm-td0.c b/drivers/gpu/drm/panel/panel-boe-bf068mwm-td0.c
> new file mode 100644
> index 0000000000000..bbf7aed196161
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-boe-bf068mwm-td0.c
[ ... ]
> +static int boe_bf068mwm_td0_prepare(struct drm_panel *panel)
> +{
> +	struct boe_bf068mwm_td0 *ctx = to_boe_bf068mwm_td0(panel);
> +	struct device *dev = &ctx->dsi->dev;
> +	struct drm_dsc_picture_parameter_set pps;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(boe_bf068mwm_td0_supplies), ctx->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	boe_bf068mwm_td0_reset(ctx);
> +
> +	ret = boe_bf068mwm_td0_on(ctx);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to initialize panel: %d\n", ret);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		regulator_bulk_disable(ARRAY_SIZE(boe_bf068mwm_td0_supplies), ctx->supplies);
> +		return ret;
> +	}
> +
> +	drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
> +
> +	ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_compression_mode(ctx->dsi, true);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable compression mode: %d\n", ret);
> +		return ret;
> +	}

[Severity: Medium]
Will this error path leave the regulators enabled and the reset gpio unasserted?

If mipi_dsi_picture_parameter_set() or mipi_dsi_compression_mode() fails, the
function returns early but does not seem to clean up the hardware resources
enabled earlier in the prepare routine, potentially leading to a resource leak
while leaving the hardware powered.

> +
> +	msleep(28);
> +
> +	return 0;
> +}
[ ... ]
> +static int boe_bf068mwm_td0_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;

[Severity: High]
Could this unprotected bitwise modification of dsi->mode_flags lead to data
races?

Since userspace can adjust backlight concurrently with display modeset
operations, altering these flags without synchronization might corrupt the
DSI mode state if read or written to by other threads.

> +
> +	ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);

[Severity: High]
Does this code send DSI commands to hardware that might be powered off?

The backlight update_status callback doesn't appear to check if the panel has
been prepared before sending DCS commands, which could result in bus timeouts
and controller errors if triggered via sysfs while unpowered.

[Severity: High]
If mipi_dsi_dcs_set_display_brightness_large() fails, will the
MIPI_DSI_MODE_LPM flag remain permanently cleared?

> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}
[ ... ]
> +static int boe_bf068mwm_td0_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct boe_bf068mwm_td0 *ctx;
> +	int ret;
[ ... ]
> +	drm_panel_add(&ctx->panel);
> +
> +	/* This panel only supports DSC; unconditionally enable it */
> +	dsi->dsc = &ctx->dsc;

[Severity: Medium]
Is it safe to register the panel via drm_panel_add() before the DSC
parameters are configured and before attaching to the DSI host?

This exposes the panel globally while it's still structurally incomplete,
potentially leading to a race condition if another driver attempts to resolve
the panel early.

> +
> +	ctx->dsc.dsc_version_major = 1;
> +	ctx->dsc.dsc_version_minor = 1;
> +	ctx->dsc.slice_height = 13;
> +	ctx->dsc.slice_width = 540;
> +
> +	ctx->dsc.slice_count = 1080 / ctx->dsc.slice_width;
> +	ctx->dsc.bits_per_component = 10;
> +	ctx->dsc.bits_per_pixel = 8 << 4; /* 4 fractional bits */
> +	ctx->dsc.block_pred_enable = true;

[Severity: High]
Will the absence of pic_width, pic_height, and rate control parameters here
result in a malformed PPS payload?

The drm_dsc_config struct is zero-initialized by devm_drm_panel_alloc().
Because these fields are omitted during probe, drm_dsc_pps_payload_pack()
will construct a zero-dimension PPS payload later, which may break display
compression.

> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		drm_panel_remove(&ctx->panel);
> +		return dev_err_probe(dev, ret, "Failed to attach to DSI host\n");
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-asteroids-panel-support-v1-0-109c6ac81c8f@pm.me?part=2