Re: [PATCH v9 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 3 potential issue(s) to consider:
- [High] drm/mediatek: incorrect scaling of fill_rate by pixels_per_iter
- [High] drm/mediatek: rw_times uses vactive instead of hactive
- [High] drm/mediatek: dimensional unit mismatch in QoS thresholds
--

commit 5e7052b5da8774e8f7f52c475c5eb2ff80c36e96
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 9d701cadf7eac..64dfc100f0338 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c

[ ... ]

> @@ -772,6 +817,102 @@ static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
> +static void mtk_dsi_config_hw_buffers(struct mtk_dsi *dsi)
> +{
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
> +	const u16 *reg_qos = data->reg_qos;
> +	u32 buffer_unit, sram_unit, num_hw_buffers;
> +	u32 preultra_hi, preultra_lo;
> +	u32 urgent_hi, urgent_lo;
> +	u32 ultra_hi, ultra_lo;
> +	u32 sodi_hi, sodi_lo;
> +	u32 data_rate_per_buf;
> +	u32 out_valid_thresh;
> +	u32 dsi_buf_bpp;
> +	u32 fill_rate;
> +	u32 pclk_mhz;
> +	u32 rw_times;
> +	u32 val;
> +	u64 tmp;

[ ... ]

> +	/* 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 min() check compare mismatched units?

The data_rate_per_buf value is calculated in units of buffer_units / us.
Consequently, out_valid_thresh is calculated in buffer_units. However, it is
clamped using MTK_DSI_MAX_FIFO_BYTES (1554), which is an absolute byte count.

Because MTK_DSI_MAX_FIFO_BYTES is an absolute byte count, clamping a
buffer_unit value against 1553 allows the threshold to drastically exceed the
actual hardware FIFO capacity. 

The same unit mix-up appears to occur below with sodi_hi, which is initialized
as a byte count but has a buffer_units / us rate subtracted from it.

> +	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,
                                            ^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Is it correct to multiply by data->pixels_per_iter here?

In mtk_dsi_config_hw_buffers(), pclk_mhz is derived from dsi->vm.pixelclock,
which is the display's total pixel clock. Multiplying this by
data->pixels_per_iter (which is 2 for MT8196) effectively doubles the pixel
rate, as pixelclock already accounts for all pixels being transmitted.

This artificially inflates the byte fill rate to a physically impossible value,
causing incorrect QoS anti-latency parameters to be programmed into the
hardware.

> +			    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;
> +	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;
                   ^^^^^^^^^^^^^^^
[Severity: High]
Should this use hactive instead of vactive for line buffer timing?

The code calculates rw_times for line buffer bursts using dsi->vm.vactive
(vertical active lines). Since this is a horizontal burst line calculation,
shouldn't it be based on the line length (hactive) rather than the vertical
height of the display? 

Using vactive leads to incorrect burst transmission timings that change
depending on the panel orientation.

> +	rw_times /= data->num_burst_lines * data->pixels_per_iter;
> +

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