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? > + 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/[email protected]?part=8
