Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues: - [High] Loop iterating over DDP components skips the last element, silently bypassing DSC hardware initialization. - [High] DSC hardware is configured after the display pipeline has already been started, and state is not reset on stop, risking pipeline hangs and state mismatches. - [High] The DSI payload word count is hardcoded to exactly 2 DSC slices per line, breaking displays with different slice topologies. - [High] CMDQ client registration failure breaks probe on non-CMDQ platforms, contradicting intended CPU fallback. - [Medium] Calling `mtk_dsi_ps_control(dsi, true)` before computing DSC parameters results in configuring DSI registers with an uninitialized `slice_chunk_size`. - [Medium] Missing initialization of `pic_width` and `pic_height` in the DSC configuration before calculating RC parameters. Pre-existing issues: - [Critical] Failure during DSC parameter setup inside `mtk_dsi_poweron` causes clocks to be disabled, leading to a kernel panic from unclocked register accesses in `mtk_dsi_start`. -- --- Patch [2]: [PATCH v6 02/11] drm/mediatek: Implement Display Stream Compression support --- commit 4b7d7243b726f6a21138b6126227ad71271efb1e 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. This also introduces DSC handling in the mtk_dsi driver and sets up validation of DSC parameters. > 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] Are we skipping the final array element in this loop? In mtk_crtc_ddp_hw_init(), the loop condition is: i < mtk_crtc->ddp_comp_nr - 1 Since the DSI interface is positioned at the end of the pipeline, comp_dsi is never assigned here, preventing DSC from being configured. > + 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]; > } > 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] Could configuring the DSC hardware after the pipeline has started cause hardware hangs? mtk_ddp_comp_start() is called for all components just above this code, starting the pipeline in BYPASS mode. Then mtk_ddp_comp_dsc_setup() dynamically disables, reconfigures, and re-enables the active DSC block on the live stream. > /* 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..bed6b77bf9a9e > --- /dev/null > +++ 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 this function reset disp_dsc->dsc_config_done on stop? If dsc_config_done is not cleared, on resume mtk_dsc_start() will skip BYPASS mode and immediately assert DSC_EN with zeroed configuration registers, which could lead to pipeline hangs or display failures. [ ... ] > +static int mtk_dsc_probe(struct platform_device *pdev) > +{ [ ... ] > +#if IS_REACHABLE(CONFIG_MTK_CMDQ) > + ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0); > + if (ret) > + return dev_err_probe(dev, ret, "failed to get gce client > reg\n"); > +#endif [Severity: High] Will this unconditional return break the driver probe on non-CMDQ platforms? If the optional mediatek,gce-client-reg device tree property is missing, cmdq_dev_get_client_reg() returns an error. Propagating this error prevents the driver from gracefully falling back to CPU writes on systems without CMDQ. > + > + platform_set_drvdata(pdev, priv); > + > + ret = devm_pm_runtime_enable(dev); > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index eb325e68aa592..3237dc765701d 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] Is it intentional to hardcode the payload word count to exactly 2 DSC slices per line? If a panel requests a different slice topology (e.g., 1, 3, or 4 slices per line), hardcoding this to 2 rather than dynamically calculating it based on dsc->slice_count could cause DSI framing errors. > + > + 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]); > + } [ ... ] > +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; [ ... ] > + dsc->initial_scale_value = drm_dsc_initial_scale_value(dsc); > + dsc->line_buf_depth = dsc->bits_per_component + 1; > + > + return drm_dsc_compute_rc_parameters(dsc); > +} [Severity: Medium] Should pic_width and pic_height be populated in the DSC config before calculating RC parameters? drm_dsc_compute_rc_parameters() relies on the slice_count calculation, which divides pic_width by slice_width. If the panel driver does not initialize them statically, they remain zero, which could cause a division by zero error. [ ... ] > @@ -741,12 +828,19 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > mtk_dsi_ps_control(dsi, true); [Severity: Medium] Is it safe to call mtk_dsi_ps_control(dsi, true) before computing the DSC parameters? mtk_dsi_config_vdo_timing() calculates RC parameters including slice_chunk_size. Calling mtk_dsi_ps_control() first causes it to utilize a slice_chunk_size of 0, briefly configuring hardware with a word count of 0 until it gets overwritten. > mtk_dsi_set_vm_cmd(dsi); > - mtk_dsi_config_vdo_timing(dsi); > + ret = mtk_dsi_config_vdo_timing(dsi); > + if (ret) > + goto err_disable_dsi_and_digital_clk; [Severity: Critical] This is a pre-existing issue, but does a failure in mtk_dsi_poweron() lead to a kernel panic later? When mtk_dsi_config_vdo_timing() fails, mtk_dsi_poweron() gracefully disables the clocks and returns an error. However, callers like mtk_dsi_ddp_start() ignore this error, and the DRM atomic commit proceeds to call mtk_dsi_start(). mtk_dsi_start() then unconditionally writes to the DSI_START register while the clocks are disabled, causing a bus fault (SError) kernel panic. This patch adds a new failure path (DSC parameter validation) that readily exposes this architectural flaw. > + > mtk_dsi_set_interrupt_enable(dsi); > mtk_dsi_lane_ready(dsi); > mtk_dsi_clk_hs_mode(dsi, 1); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
