Re: [PATCH v6 07/13] drm/imx: dc: Use TCON operation mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Regmap accesses inside the IRQ-disabled section use a non-raw spinlock, triggering a sleep-in-atomic bug on PREEMPT_RT.
- [High] TCON hardware is programmed using logical mode timings instead of the required adjusted CRTC physical timings.

Pre-existing issues:
- [High] Polling jiffies in a busy loop with local interrupts disabled leads to a hard lockup.
--

commit 1e5ba5ba9464ec8a1530f337a9b8808253bcb887
Author: Liu Ying <victor.liu@nxp.com>
drm/imx: dc: Use TCON operation mode

This commit introduces TCON operation mode to generate customized output
timings by ignoring FrameGen sync signals. It switches TCON to operation
mode during CRTC enable to allow the prefetch engine to evade the first
dumb frame.

> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 71694f1244e17..eae9bd164b230 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
[ ... ]
> @@ -273,7 +285,22 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
[ ... ]
> +	/* Don't relinquish CPU until TCON is set to operation mode. */
> +	local_irq_save(flags);
> +	preempt_disable();
>  
>  	dc_fg_enable(dc_crtc->fg);
> +	/*
> +	 * Turn TCON into operation mode as soon as the first dumb
> +	 * frame is generated by DC(we don't relinquish CPU to ensure
> +	 * this).  This makes DPR/PRG be able to evade the frame.
> +	 */
> +	DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);

[Severity: High]
This is a pre-existing issue, but duplicating this pattern here might lead
to a hard lockup.

The DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING macro eventually calls
dc_fg_wait_for_frame_index_moving(), which polls the hardware state in a
do-while loop bounded by time_before(jiffies, timeout).

Because local interrupts are disabled here via local_irq_save(), the timer
tick cannot run. If this CPU is responsible for updating jiffies (or on a
UP system), jiffies will never increment. If the hardware frame index
fails to move, will this loop spin infinitely and trigger a watchdog or
CPU stall?

(This flawed pattern already existed in dc_crtc_atomic_flush(), but this
patch duplicates it into dc_crtc_atomic_enable()).

> +	dc_tc_set_operation_mode(dc_crtc->tc);

[Severity: High]
Does this regmap write trigger a sleep-in-atomic bug on PREEMPT_RT?

Calling dc_tc_set_operation_mode() here triggers a regmap write. Since we
are inside a local_irq_save() block, and the TCON regmap (configured in
dc_tc_regmap_config) lacks .use_raw_spinlock = true, it will attempt to
acquire a sleepable rt_mutex on PREEMPT_RT, violating lock nesting rules.

> +
> +	local_irq_restore(flags);
> +	preempt_enable();
[ ... ]
> diff --git a/drivers/gpu/drm/imx/dc/dc-tc.c b/drivers/gpu/drm/imx/dc/dc-tc.c
> index d0d4faba790e5..746fcaaf97f56 100644
> --- a/drivers/gpu/drm/imx/dc/dc-tc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-tc.c
[ ... ]
> @@ -46,7 +77,7 @@ static const struct regmap_config dc_tc_regmap_config = {
>  	.fast_io = true,
>  	.wr_table = &dc_tc_regmap_access_table,
>  	.rd_table = &dc_tc_regmap_access_table,
> -	.max_register = MAPBIT31_28,
> +	.max_register = SMXFCTTABLE(3),
>  };

[Severity: High]
Does this regmap configuration need .use_raw_spinlock = true?

Since .fast_io = true is set without .use_raw_spinlock, the regmap protects
fast I/O with a standard spinlock_t. On PREEMPT_RT kernels, spinlock_t
becomes a preemptible rt_mutex which can sleep, causing crashes if accessed
while hardware interrupts are disabled.

[ ... ]
> @@ -59,10 +90,85 @@ static const u32 dc_tc_mapbit[] = {
[ ... ]
> +void dc_tc_cfg_videomode(struct dc_tc *tc, struct drm_display_mode *m)
> +{
> +	int hdisplay, hsync_start, hsync_end;
> +	int vdisplay, vsync_start, vsync_end;
> +	int y;
> +
> +	hdisplay = m->hdisplay;
> +	vdisplay = m->vdisplay;
> +	hsync_start = m->hsync_start;
> +	vsync_start = m->vsync_start;
> +	hsync_end = m->hsync_end;
> +	vsync_end = m->vsync_end;

[Severity: High]
Are these the correct timings to use for TCON hardware programming?

By extracting the logical mode timings (e.g., m->hdisplay) instead of the
physical adjusted timings (e.g., m->crtc_hdisplay), we might program the
TCON and FrameGen with mismatched timings.

FrameGen correctly uses the adjusted crtc_* variants. If a DRM bridge or
encoder adjusts the physical timings (like scaling or interleaving) while
leaving logical timings intact, will this mismatch cause sync loss, visual
corruption, or panel failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-imx8-dc-prefetch-v6-0-26643e3f6cb7@nxp.com?part=7