Re: [PATCH v11 4/7] drm/imx: Add support for i.MX94 DCIF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] drm/imx/dcif: unclocked MMIO access when runtime PM resume
  fails
- [Medium] drm/imx: dcif: missing vblank reference in
  dcif_crc_source_enable()
- [Medium] drm/imx/dcif: unmasked interrupt status handling in
  dcif_irq_handler()
- [Medium] drm/imx: dcif: unchecked runtime PM resume in
  dcif_hw_probe_init()
- [Medium] drm/imx: dcif: unbalanced clock disable if
  dcif_runtime_resume() fails
- [Low] drm/imx: dcif: signed overflow in dcif_crc_source_parse() ROI
  check
- [Low] drm/imx/dcif: split __free() declaration in
  dcif_crtc_query_output_bus_format()
- [Low] drm/imx: dcif: missing fb->pitches[0] bounds check in
  dcif_plane_atomic_check()
--

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

drm/imx: Add support for i.MX94 DCIF

This commit adds DRM/KMS driver support for the NXP i.MX94 Display Control
Interface (DCIF), including CRTC, primary and overlay planes, and CRC
generation.

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-crc.c b/drivers/gpu/drm/imx/dcif/dcif-crc.c
> new file mode 100644
> index 0000000000000..132e40d3f3d45
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-crc.c
[ ... ]
> +void dcif_crc_source_enable(struct dcif_dev *dcif, enum dcif_crc_source source,
> +			    struct drm_rect *roi, int ncrc)
> +{
> +	if (ncrc >= MAX_DCIF_CRC_NUM)
> +		return;
> +
> +	if (source == DCIF_CRC_SRC_NONE)
> +		return;
> +
> +	if (dcif->crc_is_enabled)
> +		return;
> +
> +	dcif_crc_config(dcif, roi, ncrc);
> +
> +	regmap_set_bits(dcif->regmap, DCIF_CRC_CTRL,
> +			DCIF_CRC_CTRL_CRC_MODE | DCIF_CRC_CTRL_CRC_SHADOW_LOAD_EN |
> +			DCIF_CRC_CTRL_CRC_TRIG);
> +
> +	scoped_guard(spinlock_irqsave, &dcif->drm.event_lock)
> +		dcif->crc_is_enabled = true;
> +}
> +
> +void dcif_crc_source_disable(struct dcif_dev *dcif, int ncrc)
> +{
> +	if (!dcif->crc_is_enabled)
> +		return;
> +
> +	if (ncrc >= MAX_DCIF_CRC_NUM)
> +		return;
> +
> +	regmap_clear_bits(dcif->regmap, DCIF_CRC_CTRL, DCIF_CRC_CTRL_CRC_EN(ncrc));
> +
> +	scoped_guard(spinlock_irqsave, &dcif->drm.event_lock)
> +		dcif->crc_is_enabled = false;
> +}

[Severity: Medium]
Should dcif_crc_source_enable() and dcif_crc_source_disable() acquire and
release a vblank reference with drm_crtc_vblank_get() and
drm_crtc_vblank_put()?

dcif_irq_handler() only samples DCIF_CRC_VAL_R(0) and calls
drm_crtc_add_crc_entry() when DCIF_INT0_VS_BLANK fires, and that interrupt
is only unmasked while a vblank reference is held.  On a steady display
without continuous page flips, the vblank disable timer will mask
DCIF_INT0_VS_BLANK after the atomic commit that enables CRC finishes,
causing reads from debugfs crc/data to block indefinitely.

[ ... ]

> +		while ((opt = strsep(&options, ",")) != NULL) {
> +			if (i > 3)
> +				return -EINVAL;
> +
> +			ret = kstrtouint(opt, 10, &params[i]);
> +			if (ret < 0)
> +				return ret;
> +
> +			i++;
> +		}
> +
> +		if (i != 4)
> +			return -EINVAL;
> +
> +		roi->x1 = params[0];
> +		roi->y1 = params[1];
> +		roi->x2 = params[2];
> +		roi->y2 = params[3];
> +
> +		if (!drm_rect_visible(roi))
> +			return -EINVAL;

[Severity: Low]
Can unsigned values in params[] wrap to negative ints in struct drm_rect
and bypass the ROI bounds checks?

For example, "roi:10,0,2147483648,10" sets roi->x1 = 10 and
roi->x2 = INT_MIN, which wraps in drm_rect_visible() to a positive width
and then passes dcif_crtc_check_crc_roi() because roi->x1 >= 0 and
roi->x2 <= m->hdisplay, programming truncated 12-bit coordinates into
DCIF_CRC_POS_R and DCIF_CRC_SIZE_R.

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-crtc.c b/drivers/gpu/drm/imx/dcif/dcif-crtc.c
> new file mode 100644
> index 0000000000000..d5285105fa4df
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-crtc.c
[ ... ]
> +static int 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;
> +	struct drm_bridge_state *bridge_state;
> +	u32 bus_format;
> +
> +	dcif_state->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	dcif_state->bus_flags = 0;
> +
> +	bridge = dcif_crtc_get_bridge(crtc, crtc_state);

[Severity: Low]
This isn't a bug, but per Cleanup API conventions, bridge should be defined
and initialized in a single statement rather than declared with
__free(drm_bridge_put) = NULL at the top of the function and assigned
later.

[ ... ]

> +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 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;
> +	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);
> +		return;
> +	}
> +	dcif->crtc_pm_enabled = true;
[ ... ]
> +}
> +
> +static void dcif_crtc_atomic_disable(struct drm_crtc *crtc,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct dcif_crtc_state *dcif_crtc_state = to_dcif_crtc_state(crtc_state);
> +	struct dcif_dev *dcif = crtc_to_dcif_dev(crtc);
> +	struct drm_device *drm = crtc->dev;
> +
> +	if (dcif->has_crc && dcif_crtc_state->crc.source != DCIF_CRC_SRC_NONE)
> +		dcif_crc_source_disable(dcif, 0);
> +
> +	dcif_disable_controller(dcif);
> +	dcif_disable_plane_panic(dcif);
> +
> +	drm_crtc_vblank_off(crtc);
> +
> +	if (dcif->crtc_pm_enabled) {
> +		dcif->crtc_pm_enabled = false;
> +		pm_runtime_put_sync(drm->dev);
> +	}

[Severity: High]
If pm_runtime_resume_and_get() fails in dcif_crtc_atomic_enable(), won't
subsequent callbacks still access DCIF MMIO registers while the clocks and
power domain are off?

After dcif_crtc_atomic_enable() returns early with dcif->crtc_pm_enabled
set to false, drm_atomic_helper_commit_tail_rpm() still runs
drm_atomic_helper_commit_planes() because crtc_state->active is true,
invoking dcif_plane_atomic_update() and dcif_crtc_atomic_flush() without
checking dcif->crtc_pm_enabled.  Later, dcif_crtc_atomic_disable() also
calls dcif_crc_source_disable(), dcif_disable_controller(), and
dcif_disable_plane_panic() before checking dcif->crtc_pm_enabled, causing
unclocked MMIO accesses and an external abort on ARM64.

[ ... ]

> +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);
> +
> +		scoped_guard(spinlock_irqsave, &drm->event_lock) {
> +			if (dcif->crc_is_enabled) {
> +				regmap_read(dcif->regmap, DCIF_CRC_VAL_R(0), &crc);
> +				drm_crtc_add_crc_entry(&dcif->crtc, false, 0, &crc);
> +				dev_dbg(drm->dev, "crc=0x%x\n",  crc);
> +			}
> +		}
> +	}
> +
> +	if (stat1 & (DCIF_INT1_FIFO_PANIC0 | DCIF_INT1_FIFO_PANIC1)) {
> +		u32 panic = stat1 & (DCIF_INT1_FIFO_PANIC0 | DCIF_INT1_FIFO_PANIC1);
> +
> +		dev_dbg_ratelimited(drm->dev, "FIFO panic on %s\n",
> +				    panic == (DCIF_INT1_FIFO_PANIC0 | DCIF_INT1_FIFO_PANIC1) ?
> +				    "layers 0 & 1" : panic == DCIF_INT1_FIFO_PANIC0 ? "layer 0" :
> +				    "layer 1");
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Should dcif_irq_handler() mask stat0 and stat1 with DCIF_IE0(domain) and
DCIF_IE1(domain), and return IRQ_NONE when no enabled interrupt is
pending?

DCIF_IS0(domain) latches DCIF_INT0_VS_BLANK on every vertical blank even
when it is masked in DCIF_IE0(domain), which is why
dcif_crtc_enable_vblank() clears DCIF_IS0(domain) before enabling it.
Because FIFO panic interrupts remain enabled in DCIF_IE1(domain) whenever
the CRTC is active, a FIFO panic interrupt firing mid-frame while vblanks
are disabled will see the latched DCIF_INT0_VS_BLANK bit in stat0 and
spuriously call drm_crtc_handle_vblank() and drm_crtc_add_crc_entry().

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-drv.c b/drivers/gpu/drm/imx/dcif/dcif-drv.c
> new file mode 100644
> index 0000000000000..3b1b9dbf0c6b0
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dcif/dcif-drv.c
[ ... ]
> +static void dcif_hw_probe_init(struct dcif_dev *dcif)
> +{
> +	struct drm_device *drm = &dcif->drm;
> +	u32 val, vmin, vmaj;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(drm->dev);
> +	if (ret < 0) {
> +		drm_err(drm, "failed to resume DCIF: %d\n", ret);
> +		return;
> +	}
[ ... ]
> +static int dcif_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	dcif_hw_probe_init(dcif);
> +
> +	ret = dcif_kms_prepare(dcif);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_irq(drm->dev, dcif->irq[dcif->cpu_domain],
> +			       dcif_irq_handler, 0, drm->driver->name, drm);

[Severity: Medium]
Should dcif_hw_probe_init() return an error code so dcif_probe() can abort
when pm_runtime_resume_and_get() fails?

If runtime resume fails here, dcif_hw_probe_init() returns early without
reading DCIF_VER, masking bootloader interrupts, or stopping the
controller, and dcif_probe() continues on to request the IRQ and register
the DRM device with uninitialized hardware state.  Also, calling
dcif_hw_probe_init() before dcif_kms_prepare() powers on and disables the
hardware before devm_drm_of_get_bridge() checks for probe deferral and
before dcif_kms_link_suppliers() sets up the supplier device links.

[ ... ]

> +static int dcif_suspend(struct device *dev)
> +{
> +	struct dcif_dev *dcif = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = drm_mode_config_helper_suspend(&dcif->drm);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	return dcif_runtime_suspend(dev);
> +}
> +
> +static int dcif_resume(struct device *dev)
> +{
> +	struct dcif_dev *dcif = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!pm_runtime_suspended(dev)) {
> +		ret = dcif_runtime_resume(dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return drm_mode_config_helper_resume(&dcif->drm);
> +}

[Severity: Medium]
If dcif_runtime_resume() fails in dcif_resume(), won't the runtime PM core
call dcif_runtime_suspend() a second time on already disabled clocks?

When the CRTC is active before system suspend, device_prepare() increments
the runtime PM usage count so drm_mode_config_helper_suspend() ->
pm_runtime_put_sync() leaves the device RPM_ACTIVE, and dcif_suspend()
then calls dcif_runtime_suspend() directly without updating the runtime PM
status.  If dcif_runtime_resume() fails during system resume, the clocks
remain disabled while runtime_status is still RPM_ACTIVE, so
device_complete() -> pm_runtime_put() will invoke dcif_runtime_suspend() ->
clk_bulk_disable_unprepare() again.  Using pm_runtime_force_suspend() and
pm_runtime_force_resume() would keep the runtime PM state in sync with the
clocks.

> diff --git a/drivers/gpu/drm/imx/dcif/dcif-plane.c b/drivers/gpu/drm/imx/dcif/dcif-plane.c
> new file mode 100644
> index 0000000000000..9f54f1b2d8d17
> --- /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)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct dcif_dev *dcif = plane_to_dcif_dev(plane);
> +	struct drm_framebuffer *fb = new_plane_state->fb;
> +	struct drm_crtc_state *crtc_state;
> +	int ret;
> +
> +	if (!fb)
> +		return 0;
[ ... ]
> +	if (new_plane_state->fb->format->has_alpha &&
> +	    new_plane_state->pixel_blend_mode == DRM_MODE_BLEND_COVERAGE &&
> +	    new_plane_state->alpha != DRM_BLEND_ALPHA_OPAQUE)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: Low]
Should dcif_plane_atomic_check() reject framebuffers with
fb->pitches[0] > 0xffff?

DCIF_CTRLDESC3_PITCH() is a 16-bit field (GENMASK(15, 0)), so a
framebuffer stride >= 65536 bytes is silently truncated in
dcif_plane_atomic_update(), corrupting scanout.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-dcif-upstreaming-v11-0-9e13c85ae0d8@oss.nxp.com?part=4