> -----Original Message----- > From: Dmitry Baryshkov <[email protected]> > Sent: 2024年6月24日 4:51 > To: Keith Zhao <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; Xingyu Wu > <[email protected]>; [email protected]; Jack Zhu > <[email protected]>; Shengyang Chen > <[email protected]>; [email protected]; > [email protected]; [email protected]; > [email protected] > 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
