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