Re: [PATCH v9 16/16] drm/imx: dc: Use prefetch engine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/imx: dc: missing use_raw_spinlock in dc_prg_regmap_config
- [Medium] drm/imx: dc: premature source checks in dc_crtc_disable_at_boot()
- [Medium] drm/imx: dc: unbalanced DPRC and PRG runtime PM in dc_crtc_disable_at_boot()
--

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

drm/imx: dc: Use prefetch engine

This commit configures and disables prefetch engines (DPR channel and PRGs)
alongside FetchUnits during atomic commits and boot-time cleanup, adapts
FetchUnit burst size and stride fixups for PRG requirements, and enables
use_raw_spinlock in the ExtDst PIXENGCFG regmap configuration.

> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index d03246036d4a7..55c198db9644c 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c

[ ... ]

> @@ -381,16 +464,71 @@ int dc_crtc_disable_at_boot(struct drm_crtc *crtc)
>  		goto out1;
>  	}
>  
> +	crtc_is_enabled = dc_drv_crtc_is_enabled(dc_drm, drm_crtc_index(crtc));
> +
> +	ret = pm_runtime_resume_and_get(dc_drm->pe->dev);
> +	if (ret) {
> +		dc_crtc_err(crtc, "failed to get DC pixel engine RPM: %d\n",
> +			    ret);
> +		goto out2;
> +	}
> +
>  	if (!dc_fg_wait_for_frame_index_moving(dc_crtc->fg)) {
>  		dc_crtc_dbg(crtc, "FrameGen frame index isn't moving\n");
> -		goto out2;
> +		goto out3;
>  	}
>  
> -	dc_crtc_dbg(crtc, "disabling FrameGen at boot\n");
> -	__dc_crtc_disable_fg(crtc);
> +	ret = dc_ed_pec_src_sel_get(dc_crtc->ed_cont, &ed_src);
> +	if (ret || ed_src == LINK_ID_NONE) {
> +		dc_crtc_err(crtc, "failed to get content ExtDst's source\n");
> +		if (ret == 0)
> +			ret = -EINVAL;
> +		goto out3;
> +	}
>  
> -	if (!dc_drv_crtc_is_enabled(dc_drm, drm_crtc_index(crtc)))
> -		goto out2;
> +	if (ed_src == LINK_ID_CONSTFRAME0 || ed_src == LINK_ID_CONSTFRAME1)
> +		goto disable;
> +
> +	ret = dc_lb_pec_dynamic_sec_sel_get(dc_plane->lb, &lb_sec);
> +	if (ret || lb_sec == LINK_ID_NONE) {
> +		dc_crtc_err(crtc,
> +			    "failed to get primary plane LayerBlend secondary source\n");
> +		if (ret == 0)
> +			ret = -EINVAL;
> +		goto out3;
> +	}
> +
> +	/*
> +	 * If CRTC was enabled by bootloader instead of this driver, we don't
> +	 * know if DPRC was enabled or not.  In this case, enforcing DPRC
> +	 * disablement potentially causes imbalanced clock enable count for
> +	 * DPRC and PRG clocks.  So, we presume bootloader doesn't bother to
> +	 * use prefetch engines.
> +	 */
> +	if (crtc_is_enabled)
> +		disable_dprc = true;

[Severity: Medium]
As reported by Sashiko on v8 [1], when the display was enabled by the
bootloader rather than a previous instance of this driver
(!crtc_is_enabled), disable_dprc is never set and ed_src and lb_sec are
not used anywhere else in dc_crtc_disable_at_boot().

However, dc_ed_pec_src_sel_get() and dc_lb_pec_dynamic_sec_sel_get() are
called unconditionally before checking crtc_is_enabled. If the bootloader
enabled the display using only the safety stream (leaving ed_cont at its
reset state), using LayerBlend in primary-only mode, or using a different
LayerBlend or FetchUnit not in sec_sels[], dc_crtc_disable_at_boot()
jumps to out3 without disabling FrameGen and returns an error that aborts
driver probe in dc_drm_bind():

drivers/gpu/drm/imx/dc/dc-drv.c:dc_drm_bind()
	drm_for_each_crtc(crtc, drm) {
		ret = dc_crtc_disable_at_boot(crtc);
		if (ret) {
			dev_err(dev, "failed to disable CRTC%u at boot: %d\n",
				drm_crtc_index(crtc), ret);
			goto err;
		}
	}

Should dc_crtc_disable_at_boot() check !crtc_is_enabled and jump to
disable before querying ed_src and lb_sec?

> +
> +disable:
> +	enable_irq(dc_crtc->irq_dec_seqcomplete);
> +
> +	if (disable_dprc) {
> +		dc_crtc_dbg(crtc, "disabling FrameGen along with DPRC REPEAT_EN at boot\n");
> +		dc_crtc_disable_fg_along_with_dprc_repeat_en(crtc);
> +	} else {
> +		dc_crtc_dbg(crtc, "disabling FrameGen at boot\n");
> +		dc_fg_disable(dc_crtc->fg);
> +	}
> +
> +	DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
> +	disable_irq(dc_crtc->irq_dec_seqcomplete);
> +
> +	if (!crtc_is_enabled)
> +		goto out3;
> +
> +	if (disable_dprc) {
> +		dc_crtc_dbg(crtc, "disabling DPRC at boot\n");
> +		dc_dprc_disable_at_boot(dc_plane->fu->dprc);
> +	}

[Severity: Medium]
When a plane is enabled, dc_dprc_configure() and dc_prg_configure() call
pm_runtime_resume_and_get() on dprc->dev and prg->dev. If the driver is
unbound or unloaded while the plane is active, dc_drm_unbind() calls
drm_dev_unplug() before drm_atomic_helper_shutdown(), so
dc_crtc_atomic_disable() exits early at drm_dev_enter() without calling
dc_dprc_disable().

Unlike de->dev and pe->dev, which are child devices created by
devm_of_platform_populate() in dc_probe() and recreated on rebind,
dprc->dev and prg->dev are top-level platform devices whose struct device
and power.usage_count persist across unbind/rebind and module reload.

When dc_crtc_disable_at_boot() subsequently runs on rebind or module
reload, dc_dprc_disable_at_boot() and dc_prg_disable_at_boot() disable the
clocks directly without balancing runtime PM:

drivers/gpu/drm/imx/dc/dc-dprc.c:
void dc_dprc_disable_at_boot(struct dc_dprc *dprc)
{
	dc_prg_disable_at_boot(dprc->prg);

	clk_bulk_disable_unprepare(dprc->num_clks, dprc->clks);

	dev_dbg(dprc->dev, "disable at boot\n");
}

drivers/gpu/drm/imx/dc/dc-prg.c:
void dc_prg_disable_at_boot(struct dc_prg *prg)
{
	regmap_write(prg->reg, PRG_CTRL, BYPASS);

	clk_bulk_disable_unprepare(prg->num_clks, prg->clks);
}

Won't this leave dprc->dev and prg->dev with an elevated runtime PM
usage_count after module reload (preventing runtime suspend on future
plane disablement), or leave them marked active with their clocks disabled
across a sysfs unbind/rebind of the master device so that the next
dc_dprc_configure() accesses unclocked registers?

> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c
> index 42c0a46281cb1..ba20d35355155 100644
> --- a/drivers/gpu/drm/imx/dc/dc-plane.c
> +++ b/drivers/gpu/drm/imx/dc/dc-plane.c

[ ... ]

> @@ -193,6 +220,11 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_commit *state)
>  
>  	dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu));
>  
> +	prefetch_start = needs_modeset || !old_state->fb ||
> +			 (!old_state->visible && new_state->visible);
> +	dc_dprc_configure(fu->dprc, new_state->crtc->index, src_w, src_h,
> +			  fb->pitches[0], fb->format, baseaddr, prefetch_start);

[Severity: High]
When dc_plane_atomic_update() calls dc_dprc_configure() with
prefetch_start set to true, dc_dprc_enable_ctrl_done_irq() unmasks the
control done interrupt on dprc->wrap_irq.

In dc_dprc_probe(), dprc->wrap_irq is requested with IRQF_NO_THREAD, so
dc_dprc_wrap_irq_handler() runs in hardirq context and calls into PRG
regmap:

dc_dprc_wrap_irq_handler()
  -> dc_dprc_ctrl_done_handle()
    -> dc_prg_shadow_enable()
      -> regmap_write(prg->reg, PRG_CTRL + SET, SHADOW_EN)

However, dc_prg_regmap_config in drivers/gpu/drm/imx/dc/dc-prg.c does not
set use_raw_spinlock:

static const struct regmap_config dc_prg_regmap_config = {
	.reg_bits = 32,
	.reg_stride = 4,
	.val_bits = 32,
	.fast_io = true,
	.max_register = PRG_WIDTH + TOG,
};

Without .use_raw_spinlock = true in dc_prg_regmap_config, won't
regmap_write() on prg->reg acquire a sleeping spinlock_t inside hardirq
context on preempt-rt kernels and trigger an invalid wait context warning
under lockdep?

[1] https://lore.kernel.org/r/20261030-imx8-dc-prefetch-v8-15-c4f81afd4533@nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-imx8-dc-prefetch-v9-0-3b985e7636e2@nxp.com?part=16