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 <[email protected]> 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/[email protected]?part=7
