[PATCH v4 04/10] drm/vs: Add hardware funcs for vs.

[Keith Zhao]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> Sent: 2024年6月24日 4:51
> To: Keith Zhao <keith.zhao at starfivetech.com>
> Cc: andrzej.hajda at intel.com; neil.armstrong at linaro.org; rfoss at kernel.org;
> Laurent.pinchart at ideasonboard.com; jonas at kwiboo.se;
> jernej.skrabec at gmail.com; maarten.lankhorst at linux.intel.com;
> mripard at kernel.org; tzimmermann at suse.de; airlied at gmail.com;
> daniel at ffwll.ch; robh at kernel.org; krzk+dt at kernel.org; conor+dt at kernel.org;
> hjc at rock-chips.com; heiko at sntech.de; andy.yan at rock-chips.com; Xingyu Wu
> <xingyu.wu at starfivetech.com>; p.zabel at pengutronix.de; Jack Zhu
> <jack.zhu at starfivetech.com>; Shengyang Chen
> <shengyang.chen at starfivetech.com>; dri-devel at lists.freedesktop.org;
> devicetree at vger.kernel.org; linux-kernel at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v4 04/10] drm/vs: Add hardware funcs for vs.
> 
> Hi Keith,
> 
> On Sun, Jun 23, 2024 at 07:16:47AM GMT, Keith Zhao wrote:
> > > On Tue, May 21, 2024 at 06:58:11PM +0800, keith wrote:
> > > > +}
> > > > +
> > > > +static inline void dc_set_clear(struct dc_hw *hw, u32 reg, u32
> > > > +set, u32 clear) {
> > > > +	u32 value = dc_read(hw, reg);
> > > > +
> > > > +	value &= ~clear;
> > > > +	value |= set;
> > > > +	dc_write(hw, reg, value);
> > >
> > > regmap_update_bits?
> >
> > regmap_update_bits follows 4 steps:
> >
> > 1、ret = _regmap_read(map, reg, &orig); .........
> >
> > 2、tmp = orig & ~mask;
> > 3、tmp |= val & mask;
> > ......
> > 4、ret = _regmap_write(map, reg, tmp);
> > If the value out of mask range
> > It will just clear the mask bir
> >
> > dc_set_clear will do clear and set without limit.
> >
> > Maybe the name should be dc_clear_set
> 
> This is not really better. regmap_update_bits() has clear semantics of updating a
> value in the field that is defined by a mask. You function is just clearing some bits
> and setting other bits. It's not obvious whether it is a mask and value, several
> concurrent flags or something else.
> 
> Even if you are not going to switch to regmaps (you don't have to), please use
> mask & value instead.
> 
Ok got it
> > 		}
> > > > +static void load_rgb_to_yuv(struct dc_hw *hw, u32 offset, s16
> > > > +*table)
> > >
> > > Is there any reason why load_rgb_to_yuv differs from two other
> > > functions?
> > >
> > load_rgb_to_yuv matches crtcs
> >
> > load_yuv_to_rgb matches planes
> > load_rgb_to_rgb matches planes
> 
> Then these functins should have that reflected in their names (and also
> documented, why). If the CSC programming interface is similar, please split the
> implementation to have common code and different data to be used for
> programming.
> 
Ok got it

> > the coefficient(table) is diff between load_rgb_to_yuv and
> > load_yuv_to_rgb
> 
> > > > +void plane_hw_update_scale(struct vs_dc *dc, struct drm_rect
> > > > +*src, struct
> > > drm_rect *dst,
> > > > +			   u8 id, u8 display_id, unsigned int rotation); void
> > > > +plane_hw_update_blend(struct vs_dc *dc, u16 alpha, u16
> > > pixel_blend_mode,
> > > > +			   u8 id, u8 display_id);
> > >
> > > Could you please settle on a single prefix for all your function names?
> > > Ideally it should be close to the driver name. It's hard to
> > > understand that the function comes from the verisilicon driver if
> > > its name starts from dc_ or especially with plane_.
> > Yes  starting with plane_ is not a good idea ,i will add vs_ _ ,
> > thanks
> > >
> > > I'd strongly suggest to stop defining anything outside of the
> > > selected
> > I don't quite understand what "the selected" means, I hope you can
> > fill in some specific details about it Thanks
> 
> "the selected vs_ namespace". So prefix all function names and all structures
> with vs_
Ok, got it.
> 
> 
> --
> With best wishes
> Dmitry