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

在 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/[email protected]/

> 
> Thanks,
> Icenowy
> 
> > 
> > As such, it should be fine to use the common helper and drop the
> > custom
> > code.
> > 
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> > 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));