Re: [PATCH v3 2/8] drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers

On Mon Jul 27, 2026 at 3:59 AM CST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The driver fails to trigger a CRTC modeset when the primary plane's framebuffer pixel format changes, causing the hardware to incorrectly interpret pixels.
> --
>
> --- Patch [2]: [PATCH v3 2/8] drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers ---
> Note: The format of this report has been altered to use a free-form summary
> due to code recitation restrictions. No inline code quoting is used.
>
> commit 82b41ef8182b1e0fe64dd5371b72b906d9c02892
> Author: Ze Huang <[email protected]>
> Subject: drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers
>
> This commit replaces the simple display pipe with explicit plane, CRTC, and
> encoder objects. It moves callbacks to plane and CRTC helpers, wires vblank
> handling through drm_crtc_funcs, and uses commit-local state for programming.
>
> [Severity: High]
> This is a pre-existing issue, but does the driver fail to trigger a CRTC
> modeset when the primary plane's framebuffer pixel format changes?
>
> Looking at aspeed_gfx_plane_helper_atomic_check() in 
> drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c, there doesn't seem to be a check 
> to see if the pixel format has changed between the old and new plane states.
>
> Since the hardware registers for the format (CRT_CTRL1) are only programmed 
> during the atomic_enable() path, which requires a full modeset, a format 
> change during a simple plane update will only update the DMA address in 
> aspeed_gfx_plane_helper_atomic_update(). The format configuration in the 
> hardware will remain stale.
>
> Could this lead to corrupted rendering if userspace changes the framebuffer 
> format (e.g., from RGB565 to XRGB8888) without changing the display mode? 
> Should aspeed_gfx_plane_helper_atomic_check() explicitly set 
> crtc_state->mode_changed = true if the new format differs from the old one 
> to enforce a full modeset?

We can force a modeset when the primary framebuffer format changes:

	if (old_fb && old_fb->format != fb->format)
		crtc_state->mode_changed = true;

With drm_atomic_helper_check(), this also needs a second
drm_atomic_helper_check_modeset() call, because modeset checks run before
plane checks, similar to [1]

    static int foo_atomic_check(struct drm_device *drm,
                                struct drm_atomic_commit *commit)
    {
        int ret;

        ret = drm_atomic_helper_check(drm, commit);
        if (ret)
            return ret;

        return drm_atomic_helper_check_modeset(drm, commit);
    }

However, I would prefer not to mix too much unrelated fixes into this
simple-KMS conversion series. The old simple-pipe code did not force a
modeset on framebuffer format changes either, so keeping that behavior
seems fine for this conversion.

If needed, we can fix it later in a separate patch.

Also, ARCPGU may only support programming the pixel format during CRTC
enable/modeset, not during a flicker-free plane-only update. If so,
changing the plane update path would not help.

[1] https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c#L40