Re: [PATCH v10 5/9] drm/imx: Add support for i.MX94 DCIF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/imx/dcif: unpowered MMIO access crash if PM resume fails in atomic_enable
- [High] drm/imx/dcif: vblank refcount underflow and memory leak via manual event caching
- [High] drm/imx/dcif: NULL pointer dereference in IRQ handler during probe
- [Medium] drm/imx/dcif: premature vblank event delivery exposes race with plane updates
- [Medium] drm/imx/dcif: back-to-back software reset may cause incomplete hardware reset
--

commit 5b82f4222050053f161ded1c655289268046892b
Author: Sandor Yu <sandor.yu@nxp.com>

drm/imx: Add support for i.MX94 DCIF

The i.MX94 Display Control Interface features:
 * Up to maximum 3 layers of alpha blending:
    - 1 background layer(Layer 0);
    - 1 foreground layer(Layer 1);
    - A programmable constant color behind the background layer;
 * Each layer supports:
    - programmable plane size;
    - programmable background color;
    - embedded alpha and global alpha;
 * Data output with CRC checksum for 4 programmable regions;

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-crtc.c b/drivers/gpu/drm/imx/dcif/dcif-crtc.c
> index 0000000000000..07016b5fc700d 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-crtc.c
> @@ -0,0 +1,763 @@

[ ... ]

> +static void dcif_reset_block(struct dcif_dev *dcif)
> +{
> +	regmap_set_bits(dcif->regmap, DCIF_DISP_CTRL, DCIF_DISP_CTRL_SW_RST);
> +
> +	regmap_clear_bits(dcif->regmap, DCIF_DISP_CTRL, DCIF_DISP_CTRL_SW_RST);

[Severity: Medium]
Will this 1-cycle reset pulse be sufficient for full pipeline resets across
all clock domains? Since this toggles the bit via back-to-back regmap
writes without any delay or status polling, the hardware might remain in an
indeterminate state.

> +}

[ ... ]

> +static void dcif_crtc_queue_state_event(struct drm_crtc *crtc)
> +{
> +	struct dcif_dev *dcif = crtc_to_dcif_dev(crtc);
> +
> +	scoped_guard(spinlock_irq, &crtc->dev->event_lock) {
> +		if (crtc->state->event) {
> +			WARN_ON(drm_crtc_vblank_get(crtc));
> +			WARN_ON(dcif->event);
> +			dcif->event = crtc->state->event;
> +			crtc->state->event = NULL;

[Severity: High]
Does this code leak the vblank event and underflow the refcount if
drm_crtc_vblank_get() fails? Even if the WARN_ON triggers, the event is
manually cached and the loop continues. Later, dcif_irq_handler() will
blindly call drm_crtc_vblank_put(), underflowing the refcount. 

If the hardware times out and no VSYNC fires, the event is leaked when
overwritten or ignored during disable. Could this use
drm_crtc_arm_vblank_event() instead of manual caching?

> +		}
> +	}
> +}

[ ... ]

> +static void dcif_crtc_atomic_enable(struct drm_crtc *crtc,
> +				    struct drm_atomic_commit *state)
> +{
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, crtc->primary);
> +	struct dcif_crtc_state *dcif_crtc_state = to_dcif_crtc_state(crtc_state);
> +	struct drm_display_mode *adj = &crtc_state->adjusted_mode;
> +	struct dcif_dev *dcif = crtc_to_dcif_dev(crtc);
> +	struct drm_device *drm = crtc->dev;
> +	dma_addr_t baseaddr;
> +	int ret;
> +
> +	dev_dbg(drm->dev, "mode " DRM_MODE_FMT "\n", DRM_MODE_ARG(adj));
> +
> +	/* enable power when we start to set mode for CRTC */
> +	ret = pm_runtime_resume_and_get(drm->dev);
> +	if (ret < 0) {
> +		drm_err(drm, "failed to resume DCIF, ret = %d\n", ret);
> +
> +		/* Don't leave the commit waiting for a flip done that never comes. */
> +		scoped_guard(spinlock_irq, &drm->event_lock) {
> +			if (crtc->state->event) {
> +				drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +				crtc->state->event = NULL;
> +			}
> +		}
> +		return;

[Severity: High]
If this returns early, will the DRM atomic helpers proceed to call
dcif_plane_atomic_update() anyway? Since the helpers ignore this void
return, plane updates will write to unpowered registers (e.g.,
DCIF_CTRLDESC1) and the subsequent dcif_crtc_atomic_disable() will call
dcif_disable_controller() which also accesses registers, causing a kernel
panic from unpowered MMIO access.

> +	}
> +	dcif->crtc_pm_enabled = true;
> +
> +	dcif_crtc_mode_set_nofb(crtc_state, plane_state);
> +
> +	drm_crtc_vblank_on(crtc);
> +
> +	baseaddr = drm_fb_dma_get_gem_addr(plane_state->fb, plane_state, 0);
> +	if (baseaddr)
> +		regmap_write(dcif->regmap, DCIF_CTRLDESC4(0), baseaddr);
> +
> +	dcif_enable_plane_panic(dcif);
> +	dcif_enable_controller(dcif);
> +
> +	dcif_crtc_queue_state_event(crtc);

[Severity: Medium]
Since the controller is already enabled at this point, could a VSYNC
interrupt fire and deliver the event to userspace prematurely? Userspace
might receive the completion event before drm_atomic_helper_commit_planes()
finishes calling atomic_update on overlay planes, potentially causing
framebuffers to be released too early.

> +
> +	if (dcif->has_crc && dcif_crtc_state->crc.source != DCIF_CRC_SRC_NONE) {
> +		struct drm_rect roi;
> +
> +		dcif_crtc_get_crc_roi(crtc_state, &roi);
> +		dcif_crc_source_enable(dcif, dcif_crtc_state->crc.source, &roi, 0);
> +	}
> +}

[ ... ]

> +irqreturn_t dcif_irq_handler(int irq, void *data)
> +{
> +	struct drm_device *drm = data;
> +	struct dcif_dev *dcif = to_dcif_dev(drm);
> +	int domain = dcif->cpu_domain;
> +	u32 stat0, stat1, crc;
> +
> +	regmap_read(dcif->regmap, DCIF_IS0(domain), &stat0);
> +	regmap_read(dcif->regmap, DCIF_IS1(domain), &stat1);
> +	regmap_write(dcif->regmap, DCIF_IS0(domain), stat0);
> +	regmap_write(dcif->regmap, DCIF_IS1(domain), stat1);
> +
> +	if (stat0 & DCIF_INT0_VS_BLANK) {
> +		drm_crtc_handle_vblank(&dcif->crtc);

[Severity: High]
Could this dereference a NULL crtc->dev? In dcif_probe(),
devm_request_irq() is called before the CRTC is initialized by
dcif_kms_prepare(). If the bootloader left the display active, a pending
VSYNC interrupt could fire immediately, passing the uninitialized CRTC to
drm_crtc_handle_vblank().

> +
> +		scoped_guard(spinlock_irqsave, &drm->event_lock) {
> +			if (dcif->event) {

[ ... ]

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-drv.c b/drivers/gpu/drm/imx/dcif/dcif-drv.c
> index 0000000000000..ddc2614656a79 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-drv.c
> @@ -0,0 +1,238 @@

[ ... ]

> +static int dcif_probe(struct platform_device *pdev)
> +{
> +	struct dcif_dev *dcif;
> +	struct drm_device *drm;
> +	int ret;
> +	int i;

[ ... ]

> +	ret = devm_request_irq(drm->dev, dcif->irq[dcif->cpu_domain],
> +			       dcif_irq_handler, 0, drm->driver->name, drm);
> +	if (ret < 0)
> +		return dev_err_probe(drm->dev, ret, "failed to install IRQ handler\n");
> +
> +	dcif_read_chip_info(dcif);
> +
> +	ret = dcif_kms_prepare(dcif);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-dcif-upstreaming-v10-0-bcfb28de0aae@oss.nxp.com?part=5