Re: [PATCH v8 09/13] drm/mediatek: mtk_dsi: Add support for SRAM QoS setting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/mediatek: dimensional mismatch in DSI QoS threshold calculations
- [High] drm/mediatek: DSI TX buffer RW times incorrectly based on vertical resolution
--

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

drm/mediatek: mtk_dsi: Add support for SRAM QoS setting

Add support for automatically calculating and setting the Quality of Service
(QoS) parameters and DSI IP SRAM Line Buffering in the newest DSI IPs like
the one found on MT8196, MT6991 and variants.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 6e92e4e1586f7..16bf08bee38e3 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[ ... ]
> @@ -771,6 +816,102 @@ static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
>  	return drm_dsc_compute_rc_parameters(dsc);
>  }
>  
> +static void mtk_dsi_config_hw_buffers(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 code inadvertently compare buffer units against bytes?

In mtk_dsi_config_hw_buffers(), data_rate_per_buf is calculated in
buffer units. Multiplying it by MTK_DSI_DEFAULT_QOS_VALID_FIFO_US yields
out_valid_thresh in buffer units. However, it is then capped against
MTK_DSI_MAX_FIFO_BYTES, which is a byte count. This dimensional mismatch
writes incorrect configuration values to the DSI QoS hardware registers,
which could lead to FIFO under-provisioning or values exceeding SRAM capacity.

> +	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);
> +
> +	/* 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 intentional to subtract buffer units from a value initialized in bytes?

Here in mtk_dsi_config_hw_buffers(), sodi_hi is initialized using
MTK_DSI_MAX_FIFO_BYTES (in bytes), but the subtracted amount is based on
fill_rate and data_rate_per_buf, which appear to be in buffer units. This
mismatch can corrupt the calculation and cause hardware lockups or display
corruption.

> +	sodi_lo = MTK_DSI_DEFAULT_QOS_SODI_LO_OVERHEAD * data_rate_per_buf;
> +	preultra_hi = MTK_DSI_DEFAULT_QOS_PREULTRA_HI_US * data_rate_per_buf;
> +	preultra_lo = MTK_DSI_DEFAULT_QOS_PREULTRA_LO_US * data_rate_per_buf;
> +	ultra_hi = MTK_DSI_DEFAULT_QOS_ULTRA_HI_US * data_rate_per_buf;
> +	ultra_lo = MTK_DSI_DEFAULT_QOS_ULTRA_LO_US * data_rate_per_buf;
> +	urgent_hi = MTK_DSI_DEFAULT_QOS_URGENT_HI_US * data_rate_per_buf;
> +	urgent_lo = MTK_DSI_DEFAULT_QOS_URGENT_LO_US * data_rate_per_buf;
> +	rw_times = dsi->vm.vactive * dsi_buf_bpp;
> +	rw_times /= data->num_burst_lines * data->pixels_per_iter;

[Severity: High]
Does this calculate the transmission burst times based on the vertical
resolution instead of the horizontal line width?

In mtk_dsi_config_hw_buffers(), rw_times is calculated using the display's
vertical active lines (dsi->vm.vactive). This results in computing bytes
per column instead of bytes per line, which makes the computed line
transmission burst times incorrect for the DSI SRAM line buffers.

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