[PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

[PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

[Chen-Yu Tsai]

The verisilicon driver has a custom framebuffer address calculating
helper that the common drm_fb_dma_get_addr() can substitute.

Differences from drm_fb_dma_get_addr():

- Uses drm_format_info_min_pitch() to calculate the horizontal offset;
  however the driver does not support any of the blocked formats, so
  this just ends up being the same as in drm_fb_dma_get_addr():
  "cpp[plane] * y"

- Uses clipped source coordinates instead of non-clipped coordinates
  as in drm_fb_dma_get_addr();

  For the primary plane this doesn't matter, since the primary plane
  must match the output, i.e. it cannot be clipped. Also this driver
  doesn't support scaling.

  For the cursor plane this seems wrong, as the clipping seems to be
  done by the hardware, and thus the buffer address should be unclipped.

As such, it should be fine to use the common helper and drop the custom
code.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- Fixed compile issues

This is only compile tested. I do not have the hardware.
---
 drivers/gpu/drm/verisilicon/vs_cursor_plane.c |  4 +++-
 drivers/gpu/drm/verisilicon/vs_plane.c        | 20 -------------------
 .../gpu/drm/verisilicon/vs_primary_plane.c    |  7 ++++++-
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
index fa4f601dd0c8..59778433ae84 100644
--- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
@@ -12,6 +12,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_fb_dma_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
@@ -176,7 +177,8 @@ static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
 		break;
 	}
 
-	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
+	/* hardware handles clipping as seen below */
+	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
 
 	regmap_write(dc->regs, VSDC_CURSOR_ADDRESS(output),
 		     lower_32_bits(dma_addr));
diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
index d81f7b8f4c65..38b8b536eccb 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_plane.c
@@ -107,26 +107,6 @@ int drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format)
 	return 0;
 }
 
-dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
-			      const struct drm_rect *src_rect)
-{
-	struct drm_gem_dma_object *gem;
-	dma_addr_t dma_addr;
-
-	/* Get the physical address of the buffer in memory */
-	gem = drm_fb_dma_get_gem_obj(fb, 0);
-
-	/* Compute the start of the displayed memory */
-	dma_addr = gem->dma_addr + fb->offsets[0];
-
-	/* Fixup framebuffer address for src coordinates */
-	dma_addr += drm_format_info_min_pitch(fb->format, 0,
-					      src_rect->x1 >> 16);
-	dma_addr += (src_rect->y1 >> 16) * fb->pitches[0];
-
-	return dma_addr;
-}
-
 struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vs_plane_state *vs_state, *vs_state_old;
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index 1f2be41ae496..2750016a7f2c 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -8,6 +8,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_fb_dma_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
@@ -126,7 +127,11 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
 			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
 			   vs_state->format.uv_swizzle);
 
-	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
+	/*
+	 * Primary plane cannot be moved, no clipping is involved,
+	 * so the non-clipped framebuffer address can be used.
+	 */
+	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
 
 	regmap_write(dc->regs, VSDC_FB_ADDRESS(output),
 		     lower_32_bits(dma_addr));
-- 
2.55.0.654.g21b8a5bc05-goog

Re: [PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

[sashiko-bot]

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

New issues:
- [Medium] Removing software clipping for the cursor plane exposes a preexisting register programming bug, completely breaking cursor clipping.
- [Low] Dangling prototype for `vs_fb_get_dma_addr()`.

Pre-existing issues:
- [High] NULL pointer dereference in plane atomic_disable functions when a plane is newly assigned but invisible.
- [High] `vs_primary_plane_atomic_disable` incorrectly enables the plane instead of disabling it.
--

--- Patch [1]: [PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses ---
commit d8f5b888e2f48aaf1e83d6bb5aee579fe194f0ab
Author: Chen-Yu Tsai <wenst@chromium.org>

drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

The verisilicon driver has a custom framebuffer address calculating
helper that the common drm_fb_dma_get_addr() can substitute.

> diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> index fa4f601dd0c87..59778433ae843 100644
> --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c

[ ... ]

> @@ -176,7 +177,8 @@ static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
>  		break;
>  	}
>  
> -	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
> +	/* hardware handles clipping as seen below */
> +	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);

[Severity: Medium]
Since we are removing the software clipping workaround, does the hardware
clipping actually work correctly here?

Looking further down in vs_cursor_plane_atomic_update(), the hardware
offsets are programmed without using the shift macros:

    regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                       VSDC_CURSOR_CONFIG_X_OFF_MASK,
                       -state->crtc_x);

    regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                       VSDC_CURSOR_CONFIG_Y_OFF_MASK,
                       -state->crtc_y);

Because the unshifted small negative integer falls outside the masks, won't
this evaluate to 0 during the bitwise AND, permanently programming the
offsets to 0 and completely breaking cursor clipping?

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference in the disable path that gets called just above this snippet.

If userspace binds a previously unbound plane where old_plane_state->crtc
is NULL, but places it entirely off-screen, the DRM core sets
new_plane_state->visible = false and calls vs_cursor_plane_atomic_update().

The code then calls vs_cursor_plane_atomic_disable():

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

Inside vs_cursor_plane_atomic_disable(), and identically in
vs_primary_plane_atomic_disable(), it fetches the CRTC from the old state,
which is NULL, and directly dereferences it:

    struct drm_crtc *crtc = state->crtc;
    struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
    unsigned int output = vcrtc->id;

Could this trigger a kernel panic if a malicious or buggy userspace
submits this type of atomic modeset?

[Severity: High]
This is also a pre-existing issue, but looking at
vs_primary_plane_atomic_disable(), the hardware appears to be enabled
instead of disabled:

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

Because regmap_set_bits() turns the plane on, won't the primary plane
remain actively rendering when the DRM core disables it, causing the
hardware state to diverge from the kernel state?

> diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
> index d81f7b8f4c650..38b8b536eccbd 100644
> --- a/drivers/gpu/drm/verisilicon/vs_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_plane.c

[ ... ]

> @@ -107,26 +107,6 @@ int drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format)
>  	return 0;
>  }
>  
> -dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
> -			      const struct drm_rect *src_rect)
> -{
> -	struct drm_gem_dma_object *gem;
> -	dma_addr_t dma_addr;

[Severity: Low]
Is the prototype for vs_fb_get_dma_addr() still declared in vs_plane.h?

Should we remove it from drivers/gpu/drm/verisilicon/vs_plane.h as well
to avoid leaving a dangling prototype?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807114654.1373305-1-wenst@chromium.org?part=1

Re: [PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

[Icenowy Zheng]

在 2026-08-07五的 19:46 +0800，Chen-Yu Tsai写道：
> The verisilicon driver has a custom framebuffer address calculating
> helper that the common drm_fb_dma_get_addr() can substitute.
> 
> Differences from drm_fb_dma_get_addr():
> 
> - Uses drm_format_info_min_pitch() to calculate the horizontal
> offset;
>   however the driver does not support any of the blocked formats, so

Technically this DC, advertised as part of the "Vivante" product line
(considering Vivante Corporation is acquired by VeriSilicon), seems to
support DRM_FORMAT_MOD_VIVANTE_SUPER_TILED modifier (the TH1520
documentation says the DC supports
`SuperTileX8x8/SuperTileX8x4/SuperTileY4x8`, although I think
DRM_FORMAT_MOD_VIVANTE_SUPER_TILED is just one of these tiling).

However, as my accessible SoCs with such DC have no GC-series 3D GPUs
(TH1520 does have a 2D-only GC620 GPU), I think it's quite difficult to
get this piece of thing right and it should be low-priority.

>   this just ends up being the same as in drm_fb_dma_get_addr():
>   "cpp[plane] * y"
> 
> - Uses clipped source coordinates instead of non-clipped coordinates
>   as in drm_fb_dma_get_addr();
> 
>   For the primary plane this doesn't matter, since the primary plane
>   must match the output, i.e. it cannot be clipped. Also this driver
>   doesn't support scaling.
> 
>   For the cursor plane this seems wrong, as the clipping seems to be
>   done by the hardware, and thus the buffer address should be
> unclipped.

Yes this is right and the current state of the cursor plane is broken.

However another error compensates this error so I didn't catch it when
developing -- the [XY]_OFF fields aren't properly written because I
forgot to shift the values for them (and then the value gets masked by
regmap_update_bits()), which prevents the HW clipping to happen, and
the normal-state arrow cursor happens to have no non-transparent pixels
before the hotspot. When testing with `X -retro`, the retro X cursor
gets quite glitchy with the current code; and when this patch is
applied w/o the offset fix, the cursor isn't clipped at all.

Both errors deserve fixes, I will then send the fix for the offset
writing problem.

Thanks,
Icenowy

> 
> As such, it should be fine to use the common helper and drop the
> custom
> code.
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> Changes since v1:
> - Fixed compile issues
> 
> This is only compile tested. I do not have the hardware.
> ---
>  drivers/gpu/drm/verisilicon/vs_cursor_plane.c |  4 +++-
>  drivers/gpu/drm/verisilicon/vs_plane.c        | 20 -----------------
> --
>  .../gpu/drm/verisilicon/vs_primary_plane.c    |  7 ++++++-
>  3 files changed, 9 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> index fa4f601dd0c8..59778433ae84 100644
> --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> @@ -12,6 +12,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_fb_dma_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_atomic_helper.h>
> @@ -176,7 +177,8 @@ static void vs_cursor_plane_atomic_update(struct
> drm_plane *plane,
>  		break;
>  	}
>  
> -	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
> +	/* hardware handles clipping as seen below */
> +	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
>  
>  	regmap_write(dc->regs, VSDC_CURSOR_ADDRESS(output),
>  		     lower_32_bits(dma_addr));
> diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c
> b/drivers/gpu/drm/verisilicon/vs_plane.c
> index d81f7b8f4c65..38b8b536eccb 100644
> --- a/drivers/gpu/drm/verisilicon/vs_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_plane.c
> @@ -107,26 +107,6 @@ int drm_format_to_vs_format(u32 drm_format,
> struct vs_format *vs_format)
>  	return 0;
>  }
>  
> -dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
> -			      const struct drm_rect *src_rect)
> -{
> -	struct drm_gem_dma_object *gem;
> -	dma_addr_t dma_addr;
> -
> -	/* Get the physical address of the buffer in memory */
> -	gem = drm_fb_dma_get_gem_obj(fb, 0);
> -
> -	/* Compute the start of the displayed memory */
> -	dma_addr = gem->dma_addr + fb->offsets[0];
> -
> -	/* Fixup framebuffer address for src coordinates */
> -	dma_addr += drm_format_info_min_pitch(fb->format, 0,
> -					      src_rect->x1 >> 16);
> -	dma_addr += (src_rect->y1 >> 16) * fb->pitches[0];
> -
> -	return dma_addr;
> -}
> -
>  struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane
> *plane)
>  {
>  	struct vs_plane_state *vs_state, *vs_state_old;
> diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> index 1f2be41ae496..2750016a7f2c 100644
> --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> @@ -8,6 +8,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_fb_dma_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_atomic_helper.h>
> @@ -126,7 +127,11 @@ static void
> vs_primary_plane_atomic_update(struct drm_plane *plane,
>  			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
>  			   vs_state->format.uv_swizzle);
>  
> -	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
> +	/*
> +	 * Primary plane cannot be moved, no clipping is involved,
> +	 * so the non-clipped framebuffer address can be used.
> +	 */
> +	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
>  
>  	regmap_write(dc->regs, VSDC_FB_ADDRESS(output),
>  		     lower_32_bits(dma_addr));

Re: [PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

[Icenowy Zheng]

在 2026-08-12三的 23:36 +0800，Icenowy Zheng写道：
> 在 2026-08-07五的 19:46 +0800，Chen-Yu Tsai写道：
> > The verisilicon driver has a custom framebuffer address calculating
> > helper that the common drm_fb_dma_get_addr() can substitute.
> > 
> > Differences from drm_fb_dma_get_addr():
> > 
> > - Uses drm_format_info_min_pitch() to calculate the horizontal
> > offset;
> >   however the driver does not support any of the blocked formats,
> > so
> 
> Technically this DC, advertised as part of the "Vivante" product line
> (considering Vivante Corporation is acquired by VeriSilicon), seems
> to
> support DRM_FORMAT_MOD_VIVANTE_SUPER_TILED modifier (the TH1520
> documentation says the DC supports
> `SuperTileX8x8/SuperTileX8x4/SuperTileY4x8`, although I think
> DRM_FORMAT_MOD_VIVANTE_SUPER_TILED is just one of these tiling).
> 
> However, as my accessible SoCs with such DC have no GC-series 3D GPUs
> (TH1520 does have a 2D-only GC620 GPU), I think it's quite difficult
> to
> get this piece of thing right and it should be low-priority.
> 
> >   this just ends up being the same as in drm_fb_dma_get_addr():
> >   "cpp[plane] * y"
> > 
> > - Uses clipped source coordinates instead of non-clipped
> > coordinates
> >   as in drm_fb_dma_get_addr();
> > 
> >   For the primary plane this doesn't matter, since the primary
> > plane
> >   must match the output, i.e. it cannot be clipped. Also this
> > driver
> >   doesn't support scaling.
> > 
> >   For the cursor plane this seems wrong, as the clipping seems to
> > be
> >   done by the hardware, and thus the buffer address should be
> > unclipped.
> 
> Yes this is right and the current state of the cursor plane is
> broken.
> 
> However another error compensates this error so I didn't catch it
> when
> developing -- the [XY]_OFF fields aren't properly written because I
> forgot to shift the values for them (and then the value gets masked
> by
> regmap_update_bits()), which prevents the HW clipping to happen, and
> the normal-state arrow cursor happens to have no non-transparent
> pixels
> before the hotspot. When testing with `X -retro`, the retro X cursor
> gets quite glitchy with the current code; and when this patch is
> applied w/o the offset fix, the cursor isn't clipped at all.
> 
> Both errors deserve fixes, I will then send the fix for the offset
> writing problem.

Ah I should drop a few tags here.

```
Tested-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Reviewed-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Fixes: 8c4ae2189125 ("drm: verisilicon: add support for cursor planes")
```

Thanks,
Icenowy

> 
> Thanks,
> Icenowy
> 
> > 
> > As such, it should be fine to use the common helper and drop the
> > custom
> > code.
> > 
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > Changes since v1:
> > - Fixed compile issues
> > 
> > This is only compile tested. I do not have the hardware.
> > ---
> >  drivers/gpu/drm/verisilicon/vs_cursor_plane.c |  4 +++-
> >  drivers/gpu/drm/verisilicon/vs_plane.c        | 20 ---------------
> > --
> > --
> >  .../gpu/drm/verisilicon/vs_primary_plane.c    |  7 ++++++-
> >  3 files changed, 9 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > index fa4f601dd0c8..59778433ae84 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > @@ -12,6 +12,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc.h>
> > +#include <drm/drm_fb_dma_helper.h>
> >  #include <drm/drm_fourcc.h>
> >  #include <drm/drm_framebuffer.h>
> >  #include <drm/drm_gem_atomic_helper.h>
> > @@ -176,7 +177,8 @@ static void
> > vs_cursor_plane_atomic_update(struct
> > drm_plane *plane,
> >  		break;
> >  	}
> >  
> > -	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
> > +	/* hardware handles clipping as seen below */
> > +	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
> >  
> >  	regmap_write(dc->regs, VSDC_CURSOR_ADDRESS(output),
> >  		     lower_32_bits(dma_addr));
> > diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c
> > b/drivers/gpu/drm/verisilicon/vs_plane.c
> > index d81f7b8f4c65..38b8b536eccb 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_plane.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_plane.c
> > @@ -107,26 +107,6 @@ int drm_format_to_vs_format(u32 drm_format,
> > struct vs_format *vs_format)
> >  	return 0;
> >  }
> >  
> > -dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
> > -			      const struct drm_rect *src_rect)
> > -{
> > -	struct drm_gem_dma_object *gem;
> > -	dma_addr_t dma_addr;
> > -
> > -	/* Get the physical address of the buffer in memory */
> > -	gem = drm_fb_dma_get_gem_obj(fb, 0);
> > -
> > -	/* Compute the start of the displayed memory */
> > -	dma_addr = gem->dma_addr + fb->offsets[0];
> > -
> > -	/* Fixup framebuffer address for src coordinates */
> > -	dma_addr += drm_format_info_min_pitch(fb->format, 0,
> > -					      src_rect->x1 >> 16);
> > -	dma_addr += (src_rect->y1 >> 16) * fb->pitches[0];
> > -
> > -	return dma_addr;
> > -}
> > -
> >  struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane
> > *plane)
> >  {
> >  	struct vs_plane_state *vs_state, *vs_state_old;
> > diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > index 1f2be41ae496..2750016a7f2c 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > @@ -8,6 +8,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc.h>
> > +#include <drm/drm_fb_dma_helper.h>
> >  #include <drm/drm_fourcc.h>
> >  #include <drm/drm_framebuffer.h>
> >  #include <drm/drm_gem_atomic_helper.h>
> > @@ -126,7 +127,11 @@ static void
> > vs_primary_plane_atomic_update(struct drm_plane *plane,
> >  			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
> >  			   vs_state->format.uv_swizzle);
> >  
> > -	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
> > +	/*
> > +	 * Primary plane cannot be moved, no clipping is involved,
> > +	 * so the non-clipped framebuffer address can be used.
> > +	 */
> > +	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
> >  
> >  	regmap_write(dc->regs, VSDC_FB_ADDRESS(output),
> >  		     lower_32_bits(dma_addr));

Re: [PATCH RFC v2] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

[Icenowy Zheng]

在 2026-08-12三的 23:36 +0800，Icenowy Zheng写道：
> 在 2026-08-07五的 19:46 +0800，Chen-Yu Tsai写道：
> > The verisilicon driver has a custom framebuffer address calculating
> > helper that the common drm_fb_dma_get_addr() can substitute.
> > 
> > Differences from drm_fb_dma_get_addr():
> > 
> > - Uses drm_format_info_min_pitch() to calculate the horizontal
> > offset;
> >   however the driver does not support any of the blocked formats,
> > so
> 
> Technically this DC, advertised as part of the "Vivante" product line
> (considering Vivante Corporation is acquired by VeriSilicon), seems
> to
> support DRM_FORMAT_MOD_VIVANTE_SUPER_TILED modifier (the TH1520
> documentation says the DC supports
> `SuperTileX8x8/SuperTileX8x4/SuperTileY4x8`, although I think
> DRM_FORMAT_MOD_VIVANTE_SUPER_TILED is just one of these tiling).
> 
> However, as my accessible SoCs with such DC have no GC-series 3D GPUs
> (TH1520 does have a 2D-only GC620 GPU), I think it's quite difficult
> to
> get this piece of thing right and it should be low-priority.
> 
> >   this just ends up being the same as in drm_fb_dma_get_addr():
> >   "cpp[plane] * y"
> > 
> > - Uses clipped source coordinates instead of non-clipped
> > coordinates
> >   as in drm_fb_dma_get_addr();
> > 
> >   For the primary plane this doesn't matter, since the primary
> > plane
> >   must match the output, i.e. it cannot be clipped. Also this
> > driver
> >   doesn't support scaling.
> > 
> >   For the cursor plane this seems wrong, as the clipping seems to
> > be
> >   done by the hardware, and thus the buffer address should be
> > unclipped.
> 
> Yes this is right and the current state of the cursor plane is
> broken.
> 
> However another error compensates this error so I didn't catch it
> when
> developing -- the [XY]_OFF fields aren't properly written because I
> forgot to shift the values for them (and then the value gets masked
> by
> regmap_update_bits()), which prevents the HW clipping to happen, and
> the normal-state arrow cursor happens to have no non-transparent
> pixels
> before the hotspot. When testing with `X -retro`, the retro X cursor
> gets quite glitchy with the current code; and when this patch is
> applied w/o the offset fix, the cursor isn't clipped at all.
> 
> Both errors deserve fixes, I will then send the fix for the offset
> writing problem.

That's sent as [1].

Thanks,
Icenowy

[1]
https://lore.kernel.org/all/20260812154829.671777-1-zhengxingda@iscas.ac.cn/

> 
> Thanks,
> Icenowy
> 
> > 
> > As such, it should be fine to use the common helper and drop the
> > custom
> > code.
> > 
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > Changes since v1:
> > - Fixed compile issues
> > 
> > This is only compile tested. I do not have the hardware.
> > ---
> >  drivers/gpu/drm/verisilicon/vs_cursor_plane.c |  4 +++-
> >  drivers/gpu/drm/verisilicon/vs_plane.c        | 20 ---------------
> > --
> > --
> >  .../gpu/drm/verisilicon/vs_primary_plane.c    |  7 ++++++-
> >  3 files changed, 9 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > index fa4f601dd0c8..59778433ae84 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> > @@ -12,6 +12,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc.h>
> > +#include <drm/drm_fb_dma_helper.h>
> >  #include <drm/drm_fourcc.h>
> >  #include <drm/drm_framebuffer.h>
> >  #include <drm/drm_gem_atomic_helper.h>
> > @@ -176,7 +177,8 @@ static void
> > vs_cursor_plane_atomic_update(struct
> > drm_plane *plane,
> >  		break;
> >  	}
> >  
> > -	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
> > +	/* hardware handles clipping as seen below */
> > +	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
> >  
> >  	regmap_write(dc->regs, VSDC_CURSOR_ADDRESS(output),
> >  		     lower_32_bits(dma_addr));
> > diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c
> > b/drivers/gpu/drm/verisilicon/vs_plane.c
> > index d81f7b8f4c65..38b8b536eccb 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_plane.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_plane.c
> > @@ -107,26 +107,6 @@ int drm_format_to_vs_format(u32 drm_format,
> > struct vs_format *vs_format)
> >  	return 0;
> >  }
> >  
> > -dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
> > -			      const struct drm_rect *src_rect)
> > -{
> > -	struct drm_gem_dma_object *gem;
> > -	dma_addr_t dma_addr;
> > -
> > -	/* Get the physical address of the buffer in memory */
> > -	gem = drm_fb_dma_get_gem_obj(fb, 0);
> > -
> > -	/* Compute the start of the displayed memory */
> > -	dma_addr = gem->dma_addr + fb->offsets[0];
> > -
> > -	/* Fixup framebuffer address for src coordinates */
> > -	dma_addr += drm_format_info_min_pitch(fb->format, 0,
> > -					      src_rect->x1 >> 16);
> > -	dma_addr += (src_rect->y1 >> 16) * fb->pitches[0];
> > -
> > -	return dma_addr;
> > -}
> > -
> >  struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane
> > *plane)
> >  {
> >  	struct vs_plane_state *vs_state, *vs_state_old;
> > diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > index 1f2be41ae496..2750016a7f2c 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > @@ -8,6 +8,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc.h>
> > +#include <drm/drm_fb_dma_helper.h>
> >  #include <drm/drm_fourcc.h>
> >  #include <drm/drm_framebuffer.h>
> >  #include <drm/drm_gem_atomic_helper.h>
> > @@ -126,7 +127,11 @@ static void
> > vs_primary_plane_atomic_update(struct drm_plane *plane,
> >  			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
> >  			   vs_state->format.uv_swizzle);
> >  
> > -	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
> > +	/*
> > +	 * Primary plane cannot be moved, no clipping is involved,
> > +	 * so the non-clipped framebuffer address can be used.
> > +	 */
> > +	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
> >  
> >  	regmap_write(dc->regs, VSDC_FB_ADDRESS(output),
> >  		     lower_32_bits(dma_addr));