Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] mtk_crtc: DSC hardware enabled before being configured - [High] drm/mediatek: Hardcoded DSC slice geometry breaks configurable topology - [High] mtk_crtc: Component loop excludes terminal DSI component, skipping DSC init - [High] mtk_dsi: Hardware state updated using uninitialized slice_chunk_size - [High] mtk_dsi: Data rate and mode validation ignore DSC compression benefits - [Medium] mtk_disp_dsc: State flag dsc_config_done not cleared on hardware stop - [Low] mtk_dsi: Reversed string formatting arguments in debug messages --
commit 842ba0cd3cf93b49cd15a2061c815f58c2c063f1 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: Implement Display Stream Compression support This commit adds a new driver for the Display Stream Compression (DSC) Display Controller IP, implementing support for DSC v1.1. It introduces DSC handling in the mtk_dsi driver and removes basic bypass setup from mtk_ddp_comp, migrating it to the new mtk_disp_dsc 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 > @@ -396,6 +399,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] Is the terminal DSI component missed by the bounds condition in this loop? This logic is evaluated inside a loop defined as for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++). If the DSI component is the last element in the display pipeline, it is never evaluated by the loop because of the bound check. As a result, comp_dsi would remain NULL, and DSC initialization would be silently skipped entirely. [ ... ] > @@ -411,6 +425,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 this execution order lead to pipeline synchronization glitches or data corruption? The pipeline starts transmitting data when the loop above calls mtk_ddp_comp_start() for all components. The DSC block is not configured until mtk_ddp_comp_dsc_setup() runs after the loop completes. This momentary transmission in an unconfigured or bypass state, followed by programming and re-enabling on the fly, might lock up the hardware FIFOs. > 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_setup(struct device *dev, struct drm_dsc_config *dsc_cfg) > +{ > + struct mtk_dsc *disp_dsc = dev_get_drvdata(dev); [ ... ] > + pic_height_ext_num = dsc_cfg->pic_height + dsc_cfg->slice_height - 1; > + pic_group_width = (dsc_cfg->slice_width * 4) / 3; [Severity: High] Is the picture group width hardcoded to exactly 4 slices per line here? This seems to mathematically conflict with the word count calculation in mtk_dsi_ps_control_dsc() which assumes 2 slices per line. Hardcoded, conflicting slice geometry assumptions rather than utilizing the slice_count from the configuration might guarantee data corruption on panels that do not match these exact assumptions. [ ... ] > +void mtk_dsc_start(struct device *dev) > +{ > + struct mtk_dsc *disp_dsc = dev_get_drvdata(dev); > + u32 val; > + > + val = readl(disp_dsc->reg + DISP_REG_DSC_CON); > + > + /* If no DSC or config not done, stop the HW temporarily and set bypass > mode */ > + if (!disp_dsc->dsc_config_done) { > + val &= ~DSC_EN; > + val |= DSC_BYPASS | DSC_UFOE_SEL | DSC_DUAL_INOUT; > + writel(val, disp_dsc->reg + DISP_REG_DSC_CON); > + } [ ... ] > +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: Medium] Does the config flag remain true on subsequent modesets after being stopped? Since mtk_dsc_stop() resets the hardware register but doesn't clear the disp_dsc->dsc_config_done flag, a subsequent modeset enable might see this flag as true. This would skip the bypass mode logic and instead re-enable the IP with zeroed configuration bits, momentarily corrupting the stream until setup executes. > 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_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 word count to exactly 2 slices per line? This conflicts with the 4 slice assumption in mtk_dsc_setup() and breaks configurable topologies that rely on dsi->dsc->slice_count. [ ... ] > @@ -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] Is it possible for this hardware state to be updated using an uninitialized slice_chunk_size? The mtk_dsi_ps_control() call consumes dsi->dsc->slice_chunk_size to program active DSI hardware, but this variable is not calculated until mtk_dsi_config_vdo_timing() runs on the subsequent line. This could write uninitialized memory into critical hardware registers. [ ... ] > @@ -884,6 +978,28 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge, > if (mode->clock * bpp / dsi->lanes > 1500000) > return MODE_CLOCK_HIGH; > > + if (dsi->dsc) { [Severity: High] Are we validating the mode and calculating the data rate using the uncompressed BPP? In mtk_dsi_bridge_mode_valid() (and similarly in mtk_dsi_poweron()), the bpp is retrieved using mipi_dsi_pixel_format_to_bpp() before checking if DSC is enabled. Applying the uncompressed BPP to clock limits and data rate calculations ignores DSC bandwidth savings. This forces the DSI PHY to be clocked much faster than necessary and artificially rejects valid high-bandwidth modes. > + if (dsi->dsc->slice_width == 0 || dsi->dsc->slice_height == 0) { > + dev_err(dsi->host.dev, > + "DSC: Slice width %u height %u not valid!\n", > + dsi->dsc->slice_width, dsi->dsc->slice_height); > + return MODE_BAD; > + } > + > + if (mode->hdisplay % dsi->dsc->slice_width) { > + dev_dbg(dsi->host.dev, > + "DSC: hdisplay %u is not a multiple of slice > width %u\n", > + dsi->dsc->slice_width, mode->hdisplay); [Severity: Low] Have the string formatting arguments been reversed here? The slice_width variable is passed to the first format specifier which expects hdisplay, and mode->hdisplay is passed to the second which expects the slice width. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
