Re: [PATCH v6 02/11] drm/mediatek: Implement Display Stream Compression support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Loop iterating over DDP components skips the last element, silently bypassing DSC hardware initialization.
- [High] DSC hardware is configured after the display pipeline has already been started, and state is not reset on stop, risking pipeline hangs and state mismatches.
- [High] The DSI payload word count is hardcoded to exactly 2 DSC slices per line, breaking displays with different slice topologies.
- [High] CMDQ client registration failure breaks probe on non-CMDQ platforms, contradicting intended CPU fallback.
- [Medium] Calling `mtk_dsi_ps_control(dsi, true)` before computing DSC parameters results in configuring DSI registers with an uninitialized `slice_chunk_size`.
- [Medium] Missing initialization of `pic_width` and `pic_height` in the DSC configuration before calculating RC parameters.

Pre-existing issues:
- [Critical] Failure during DSC parameter setup inside `mtk_dsi_poweron` causes clocks to be disabled, leading to a kernel panic from unclocked register accesses in `mtk_dsi_start`.
--

--- Patch [2]: [PATCH v6 02/11] drm/mediatek: Implement Display Stream Compression support ---
commit 4b7d7243b726f6a21138b6126227ad71271efb1e
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: drm/mediatek: Implement Display Stream Compression support

Add a real driver for the Display Stream Compression (DSC) Display
Controller IP, implementing support for DSC v1.1 to v1.2. This also
introduces DSC handling in the mtk_dsi driver and sets up validation
of DSC parameters.

> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 8e552cdc3b53b..d4be40b2574a1 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];
> +

[Severity: High]
Are we skipping the final array element in this loop?

In mtk_crtc_ddp_hw_init(), the loop condition is:
i < mtk_crtc->ddp_comp_nr - 1

Since the DSI interface is positioned at the end of the pipeline, comp_dsi is
never assigned here, preventing DSC from being configured.

> +		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];
>  	}
>  	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);
> @@ -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]
Could configuring the DSC hardware after the pipeline has started cause hardware
hangs?

mtk_ddp_comp_start() is called for all components just above this code,
starting the pipeline in BYPASS mode. Then mtk_ddp_comp_dsc_setup() dynamically
disables, reconfigures, and re-enables the active DSC block on the live stream.

>  	/* Initially configure all planes */
>  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>  		struct drm_plane *plane = &mtk_crtc->planes[i];
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
> new file mode 100644
> index 0000000000000..bed6b77bf9a9e
> --- /dev/null
> +++ 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 this function reset disp_dsc->dsc_config_done on stop?

If dsc_config_done is not cleared, on resume mtk_dsc_start() will skip BYPASS
mode and immediately assert DSC_EN with zeroed configuration registers, which
could lead to pipeline hangs or display failures.

[ ... ]
> +static int mtk_dsc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> +	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get gce client reg\n");
> +#endif

[Severity: High]
Will this unconditional return break the driver probe on non-CMDQ platforms?

If the optional mediatek,gce-client-reg device tree property is missing,
cmdq_dev_get_client_reg() returns an error. Propagating this error prevents
the driver from gracefully falling back to CPU writes on systems without CMDQ.

> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = devm_pm_runtime_enable(dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index eb325e68aa592..3237dc765701d 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]
Is it intentional to hardcode the payload word count to exactly 2 DSC slices
per line?

If a panel requests a different slice topology (e.g., 1, 3, or 4 slices per
line), hardcoding this to 2 rather than dynamically calculating it based on
dsc->slice_count could cause DSI framing errors.

> +
> +	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]);
> +	}
[ ... ]
> +static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
> +{
> +	struct drm_dsc_config *dsc = dsi->dsc;
> +	struct device *dev = dsi->host.dev;
> +	int ret;
[ ... ]
> +	dsc->initial_scale_value = drm_dsc_initial_scale_value(dsc);
> +	dsc->line_buf_depth = dsc->bits_per_component + 1;
> +
> +	return drm_dsc_compute_rc_parameters(dsc);
> +}

[Severity: Medium]
Should pic_width and pic_height be populated in the DSC config before
calculating RC parameters?

drm_dsc_compute_rc_parameters() relies on the slice_count calculation, which
divides pic_width by slice_width. If the panel driver does not initialize
them statically, they remain zero, which could cause a division by zero error.

[ ... ]
> @@ -741,12 +828,19 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  
>  	mtk_dsi_ps_control(dsi, true);

[Severity: Medium]
Is it safe to call mtk_dsi_ps_control(dsi, true) before computing the DSC
parameters?

mtk_dsi_config_vdo_timing() calculates RC parameters including
slice_chunk_size. Calling mtk_dsi_ps_control() first causes it to utilize a
slice_chunk_size of 0, briefly configuring hardware with a word count of 0
until it gets overwritten.

>  	mtk_dsi_set_vm_cmd(dsi);
> -	mtk_dsi_config_vdo_timing(dsi);
> +	ret = mtk_dsi_config_vdo_timing(dsi);
> +	if (ret)
> +		goto err_disable_dsi_and_digital_clk;

[Severity: Critical]
This is a pre-existing issue, but does a failure in mtk_dsi_poweron() lead to a
kernel panic later?

When mtk_dsi_config_vdo_timing() fails, mtk_dsi_poweron() gracefully disables
the clocks and returns an error. However, callers like mtk_dsi_ddp_start()
ignore this error, and the DRM atomic commit proceeds to call mtk_dsi_start().

mtk_dsi_start() then unconditionally writes to the DSI_START register while
the clocks are disabled, causing a bus fault (SError) kernel panic. This patch
adds a new failure path (DSC parameter validation) that readily exposes this
architectural flaw.

> +
>  	mtk_dsi_set_interrupt_enable(dsi);
>  	mtk_dsi_lane_ready(dsi);
>  	mtk_dsi_clk_hs_mode(dsi, 1);

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