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