Re: [PATCH RESEND 3/3] drm/verisilicon: stop to reuse atomic_disable for invisible planes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm/verisilicon: uninitialized dc and output in plane atomic_update()
- [High] drm/verisilicon: plane visibility state mismatch with atomic_enable()
--

commit 3080d00108f48782cd8c1c8ee016e9473b324e57
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>

drm/verisilicon: stop to reuse atomic_disable for invisible planes

This commit factors out helper functions to disable cursor and primary
planes directly from their atomic_update callbacks when state->visible is
false, avoiding a NULL CRTC dereference in the old plane state when calling
atomic_disable.

> diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> index 1fa6b7578b27e..8dd6caadb7075 100644
> --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> @@ -99,6 +99,15 @@ static void vs_cursor_plane_commit(struct vs_dc *dc, unsigned int output)
>  			VSDC_CURSOR_CONFIG_IMG_UPDATE);
>  }
>  
> +static void vs_cursor_plane_disable(struct vs_dc *dc, unsigned int output)
> +{
> +	regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
> +			   VSDC_CURSOR_CONFIG_FMT_MASK,
> +			   VSDC_CURSOR_CONFIG_FMT_OFF);
> +
> +	vs_cursor_plane_commit(dc, output);
> +}
> +
>  static void vs_cursor_plane_atomic_enable(struct drm_plane *plane,
>  					   struct drm_atomic_commit *atomic_state)
>  {
> @@ -126,11 +135,7 @@ static void vs_cursor_plane_atomic_disable(struct drm_plane *plane,
>  	unsigned int output = vcrtc->id;
>  	struct vs_dc *dc = vcrtc->dc;
>  
> -	regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
> -			   VSDC_CURSOR_CONFIG_FMT_MASK,
> -			   VSDC_CURSOR_CONFIG_FMT_OFF);
> -
> -	vs_cursor_plane_commit(dc, output);
> +	vs_cursor_plane_disable(dc, output);
>  }
>  
>  static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
> @@ -146,7 +151,7 @@ static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
>  	dma_addr_t dma_addr;
>  
>  	if (!state->visible) {
> -		vs_cursor_plane_atomic_disable(plane, atomic_state);
> +		vs_cursor_plane_disable(dc, output);
>  		return;
>  	}

[Severity: Critical]
Does vs_cursor_plane_atomic_update() pass uninitialized dc and output values
to vs_cursor_plane_disable() when !state->visible is true?

In drivers/gpu/drm/verisilicon/vs_cursor_plane.c, vcrtc, output, and dc are
only initialized from state->crtc after the !state->visible early return:

drivers/gpu/drm/verisilicon/vs_cursor_plane.c:vs_cursor_plane_atomic_update() {
	struct drm_crtc *crtc = state->crtc;
	struct vs_dc *dc;
	struct vs_crtc *vcrtc;
	unsigned int output;
	dma_addr_t dma_addr;

	if (!state->visible) {
		vs_cursor_plane_disable(dc, output);
		return;
	}

	vcrtc = drm_crtc_to_vs_crtc(crtc);
	output = vcrtc->id;
	dc = vcrtc->dc;
}

When a cursor plane is updated while invisible, vs_cursor_plane_disable()
will dereference the uninitialized pointer dc->regs and index
VSDC_CURSOR_CONFIG(output) with an uninitialized output variable.

[Severity: High]
How does disabling the plane in hardware on !state->visible interact with
vs_cursor_plane_atomic_enable() and subsequent visible updates?

In drm_atomic_helper_commit_planes(), funcs->atomic_update() runs before
funcs->atomic_enable(). When a previously unbound plane (old_plane_state->crtc
is NULL) is bound to a CRTC while invisible (new_plane_state->crtc is
non-NULL and new_plane_state->visible is false):

1. vs_cursor_plane_atomic_update() calls vs_cursor_plane_disable() and
   returns early without programming VSDC_CURSOR_ADDRESS(output), size, or
   position registers.
2. drm_atomic_plane_enabling() evaluates to true because !old_plane_state->crtc
   && new_plane_state->crtc, so drm_atomic_helper_commit_planes() immediately
   calls vs_cursor_plane_atomic_enable():

drivers/gpu/drm/verisilicon/vs_cursor_plane.c:vs_cursor_plane_atomic_enable() {
	...
	regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
			   VSDC_CURSOR_CONFIG_FMT_MASK,
			   VSDC_CURSOR_CONFIG_FMT_ARGB8888);

	vs_cursor_plane_commit(dc, output);
}

Won't vs_cursor_plane_atomic_enable() unconditionally re-enable the plane in
hardware without a valid framebuffer address or geometry configured?

Conversely, if a plane that is already bound to a CRTC moves off-screen so
state->visible becomes false, vs_cursor_plane_atomic_update() sets
VSDC_CURSOR_CONFIG_FMT_OFF via vs_cursor_plane_disable(). When a later commit
moves the plane back on-screen while remaining on the same CRTC
(old_plane_state->crtc and new_plane_state->crtc are both non-NULL, and
new_plane_state->visible becomes true), drm_atomic_plane_enabling() is false,
so vs_cursor_plane_atomic_enable() is not called and
vs_cursor_plane_atomic_update() never restores VSDC_CURSOR_CONFIG_FMT_ARGB8888.
Will the cursor plane remain disabled in hardware?

> diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> index c12fe54096c33..56e4c0e168871 100644
> --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> @@ -60,6 +60,14 @@ static void vs_primary_plane_commit(struct vs_dc *dc, unsigned int output)
>  			VSDC_FB_CONFIG_EX_COMMIT);
>  }
>  
> +static void vs_primary_plane_disable(struct vs_dc *dc, unsigned int output)
> +{
> +	regmap_clear_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> +			  VSDC_FB_CONFIG_EX_FB_EN);
> +
> +	vs_primary_plane_commit(dc, output);
> +}
> +
>  static void vs_primary_plane_atomic_enable(struct drm_plane *plane,
>  					   struct drm_atomic_commit *atomic_state)
>  {
> @@ -89,10 +97,7 @@ static void vs_primary_plane_atomic_disable(struct drm_plane *plane,
>  	unsigned int output = vcrtc->id;
>  	struct vs_dc *dc = vcrtc->dc;
>  
> -	regmap_clear_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> -			  VSDC_FB_CONFIG_EX_FB_EN);
> -
> -	vs_primary_plane_commit(dc, output);
> +	vs_primary_plane_disable(dc, output);
>  }
>  
>  static void vs_primary_plane_atomic_update(struct drm_plane *plane,
> @@ -109,7 +114,7 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
>  	dma_addr_t dma_addr;
>  
>  	if (!state->visible) {
> -		vs_primary_plane_atomic_disable(plane, atomic_state);
> +		vs_primary_plane_disable(dc, output);
>  		return;
>  	}

[Severity: Critical]
Similarly, in drivers/gpu/drm/verisilicon/vs_primary_plane.c, doesn't
vs_primary_plane_atomic_update() also call vs_primary_plane_disable(dc,
output) before vcrtc, output, and dc are initialized from state->crtc,
dereferencing uninitialized dc->regs and using an uninitialized output index
in VSDC_FB_CONFIG_EX(output) when !state->visible?

[Severity: High]
Does vs_primary_plane_atomic_update() have the same state mismatch with
vs_primary_plane_atomic_enable()?

When old_plane_state->crtc is NULL, new_plane_state->crtc is non-NULL, and
new_plane_state->visible is false, vs_primary_plane_atomic_update() returns
early without programming VSDC_FB_ADDRESS(output) or geometry registers, and
then vs_primary_plane_atomic_enable() sets VSDC_FB_CONFIG_EX_FB_EN and
commits the plane in hardware anyway.

And if an already-bound primary plane transitions from visible to invisible
(clearing VSDC_FB_CONFIG_EX_FB_EN in vs_primary_plane_disable()) and then
back to visible on the same CRTC, neither vs_primary_plane_atomic_enable()
nor vs_primary_plane_atomic_update() sets VSDC_FB_CONFIG_EX_FB_EN again.

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