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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/mediatek: dsc: Hardware enabled before configuration causing mid-flight reconfiguration
- [High] drm/mediatek: dsc: Stale dsc_config_done flag breaks non-DSC display modes
- [Medium] drm/mediatek: mtk_dsi: Uninitialized DSC slice_chunk_size used in early PS control write
--

commit 28d40a7882b85945a98abc9cb60efffdd694dc30
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: Implement Display Stream Compression support

This commit introduces a new driver for the Display Stream Compression (DSC)
IP and adds DSC parameter handling to the mtk_dsi driver to compute and
configure the required compression parameters.

> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
[ ... ]
> @@ -406,6 +409,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc *mtk_crtc)
>  	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
>  		struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
[ ... ]
>  		mtk_ddp_comp_config(comp, width, height, vrefresh, bpc, NULL);
>  		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]
In the loop above, mtk_ddp_comp_start() is called for all components, which
would execute mtk_dsc_start() and set the DSC_EN bit. Since
mtk_ddp_comp_dsc_setup() is only called after this loop finishes, does this
enable the DSC IP hardware before its configuration registers are initialized?

> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
> --- /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]
When the hardware is stopped here, the disp_dsc->dsc_config_done flag isn't
cleared. If the display pipeline is later restarted in a non-DSC mode, will
this stale flag cause mtk_dsc_start() to skip the bypass mode setup and
incorrectly enable the compression hardware?

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -393,9 +397,35 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
> +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, dsi->dsc->slice_count * dsi->dsc->slice_chunk_size);

[ ... ]
> @@ -741,10 +828,17 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
[ ... ]
>  	mtk_dsi_phy_timconfig(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: Medium]
When mtk_dsi_poweron() calls mtk_dsi_ps_control(dsi, true), it looks like
mtk_dsi_config_vdo_timing() hasn't run yet. Since the DSC parameters like
slice_chunk_size are computed inside mtk_dsi_config_vdo_timing(), does this
write an uninitialized or incorrect word count to the hardware registers
initially?

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