Re: [PATCH 2/2] drm/verisilicon: add support for Nuvoton MA35D1 DCUltra Lite display controller

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

在 2026-05-12二的 17:06 +0800，Joey Lu写道：

======= 8< =============
> > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_bridge.c
> > > > > b/drivers/gpu/drm/verisilicon/vs_bridge.c
> > > > > index 7a93049368db..225af322de32 100644
> > > > > --- a/drivers/gpu/drm/verisilicon/vs_bridge.c
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_bridge.c
> > > > > @@ -164,13 +164,16 @@ static void
> > > > > vs_bridge_enable_common(struct
> > > > > vs_crtc *crtc,
> > > > >    			VSDC_DISP_PANEL_CONFIG_CLK_EN);
> > > > >    	regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG(output),
> > > > >    			VSDC_DISP_PANEL_CONFIG_RUNNING);
> > > > > -	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > > > -			
> > > > > VSDC_DISP_PANEL_START_MULTI_DISP_SYNC);
> > > > > -	regmap_set_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > > > -
> > > > > 			VSDC_DISP_PANEL_START_RUNNING(output));
> > > > >    
> > > > > -	regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG_EX(crtc-
> > > > > > id),
> > > > > -			VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
> > > > > +	if (dc->info->has_config_ex) {
> > > > > +		regmap_clear_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_START,
> > > > > +				
> > > > > VSDC_DISP_PANEL_START_MULTI_DISP_SYNC);
> > > > > +		regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_START,
> > > > > +				VSDC_DISP_PANEL_START_RUNNIN
> > > > > G(ou
> > > > > tput
> > > > > ));
> > > > > +
> > > > > +		regmap_set_bits(dc->regs,
> > > > > VSDC_DISP_PANEL_CONFIG_EX(crtc->id),
> > > > > +				VSDC_DISP_PANEL_CONFIG_EX_CO
> > > > > MMIT
> > > > > );
> > > > Should the commit operation happen on DC8000/DCUltraLite too?
> > > > (By
> > > > writing to DcregFrameBufferConfig0.VALID).
> > > > 
> > > > Many registers written has "Note: This field is double
> > > > buffered" in
> > > > the
> > > > DCUltraLite documentation.
> > > > 
> > > > I suggest create a static function for commit -- write to the
> > > > corresponding commit bit on DC8200, and write to
> > > > DcregFrameBufferConfig0.VALID on DC8000/DCUltraLite.
> > > [a] There is no commit operation for DCUltra Lite.
> > > I'll not add a `VSDC_FB_CONFIG_VALID` macro. VALID (BIT(3)) is a
> > > hardware-managed double-buffer status bit: hardware writes
> > > 1=PENDING
> > > when a new register set is ready and clears to 0=WORKING after
> > > the
> > > VBLANK copy. Software must never write it, and there is no
> > > polling
> > > use
> > It seems to be writable and controls whether register buffering is
> > enabled, see [1].
> > 
> > The description of this bit in MA35D1 TRM says "This ensures a
> > frame
> > will always start with a valid working set if this register is
> > programmed last, which reduces the need for SW to wait for the
> > start of
> > a VBLANK signal in order to ensure all states are loaded before the
> > next VBLANK", which indicates some kind of "committing write",
> > although
> > the code at [1] seems to indicate that double buffering is only
> > enabled
> > when bit is cleared.
> > 
> > Anyway this bit should be programmable, and "Software must never
> > write
> > it" contradicts with the MA35D1 TRM.
> > 
> > Thanks,
> > Icenowy
> > 
> > [1]
> > https://github.com/rockos-riscv/rockos-kernel/blob/rockos-v6.6.y/drivers/gpu/drm/eswin/es_dc_hw.c#L993
> Thank you for the correction. I'll add
> `#define VSDC_FB_CONFIG_VALID BIT(3)` to vs_primary_plane_regs.h and
> write it in `vs_primary_plane_commit()` for non-config_ex variants.
> > > case in the driver that requires a named constant. For non-
> > > config_ex
> > > variants, `vs_primary_plane_commit()` performs no commit
> > > operation —
> > > `VSDC_FB_CONFIG_ENABLE` (OUTPUT, BIT(0)) is set in
> > > `vs_crtc_atomic_enable()` and `VSDC_FB_CONFIG_RESET` (BIT(4)) is
> > > set/cleared in the bridge enable/disable paths.

Well according to the driver code for DC8000 from Eswin, and the bit
named "VALID", maybe it should be cleared before programming the
registers, and set after programming registers, to make the process of
programming registers atomic from the perspective of the display
controller.

Anyway this should require testing on real hardware to verify.

By the way, I see multiple peripheral drivers for MA35D1 get applied in
the torvalds tree, but the device tree is still only a skeleton; when
will the device tree be updated?

Thanks,
Icenowy

> > ========= 8< ==========
> >