Re: [PATCH v5 08/11] drm/mediatek: mtk_dsi: Add support for MT8196

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dimensional mismatch in QoS Anti-Latency FIFO threshold calculations results in grossly inflated thresholds.
- [Medium] Unconditional mask of `DSI_VDE` can silently corrupt the `DSI_START` register on generic SoCs.
--

--- Patch [8]: [PATCH v5 08/11] drm/mediatek: mtk_dsi: Add support for MT8196 ---
commit 1786ad54a3e9896a0d86f110a26f6c5cb91b0619
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dsi: Add support for MT8196

Add support for the new DSI IP found in the Kompanio Ultra MT8196 SoC.
It requires initialization of QoS parameters and can use a DSI SRAM
reserved buffer.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 3f58683be70fa..4d8a02cbee385 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c

[ ... ]

> @@ -776,6 +873,102 @@ static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)

[ ... ]

> +	/* Read as: [Data rate (MHz)] * [Number of DSI lanes] / [8 buffer blocks] */
> +	tmp = (u64)dsi->data_rate * dsi->lanes;
> +	data_rate_per_buf = div_u64(tmp, 8 * buffer_unit * HZ_PER_MHZ);
> +
> +	/*
> +	 * Anti-latency buffer output threshold for absolute timer mode: this
> +	 * parameter controls the maximum amount of output data that the FIFO
> +	 * can hold before running out of buffer space.
> +	 *
> +	 * The data will therefore be sent either when the DSI IP0s internal
> +	 * vblank vs bus QoS timer expires or when it reaches the amount of
> +	 * buffers set in BUF_OUT_VALID_THRESHOLD (regardless of QoS) to avoid
> +	 * partially, or entirely, losing frame(s).
> +	 */
> +	out_valid_thresh = MTK_DSI_DEFAULT_QOS_VALID_FIFO_US * data_rate_per_buf;
> +	out_valid_thresh = min(out_valid_thresh, MTK_DSI_MAX_FIFO_BYTES - 1);

[Severity: High]
Does this mix dimensional units? out_valid_thresh is calculated using
data_rate_per_buf (which appears to be in blocks), but it is capped
against MTK_DSI_MAX_FIFO_BYTES, which is in bytes. Will this prevent the cap
from functioning correctly?

> +	mtk_dsi_mask(dsi, reg_qos[DSI_QOS_BUF_CON1], BUF_OUT_VALID_THRESH, out_valid_thresh);
> +
> +	/* Enable ULTRA signal trigger between SOF and VACT */
> +	mtk_dsi_mask(dsi, data->reg_adv[DSI_VDE], VDE_BLOCK_ULTRA, 0);

[Severity: Medium]
If a future patch enables HW buffers for an older SoC (as the commit message
suggests is possible) but doesn't define DSI_VDE in reg_adv, will
reg_adv[DSI_VDE] evaluate to 0?

If so, this offset might map to DSI_START in the main register space. Could
this silently clear BIT(29) of the DSI_START register? Would it be safer
to check if DSI_VDE is defined before applying this mask?

> +
> +	/* Calculate fill rate with line counter mode for DSI Video Mode */
> +	if (dsi->format == MIPI_DSI_FMT_RGB565)
> +		dsi_buf_bpp = 2;
> +	else
> +		dsi_buf_bpp = 3;
> +
> +	pclk_mhz = dsi->vm.pixelclock / HZ_PER_MHZ;
> +	fill_rate = div_u64((u64)pclk_mhz * data->pixels_per_iter * dsi_buf_bpp,
> +			    buffer_unit);
> +
> +	/* Calculate QoS Anti-Latency parameters */
> +	sodi_hi = MTK_DSI_MAX_FIFO_BYTES * num_hw_buffers;
> +	sodi_hi -= (fill_rate - data_rate_per_buf) * 12 / 10;

[Severity: High]
Is it possible we are also mixing bytes and blocks here? MTK_DSI_MAX_FIFO_BYTES
is used without dividing by buffer_unit, which could result in a threshold
of 1554 blocks instead of 1554 bytes.

Could this grossly inflated threshold prevent the hardware from asserting
the anti-latency trigger and cause FIFO over/underflows?

> +	sodi_lo = MTK_DSI_DEFAULT_QOS_SODI_LO_OVERHEAD * data_rate_per_buf;

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