Re: [PATCH v7 02/13] drm/mediatek: Implement Display Stream Compression support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] DSI and DSC component detection loop misses the terminal component of the display pipeline, preventing DSC configuration.
- [High] Incorrect execution order for DSC setup and missing state reset causes DSC hardware corruption on subsequent modesets or resume.
- [High] DSI link data rate, horizontal blanking timings, and clock lane word counts do not adjust for DSC compression, breaking timing synchronization.
- [High] The pixel stream word count incorrectly hardcodes an assumption of exactly 2 DSC slices per line.
- [High] DSI power-on accesses uninitialized DSC parameters by invoking `mtk_dsi_ps_control` before computing them.
- [Medium] The `dsi->dsc` pointer is set to NULL during panel unbind without serialization, leading to a Use-After-Free or NULL pointer dereference during modeset.
--

commit d2929d85efc73eb974427b3e2c2a0afbfbe66701
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: drm/mediatek: Implement Display Stream Compression support

This commit adds Display Stream Compression (DSC) support for the MediaTek
DRM driver. It introduces a new standalone mtk_disp_dsc driver and modifies
the existing CRTC and DSI drivers to configure and integrate DSC.

> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 97e3ff412e6ee..c0a53c153b8d5 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -398,6 +401,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc *mtk_crtc)
>  		if (!mtk_ddp_comp_add(mtk_crtc->ddp_comp[i], mtk_crtc->mutex))
>  			mtk_mutex_add_comp(mtk_crtc->mutex,
>  					   mtk_crtc->ddp_comp[i]->id);
> +
> +		/* For now, only single DSI is supported */
> +		if (mtk_crtc->ddp_comp[i]->id >= DDP_COMPONENT_DSI0 &&
> +		    mtk_crtc->ddp_comp[i]->id <= DDP_COMPONENT_DSI3)
> +			if (!comp_dsi)
> +				comp_dsi = mtk_crtc->ddp_comp[i];
> +
> +		if (mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC0 ||
> +		    mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC1)
> +			if (!comp_dsc)
> +				comp_dsc = mtk_crtc->ddp_comp[i];
>  	}

[Severity: High]
Will this loop successfully find the DSI component? 

The loop condition in mtk_crtc_ddp_hw_init iterates until
mtk_crtc->ddp_comp_nr - 1. Because the DSI encoder is always the terminal
component of the display pipeline at ddp_comp_nr - 1, the loop appears to
terminate before inspecting it.

Could this leave comp_dsi as NULL and skip the DSC configuration entirely?

> @@ -413,6 +427,13 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc *mtk_crtc)
>  		mtk_ddp_comp_start(comp);
>  	}
>  
> +	/* Setup the DSC if present, with the config coming from DSI */
> +	if (comp_dsc && comp_dsi) {
> +		dsc_cfg = mtk_dsi_get_dsc_config(comp_dsi->dev);
> +		if (dsc_cfg)
> +			mtk_ddp_comp_dsc_setup(comp_dsc, dsc_cfg);
> +	}

[Severity: High]
Is the hardware started before it is fully configured? 

The preceding loop calls mtk_ddp_comp_start on all components, which will
invoke mtk_dsc_start and write the DSC_EN bit to the control register.

If mtk_ddp_comp_dsc_setup is executed afterwards, could the hardware enter
an invalid state by being enabled before its parameters are applied?

> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
[ ... ]
> +void mtk_dsc_stop(struct device *dev)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
> +
> +	writel(0, disp_dsc->reg + DISP_REG_DSC_CON);
> +}

[Severity: High]
Should dsc_config_done be reset to false when stopping the hardware?

If mtk_dsc_stop writes 0 to the register but leaves dsc_config_done as
true, a subsequent resume or modeset might cause mtk_dsc_start to see
dsc_config_done == true. 

Would this skip the bypass mode fallback and write DSC_EN over a zeroed
configuration register, potentially hanging the display pipeline?

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index f82506e10fd59..97b8a91874f5b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> +static void mtk_dsi_ps_control_dsc(struct mtk_dsi *dsi, bool config_vact)
> +{
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
> +	const u16 *reg_main = dsi->driver_data->reg_main;
> +	const short dsi_buf_bpp = 3;
> +	u32 ps_wc;
> +
> +	/* Word count */
> +	ps_wc = FIELD_PREP(DSI_PS_WC, 2 * dsi->dsc->slice_chunk_size);

[Severity: High]
Can this hardcoded multiplier cause incorrect word counts on different panels?

Using 2 here assumes there are always exactly 2 DSC slices per line.

Should this use dsi->dsc->slice_count to ensure the payload bytes are
calculated correctly for displays with other slice layouts?

> +	if (config_vact) {
> +		writel(FIELD_PREP(VACT_NL, dsi->vm.vactive),
> +		       dsi->regs + reg_main[DSI_VACT_NL]);
> +		writel(ps_wc, dsi->regs + reg_main[DSI_HSTX_CKL_WC]);
> +	}

[Severity: High]
Does this blindly overwrite the clock lane word count register?

In mtk_dsi_config_vdo_timing_per_frame_lp, hstx_cklp_wc is carefully
calculated based on front porch, back porch, and trailing lengths, and then
written to DSI_HSTX_CKL_WC. When mtk_dsi_ps_control_dsc is subsequently
called, it overwrites that same register with ps_wc (the raw compressed
payload size). 

Additionally, the calculations for dsi->data_rate in mtk_dsi_poweron and
horizontal blanking timings use uncompressed pixel bit depths rather than
adjusting for DSC compression.

Will these issues combined break clock lane timing synchronization?

> @@ -741,10 +828,17 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  
>  	mtk_dsi_ps_control(dsi, true);
>  	mtk_dsi_set_vm_cmd(dsi);
> -	mtk_dsi_config_vdo_timing(dsi);
> +	ret = mtk_dsi_config_vdo_timing(dsi);

[Severity: High]
Could this sequence write uninitialized values to the hardware?

mtk_dsi_poweron calls mtk_dsi_ps_control, which reads
dsi->dsc->slice_chunk_size to program the DSI payload size registers.

However, dsi->dsc->slice_chunk_size is only populated when
mtk_dsi_config_vdo_timing eventually calls drm_dsc_compute_rc_parameters.

Does this imply the payload size registers are initially programmed with
zeros or uninitialized memory?

> @@ -1030,6 +1157,8 @@ static int mtk_dsi_host_detach(struct mipi_dsi_host *host,
>  
>  	component_del(host->dev, &mtk_dsi_component_ops);
>  	drm_bridge_remove(&dsi->bridge);
> +	dsi->dsc = NULL;
> +
>  	return 0;
>  }

[Severity: Medium]
Is it safe to clear dsi->dsc without modeset synchronization?

If a user unbinds the panel concurrently while a display modeset is active
(for example, while executing mtk_dsi_config_vdo_timing), could the pointer
become NULL immediately after the check for dsi->dsc? 

Could this lead to a NULL pointer dereference or use-after-free in
mtk_dsi_set_dsc_params?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914115812.22751-1-angelogioacchino.delregno@collabora.com?part=2