> -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > Sent: 2024年6月24日 4:51 > To: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx> > Cc: andrzej.hajda@xxxxxxxxx; neil.armstrong@xxxxxxxxxx; rfoss@xxxxxxxxxx; > Laurent.pinchart@xxxxxxxxxxxxxxxx; jonas@xxxxxxxxx; > jernej.skrabec@xxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx; > mripard@xxxxxxxxxx; tzimmermann@xxxxxxx; airlied@xxxxxxxxx; > daniel@xxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; > hjc@xxxxxxxxxxxxxx; heiko@xxxxxxxxx; andy.yan@xxxxxxxxxxxxxx; Xingyu Wu > <xingyu.wu@xxxxxxxxxxxxxxxx>; p.zabel@xxxxxxxxxxxxxx; Jack Zhu > <jack.zhu@xxxxxxxxxxxxxxxx>; Shengyang Chen > <shengyang.chen@xxxxxxxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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