Re: [PATCH v4 07/10] 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 unit mismatch in HW QoS calculations mixing 'bytes' and 'buffer units' leads to FIFO overflow misconfiguration
- [High] Incorrect use of `vactive` instead of `hactive` for horizontal TX buffer read/write times calculation
--

--- Patch [7]: [PATCH v4 07/10] drm/mediatek: mtk_dsi: Add support for MT8196 ---
commit 53b751cd3a037b336029e926c7c021a5e451f750
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 initializes QoS parameters and makes use of a DSI SRAM reserved
buffer.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 5f06749349289..93bc507d77e20 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)
>  	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 incorrectly cap a value in buffer blocks against a constant in bytes?

Since data_rate_per_buf is computed in buffer blocks per microsecond,
out_valid_thresh is calculated in buffer blocks. Capping it against 
MTK_DSI_MAX_FIFO_BYTES - 1 (1553) allows up to 1553 blocks. This cap
evaluates to 49,696 bytes, which defeats the safety limit for the
1554-byte physical FIFO limit.

> +	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]
Does this mix incompatible units?

sodi_hi initializes as bytes (MTK_DSI_MAX_FIFO_BYTES * num_hw_buffers),
but then subtracts a value in buffer blocks per microsecond
((fill_rate - data_rate_per_buf) * 1.2). This mixes incompatible units
and may result in mathematically meaningless values being programmed
into the hardware.

> +	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]
Should this use dsi->vm.hactive instead of vactive?

Multiplying screen height (vactive) by bytes per pixel seems physically
meaningless for calculating horizontal line buffering burst characteristics.
Using hactive (horizontal pixels per line) might be the intended metric
to compute horizontal burst timings correctly.

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