Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/ingenic: premature plane cleanup via drm_atomic_helper_wait_for_vblanks() - [High] drm/ingenic: missing visibility check in x1600_lcd_plane_atomic_check() - [Medium] drm/ingenic: premature vblank event in x1600_lcd_crtc_atomic_flush() - [Medium] drm/ingenic: unclocked MMIO on disable in x1600_lcd_crtc_atomic_disable() --
commit ff594f68d62e2b467c0fa558eebe60679b4a51ba Author: Olivier Louvignes <[email protected]> drm/ingenic: Add a driver for the X1600 LCD controller This commit adds a DRM driver for the Ingenic X1600 LCD controller, supporting parallel RGB panels on a 24-bit bus with a single full-screen RGB565 primary plane and descriptor-chain page flipping. > diff --git a/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c > b/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c > new file mode 100644 > index 0000000000000..34fbfc7de1c4b > --- /dev/null > +++ b/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c [ ... ] > +static void x1600_lcd_crtc_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ > + struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev); > + struct drm_display_mode *mode = &crtc->state->adjusted_mode; > + unsigned int hde = mode->htotal - mode->hsync_start; > + unsigned int vde = mode->vtotal - mode->vsync_start; > + u32 val; > + int ret; > + > + ret = clk_set_rate(lcd->clk, mode->crtc_clock * 1000); > + if (ret) > + dev_warn(lcd->drm.dev, "Unable to set the pixel clock: %d\n", > ret); > + ret = clk_prepare_enable(lcd->clk); > + if (ret) { > + dev_err(lcd->drm.dev, "Unable to start the pixel clock: %d\n", > ret); > + return; > + } [ ... ] > + writel(x1600_lcd_desc_addr(lcd, lcd->cur), lcd->base + > X1600_LCD_SRD_CHAIN_ADDR); > + writel(1, lcd->base + X1600_LCD_SRD_CHAIN_START); > + lcd->running = true; > + > + drm_crtc_vblank_on(crtc); > +} > + > +static void x1600_lcd_crtc_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ > + struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev); > + > + if (!x1600_lcd_stop(lcd)) > + dev_warn(lcd->drm.dev, "The controller did not stop\n"); > + lcd->running = false; > + > + spin_lock_irq(&crtc->dev->event_lock); > + if (lcd->event) { > + drm_crtc_send_vblank_event(crtc, lcd->event); > + drm_crtc_vblank_put(crtc); > + lcd->event = NULL; > + } > + spin_unlock_irq(&crtc->dev->event_lock); > + > + drm_crtc_vblank_off(crtc); > + clk_disable_unprepare(lcd->clk); > +} [Severity: Medium] In drivers/gpu/drm/ingenic/ingenic-x1600-drm.c, if clk_prepare_enable() fails in x1600_lcd_crtc_atomic_enable(), the function returns early with lcd->clk disabled and lcd->running left false, while the atomic state still marks the crtc as enabled. When the crtc is later disabled (for example via DPMS off, a subsequent modeset, or drm_atomic_helper_shutdown() in x1600_lcd_remove()), x1600_lcd_crtc_atomic_disable() unconditionally calls x1600_lcd_stop(lcd) and clk_disable_unprepare(lcd->clk). Could this access hardware registers in x1600_lcd_stop() while lcd->clk is gated and underflow the clock prepare/enable counts in clk_disable_unprepare(lcd->clk) if x1600_lcd_crtc_atomic_disable() does not check lcd->running first? [ ... ] > +static void x1600_lcd_crtc_atomic_flush(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ > + struct drm_crtc_state *crtc_state = > drm_atomic_get_new_crtc_state(state, crtc); > + struct drm_pending_vblank_event *event = crtc_state->event; > + struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev); > + > + if (!event) > + return; > + > + crtc_state->event = NULL; > + > + /* > + * The controller takes a new descriptor "at appropriate time" (manual, > + * 10.6.4): wait until it reports reading the one just handed over. > + */ > + spin_lock_irq(&crtc->dev->event_lock); > + if (lcd->running && !WARN_ON(lcd->event) && drm_crtc_vblank_get(crtc) > == 0) { > + lcd->event = event; > + lcd->event_desc = x1600_lcd_desc_addr(lcd, lcd->cur); > + } else { > + drm_crtc_send_vblank_event(crtc, event); > + } > + spin_unlock_irq(&crtc->dev->event_lock); > +} [Severity: Medium] In x1600_lcd_probe(), drm->mode_config.helper_private is left unset, so atomic commits use the default drm_atomic_helper_commit_tail() ordering: drm_atomic_helper_commit_tail() drm_atomic_helper_commit_modeset_disables() drm_atomic_helper_commit_planes() x1600_lcd_plane_atomic_update() x1600_lcd_crtc_atomic_flush() drm_atomic_helper_commit_modeset_enables() x1600_lcd_crtc_atomic_enable() During any commit that enables or re-enables the crtc, x1600_lcd_crtc_atomic_flush() runs before x1600_lcd_crtc_atomic_enable() sets lcd->running = true and calls drm_crtc_vblank_on(crtc). Won't the check in x1600_lcd_crtc_atomic_flush() always take the else branch during a modeset enable, sending the vblank event and completing flip_done before the controller and panel bridge have even started? [ ... ] > +static irqreturn_t x1600_lcd_irq(int irq, void *data) > +{ > + struct x1600_lcd *lcd = data; > + u32 st = readl(lcd->base + X1600_LCD_INT_FLAG); > + > + if (!st) > + return IRQ_NONE; > + > + writel(st, lcd->base + X1600_LCD_CLR_ST); > + if (st & X1600_LCD_ST_STOP_SRD_ACK) > + complete(&lcd->stopped); > + > + if (st & X1600_LCD_ST_DISP_END) { > + drm_crtc_handle_vblank(&lcd->crtc); > + > + spin_lock(&lcd->drm.event_lock); > + if (lcd->event && > + readl(lcd->base + X1600_LCD_CHAIN_SITE) == lcd->event_desc) > { > + drm_crtc_send_vblank_event(&lcd->crtc, lcd->event); > + drm_crtc_vblank_put(&lcd->crtc); > + lcd->event = NULL; > + } > + spin_unlock(&lcd->drm.event_lock); > + } > + > + return IRQ_HANDLED; > +} [Severity: High] When a page flip is programmed near the end of a frame and the controller has not yet switched to the new descriptor in lcd->event_desc when the next end-of-frame interrupt fires, x1600_lcd_irq() holds back the flip event because X1600_LCD_CHAIN_SITE does not match lcd->event_desc yet, but it still calls drm_crtc_handle_vblank(&lcd->crtc) unconditionally. Because drm->mode_config.helper_private is unset in x1600_lcd_probe(), the default drm_atomic_helper_commit_tail() waits for drm_atomic_helper_wait_for_vblanks() rather than drm_atomic_helper_wait_for_flip_done(): drm_atomic_helper_commit_tail() drm_atomic_helper_wait_for_vblanks() drm_atomic_helper_cleanup_planes() Can that first vblank interrupt wake drm_atomic_helper_wait_for_vblanks() and allow drm_atomic_helper_cleanup_planes() to release the old framebuffer while the controller is still scanning out the previous descriptor for another frame? > + > +static int x1600_lcd_plane_atomic_check(struct drm_plane *plane, > + struct drm_atomic_commit *state) > +{ > + struct drm_plane_state *plane_state = > drm_atomic_get_new_plane_state(state, plane); > + struct drm_crtc_state *crtc_state; > + > + if (!plane_state->crtc) > + return 0; > + > + crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc); > + if (WARN_ON(!crtc_state)) > + return -EINVAL; > + > + /* The plane covers the whole screen: no scaling, no position. */ > + return drm_atomic_helper_check_plane_state(plane_state, crtc_state, > + DRM_PLANE_NO_SCALING, > + DRM_PLANE_NO_SCALING, > + false, true); > +} > + > +static void x1600_lcd_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit *state) > +{ > + struct x1600_lcd *lcd = to_x1600_lcd(plane->dev); > + struct drm_plane_state *plane_state = > drm_atomic_get_new_plane_state(state, plane); > + struct drm_framebuffer *fb = plane_state->fb; > + unsigned int next = lcd->running ? !lcd->cur : lcd->cur; > + struct x1600_lcd_desc *desc = &lcd->desc[next]; > + > + if (!fb) > + return; > + > + desc->next = x1600_lcd_desc_addr(lcd, next); > + desc->addr = drm_fb_dma_get_gem_addr(fb, plane_state, 0); > + desc->stride = fb->pitches[0] / fb->format->cpp[0]; [Severity: High] In drm_atomic_helper_check_plane_state(), if userspace places the plane's destination rectangle completely outside the active crtc dimensions, the helper sets plane_state->visible = false and returns 0 before checking can_position: drm_atomic_helper_check_plane_state() plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); ... if (!plane_state->visible) return 0; if (!can_position && !drm_rect_equals(dst, &clip)) return -EINVAL; If userspace submits a small framebuffer positioned off-screen, won't x1600_lcd_plane_atomic_check() accept the state and x1600_lcd_plane_atomic_update() program desc->addr to scan out a full frame past the end of the small buffer unless !plane_state->visible is rejected? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
