Re: [PATCH 3/5] drm/ingenic: Add a driver for the X1600 LCD controller

[sashiko-bot@kernel.org]

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 <olivier@mgcrea.io>

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/20261025085816.60405-1-olivier@mgcrea.io?part=3