Re: [PATCH v3 3/5] drm/verisilicon: introduce per-variant hardware ops table

[Icenowy Zheng <zhengxingda@iscas.ac.cn>]

在 2026-06-08一的 17:45 +0800，Joey Lu写道：
> > > diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > > b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > > index 1f2be41ae496..75bc36a078f7 100644
> > > --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > > +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > > @@ -53,12 +53,6 @@ static int
> > > vs_primary_plane_atomic_check(struct
> > > drm_plane *plane,
> > >   	return 0;
> > >   }
> > >   
> > > -static void vs_primary_plane_commit(struct vs_dc *dc, unsigned
> > > int
> > > output)
> > > -{
> > > -	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> > > -			VSDC_FB_CONFIG_EX_COMMIT);
> > > -}
> > > -
> > >   static void vs_primary_plane_atomic_enable(struct drm_plane
> > > *plane,
> > >   					   struct
> > > drm_atomic_commit
> > > *atomic_state)
> > >   {
> > > @@ -69,13 +63,8 @@ static void
> > > vs_primary_plane_atomic_enable(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_update_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> > > -			   VSDC_FB_CONFIG_EX_DISPLAY_ID_MASK,
> > > -			  
> > > VSDC_FB_CONFIG_EX_DISPLAY_ID(output));
> > > -
> > > -	vs_primary_plane_commit(dc, output);
> > > +	if (dc->funcs->plane_enable_ex)
> > > +		dc->funcs->plane_enable_ex(dc, output);
> > Please note that all theae codes are for primary planes, maybe the
> > helper should be named mentioning primary. Overlay planes will need
> > a
> > different codepath because they change different registers.
> > 
> > Thanks,
> > Icenowy
> Understood. To avoid confusion, I will rename `plane_enable_ex`, 
> `plane_disable_ex`, and `plane_update_ex` to `primary_plane_enable`, 
> `primary_plane_disable`, and `primary_plane_update` in `vs_dc_funcs`,
> `vs_dc8200.c`, and `vs_primary_plane.c`.

Maybe keep the `_ex` here as some operations is still on the common
codepath?

Thanks,
Icenowy

> > >   }
> > >   
> > >   static void vs_primary_plane_atomic_disable(struct drm_plane
> > > *plane,
> > > @@ -88,10 +77,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);
> > > -
> > > -	vs_primary_plane_commit(dc, output);
> > > +	if (dc->funcs->plane_disable_ex)
> > > +		dc->funcs->plane_disable_ex(dc, output);
> > >   }
> > >   
> > >   static void vs_primary_plane_atomic_update(struct drm_plane
> > > *plane,
> > > @@ -133,18 +120,11 @@ static void
> > > vs_primary_plane_atomic_update(struct drm_plane *plane,
> > >   	regmap_write(dc->regs, VSDC_FB_STRIDE(output),
> > >   		     fb->pitches[0]);
> > >   
> > > -	regmap_write(dc->regs, VSDC_FB_TOP_LEFT(output),
> > > -		     VSDC_MAKE_PLANE_POS(state->crtc_x, state-
> > > > crtc_y));
> > > -	regmap_write(dc->regs, VSDC_FB_BOTTOM_RIGHT(output),
> > > -		     VSDC_MAKE_PLANE_POS(state->crtc_x + state-
> > > > crtc_w,
> > > -					 state->crtc_y + state-
> > > > crtc_h));
> > >   	regmap_write(dc->regs, VSDC_FB_SIZE(output),
> > >   		     VSDC_MAKE_PLANE_SIZE(state->crtc_w, state-
> > > > crtc_h));
> > >   
> > > -	regmap_write(dc->regs, VSDC_FB_BLEND_CONFIG(output),
> > > -		     VSDC_FB_BLEND_CONFIG_BLEND_DISABLE);
> > > -
> > > -	vs_primary_plane_commit(dc, output);
> > > +	if (dc->funcs->plane_update_ex)
> > > +		dc->funcs->plane_update_ex(dc, output, state);
> > >   }
> > >   
> > >   static const struct drm_plane_helper_funcs
> > > vs_primary_plane_helper_funcs = {