Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Unconditional NULL pointer dereference due to double-queuing of writeback job. - [Critical] Hardware Use-After-Free and continuous memory corruption due to missing one-shot WDMA disable. - [Critical] WDMA destination address is erroneously programmed with the input plane's source buffer. - [High] Direct CPU MMIO writes bypass the CMDQ and VBLANK synchronization, risking tearing. - [High] PM Runtime usage counter leak on failure. --
commit 755cfd13f385380e700d97174dac89e1c3a5c181 Author: AngeloGioacchino Del Regno <[email protected]> Subject: drm/mediatek: Add Write DMA (WDMA) Engine for Writeback support This commit adds a basic driver for the Write DMA Engine (WDMA) on the MediaTek MT8173 and Dimensity 1200 MT6893 platforms. It enables the writeback engine used for display image capturing. > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_wdma.c > b/drivers/gpu/drm/mediatek/mtk_disp_wdma.c > new file mode 100644 > index 0000000000000..0575fade92c6b > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_disp_wdma.c [ ... ] > +static irqreturn_t mtk_disp_wdma_irq_handler(int irq, void *dev_id) > +{ > + struct mtk_disp_wdma *wdma = dev_id; > + > + /* Clear frame completion interrupt */ > + writel(0x0, wdma->regs + DISP_REG_WDMA_INT_STATUS); > + > + if (wdma->vblank_cb) > + wdma->vblank_cb(wdma->vblank_cb_data); > + > + /* TODO: Move completion signaling to CMDQ interrupt callback */ > + if (wdma->wb_pending) { > + drm_writeback_signal_completion(&wdma->wb_connector, 0); > + wdma->wb_pending = false; > + } > + > + return IRQ_HANDLED; > +} [Severity: Critical] Is it possible this leaves the hardware engine running continuously? DRM writeback operations are typically one-shot per queued job, and when this interrupt handler signals completion, buffer ownership returns to userspace. However, it appears WDMA_ENGINE_EN is never cleared here. Because the underlying display pipeline stays active, could the hardware continue capturing frames and silently overwrite the completed buffer on subsequent vblanks if userspace reuses or frees it? [ ... ] > +void mtk_wdma_config(struct device *dev, unsigned int width, > + unsigned int height, unsigned int vrefresh, > + unsigned int bpc, struct cmdq_pkt *cmdq_pkt) > +{ > + struct mtk_disp_wdma *wdma = dev_get_drvdata(dev); > + > + writel(WDMA_FORCE_COMMIT | WDMA_BYPASS_SHADOW, > + wdma->regs + DISP_REG_WDMA_SHADOW_CTRL); > +} [Severity: High] Does bypassing the shadow registers here break vblank synchronization? Since atomic commits execute asynchronously, forcing an immediate shadow register commit with a direct CPU writel() might race with the active hardware engine mid-frame and cause display tearing. The pipeline usually relies on the command engine via cmdq_pkt to serialize updates synchronously at vblank. [ ... ] > +static void mtk_wdma_format_config(struct mtk_disp_wdma *wdma, > + struct mtk_plane_pending_state *pending, > + const struct drm_format_info *fmt_info, > + struct cmdq_pkt *cmdq_pkt) > +{ > + unsigned int u_off, u_stride, u_size, v_off; > + u32 val; > + > + /* > + * For RGB formats, this sets the image destination address; > + * For YUV formats, this sets the Y component destination address. > + */ > + mtk_wdma_ddp_write_dst_addr(cmdq_pkt, pending->addr, 0, wdma); [Severity: Critical] Could this accidentally overwrite the active display's source memory buffer? mtk_wdma_layer_config() operates on the input plane being scanned out and passes its pending state into this function. Writing pending->addr (the DMA address of the source input plane) into the WDMA destination registers means the hardware might capture the display output and dump it straight back into the source buffer, corrupting it. [ ... ] > +void mtk_wdma_layer_config(struct device *dev, unsigned int idx, > + struct mtk_plane_state *state, > + struct cmdq_pkt *cmdq_pkt) > +{ [ ... ] > + mtk_wdma_format_config(wdma, pending, fmt_info, cmdq_pkt); > + > + drm_writeback_queue_job(&wdma->wb_connector, > wdma->wb_connector.base.state); > +} [Severity: Critical] Will this trigger a null pointer dereference? mtk_wdma_wb_connector_atomic_commit() correctly consumes the writeback job by queuing it, setting conn_state->writeback_job to NULL. Later in the pipeline, this secondary call passes the connector state where the job pointer is now NULL (or was already NULL if no job was requested), which drm_writeback_queue_job() will immediately try to dereference when adding to the job queue. [ ... ] > +static void mtk_wdma_wb_connector_atomic_commit(struct drm_connector > *connector, > + struct drm_atomic_commit *state) > +{ > + struct drm_connector_state *conn_state = > + drm_atomic_get_new_connector_state(state, connector); > + struct mtk_disp_wdma *wdma = connector_to_wdma(connector); > + struct drm_framebuffer *fb; > + struct drm_gem_object *gem; > + struct drm_gem_dma_object *dma_obj; > + dma_addr_t addr; > + > + if (WARN_ON(!conn_state->writeback_job)) > + return; > + > + fb = conn_state->writeback_job->fb; > + if (!fb) > + return; > + > + gem = fb->obj[0]; > + dma_obj = to_drm_gem_dma_obj(gem); > + addr = dma_obj->dma_addr; > + > + /* Store writeback pending state before queuing the job */ > + wdma->wb_pending = true; > + > + mtk_wdma_ddp_write_dst_addr(NULL, addr, 0, wdma); [Severity: High] Similar to the shadow register bypass above, does passing NULL for the cmdq_pkt here risk tearing? This forces an immediate, unsynchronized writel() of the destination address during the atomic commit setup, rather than queuing it for the command engine to apply synchronously at vblank. > + drm_writeback_queue_job(&wdma->wb_connector, conn_state); > +} [ ... ] > +static int mtk_disp_wdma_bind(struct device *dev, struct device *master, > + void *data) > +{ [ ... ] > + ret = pm_runtime_get_sync(dev); > + if (ret) > + dev_warn(dev, "Could not get PM Runtime!\n"); > + > + /* > + * Disable and clear pending interrupts - write unconditionally as no > + * crash (SError, etc) will happen as if unpowered/unclocked this is > + * simply going to be ignored (and the registers will be 0 at reset). > + */ > + writel(0x0, wdma->regs + DISP_REG_WDMA_INT_ENABLE); > + writel(0x0, wdma->regs + DISP_REG_WDMA_INT_STATUS); > + > + /* Put RPM only if refcount was incremented before to avoid unbalancing > */ > + if (ret >= 0) > + pm_runtime_put(dev); [Severity: High] Does this leak the PM runtime usage counter on failure? pm_runtime_get_sync() increments the usage counter unconditionally. By skipping the put when ret < 0, the device and its power domain might never be able to suspend. Also, pm_runtime_get_sync() returns 1 if the device is already active. This would be treated as an error by the preceding if (ret) check and emit a spurious warning. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
