Re: [PATCH 1/2] drm/verisilicon: set blend mode for the primary plane

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The return value of `drm_plane_create_blend_mode_property` is ignored.

Pre-existing issues:
- [Critical] Dereferencing a NULL `crtc` pointer in `vs_primary_plane_atomic_disable` during offscreen plane transitions.
- [High] `vs_primary_plane_atomic_disable` incorrectly sets the enable bit instead of clearing it.
--

commit 9aa67f5c63b091d89b8b4a7a3066a896f35cf108
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>

drm/verisilicon: set blend mode for the primary plane

Blend modes are now required to expose pixel formats w/ alpha.
    
As it's the primary plane and blending is explicitly disabled, just
expose PIXEL_NONE blend mode.

> diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> index 1f2be41ae496c..8d58682d88ef8 100644
> --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> @@ -7,6 +7,7 @@
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_blend.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but can this dereference a NULL pointer during
offscreen plane transitions?

If userspace commits an atomic state where a plane transitions from fully
disabled (old_state->crtc == NULL) to assigned but offscreen
(new_state->crtc != NULL but new_state->visible == false), the DRM core
calls vs_primary_plane_atomic_update().

This function sees !state->visible and delegates to the disable function:

vs_primary_plane_atomic_update() {
    ...
	if (!state->visible) {
		vs_primary_plane_atomic_disable(plane, atomic_state);
		return;
	}
    ...
}

The disable function then fetches old_state->crtc, which is NULL, and
dereferences it:

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;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does
vs_primary_plane_atomic_disable() incorrectly set the enable bit instead of
clearing it?

vs_primary_plane_atomic_disable() {
    ...
	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
			VSDC_FB_CONFIG_EX_FB_EN);

	vs_primary_plane_commit(dc, output);
}

When the plane needs to be disabled, the core calls this function, which uses
regmap_set_bits() with the VSDC_FB_CONFIG_EX_FB_EN bit.

Should this use regmap_clear_bits() instead? The hardware plane is never
disabled, which can cause visual corruption, unexpected display state, and
potential memory access faults if the buffer memory is subsequently freed
while the hardware is still scanning it out.

> @@ -179,5 +180,7 @@ struct drm_plane *vs_primary_plane_init(struct drm_device *drm_dev, struct vs_dc
>  
>  	drm_plane_helper_add(plane, &vs_primary_plane_helper_funcs);
>  
> +	drm_plane_create_blend_mode_property(plane,
> +					     BIT(DRM_MODE_BLEND_PIXEL_NONE));

[Severity: Medium]
Is it intentional to ignore the return value of
drm_plane_create_blend_mode_property()?

If property creation fails (e.g. due to memory exhaustion), the plane is
initialized without the requested blend mode property, breaking userspace
expectations for alpha formats. Should this function check for an error code
and handle the failure?

>  	return plane;
>  }

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