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

New issues:
- [High] The newly introduced `crtc_disable` hardware hook is invoked after disabling the pixel clock, which will cause unclocked hardware accesses and potential bus hangs.
- [High] The disable sequence for the display panel incorrectly swaps the order of clearing PANEL_START and PANEL_CONFIG, violating the required reverse-enable hardware sequence.

Pre-existing issues:
- [High] Sleepable clock control functions (`clk_prepare_enable` and `clk_disable_unprepare`) are called inside DRM atomic commit callbacks where sleeping is strictly forbidden.
- [High] The primary plane disable logic incorrectly sets the `VSDC_FB_CONFIG_EX_FB_EN` bit instead of clearing it, failing to actually disable the plane.
- [High] Calling `regmap_read` in a hardirq handler uses a standard spinlock which maps to a sleeping `rt_mutex` on PREEMPT_RT.
- [High] Primary plane update programs hardware registers with unclipped destination coordinates (`state->crtc_x` / `state->crtc_y`) which can be negative.
--

commit 7db852133467fb896e651ada4802a06233540439
Author: Joey Lu [off-list ref]
drm/verisilicon: introduce per-variant hardware ops table

This commit introduces a hardware operations table for different variants
of the Verisilicon display controller. It extracts DC8200-specific ops
into a new file and updates the bridge, CRTC, and plane code to use the
new vtable instead of direct register access.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does calling clk_disable_unprepare() here
and clk_prepare_enable() in vs_crtc_atomic_enable() cause a sleep in atomic
context?

Since atomic_enable and atomic_disable callbacks execute in an atomic
context during non-blocking commits, calling sleepable clock control
functions could lead to deadlocks or kernel panics.

+
+	if (dc->funcs->crtc_disable)
+		dc->funcs->crtc_disable(dc, output);
 }

[Severity: High]
Does this sequence write to hardware registers while the pixel clock is
disabled?

Calling dc->funcs->crtc_disable(dc, output) after clk_disable_unprepare()
means we are performing register accesses without a running clock, which
could lead to bus hangs or failed writes.

Should the order be reversed to match the symmetric enable sequence?

[ ... ]

[ ... ]

+static void vs_dc8200_panel_disable_ex(struct vs_dc *dc, unsigned int output)
+{
+	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
+			  VSDC_DISP_PANEL_CONFIG_RUNNING);
+	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
+			  VSDC_DISP_PANEL_START_MULTI_DISP_SYNC |
+			  VSDC_DISP_PANEL_START_RUNNING(output));
+
+	regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(output),
+			VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
+}

[Severity: High]
Does this swap the order of clearing the panel registers compared to the
original vs_bridge_atomic_disable() code?

The original code correctly cleared VSDC_DISP_PANEL_START before
VSDC_DISP_PANEL_CONFIG.

Clearing CONFIG while the display pipeline is still actively running could
cause an unprotected hardware state transition, leading to pipeline
underflows or hardware hangs.

[ ... ]

+static void vs_dc8200_primary_plane_disable_ex(struct vs_dc *dc, unsigned int output)
+{
+	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
+			VSDC_FB_CONFIG_EX_FB_EN);
+
+	vs_dc8200_plane_commit(dc, output);
+}

[Severity: High]
This is a pre-existing issue, but does this function actually disable the
plane?

It looks like regmap_set_bits() is used on VSDC_FB_CONFIG_EX_FB_EN, which
would keep the plane enabled in hardware.

Should this use regmap_clear_bits() instead to properly clear the bit and
disable the plane?

+static void vs_dc8200_primary_plane_update_ex(struct vs_dc *dc, unsigned int output,
+				       struct drm_plane_state *state)
+{
+	regmap_write(dc->regs, VSDC_FB_TOP_LEFT(output),
+		     VSDC_MAKE_PLANE_POS(state->crtc_x, state->crtc_y));
+	regmap_write(dc->regs, VSDC_FB_BOTTOM_RIGHT(output),
+		     VSDC_MAKE_PLANE_POS(state->crtc_x + state->crtc_w,
+					 state->crtc_y + state->crtc_h));

[Severity: High]
This is a pre-existing issue, but what happens if the user provides negative
coordinates when a plane is partially off-screen?

Because state->crtc_x and state->crtc_y are unclipped, a negative value
passed to VSDC_MAKE_PLANE_POS() might be bitwise ANDed with 0x7fff,
producing a massive positive value (for example, -10 becomes 32758).

Should clipped coordinates be used here instead to avoid incorrect display
output?

[ ... ]

+static u32 vs_dc8200_irq_ack(struct vs_dc *dc)
+{
+	u32 irqs;
+
+	regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &irqs);
+	return irqs;
+}

[Severity: High]
This is a pre-existing issue, but could this regmap_read() cause a
sleep-in-atomic panic on PREEMPT_RT systems?

Since this is invoked from vs_dc_irq_handler() in hardirq context, and the
regmap configuration does not seem to set use_raw_spinlock = true, the
regmap infrastructure uses a standard spinlock.

On PREEMPT_RT, this maps to a sleepable rt_mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615065003.76661-1-a0987203069@gmail.com?part=3