Re: [PATCH v4 2/6] drm/hibmc: Fix list of formats on the primary plane

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The display plane atomic update ignores the plane state's source coordinates and dimensions, causing corrupted output for cropped or panned framebuffers.
- [High] The CRTC atomic flush immediately sends the vblank event instead of arming it, breaking page-flip synchronization.
--

commit c951a1d1c3d9caa9dc42e8ab8693ac3a8121b925
Author: Thomas Zimmermann <tzimmermann@suse.de>
Subject: drm/hibmc: Fix list of formats on the primary plane

This commit removes unsupported formats from the primary plane, leaving
only XRGB8888 and RGB565. It addresses issues where planes with alpha
channels or BGR order were not properly supported, and fixes distorted
output for RGB888.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 7c0b88c774b5e..2e6e189bec1a0 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -135,10 +135,8 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does
hibmc_plane_atomic_update() ignore the plane state's source coordinates?

It programs the hardware's visible scanout width using the full fb->width
instead of the active source width (new_state->src_w >> 16):

	writel(gpu_addr, priv->mmio + HIBMC_CRT_FB_ADDRESS);
	reg = drm_format_info_min_pitch(fb->format, 0, fb->width);
	line_l = fb->pitches[0];
	writel(HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_WIDTH, reg) |
	       HIBMC_FIELD(HIBMC_CRT_FB_WIDTH_OFFS, line_l),
	       priv->mmio + HIBMC_CRT_FB_WIDTH);

Since the shadow plane damage copy maintains the original framebuffer's
layout in VRAM, scanning out from offset 0 with fb->width means the
hardware will always display the top-left of the framebuffer. If userspace
provides a framebuffer larger than the CRTC (e.g., for panning) or requests
cropping, will the hardware fetch the wrong number of pixels per scanline
and display the wrong region?

>  
>  static const u32 channel_formats1[] = {
> -	DRM_FORMAT_RGB565, DRM_FORMAT_BGR565, DRM_FORMAT_RGB888,
> -	DRM_FORMAT_BGR888, DRM_FORMAT_XRGB8888, DRM_FORMAT_XBGR8888,
> -	DRM_FORMAT_RGBA8888, DRM_FORMAT_BGRA8888, DRM_FORMAT_ARGB8888,
> -	DRM_FORMAT_ABGR8888
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_RGB565,
>  };

[Severity: High]
This is a pre-existing issue, but does hibmc_crtc_atomic_flush()
immediately send the vblank event instead of arming it?

	if (crtc->state->event)
		drm_crtc_send_vblank_event(crtc, crtc->state->event);
	crtc->state->event = NULL;

Because the driver has working vblank interrupts (as shown by
hibmc_crtc_enable_vblank()), shouldn't it defer the event until the actual
vertical blanking interval with drm_crtc_arm_vblank_event()? Immediately
completing the event violates the atomic KMS API contract, causing
userspace compositors to render the next frame prematurely, which results
in screen tearing and breaks presentation timing guarantees.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618123142.92298-1-tzimmermann@suse.de?part=2