On 7/15/26 14:08, [email protected] wrote:
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 <[email protected]>
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?
No that's good.
+ 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 horses start flying, would them crash into airplanes?
Sorry, I had to make fun of this reply :-P
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?
Too much defensive coding, no, this stays like I wrote.
+
+ /* 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?
Nah that's programmed to the SODI_HI and it's the expected calculation. It's
good.
