Thank you for your contribution! Sashiko AI review found 16 potential issue(s) to consider:
New issues: - [High] DSC setup overwrites the DSC control register, disabling the IP after it was started. - [High] Uninitialized `slice_chunk_size` is used to configure hardware registers. - [High] Integer truncation on `range_data` zeroes out the second set of RC range parameters. - [High] Off-by-one loop boundary misses PPS 19 and the final RC range parameter. - [High] Incorrect bitmask validation erroneously rejects valid integer BPP configurations. - [High] State leakage of `dsc_config_done` causes bypass to be skipped when switching out of DSC modes. - [High] Loop skips the final pipeline component, causing DSI and DSC to not be found and DSC configuration to be ignored. - [High] Integer truncation on `dsc_pre_pad_sz` zeroes out the `DSC_PIC_PREPAD_WIDTH` configuration. - [High] The DSI pixel stream word count assumes exactly 2 DSC slices per line, violating panel variability. - [High] DSI link timings use uncompressed byte counts instead of compressed payloads when DSC is active. - [High] Dangling pointer to `drm_dsc_config` leads to Use-After-Free if the DSI panel is unbound. - [High] Unscaled RC buffer thresholds are packed into PPS registers, causing bitwise overflows. - [Medium] Missing hardware cleanup in `mtk_dsi_poweron()` error path leaves DSI enabled but unclocked. - [Medium] `COMPRESSED_PS_DSC` stream type is conditionally set only when the format is `MIPI_DSI_FMT_RGB888`. - [Medium] Missing zero validation for `slice_width` and `slice_height` risks division by zero panics. Pre-existing issues: - [High] Caller ignores `mtk_dsi_poweron()` errors, causing unprotected writes to unclocked DSI MMIO registers. -- --- Patch [2]: [PATCH v2 02/10] drm/mediatek: Implement Display Stream Compression support --- commit 1aa5619e9968680600a369e5067b3b6f174817c1 Author: AngeloGioacchino Del Regno <[email protected]> Subject: drm/mediatek: Implement Display Stream Compression support Add a real driver for the Display Stream Compression (DSC) Display Controller IP, implementing support for DSC v1.1 to v1.2. > 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]; > + > + 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] Does this loop skip the final pipeline component? Because it iterates only to mtk_crtc->ddp_comp_nr - 1, the final component (which is often DSI) won't be checked, leaving comp_dsi as NULL and silently bypassing DSC configuration. > 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); > @@ -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] Can calling mtk_ddp_comp_dsc_setup() after mtk_ddp_comp_start() disable the DSC IP? Looking at mtk_dsc_setup(), it performs a direct writel() to DISP_REG_DSC_CON which clears the DSC_EN bit that was just set during the start phase. > /* Initially configure all planes */ > for (i = 0; i < mtk_crtc->layer_nr; i++) { > struct drm_plane *plane = &mtk_crtc->planes[i]; > 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..79b66bd7e5f3c > --- /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 |= dsc_cfg->rc_buf_thresh[buf_index] << data_shift; [Severity: High] Are the RC buffer thresholds missing scaling here? The rc_buf_thresh values can be up to 14 bits wide, but they are packed into 8-bit fields without being right-shifted by 6, which could cause bitwise overflows into adjacent fields. > + } > + writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(8 + i)); > + } > + > + /* PPS 12..19 - Each register holds two sets of RC range parameters (19 > has one) */ > + for (i = 0; i < 7; i++) { [Severity: High] Does this loop terminate before configuring PPS 19? The condition i < 7 means it iterates through i = 6, which covers up to PPS 18 (12 + 6). The 15th RC range parameter in PPS 19 appears to be skipped. > + struct drm_dsc_rc_range_parameters *rcrp = > dsc_cfg->rc_range_params; > + u8 block_num = i * 2; > + data = 0; > + > + for (j = 0; j < 2; j++) { > + u8 buf_index = block_num + j; > + u8 data_shift = j * 16; > + u16 range_data; > + > + range_data = FIELD_PREP(DSC_P12_RC_RANGE_MIN_QP, > + rcrp[buf_index].range_min_qp); > + range_data |= FIELD_PREP(DSC_P12_RC_RANGE_MAX_QP, > + rcrp[buf_index].range_max_qp); > + range_data |= FIELD_PREP(DSC_P12_RC_RANGE_BPG_OFFSET, > + > rcrp[buf_index].range_bpg_offset); > + > + range_data <<= data_shift; [Severity: High] Could this shift cause integer truncation? The range_data variable is declared as u16. When j=1, data_shift is 16, shifting all bits out of the u16 and truncating it to 0, which would lose the configuration for the odd-indexed RC range parameters. > + data |= range_data; > + > + /* rc_range_params holds 15 elements in total */ > + if (buf_index == 14) > + break; > + } > + writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(12 + i)); > + } > +} > + > +void mtk_dsc_setup(struct device *dev, struct drm_dsc_config *dsc_cfg) > +{ > + struct mtk_dsc *disp_dsc = dev_get_drvdata(dev); > + u32 dsc_slice_w, dsc_slice_h, dsc_mode, dsc_cfg_rval, dsc_shadow; > + u32 dsc_dbg_con, dsc_con, dsc_enc_width, dsc_pic_w, dsc_pic_h; > + u16 pic_group_width, pic_height_ext_num, slice_group_width; > + u16 chunk_size, dsc_pad_num, dsc_pre_pad_sz; [ ... ] > + dsc_slice_w = FIELD_PREP(DSC_SLICE_GROUP_WIDTH_M1, slice_group_width - > 1); > + dsc_slice_w |= FIELD_PREP(DSC_SLICE_WIDTH, dsc_cfg->slice_width); > + dsc_slice_h = FIELD_PREP(DSC_SLICE_WIDTH_MOD3, dsc_cfg->slice_width % > 3); > + dsc_slice_h |= FIELD_PREP(DSC_SLICE_NUM_M1, > + (pic_height_ext_num / dsc_cfg->slice_height) > - 1); [Severity: Medium] Is it possible for dsc_cfg->slice_height to be zero here, leading to a division by zero panic? A validation check for zero slice height might be needed. > + dsc_slice_h |= FIELD_PREP(DSC_SLICE_HEIGHT_M1, dsc_cfg->slice_height - > 1); > + > + dsc_pad_num = 3 - ((chunk_size * 2) % 3) % 3; > + dsc_pad_num = FIELD_PREP(DSC_PAD_NUMBER, dsc_pad_num); > + > + dsc_pre_pad_sz = FIELD_PREP(DSC_PIC_PREPAD_HEIGHT, dsc_cfg->pic_height); > + dsc_pre_pad_sz |= FIELD_PREP(DSC_PIC_PREPAD_WIDTH, dsc_cfg->pic_width); [Severity: High] Will this truncate the DSC_PIC_PREPAD_WIDTH value? The variable dsc_pre_pad_sz is declared as a u16, but DSC_PIC_PREPAD_WIDTH is defined as GENMASK(31, 16), which means the OR operation will overflow the u16 and lose the width data. > + > + dsc_mode = FIELD_PREP(DSC_INIT_DELAY_HEIGHT, 4); [ ... ] > +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) { [Severity: High] Does this handle transitioning from DSC to non-DSC modes correctly? Because disp_dsc->dsc_config_done is never cleared after being set in mtk_dsc_setup(), it looks like setting bypass mode will be skipped on subsequent starts in non-DSC configurations. > + mtk_ddp_write_mask(NULL, DSC_BYPASS, &disp_dsc->cmdq_reg, > + disp_dsc->reg, DISP_REG_DSC_CON, DSC_BYPASS); > + mtk_ddp_write_mask(NULL, DSC_UFOE_SEL, &disp_dsc->cmdq_reg, > + disp_dsc->reg, DISP_REG_DSC_CON, > DSC_UFOE_SEL); > + mtk_ddp_write_mask(NULL, DSC_DUAL_INOUT, &disp_dsc->cmdq_reg, > + disp_dsc->reg, DISP_REG_DSC_CON, > DSC_DUAL_INOUT); > + } > + > + mtk_ddp_write_mask(NULL, DSC_EN, &disp_dsc->cmdq_reg, > + disp_dsc->reg, DISP_REG_DSC_CON, DSC_EN); > +} > + > +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); > +} > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 3f3f56eed3f92..3e22553ca990f 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c [ ... ] > @@ -395,7 +399,7 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) > > static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact) > { > - u32 dsi_buf_bpp, ps_val, ps_wc, vact_nl; > + u32 dsi_buf_bpp, ps_val, ps_wc, size_val, vact_nl; > > if (dsi->format == MIPI_DSI_FMT_RGB565) > dsi_buf_bpp = 2; > else > dsi_buf_bpp = 3; > > /* Word count */ > - ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp); > + if (dsi->dsc) > + ps_wc = FIELD_PREP(DSI_PS_WC, 2 * dsi->dsc->slice_chunk_size); [Severity: High] Is it safe to assume exactly 2 DSC slices per line here? If a panel requires 1, 4, or another slice count, hardcoding the multiplier to 2 could result in improperly sized MIPI DSI packets. > + else > + ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp); > ps_val = ps_wc; > > /* Pixel Stream type */ > @@ -411,7 +418,10 @@ static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool > config_vact) > default: > fallthrough; > case MIPI_DSI_FMT_RGB888: > - ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_24BIT_RGB888); > + if (dsi->dsc) > + ps_val |= FIELD_PREP(DSI_PS_SEL, COMPRESSED_PS_DSC); > + else > + ps_val |= FIELD_PREP(DSI_PS_SEL, > PACKED_PS_24BIT_RGB888); > break; [Severity: Medium] Will this prevent compressed streams for formats other than RGB888? Since the COMPRESSED_PS_DSC assignment is fully nested inside the MIPI_DSI_FMT_RGB888 case, using DSC with RGB666 would mistakenly signal PACKED_PS_18BIT_RGB666 over the bus. > case MIPI_DSI_FMT_RGB666: > ps_val |= FIELD_PREP(DSI_PS_SEL, LOOSELY_PS_24BIT_RGB666); > @@ -565,7 +587,43 @@ static void mtk_dsi_config_vdo_timing_per_line_lp(struct > mtk_dsi *dsi) > writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC); > } > > -static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > +static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi) > +{ > + struct drm_dsc_config *dsc = dsi->dsc; > + struct device *dev = dsi->host.dev; > + int ret; > + > + if (dsc->bits_per_pixel & GENMASK(7, 0)) { > + dev_err(dev, "Fractional bits_per_pixel not supported\n"); > + return -EINVAL; > + } [Severity: High] Could this bitmask erroneously reject valid integer bits-per-pixel values? The bits_per_pixel value is shifted by 4 (to allow 4 fractional bits). GENMASK(7, 0) covers the lower 8 bits, which includes 4 of the integer bits. This would fail for a valid 8 BPP configuration. > + > + if (dsc->bits_per_component != 8) { > + dev_err(dev, "%u bits per component is not supported\n", > + dsc->bits_per_component); [ ... ] > +static int mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > { > struct videomode *vm = &dsi->vm; > > @@ -574,17 +632,17 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi > *dsi) > 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) > + return mtk_dsi_set_dsc_params(dsi); > + > + return 0; > } [Severity: High] Is there an ordering issue here with uninitialized parameters? mtk_dsi_ps_control() reads dsi->dsc->slice_chunk_size, but it hasn't been computed yet because mtk_dsi_set_dsc_params() is called afterwards. Additionally, do the DSI timing calculations use uncompressed byte counts when DSC is active? The functions mtk_dsi_config_vdo_timing_per_frame_lp() and per_line_lp() calculate ps_wc using vm->hactive * dsi_tmp_buf_bpp, which ignores the reduced compressed payload size, potentially miscomputing the front/back porch and roundup metrics. > @@ -741,12 +799,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); > + if (ret) > + goto err_disable_digital_clk; > + > mtk_dsi_set_interrupt_enable(dsi); > mtk_dsi_lane_ready(dsi); > mtk_dsi_clk_hs_mode(dsi, 1); > > return 0; > +err_disable_digital_clk: > + clk_disable_unprepare(dsi->digital_clk); > err_disable_engine_clk: > clk_disable_unprepare(dsi->engine_clk); > err_phy_power_off: [Severity: Medium] Does this error path miss cleaning up the DSI hardware state? If mtk_dsi_config_vdo_timing() fails, jumping to err_disable_digital_clk skips calling mtk_dsi_disable(dsi), which was enabled earlier in mtk_dsi_poweron(), leaving it enabled but unclocked. [Severity: High] This is a pre-existing issue, but does mtk_dsi_ddp_start() ignore mtk_dsi_poweron() errors? Since mtk_dsi_poweron() can now return errors (like when DSC parameter setup fails), mtk_dsi_ddp_start() might proceed to call mtk_dsi_start() on unclocked hardware. > @@ -883,6 +946,21 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge, > if (mode->clock * bpp / dsi->lanes > 1500000) > return MODE_CLOCK_HIGH; > > + if (dsi->dsc) { > + if (mode->hdisplay % dsi->dsc->slice_width) { [Severity: Medium] Is there a risk of a division by zero panic here? If an external panel driver supplies a zero slice_width, this modulo operation will crash. > + dev_err(dsi->host.dev, > + "DSC: hdisplay %u is not a multiple of slice > width %u\n", > + dsi->dsc->slice_width, mode->hdisplay); > + return MODE_H_ILLEGAL; > + } > + if (mode->vdisplay % dsi->dsc->slice_height) { > + dev_err(dsi->host.dev, > + "DSC: vdisplay %u is not a multiple of slice > height %u\n", > + dsi->dsc->slice_height, mode->vdisplay); > + return MODE_V_ILLEGAL; > + } > + } > + > return MODE_OK; > } [ ... ] > @@ -1006,6 +1091,9 @@ static int mtk_dsi_host_attach(struct mipi_dsi_host > *host, > return PTR_ERR(dsi->next_bridge); > } > > + if (device->dsc) > + dsi->dsc = device->dsc; > + [Severity: High] Does this create a dangling pointer if the DSI panel is unbound? A pointer to device->dsc is captured here, but there appears to be no corresponding code in mtk_dsi_host_detach() to set it back to NULL, which could lead to a use-after-free. > drm_bridge_add(&dsi->bridge); > > ret = component_add(host->dev, &mtk_dsi_component_ops); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
