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 <[email protected]> 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. > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c > b/drivers/gpu/drm/verisilicon/vs_crtc.c > index 0b8a35d09cd2e..679d6541ba1b9 100644 > --- a/drivers/gpu/drm/verisilicon/vs_crtc.c > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c [ ... ] > @@ -26,10 +49,16 @@ static void vs_crtc_atomic_disable(struct drm_crtc *crtc, > struct vs_dc *dc = vcrtc->dc; > unsigned int output = vcrtc->id; > > drm_crtc_vblank_off(crtc); > > clk_disable_unprepare(dc->pix_clk[output]); [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? [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc8200.c > b/drivers/gpu/drm/verisilicon/vs_dc8200.c > new file mode 100644 > index 0000000000000..800df9279e9b6 > --- /dev/null > +++ b/drivers/gpu/drm/verisilicon/vs_dc8200.c [ ... ] > +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/[email protected]?part=3
