Re: [PATCH 2/2] drm/verisilicon: remove ARGB formats from primary plane

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unintentional removal of 10-bit color formats from the primary plane.

Pre-existing issues:
- [High] Plane `atomic_disable` crashes when called on a previously disabled plane.
- [High] Primary plane `atomic_disable` mistakenly enables the plane in the hardware register.
--

commit 53c18bf18dd1a86e14f16784c764ae73c2637d54
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>

drm/verisilicon: remove ARGB formats from primary plane
    
As the blending of the primary plane is currently explicitly disabled
(and it's not possible on DC8000), remove the ARGB formats from the
primary plane format tables.

> diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c
> index 56aa450443068..ebf6f843bc885 100644
> --- a/drivers/gpu/drm/verisilicon/vs_hwdb.c
> +++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c
> @@ -15,32 +15,16 @@ static const u32 vs_primary_formats_array_no_yuv444[] = {
[ ... ]
>  	DRM_FORMAT_XRGB8888,
>  	DRM_FORMAT_XBGR8888,
>  	DRM_FORMAT_RGBX8888,
>  	DRM_FORMAT_BGRX8888,
> -	DRM_FORMAT_ARGB8888,
> -	DRM_FORMAT_ABGR8888,
> -	DRM_FORMAT_RGBA8888,
> -	DRM_FORMAT_BGRA8888,
> -	DRM_FORMAT_ARGB2101010,
> -	DRM_FORMAT_ABGR2101010,
> -	DRM_FORMAT_RGBA1010102,
> -	DRM_FORMAT_BGRA1010102,

[Severity: Medium]
Does the removal of DRM_FORMAT_ARGB2101010 and its variants drop all 10-bit
color support from the primary plane?

Since the opaque 10-bit counterparts (like DRM_FORMAT_XRGB2101010) are
missing from vs_primary_formats_array_no_yuv444, any userspace compositor
attempting to use 10-bit color will find no supported formats and fail.

>  	/* TODO: non-RGB formats */
>  };
>  
> @@ -49,32 +33,16 @@ static const u32 vs_primary_formats_array_with_yuv444[] = {
[ ... ]
>  	DRM_FORMAT_XRGB8888,
>  	DRM_FORMAT_XBGR8888,
>  	DRM_FORMAT_RGBX8888,
>  	DRM_FORMAT_BGRX8888,
> -	DRM_FORMAT_ARGB8888,
> -	DRM_FORMAT_ABGR8888,
> -	DRM_FORMAT_RGBA8888,
> -	DRM_FORMAT_BGRA8888,
> -	DRM_FORMAT_ARGB2101010,
> -	DRM_FORMAT_ABGR2101010,
> -	DRM_FORMAT_RGBA1010102,
> -	DRM_FORMAT_BGRA1010102,

[Severity: Medium]
Will this also remove 10-bit color support from the secondary array
vs_primary_formats_array_with_yuv444?

Just like the array above, the opaque 10-bit formats were not added,
removing 10-bit color support for userspace compositors on this plane.

>  	/* TODO: non-RGB formats */
>  };

[Severity: High]
This isn't a bug introduced by this patch, but can
vs_primary_plane_atomic_disable() crash when called on a previously disabled
plane?

drivers/gpu/drm/verisilicon/vs_primary_plane.c:vs_primary_plane_atomic_disable() {
	struct drm_plane_state *state = drm_atomic_get_old_plane_state(atomic_state,
								       plane);
	struct drm_crtc *crtc = state->crtc;
	struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
	unsigned int output = vcrtc->id;
        ...

If a userspace process performs an atomic commit that targets a disabled plane
and keeps it disabled or moves it out of bounds (!state->visible), DRM helpers
may call atomic_update. The driver's atomic_update unconditionally calls
atomic_disable for invisible planes.

When atomic_disable retrieves old_plane_state->crtc, it is NULL since the
plane was already disabled. Would this result in a NULL pointer dereference
at vcrtc->id?

This exact same pattern exists in the cursor plane's disable path in
drivers/gpu/drm/verisilicon/vs_cursor_plane.c for
vs_cursor_plane_atomic_disable():

drivers/gpu/drm/verisilicon/vs_cursor_plane.c:vs_cursor_plane_atomic_disable() {
	struct drm_plane_state *state = drm_atomic_get_old_plane_state(atomic_state,
								       plane);
	struct drm_crtc *crtc = state->crtc;
	struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
	unsigned int output = vcrtc->id;
        ...

[Severity: High]
This is a pre-existing issue, but does vs_primary_plane_atomic_disable()
mistakenly enable the plane in the hardware register instead of disabling it?

drivers/gpu/drm/verisilicon/vs_primary_plane.c:vs_primary_plane_atomic_disable() {
    ...
	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
			VSDC_FB_CONFIG_EX_FB_EN);
    ...
}

Whenever the primary plane is disabled (e.g., DPMS off, mode set, or
compositor exit), this code uses regmap_set_bits with VSDC_FB_CONFIG_EX_FB_EN.
This leaves the hardware plane active instead of disabling it.

When the DRM core subsequently frees the framebuffer memory, the display
controller hardware will continue to DMA read from the freed memory. Can this
cause a Use-After-Free and potential memory corruption or IOMMU faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910095000.3505878-1-zhengxingda@iscas.ac.cn?part=2