Re: [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/verisilicon: vs_primary_plane_atomic_disable() misuse on invisible plane
--

commit b5d25e699c72e0d03ee4c0a85f55bde0b05d2253
Author: Joey Lu <a0987203069@gmail.com>

drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN

This commit changes vs_primary_plane_atomic_disable() to call
regmap_clear_bits() instead of regmap_set_bits() on VSDC_FB_CONFIG_EX_FB_EN
when disabling the primary plane.

> diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> index 95f4a06039e76..c12fe54096c33 100644
> --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> @@ -89,8 +89,8 @@ static void vs_primary_plane_atomic_disable(struct drm_plane *plane,
>  	unsigned int output = vcrtc->id;
>  	struct vs_dc *dc = vcrtc->dc;
>  
> -	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> -			VSDC_FB_CONFIG_EX_FB_EN);
> +	regmap_clear_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> +			  VSDC_FB_CONFIG_EX_FB_EN);
>  
>  	vs_primary_plane_commit(dc, output);
>  }

[Severity: High]
Now that vs_primary_plane_atomic_disable() clears VSDC_FB_CONFIG_EX_FB_EN, can
calling it from vs_primary_plane_atomic_update() when !state->visible leave
VSDC_FB_CONFIG_EX_FB_EN permanently cleared when the plane becomes visible
again on the same CRTC?

In drivers/gpu/drm/verisilicon/vs_primary_plane.c,
vs_primary_plane_atomic_update() handles invisible planes by calling
vs_primary_plane_atomic_disable():

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

When a primary plane is already attached to a CRTC (old_plane_state->crtc !=
NULL) and transitions from visible to !visible (for example, when the CRTC is
disabled while the plane stays attached, which vs_primary_plane_atomic_check()
allows via can_update_disabled = true), vs_primary_plane_atomic_update() calls
vs_primary_plane_atomic_disable() and clears VSDC_FB_CONFIG_EX_FB_EN.

When that plane later transitions back from !visible to visible on the same
CRTC (old_plane_state->crtc != NULL && new_plane_state->crtc != NULL),
drm_atomic_helper_commit_planes() calls vs_primary_plane_atomic_update() and
checks drm_atomic_plane_enabling(old_plane_state, new_plane_state), which only
tests (!old_plane_state->crtc && new_plane_state->crtc). Because
old_plane_state->crtc is non-NULL, vs_primary_plane_atomic_enable() is not
called, and vs_primary_plane_atomic_update() never sets
VSDC_FB_CONFIG_EX_FB_EN:

static void vs_primary_plane_atomic_enable(struct drm_plane *plane,
					   struct drm_atomic_commit *atomic_state)
{
	...
	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
			VSDC_FB_CONFIG_EX_FB_EN);

Conversely, if a previously disabled plane (old_plane_state->crtc == NULL) is
attached to a CRTC with new_plane_state->visible == false:

1. vs_primary_plane_atomic_update() calls vs_primary_plane_atomic_disable(),
   which looks up old_plane_state via drm_atomic_get_old_plane_state() and
   dereferences state->crtc (NULL) via vcrtc->id and vcrtc->dc:

	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;
	struct vs_dc *dc = vcrtc->dc;

2. drm_atomic_helper_commit_planes() then sees drm_atomic_plane_enabling() ==
   true and calls vs_primary_plane_atomic_enable(), setting
   VSDC_FB_CONFIG_EX_FB_EN and committing the plane even though
   vs_primary_plane_atomic_update() returned early without programming
   VSDC_FB_ADDRESS, VSDC_FB_STRIDE, or VSDC_FB_SIZE.

Commit 3080d00108f4 ("drm/verisilicon: stop to reuse atomic_disable for
invisible planes") later in the series replaces the
vs_primary_plane_atomic_disable() call in vs_primary_plane_atomic_update()
with vs_primary_plane_disable(dc, output), but calls it before dc and output
are initialized and still leaves VSDC_FB_CONFIG_EX_FB_EN cleared after a
!visible to visible transition on the same CRTC (and enabled on an initial
!visible attach).

Should VSDC_FB_CONFIG_EX_FB_EN and VSDC_FB_CONFIG_EX_DISPLAY_ID instead be
managed in vs_primary_plane_atomic_update() based on state->visible rather
than in a separate atomic_enable callback?

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