All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RESEND 0/3] drm/verisilicon: misc fixes
@ 2026-10-30 13:32 Icenowy Zheng
  2026-10-30 13:32 ` [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN Icenowy Zheng
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Icenowy Zheng @ 2026-10-30 13:32 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
  Cc: David Airlie, Simona Vetter, Dmitry Baryshkov, Joey Lu, Han Gao,
	dri-devel, linux-kernel, Icenowy Zheng, Icenowy Zheng

This patchset fixes several problems. Two of them are pointed out by
Sashiko, and the remaining one is found when running the driver on some
weird TH1520 firmware that does mode set but fixes to 800x600.

This patchset is re-sent because of forgetting to Cc the mailing list
previously.

Icenowy Zheng (2):
  drm/verisilicon: disable the output before enabling it
  drm/verisilicon: stop to reuse atomic_disable for invisible planes

Joey Lu (1):
  drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN

 drivers/gpu/drm/verisilicon/vs_bridge.c        |  8 ++++++++
 drivers/gpu/drm/verisilicon/vs_cursor_plane.c  | 17 +++++++++++------
 drivers/gpu/drm/verisilicon/vs_primary_plane.c | 15 ++++++++++-----
 3 files changed, 29 insertions(+), 11 deletions(-)

-- 
2.56.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN
  2026-10-30 13:32 [PATCH RESEND 0/3] drm/verisilicon: misc fixes Icenowy Zheng
@ 2026-10-30 13:32 ` Icenowy Zheng
  2026-10-30 13:43   ` sashiko-bot
  2026-10-30 13:32 ` [PATCH RESEND 2/3] drm/verisilicon: disable the output before enabling it Icenowy Zheng
  2026-10-30 13:32 ` [PATCH RESEND 3/3] drm/verisilicon: stop to reuse atomic_disable for invisible planes Icenowy Zheng
  2 siblings, 1 reply; 6+ messages in thread
From: Icenowy Zheng @ 2026-10-30 13:32 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
  Cc: David Airlie, Simona Vetter, Dmitry Baryshkov, Joey Lu, Han Gao,
	dri-devel, linux-kernel, Icenowy Zheng, Icenowy Zheng

From: Joey Lu <a0987203069@gmail.com>

vs_primary_plane_atomic_disable() calls regmap_set_bits() on
VSDC_FB_CONFIG_EX_FB_EN instead of regmap_clear_bits(), so disabling
the primary plane on DC8200-family hardware actually leaves the
framebuffer enable bit instead of clearing it.

Fixes: dbf21777caa8 ("drm: verisilicon: add a driver for Verisilicon display controllers")
Signed-off-by: Joey Lu <a0987203069@gmail.com>
[Icenowy: rebased on top of torvalds/master and reworked the commit
 message]
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/gpu/drm/verisilicon/vs_primary_plane.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index 8e94491693019..d5563c3b825c3 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -88,8 +88,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);
 }
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH RESEND 2/3] drm/verisilicon: disable the output before enabling it
  2026-10-30 13:32 [PATCH RESEND 0/3] drm/verisilicon: misc fixes Icenowy Zheng
  2026-10-30 13:32 ` [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN Icenowy Zheng
@ 2026-10-30 13:32 ` Icenowy Zheng
  2026-10-30 13:32 ` [PATCH RESEND 3/3] drm/verisilicon: stop to reuse atomic_disable for invisible planes Icenowy Zheng
  2 siblings, 0 replies; 6+ messages in thread
From: Icenowy Zheng @ 2026-10-30 13:32 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
  Cc: David Airlie, Simona Vetter, Dmitry Baryshkov, Joey Lu, Han Gao,
	dri-devel, linux-kernel, Icenowy Zheng, Icenowy Zheng

When either of the RUNNING bits are set, change to some timing
parameters won't take effect.

This will lead to failure of the first modeset done by the kernel, if
the bootloader sets a different mode and keeps the display running
before it.

Flip these RUNNING bits off before setting it on to fix this problem.

Fixes: dbf21777caa8 ("drm: verisilicon: add a driver for Verisilicon display controllers")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/gpu/drm/verisilicon/vs_bridge.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/verisilicon/vs_bridge.c b/drivers/gpu/drm/verisilicon/vs_bridge.c
index dc7c85b07fe32..a095b20e81c00 100644
--- a/drivers/gpu/drm/verisilicon/vs_bridge.c
+++ b/drivers/gpu/drm/verisilicon/vs_bridge.c
@@ -162,6 +162,14 @@ static void vs_bridge_enable_common(struct vs_crtc *crtc,
 			VSDC_DISP_PANEL_CONFIG_DE_EN |
 			VSDC_DISP_PANEL_CONFIG_DAT_EN |
 			VSDC_DISP_PANEL_CONFIG_CLK_EN);
+
+	/* Disable it first to prevent stale hardware state */
+	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
+			  VSDC_DISP_PANEL_START_MULTI_DISP_SYNC |
+			  VSDC_DISP_PANEL_START_RUNNING(output));
+	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
+			  VSDC_DISP_PANEL_CONFIG_RUNNING);
+
 	regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
 			VSDC_DISP_PANEL_CONFIG_RUNNING);
 	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH RESEND 3/3] drm/verisilicon: stop to reuse atomic_disable for invisible planes
  2026-10-30 13:32 [PATCH RESEND 0/3] drm/verisilicon: misc fixes Icenowy Zheng
  2026-10-30 13:32 ` [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN Icenowy Zheng
  2026-10-30 13:32 ` [PATCH RESEND 2/3] drm/verisilicon: disable the output before enabling it Icenowy Zheng
@ 2026-10-30 13:32 ` Icenowy Zheng
  2026-10-30 13:45   ` sashiko-bot
  2 siblings, 1 reply; 6+ messages in thread
From: Icenowy Zheng @ 2026-10-30 13:32 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
  Cc: David Airlie, Simona Vetter, Dmitry Baryshkov, Joey Lu, Han Gao,
	dri-devel, linux-kernel, Icenowy Zheng, Icenowy Zheng

When a plane is configured to be invisible, the atomic_update
implementation currently just calls the atomic_disable function.

However these two functions behave differently when disabling planes:
atomic_update disables the plane specified in the NEW state, but
atomic_disable disables the plane specified in the OLD state, so this
shortcut may lead to NULL dereference if the old state doesn't have a
CRTC bound.

Fixes: dbf21777caa8 ("drm: verisilicon: add a driver for Verisilicon display controllers")
Fixes: 8c4ae2189125 ("drm: verisilicon: add support for cursor planes")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/gpu/drm/verisilicon/vs_cursor_plane.c  | 17 +++++++++++------
 drivers/gpu/drm/verisilicon/vs_primary_plane.c | 15 ++++++++++-----
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
index da456dced88aa..725921ac25d48 100644
--- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
@@ -98,6 +98,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)
 {
@@ -125,11 +134,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,
@@ -145,7 +150,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;
 	}
 
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index d5563c3b825c3..594d56bf32410 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -59,6 +59,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)
 {
@@ -88,10 +96,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,
@@ -108,7 +113,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;
 	}
 
-- 
2.56.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN
  2026-10-30 13:32 ` [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN Icenowy Zheng
@ 2026-10-30 13:43   ` sashiko-bot
  0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-10-30 13:43 UTC (permalink / raw)
  To: Icenowy Zheng; +Cc: dri-devel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RESEND 3/3] drm/verisilicon: stop to reuse atomic_disable for invisible planes
  2026-10-30 13:32 ` [PATCH RESEND 3/3] drm/verisilicon: stop to reuse atomic_disable for invisible planes Icenowy Zheng
@ 2026-10-30 13:45   ` sashiko-bot
  0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-10-30 13:45 UTC (permalink / raw)
  To: Icenowy Zheng; +Cc: dri-devel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-10-30 13:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-30 13:32 [PATCH RESEND 0/3] drm/verisilicon: misc fixes Icenowy Zheng
2026-10-30 13:32 ` [PATCH RESEND 1/3] drm/verisilicon: fix DC8200 primary plane disable clearing FB_EN Icenowy Zheng
2026-10-30 13:43   ` sashiko-bot
2026-10-30 13:32 ` [PATCH RESEND 2/3] drm/verisilicon: disable the output before enabling it Icenowy Zheng
2026-10-30 13:32 ` [PATCH RESEND 3/3] drm/verisilicon: stop to reuse atomic_disable for invisible planes Icenowy Zheng
2026-10-30 13:45   ` sashiko-bot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.