Re: [PATCH v8 08/15] drm/imx: dc: Add DPR channel support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/imx: dc-dprc: deadlock and invalid lock type in dc_dprc_disable_repeat_en()
- [High] drm/imx: dc-dprc: runtime PM race and leak in dc_dprc_wrap_irq_handler()
- [Medium] drm/imx: dc-dprc: RPM reference leak and PRG imbalance in dc_dprc_configure()
--

commit 0da98ec378c5f86195b2badbfc2867b3e806cb34
Author: Liu Ying <victor.liu@nxp.com>

drm/imx: dc: Add DPR channel support

This commit adds a platform driver for the Display Prefetch Resolve
Channel (DPRC), which fetches display data, transforms it to linear
format, and stores it in RTRAM alongside its associated PRG.

> diff --git a/drivers/gpu/drm/imx/dc/dc-dprc.c b/drivers/gpu/drm/imx/dc/dc-dprc.c
> new file mode 100644
> index 0000000000000..29188fa72a327
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-dprc.c
[ ... ]
> +void dc_dprc_configure(struct dc_dprc *dprc, unsigned int stream_id,
> +		       unsigned int width, unsigned int height,
> +		       unsigned int stride,
> +		       const struct drm_format_info *format,
> +		       dma_addr_t baddr, bool start)
> +{
> +	unsigned int prg_stride = width * format->cpp[0];
> +	unsigned int bpp = format->cpp[0] * 8;
> +	struct device *dev = dprc->dev;
> +	unsigned int p1_w, p1_h;
> +	u32 val;
> +	int ret;
> +
> +	if (start) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to get RPM: %d\n", ret);
> +			return;
> +		}
> +
> +		dc_dprc_set_stream_id(dprc, stream_id);
> +	}
> +
> +	p1_w = round_up(width, format->cpp[0] == 2 ? 32 : 16);
> +	p1_h = round_up(height, 4);
> +
> +	regmap_write(dprc->reg, FRAME_CTRL0, PITCH(stride));
> +	regmap_write(dprc->reg, FRAME_1P_CTRL0, BYTE_1K);
> +	regmap_write(dprc->reg, FRAME_1P_PIX_X_CTRL, NUM_X_PIX_WIDE(p1_w));
> +	regmap_write(dprc->reg, FRAME_1P_PIX_Y_CTRL, NUM_Y_PIX_HIGH(p1_h));
> +	regmap_write(dprc->reg, FRAME_1P_BASE_ADDR_CTRL0, baddr);
> +	regmap_write(dprc->reg, FRAME_PIX_X_ULC_CTRL, CROP_ULC_X(0));
> +	regmap_write(dprc->reg, FRAME_PIX_Y_ULC_CTRL, CROP_ULC_Y(0));
> +
> +	regmap_write(dprc->reg, RTRAM_CTRL0, THRES_LOW(3) | THRES_HIGH(7));
> +
> +	val = LINE4 | BUF2;
> +	switch (format->format) {
> +	case DRM_FORMAT_XRGB8888:
> +		/*
> +		 * It turns out pixel components are mapped directly
> +		 * without position change via DPR processing with
> +		 * the following color component configurations.
> +		 * Leave the pixel format to be handled by the
> +		 * display controllers.
> +		 */
> +		val |= A_COMP_SEL(3) | R_COMP_SEL(2) |
> +		       G_COMP_SEL(1) | B_COMP_SEL(0);
> +		val |= PIX_SIZE_32BIT;
> +		break;
> +	default:
> +		dev_err(dev, "unsupported format 0x%08x\n", format->format);
> +		return;
> +	}
> +	regmap_write(dprc->reg, MODE_CTRL0, val);
> +
> +	if (start) {
> +		/* software shadow load for the first frame */
> +		val = SW_SHADOW_LOAD_SEL | SHADOW_LOAD_EN;
> +		regmap_write(dprc->reg, SYSTEM_CTRL0, val);
> +
> +		/* and then, run... */
> +		val |= RUN_EN | REPEAT_EN;
> +		regmap_write(dprc->reg, SYSTEM_CTRL0, val);
> +	}
> +
> +	dc_prg_configure(dprc->prg, width, height, prg_stride, bpp, baddr, start);
> +
> +	if (start)
> +		dc_dprc_enable(dprc);
> +
> +	dc_dprc_reg_update(dprc);

[Severity: Medium]
As reported by Sashiko on v7 [1], can dc_dprc_configure() leak the runtime
PM reference on dprc->dev and leave dprc->prg->dev unbalanced or unclocked
when start is true?

In dc_dprc_configure(), pm_runtime_resume_and_get(dev) is called when start
is true before checking format->format. If format->format hits the default
case, the function returns early without calling pm_runtime_put(dev) and
before calling dc_prg_configure(). That leaks the runtime PM reference on
dprc->dev and leaves dprc->prg->dev runtime-suspended; a subsequent call to
dc_dprc_disable() -> dc_prg_disable() will then write to prg->reg while
unclocked and underflow prg->dev's runtime PM usage count.

Similarly, when dc_dprc_configure() calls dc_prg_configure() with start
set to true:

drivers/gpu/drm/imx/dc/dc-prg.c:dc_prg_configure() {
	if (start) {
		ret = pm_runtime_resume_and_get(dev);
		if (ret < 0) {
			dev_err(dev, "failed to get RPM: %d\n", ret);
			return;
		}
	}
}

Because dc_prg_configure() returns void on pm_runtime_resume_and_get()
failure, dc_dprc_configure() proceeds unconditionally to dc_dprc_enable()
-> dc_prg_enable() and dc_dprc_reg_update() -> dc_prg_reg_update(), writing
to prg->reg while prg->clks are disabled and leaving the runtime PM
references between dprc->dev and prg->dev unbalanced.

> +
> +	if (start)
> +		dc_dprc_enable_ctrl_done_irq(dprc);
> +
> +	dev_dbg(dev, "w: %u, h: %u, s: %u, fmt: 0x%08x\n",
> +		width, height, stride, format->format);
> +}
> +
> +void dc_dprc_disable_repeat_en(struct dc_dprc *dprc)
> +{
> +	guard(spinlock)(&dprc->lock);
> +	regmap_write(dprc->reg, IRQ_MASK + SET, IRQ_DPR_CRTL_DONE);
> +
> +	synchronize_hardirq(dprc->wrap_irq);
> +
> +	regmap_write(dprc->reg, SYSTEM_CTRL0 + CLR, REPEAT_EN);
> +	dev_dbg(dprc->dev, "disable REPEAT_EN\n");
> +}

[Severity: High]
As reported by Sashiko on v7 [1], won't holding dprc->lock across
synchronize_hardirq(dprc->wrap_irq) in dc_dprc_disable_repeat_en()
deadlock if dc_dprc_wrap_irq_handler() is concurrently running?

In dc_dprc_disable_repeat_en(), guard(spinlock)(&dprc->lock) holds
dprc->lock for the entire function scope, including while waiting in
synchronize_hardirq(dprc->wrap_irq). If wrap_irq fires on another CPU (either
from DPRC before IRQ_MASK + SET takes effect, or from another device sharing
the IRQF_SHARED interrupt line) and enters dc_dprc_wrap_irq_handler(),
IRQD_IRQ_INPROGRESS is set before dc_dprc_wrap_irq_handler() attempts to
acquire dprc->lock:

CPU 0                                  CPU 1 (hardirq wrap_irq)
dc_dprc_disable_repeat_en()            handle_irq_event()
  guard(spinlock)(&dprc->lock)           /* sets IRQD_IRQ_INPROGRESS */
  ...                                    dc_dprc_wrap_irq_handler()
  synchronize_hardirq(dprc->wrap_irq)      scoped_guard(spinlock, &dprc->lock)
    /* waits for IRQD_IRQ_INPROGRESS */      /* spins waiting for dprc->lock */

Also, if dc_dprc_disable_repeat_en() is ever called with local IRQs enabled,
guard(spinlock) does not disable local interrupts, so wrap_irq on the same
CPU can preempt and self-deadlock on dprc->lock.

In addition, as noted in the comment on dc_dprc_regmap_config,
dc_dprc_disable_repeat_en() is called with local IRQs and preemption
disabled. However, dprc->lock is declared as spinlock_t rather than
raw_spinlock_t, and dc_dprc_wrap_irq_handler() -> dc_dprc_ctrl_done_handle()
-> dc_prg_shadow_enable() writes to prg->reg (whose dc_prg_regmap_config
does not set use_raw_spinlock = true) from hardirq context. On PREEMPT_RT or
with CONFIG_PROVE_RAW_LOCK_NESTING enabled, acquiring a non-raw spinlock_t
in hardirq context or with local IRQs/preemption disabled triggers an
invalid wait-context splat.

Finally, because devm_request_irq() is called without IRQF_NO_THREAD,
dc_dprc_wrap_irq_handler() is force-threaded under PREEMPT_RT or threadirqs.
In that configuration, synchronize_hardirq() does not wait for the IRQ
thread to complete, so dc_dprc_ctrl_done_handle() can still run concurrently
and write REPEAT_EN back to SYSTEM_CTRL0 and SHADOW_EN to prg->reg after
dc_dprc_disable_repeat_en() and dc_dprc_disable().

> +
> +/*
> + * dc_dprc_disable_repeat_en() must be called beforehand to mask
> + * IRQ_DPR_CRTL_DONE and synchronize wrap_irq.
> + */
> +void dc_dprc_disable(struct dc_dprc *dprc)
> +{
> +	dc_prg_disable(dprc->prg);
> +
> +	pm_runtime_put(dprc->dev);
> +
> +	dev_dbg(dprc->dev, "disable\n");
> +}
[ ... ]
> +static irqreturn_t dc_dprc_wrap_irq_handler(int irq, void *data)
> +{
> +	struct dc_dprc *dprc = data;
> +	struct device *dev = dprc->dev;
> +	u32 mask = 0, status = 0;
> +	irqreturn_t result;
> +
> +	pm_runtime_get_noresume(dev);
> +	if (!pm_runtime_active(dev)) {
> +		result = IRQ_NONE;
> +		goto out;
> +	}
> +
> +	scoped_guard(spinlock, &dprc->lock) {
> +		/* cache valid IRQ status */
> +		regmap_read(dprc->reg, IRQ_MASK, &mask);
> +		regmap_read(dprc->reg, IRQ_MASK_STATUS, &status);
> +		status &= ~mask;
> +
> +		/* mask the IRQ(s) being handled */
> +		regmap_write(dprc->reg, IRQ_MASK + SET, status);
> +
> +		/* clear status register */
> +		regmap_write(dprc->reg, IRQ_MASK_STATUS, status);
> +	}
> +
> +	if (status == 0) {
> +		result = IRQ_NONE;
> +		goto out;
> +	}
> +
> +	if (status & DPR2RTR_FIFO_LOAD_BUF_RDY_UV_ERROR)
> +		dev_err(dev, "DPR to RTRAM FIFO load UV buffer ready error\n");
> +
> +	if (status & DPR2RTR_FIFO_LOAD_BUF_RDY_YRGB_ERROR)
> +		dev_err(dev, "DPR to RTRAM FIFO load YRGB buffer ready error\n");
> +
> +	if (status & DPR2RTR_UV_FIFO_OVFL)
> +		dev_err(dev, "DPR to RTRAM FIFO UV FIFO overflow\n");
> +
> +	if (status & DPR2RTR_YRGB_FIFO_OVFL)
> +		dev_err(dev, "DPR to RTRAM FIFO YRGB FIFO overflow\n");
> +
> +	if (status & IRQ_AXI_READ_ERROR)
> +		dev_err(dev, "AXI read error\n");
> +
> +	if (status & IRQ_DPR_CRTL_DONE)
> +		dc_dprc_ctrl_done_handle(dprc);
> +
> +	result = IRQ_HANDLED;
> +out:
> +	pm_runtime_put_noidle(dev);
> +	return result;
> +}

[Severity: High]
As reported by Sashiko on v7 [1], can dc_dprc_wrap_irq_handler() still race
with runtime suspend or leak the active runtime PM state when wrap_irq is
shared?

Because wrap_irq is requested with IRQF_SHARED, another DPRC instance can
trigger dc_dprc_wrap_irq_handler() at any time:

1. In rpm_suspend(), after rpm_check_suspend_allowed() verifies usage_count
   is 0, it sets runtime_status to RPM_SUSPENDING and drops dev->power.lock
   to invoke dc_dprc_runtime_suspend(), which disables dprc->clks. Because
   pm_runtime_get_noresume(dev) and pm_runtime_active(dev) in
   dc_dprc_wrap_irq_handler() do not synchronize with dev->power.lock or
   in-flight suspend, if dc_dprc_wrap_irq_handler() runs right before
   RPM_SUSPENDING is set, it passes the pm_runtime_active(dev) check and
   reads IRQ_MASK and IRQ_MASK_STATUS from dprc->reg while
   dc_dprc_runtime_suspend() concurrently disables dprc->clks.

2. pm_runtime_active(dev) returns true whenever dev->power.disable_depth > 0,
   even if the device is runtime-suspended with its clocks off (for example,
   during late system suspend after __device_suspend_late() disables runtime
   PM, during devres teardown on unbind, or when CONFIG_PM is disabled). Any
   shared IRQ during those windows will read dprc->reg with clocks disabled.

3. If dc_dprc_wrap_irq_handler() increments usage_count from 1 to 2 via
   pm_runtime_get_noresume(dev) while dc_dprc_disable() concurrently calls
   pm_runtime_put(dprc->dev), dc_dprc_disable() drops usage_count from 2 to
   1 without scheduling idle/suspend. When dc_dprc_wrap_irq_handler() then
   calls pm_runtime_put_noidle(dev), usage_count drops from 1 to 0 without
   triggering rpm_idle() or rpm_suspend(), leaving dprc->dev permanently
   active with its clocks enabled.

[1] https://lore.kernel.org/r/20261026-imx8-dc-prefetch-v7-8-a0b50cc4fc65@nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-imx8-dc-prefetch-v8-0-c4f81afd4533@nxp.com?part=8