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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [High] Read-modify-write (RMW) operations on Write-1-to-Clear (W1C) interrupt status registers cause lost interrupts.
- [High] Module reference count underflow and use-after-free caused by unbalanced `drm_bridge_put`.
- [High] CRTC `bus_format` and `bus_flags` state is corrupted and permanently lost during plane-only atomic updates.
- [High] Unprotected hardware register accesses in the IRQ handler risk bus errors on runtime suspended devices.
- [High] System resume fails to enable the display because `pm_runtime_resume_and_get` is called while PM transitions are disabled.
- [High] An unhandled pending vblank event in `dcif->event` during CRTC disable causes userspace to hang indefinitely and memory to leak.
- [High] Unlocked access to `crtc->state` in `dcif_crc_source_parse` causes a data race and potential use-after-free.
- [High] Failure of `pm_runtime_resume_and_get` causes the atomic commit to hang permanently.
- [High] Use of sleeping functions and inappropriate lock guards in atomic context paths.
- [High] Ignoring the return value of `drm_crtc_vblank_get()` causes vblank reference count underflow on failure.
- [High] Disabling Layer 0 without triggering `SHADOW_LOAD_EN` causes an abrupt hardware stop and AXI bus lockup risks.
- [High] `drm_crtc_vblank_on()` is called before software reset, permanently breaking vblank interrupts.
- [Medium] Dynamic plane color encoding and color range updates are silently ignored.
- [Medium] Transitions between active CRC sources (e.g., updating ROI coordinates) are ignored.
- [Medium] Data race (TOCTOU) on `dcif->crc_is_enabled` causes garbage CRC reads.
--

commit 4328341d625e1f0fbf8be8ca733a73ea0e76115e
Author: Sandor Yu <sandor.yu@nxp.com>

drm/imx: Add support for i.MX94 DCIF

This commit introduces DRM support for the i.MX94 Display Control
Interface (DCIF). It includes core CRTC, plane, and KMS integration,
along with CRC generation support.

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-crc.h b/drivers/gpu/drm/imx/dcif/dcif-crc.h
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-crc.h
[ ... ]
> +static inline bool enable_dcif_crc_needed(struct dcif_crtc_state *new_dcstate,
> +					  struct dcif_crtc_state *old_dcstate)
> +{
> +	return old_dcstate->crc.source == DCIF_CRC_SRC_NONE &&
> +	       new_dcstate->crc.source != DCIF_CRC_SRC_NONE;
> +}

[Severity: Medium]
Does this prevent updates when transitioning directly between two active
CRC configurations? If userspace modifies the ROI region while keeping the
source active, both helpers will return false, meaning the hardware might
continue capturing the old ROI coordinates.

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-crc.c b/drivers/gpu/drm/imx/dcif/dcif-crc.c
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-crc.c
[ ... ]
> +void dcif_crc_source_disable(struct dcif_dev *dcif, int ncrc)
> +{
[ ... ]
> +	regmap_clear_bits(dcif->regmap, DCIF_CRC_CTRL, DCIF_CRC_CTRL_CRC_EN(ncrc));
> +
> +	dcif->crc_is_enabled = false;
> +}

[Severity: Medium]
Is there a data race on the crc_is_enabled flag here? This flag is modified
without holding the event_lock, while the IRQ handler evaluates it with the
lock held. This could allow the module to power off while the IRQ handler
attempts to read the disabled register, generating spurious payload data.

[ ... ]
> +static int dcif_crc_source_parse(struct drm_crtc *crtc, const char *source_name,
> +				 enum dcif_crc_source *s, struct drm_rect *roi)
> +{
> +	static const char roi_prefix[] = "roi:";
> +
> +	if (!source_name) {
> +		*s = DCIF_CRC_SRC_NONE;
> +	} else if (!strcmp(source_name, "auto")) {
> +		struct drm_display_mode *mode = &crtc->state->adjusted_mode;

[Severity: High]
Can this unlocked access to the atomic state lead to a use-after-free? This
parser is called before acquiring any connection locks. A concurrent atomic
commit could swap and free the atomic state while this thread is referencing
the adjusted_mode.

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-crtc.c b/drivers/gpu/drm/imx/dcif/dcif-crtc.c
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-crtc.c
[ ... ]
> +static void dcif_disable_controller(struct dcif_dev *dcif)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	/* Disable layer 0 */
> +	regmap_clear_bits(dcif->regmap, DCIF_CTRLDESC0(0), DCIF_CTRLDESC0_EN);

[Severity: High]
Does clearing the enable bit without also triggering the shadow load bit
leave the layer hardware active? The hardware typically ignores the register
change without the shadow load bit, meaning it will continue fetching data
via DMA. Severing the display controller abruptly while DMA is active could
risk AXI bus lockups.

[ ... ]
> +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;

[Severity: High]
What happens if drm_crtc_vblank_get returns an error? The code continues to
queue the event without having incremented the vblank reference count. When
the interrupt handler later processes the event and calls the corresponding
put function, it would cause a reference count underflow.

[ ... ]
> +static void dcif_crtc_query_output_bus_format(struct drm_crtc *crtc,
> +					      struct drm_crtc_state *crtc_state)
> +{
> +	struct dcif_crtc_state *dcif_state = to_dcif_crtc_state(crtc_state);
> +	struct drm_bridge *bridge __free(drm_bridge_put) = NULL;

[Severity: High]
Will the cleanup attribute here cause a use-after-free? The bridge is
obtained via drm_bridge_chain_get_first_bridge, which does not increment
the module reference count. When the function returns, drm_bridge_put will
decrement a reference that was never acquired, potentially allowing the
bridge module to unload prematurely.

> +	struct drm_bridge_state *bridge_state;
> +
> +	dcif_state->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	dcif_state->bus_flags = 0;
> +
> +	bridge = dcif_crtc_get_bridge(crtc, crtc_state);
> +	if (!bridge)
> +		return;

[Severity: High]
Could this overwrite destroy the existing bus_format and bus_flags during
a plane-only update? In a plane update, the bridge is not part of the atomic
commit state, causing the bridge state query below to return NULL. Returning
early leaves the state permanently overwritten with default values.

[ ... ]
> +static void dcif_crtc_atomic_enable(struct drm_crtc *crtc,
> +				    struct drm_atomic_commit *state)
> +{
[ ... ]
> +	/* 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);
> +		return;
> +	}

[Severity: High]
Is pm_runtime_resume_and_get safe to call here? This is an atomic context
path during non-blocking commits, and this PM function can sleep.

Additionally, if this path executes during system resume, runtime PM
transitions are disabled and it will return an error, leaving the display
blanked.

If it does return early, the pending event in crtc->state->event is never
consumed. The DRM core expects the driver to consume the event, and
abandoning it here will cause the atomic commit to hang indefinitely
waiting for a flip completion.

> +	dcif->crtc_pm_enabled = true;
> +
> +	drm_crtc_vblank_on(crtc);
> +
> +	dcif_crtc_mode_set_nofb(crtc_state, plane_state);

[Severity: High]
Will the software reset performed inside dcif_crtc_mode_set_nofb destroy the
vblank interrupt enablement? Since drm_crtc_vblank_on is called right before
the reset, the hardware state and interrupt enable bits will be cleared,
permanently breaking vblanks.

[ ... ]
> +static void dcif_crtc_atomic_disable(struct drm_crtc *crtc,
> +				     struct drm_atomic_commit *state)
> +{
[ ... ]
> +	if (dcif->crtc_pm_enabled) {
> +		dcif->crtc_pm_enabled = false;
> +		pm_runtime_put_sync(drm->dev);
> +	}
> +
> +	scoped_guard(spinlock_irq, &crtc->dev->event_lock) {
> +		if (crtc->state->event && !crtc->state->active) {
> +			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +			crtc->state->event = NULL;
> +		}
> +	}
> +}

[Severity: High]
Similar to the enable path, pm_runtime_put_sync is a sleeping function
called in an atomic context.

Also, does the use of the spinlock_irq scoped guard here cause problems?
When exiting the scoped guard, it unconditionally re-enables hardware
interrupts, which breaks the caller's atomic execution context.

Finally, if a non-blocking commit queued an event into dcif->event, it
seems to be ignored here. If the hardware interrupts are turned off, the
pending event in dcif->event is stranded and never signaled, leaving
userspace blocked.

[ ... ]
> +static int dcif_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> +	struct dcif_dev *dcif = crtc_to_dcif_dev(crtc);
> +	int domain = dcif->cpu_domain;
> +
> +	/* Clear and enable VS_BLANK IRQ */
> +	regmap_set_bits(dcif->regmap, DCIF_IS0(domain), DCIF_INT0_VS_BLANK);
> +	regmap_set_bits(dcif->regmap, DCIF_IE0(domain), DCIF_INT0_VS_BLANK);

[Severity: High]
Does using a read-modify-write helper on a Write-1-to-Clear register cause
lost interrupts? regmap_set_bits will read the register, OR the new bit, and
write it back, which inadvertently clears any already pending interrupts
before the handler can process them. W1C registers usually need to be
written directly.

[ ... ]
> +static void dcif_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct dcif_dev *dcif = crtc_to_dcif_dev(crtc);
> +	int domain = dcif->cpu_domain;
> +
> +	/* Disable and clear VS_BLANK IRQ */
> +	regmap_clear_bits(dcif->regmap, DCIF_IE0(domain), DCIF_INT0_VS_BLANK);
> +	regmap_clear_bits(dcif->regmap, DCIF_IS0(domain), DCIF_INT0_VS_BLANK);
> +}

[Severity: High]
The same applies here. regmap_clear_bits will write a 1 to all pending
interrupts while writing a 0 to the target bit, improperly clearing unrelated
interrupts and failing to clear the target bit.

[ ... ]
> +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);

[Severity: High]
Is it safe to read hardware registers here without verifying the power
management active state? If the device is in runtime suspend and its clocks
are turned off, a spurious interrupt could trigger this handler, and
accessing unclocked registers can cause bus faults or system lockups. Should
this be protected with pm_runtime_get_if_active?

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-plane.c b/drivers/gpu/drm/imx/dcif/dcif-plane.c
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-plane.c
[ ... ]
> +static int dcif_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_commit *state)
> +{
[ ... ]
> +	/*
> +	 * Force CRTC mode change if framebuffer stride or pixel format have changed.
> +	 */
> +	if (plane->type == DRM_PLANE_TYPE_PRIMARY && old_fb &&
> +	    (fb->pitches[0] != old_fb->pitches[0] || fb->format->format != old_fb->format->format))
> +		crtc_state->mode_changed = true;

[Severity: Medium]
Will dynamic updates to plane color encoding or color ranges be silently
ignored? The hardware CSC coefficients are programmed during a full modeset,
but this check does not flag a mode change if only the color properties are
modified.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-dcif-upstreaming-v9-0-8d0ff89aa3c5@oss.nxp.com?part=5