Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] DSI and DSC component detection loop misses the terminal component of the display pipeline, preventing DSC configuration. - [High] Incorrect execution order for DSC setup and missing state reset causes DSC hardware corruption on subsequent modesets or resume. - [High] DSI link data rate, horizontal blanking timings, and clock lane word counts do not adjust for DSC compression, breaking timing synchronization. - [High] The pixel stream word count incorrectly hardcodes an assumption of exactly 2 DSC slices per line. - [High] DSI power-on accesses uninitialized DSC parameters by invoking `mtk_dsi_ps_control` before computing them. - [Medium] The `dsi->dsc` pointer is set to NULL during panel unbind without serialization, leading to a Use-After-Free or NULL pointer dereference during modeset. --
commit d2929d85efc73eb974427b3e2c2a0afbfbe66701 Author: AngeloGioacchino Del Regno <[email protected]> Subject: drm/mediatek: Implement Display Stream Compression support This commit adds Display Stream Compression (DSC) support for the MediaTek DRM driver. It introduces a new standalone mtk_disp_dsc driver and modifies the existing CRTC and DSI drivers to configure and integrate DSC. > diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c > b/drivers/gpu/drm/mediatek/mtk_crtc.c > index 97e3ff412e6ee..c0a53c153b8d5 100644 > --- a/drivers/gpu/drm/mediatek/mtk_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c > @@ -398,6 +401,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc > *mtk_crtc) > if (!mtk_ddp_comp_add(mtk_crtc->ddp_comp[i], mtk_crtc->mutex)) > mtk_mutex_add_comp(mtk_crtc->mutex, > mtk_crtc->ddp_comp[i]->id); > + > + /* For now, only single DSI is supported */ > + if (mtk_crtc->ddp_comp[i]->id >= DDP_COMPONENT_DSI0 && > + mtk_crtc->ddp_comp[i]->id <= DDP_COMPONENT_DSI3) > + if (!comp_dsi) > + comp_dsi = mtk_crtc->ddp_comp[i]; > + > + if (mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC0 || > + mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC1) > + if (!comp_dsc) > + comp_dsc = mtk_crtc->ddp_comp[i]; > } [Severity: High] Will this loop successfully find the DSI component? The loop condition in mtk_crtc_ddp_hw_init iterates until mtk_crtc->ddp_comp_nr - 1. Because the DSI encoder is always the terminal component of the display pipeline at ddp_comp_nr - 1, the loop appears to terminate before inspecting it. Could this leave comp_dsi as NULL and skip the DSC configuration entirely? > @@ -413,6 +427,13 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc > *mtk_crtc) > mtk_ddp_comp_start(comp); > } > > + /* Setup the DSC if present, with the config coming from DSI */ > + if (comp_dsc && comp_dsi) { > + dsc_cfg = mtk_dsi_get_dsc_config(comp_dsi->dev); > + if (dsc_cfg) > + mtk_ddp_comp_dsc_setup(comp_dsc, dsc_cfg); > + } [Severity: High] Is the hardware started before it is fully configured? The preceding loop calls mtk_ddp_comp_start on all components, which will invoke mtk_dsc_start and write the DSC_EN bit to the control register. If mtk_ddp_comp_dsc_setup is executed afterwards, could the hardware enter an invalid state by being enabled before its parameters are applied? > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c > b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c [ ... ] > +void mtk_dsc_stop(struct device *dev) > +{ > + struct mtk_dsc *disp_dsc = dev_get_drvdata(dev); > + > + writel(0, disp_dsc->reg + DISP_REG_DSC_CON); > +} [Severity: High] Should dsc_config_done be reset to false when stopping the hardware? If mtk_dsc_stop writes 0 to the register but leaves dsc_config_done as true, a subsequent resume or modeset might cause mtk_dsc_start to see dsc_config_done == true. Would this skip the bypass mode fallback and write DSC_EN over a zeroed configuration register, potentially hanging the display pipeline? > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index f82506e10fd59..97b8a91874f5b 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c [ ... ] > +static void mtk_dsi_ps_control_dsc(struct mtk_dsi *dsi, bool config_vact) > +{ > + const struct mtk_dsi_driver_data *data = dsi->driver_data; > + const u16 *reg_main = dsi->driver_data->reg_main; > + const short dsi_buf_bpp = 3; > + u32 ps_wc; > + > + /* Word count */ > + ps_wc = FIELD_PREP(DSI_PS_WC, 2 * dsi->dsc->slice_chunk_size); [Severity: High] Can this hardcoded multiplier cause incorrect word counts on different panels? Using 2 here assumes there are always exactly 2 DSC slices per line. Should this use dsi->dsc->slice_count to ensure the payload bytes are calculated correctly for displays with other slice layouts? > + if (config_vact) { > + writel(FIELD_PREP(VACT_NL, dsi->vm.vactive), > + dsi->regs + reg_main[DSI_VACT_NL]); > + writel(ps_wc, dsi->regs + reg_main[DSI_HSTX_CKL_WC]); > + } [Severity: High] Does this blindly overwrite the clock lane word count register? In mtk_dsi_config_vdo_timing_per_frame_lp, hstx_cklp_wc is carefully calculated based on front porch, back porch, and trailing lengths, and then written to DSI_HSTX_CKL_WC. When mtk_dsi_ps_control_dsc is subsequently called, it overwrites that same register with ps_wc (the raw compressed payload size). Additionally, the calculations for dsi->data_rate in mtk_dsi_poweron and horizontal blanking timings use uncompressed pixel bit depths rather than adjusting for DSC compression. Will these issues combined break clock lane timing synchronization? > @@ -741,10 +828,17 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > mtk_dsi_ps_control(dsi, true); > mtk_dsi_set_vm_cmd(dsi); > - mtk_dsi_config_vdo_timing(dsi); > + ret = mtk_dsi_config_vdo_timing(dsi); [Severity: High] Could this sequence write uninitialized values to the hardware? mtk_dsi_poweron calls mtk_dsi_ps_control, which reads dsi->dsc->slice_chunk_size to program the DSI payload size registers. However, dsi->dsc->slice_chunk_size is only populated when mtk_dsi_config_vdo_timing eventually calls drm_dsc_compute_rc_parameters. Does this imply the payload size registers are initially programmed with zeros or uninitialized memory? > @@ -1030,6 +1157,8 @@ static int mtk_dsi_host_detach(struct mipi_dsi_host > *host, > > component_del(host->dev, &mtk_dsi_component_ops); > drm_bridge_remove(&dsi->bridge); > + dsi->dsc = NULL; > + > return 0; > } [Severity: Medium] Is it safe to clear dsi->dsc without modeset synchronization? If a user unbinds the panel concurrently while a display modeset is active (for example, while executing mtk_dsi_config_vdo_timing), could the pointer become NULL immediately after the check for dsi->dsc? Could this lead to a NULL pointer dereference or use-after-free in mtk_dsi_set_dsc_params? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
