Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Truncation of RC buffer thresholds due to incorrect bitwise masking instead of right-shifting. - [High] Hardcoded and conflicting slice count assumptions without panel validation. - [High] Component discovery loop skips DSI component, preventing DSC initialization. - [High] DSI timing calculation ignores compression and overwrites D-PHY clock lane register. - [Medium] State retention bug where `dsc_config_done` is never reset to false. --
--- Patch [2]: [PATCH v4 02/10] drm/mediatek: Implement Display Stream Compression support --- commit 94686398e3eedc1a9e39476763b6747a4bcfb4c9 Author: AngeloGioacchino Del Regno <[email protected]> Subject: drm/mediatek: Implement Display Stream Compression support This commit introduces a new driver for the Display Stream Compression (DSC) Display Controller IP, implementing support for DSC v1.1 to v1.2. It adds DSC handling into the DSI driver and integrates the bypass setup via CPU writes instead of CMDQ. > diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c > b/drivers/gpu/drm/mediatek/mtk_crtc.c > index 8e552cdc3b53b..d4be40b2574a1 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]; > + [Severity: High] Does this loop terminate one iteration early, causing the DSI component to be skipped? Because the loop condition is i < mtk_crtc->ddp_comp_nr - 1, and the DSI interface is typically the final component in the display pipeline, it appears comp_dsi will remain NULL here. This would cause the subsequent DSC initialization check to silently fail and bypass the DSC setup entirely. > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c > b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c > new file mode 100644 > index 0000000000000..bed6b77bf9a9e > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c [ ... ] > +static void mtk_dsc_pps_setup(struct mtk_dsc *disp_dsc, struct > drm_dsc_config *dsc_cfg) > +{ [ ... ] > + /* PPS 8..11 - Each register holds 4 RC buffer thresholds (PPS 11 has > two) */ > + for (i = 0; i < 4; i++) { > + u8 block_num = i * 4; > + data = 0; > + > + for (j = 0; j < 4; j++) { > + u8 buf_index = block_num + j; > + u8 data_shift = j * 8; > + > + /* rc_buf_thresh holds 14 elements in total */ > + if (buf_index > 13) > + break; > + > + data |= (rbt[buf_index] & DSC_P8_RC_BUF_THR_X) << > data_shift; [Severity: High] Does this code truncate the RC buffer thresholds by masking with 0xFF instead of shifting right? The VESA DSC specification requires programming the Picture Parameter Set registers with RC buffer thresholds divided by 64. Using a bitwise AND mask instead of shifting right by 6 appears to write truncated values to the PPS registers, which could break rate control. [ ... ] > +void mtk_dsc_setup(struct device *dev, struct drm_dsc_config *dsc_cfg) > +{ [ ... ] > + pic_height_ext_num = dsc_cfg->pic_height + dsc_cfg->slice_height - 1; > + pic_group_width = dsc_cfg->slice_width * 4; > + pic_group_width /= 3; [Severity: High] Does this calculation hardcode the picture group width to assume exactly four slices per line? If a panel requests a different slice count, this could conflict with the payload size assumptions made elsewhere in the pipeline and potentially cause display corruption. [ ... ] > + if (dsc_en_bit) > + dsc_con |= DSC_EN; > + > + writel(dsc_con, disp_dsc->reg + DISP_REG_DSC_CON); > + > + disp_dsc->dsc_config_done = true; > +} [Severity: Medium] Can this state retention cause issues on subsequent modesets? Since the dsc_config_done flag is never reset to false on teardown, will a subsequent non-DSC modeset bypass the check below in mtk_dsc_start() and leave the DSC IP active with stale parameters? [ ... ] > +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) { [ ... ] > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 3f3f56eed3f92..8ab5c3431dbbc 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] Does this hardcode the DSI payload to assume exactly two slices per line? This appears to conflict with the assumption of four slices per line inside mtk_dsc_setup(). Is it possible to validate the panel's requested slice count against these assumptions during the mode_valid check? > + > + 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] Will writing the compressed payload size here overwrite the correctly calculated D-PHY High-Speed Clock Lane Word Count? [ ... ] > -static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > +static int mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > { > struct videomode *vm = &dsi->vm; > + int ret; > > writel(vm->vsync_len, dsi->regs + DSI_VSA_NL); > writel(vm->vback_porch, dsi->regs + DSI_VBP_NL); > writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL); > writel(vm->vactive, dsi->regs + DSI_VACT_NL); > > - if (dsi->driver_data->has_size_ctl) > - writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) | > - FIELD_PREP(DSI_WIDTH, vm->hactive), > - dsi->regs + DSI_SIZE_CON); > - > if (dsi->driver_data->support_per_frame_lp) > mtk_dsi_config_vdo_timing_per_frame_lp(dsi); > else > mtk_dsi_config_vdo_timing_per_line_lp(dsi); > > - mtk_dsi_ps_control(dsi, false); > + if (dsi->dsc) { > + ret = mtk_dsi_set_dsc_params(dsi); > + if (ret) > + return ret; > + > + mtk_dsi_ps_control(dsi, true); [Severity: High] Do the DSI lane timings calculated earlier in mtk_dsi_config_vdo_timing_per_frame_lp() incorrectly use the uncompressed pixel count? By passing config_vact = true into mtk_dsi_ps_control() for DSC streams, it looks like the raw compressed payload size will subsequently overwrite the timing synchronized register DSI_HSTX_CKL_WC. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
