Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Calculation of QoS `rw_times` uses vertical active lines (`vactive`) instead of horizontal active pixels (`hactive`). Pre-existing issues: - [High] Data race between hardirq and process contexts on the `dsi->irq_data` state variable can result in lost interrupts. - [High] Lack of serialization in MIPI DSI host transfer operations (`mtk_dsi_host_transfer`), allowing concurrent transfers to corrupt the command queue and state. -- --- Patch [8]: [PATCH v6 08/11] drm/mediatek: mtk_dsi: Add support for MT8196 --- commit a997b4c68ea4198553bbed678f80fe423d11793b 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 and its Dimensity and Genio variants. Differently from the older DSI IPs, the one from MT8196 requires the initialization of all of the QoS parameters and can make use of a DSI SRAM reserved buffer (present also on older SoCs but not mandatory on those). > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 95f7d94bdad3b..d2fdeaeb97bea 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) > +{ [ ... ] > + /* 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; > + rw_times /= data->num_burst_lines * data->pixels_per_iter; [Severity: High] Does this calculation incorrectly use the vertical active lines (vactive) instead of horizontal active pixels (hactive)? Since DSI line buffers operate on horizontal lines, their size and bandwidth depend on the line width. Using vactive here seems dimensionally incorrect and might be a typo. Could this result in incorrect QoS parameters for anti-latency buffers and cause display corruption or underflow? > + > + /* Write all QoS parameters: Screen On Deep Idle, (pre)Ultra, Urgent, > RW times */ > + mtk_dsi_mask(dsi, reg_qos[DSI_QOS_SODI_HIGH], BUF_THRESHOLD_PARAM, > sodi_hi); [ ... ] [Severity: High] This is a pre-existing issue, but can mtk_dsi_irq_data_clear() and mtk_dsi_irq_data_set() drop interrupts due to a data race? In mtk_dsi_irq_data_clear() (process context): dsi->irq_data &= ~irq_bit; And mtk_dsi_irq_data_set() (hardirq context): dsi->irq_data |= irq_bit; If the hardirq fires while process context is executing the non-atomic bit clear, could an interrupt completion flag (like VM_DONE_INT_FLAG) be silently erased? This could cause wait_event_interruptible_timeout() to fail and log a timeout, potentially hanging the display pipeline. [Severity: High] This is a pre-existing issue, but does mtk_dsi_host_transfer() lack required serialization? Unlike many DSI host drivers, mtk_dsi_host_transfer() does not take a mutex. Can concurrent transfers from userspace via sysfs, modeset threads, or backlights corrupt the command queue or DSI state by concurrently calling mtk_dsi_host_send_cmd()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
