Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] Hardware Initialization Bypass due to skipped array element in DDP component loop. - [High] DSC configuration is written after the component is started, causing pipeline corruption. - [High] State leak of `dsc_config_done` breaks non-DSC display modes. - [High] DSI clock lane timing register `DSI_HSTX_CKL_WC` is destroyed by being overwritten with the pixel payload word count. - [High] Hardcoded 2x multiplier for DSC slice count results in incorrect DSI payload word count. - [High] Bandwidth calculations use uncompressed bits-per-pixel (bpp), falsely rejecting valid DSC modes and misconfiguring PHY clocks. - [Medium] Uninitialized variable `slice_chunk_size` is used for hardware configuration during DSI power-on. --
--- Patch [2]: [PATCH v5 02/11] drm/mediatek: Implement Display Stream Compression support --- commit e5bc691dc168426274a3882d1ba1c028fccf77c4 Author: AngeloGioacchino Del Regno <[email protected]> Subject: drm/mediatek: Implement Display Stream Compression support This commit introduces Display Stream Compression (DSC) support for the MediaTek display controller IP. It extracts DSC IP bypass setup from mtk_ddp_comp into a new mtk_disp_dsc driver and adds DSC handling to the mtk_dsi driver. > diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c > b/drivers/gpu/drm/mediatek/mtk_crtc.c > --- 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]; [Severity: High] Does this loop skip checking the DSI component? Looking at the loop definition in mtk_crtc_ddp_hw_init(), it iterates up to ddp_comp_nr - 1. Since DSI is typically the sink and sits at the very end of the pipeline, it is at index ddp_comp_nr - 1. This means the loop body checking for DSI components never executes for it, leaving comp_dsi as NULL. Consequently, the subsequent mtk_ddp_comp_dsc_setup() call is silently bypassed. > + > + 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]; > } [ ... ] > @@ -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 mtk_ddp_comp_dsc_setup() called too late here? The preceding loop calls mtk_ddp_comp_start(comp) for all components, which starts the DSC in bypass mode. Then mtk_dsc_setup() disables the active DSC engine: writel(dsc_con & ~DSC_EN, disp_dsc->reg + DISP_REG_DSC_CON); It then reconfigures and re-enables the engine. Could disabling and reconfiguring the active component while the pipeline is already streaming video data cause hardware hangs or stream corruption? > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c > b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c [ ... ] > +void mtk_dsc_start(struct device *dev) > +{ > + struct mtk_dsc *disp_dsc = dev_get_drvdata(dev); > + > + /* If no DSC or config not done, set bypass mode */ > + if (!disp_dsc->dsc_config_done) { > + mtk_ddp_write_mask(NULL, DSC_BYPASS, &disp_dsc->cmdq_reg, > + disp_dsc->reg, DISP_REG_DSC_CON, DSC_BYPASS); [Severity: High] Does disp_dsc->dsc_config_done need to be cleared when the DSC is stopped? When switching from a DSC mode to a non-DSC mode, mtk_dsc_stop() zeros DISP_REG_DSC_CON but leaves dsc_config_done set to true. Later, mtk_dsc_start() evaluates !disp_dsc->dsc_config_done as false and skips writing the DSC_BYPASS bit. Since mtk_dsc_stop() previously zeroed the register, DSC_BYPASS is 0, leaving compression enabled on a pipeline that now expects uncompressed video. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c [ ... ] > -static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact) > +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] Is it safe to hardcode the slice multiplier to 2? If a DSC panel uses a different slice count, such as 1 or 4, this calculates an incorrect payload word count. Should this dynamically multiply by dsi->dsc->slice_count instead? > + > + 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 overwrite the clock lane timing configuration? In mtk_dsi_config_vdo_timing(), the code calculates and writes intricate clock timing to DSI_HSTX_CKL_WC via mtk_dsi_config_vdo_timing_per_frame_lp(). Immediately after, it calls mtk_dsi_ps_control(dsi, true), which executes this writel. Could this destroy the previously calculated clock timing by completely overwriting it with just the pixel payload word count? [ ... ] > @@ -741,12 +828,19 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > mtk_dsi_ps_control(dsi, true); [Severity: Medium] Is dsi->dsc->slice_chunk_size fully initialized before this call? Here, mtk_dsi_ps_control(dsi, true) routes to mtk_dsi_ps_control_dsc(), which reads dsi->dsc->slice_chunk_size. However, the chunk size is only calculated later when mtk_dsi_config_vdo_timing() calls drm_dsc_compute_rc_parameters(dsc). Will this result in uninitialized values being temporarily written to DSI hardware registers during power-on? [ ... ] > @@ -883,6 +977,28 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge, > if (mode->clock * bpp / dsi->lanes > 1500000) > return MODE_CLOCK_HIGH; [Severity: High] Does this bandwidth calculation falsely reject valid DSC modes? The bpp value is calculated earlier using mipi_dsi_pixel_format_to_bpp(), which returns the uncompressed bits-per-pixel. Using the uncompressed bpp for DSC modes calculates a much higher required bandwidth, which can exceed hardware limits and reject legitimate modes. Similarly, in mtk_dsi_poweron(), using the uncompressed bpp calculates a data_rate that misconfigures the PHY clock. Should these calculations use the compressed bpp (dsi->dsc->bits_per_pixel >> 4) when DSC is enabled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
