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

[Dmitry Baryshkov]

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.

> 		}
> > > +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.

> 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_


-- 
With best wishes
Dmitry