linux-kernel - RE: [PATCH v4 04/10] drm/vs: Add hardware funcs for vs.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 25 Jun 2024 08:32:49 +0000
From: Keith Zhao <keith.zhao@...rfivetech.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: "andrzej.hajda@...el.com" <andrzej.hajda@...el.com>,
	"neil.armstrong@...aro.org" <neil.armstrong@...aro.org>, "rfoss@...nel.org"
	<rfoss@...nel.org>, "Laurent.pinchart@...asonboard.com"
	<Laurent.pinchart@...asonboard.com>, "jonas@...boo.se" <jonas@...boo.se>,
	"jernej.skrabec@...il.com" <jernej.skrabec@...il.com>,
	"maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
	"mripard@...nel.org" <mripard@...nel.org>, "tzimmermann@...e.de"
	<tzimmermann@...e.de>, "airlied@...il.com" <airlied@...il.com>,
	"daniel@...ll.ch" <daniel@...ll.ch>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "hjc@...k-chips.com" <hjc@...k-chips.com>,
	"heiko@...ech.de" <heiko@...ech.de>, "andy.yan@...k-chips.com"
	<andy.yan@...k-chips.com>, Xingyu Wu <xingyu.wu@...rfivetech.com>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>, Jack Zhu
	<jack.zhu@...rfivetech.com>, Shengyang Chen
	<shengyang.chen@...rfivetech.com>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v4 04/10] drm/vs: Add hardware funcs for vs.



> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> Sent: 2024年6月24日 4:51
> To: Keith Zhao <keith.zhao@...rfivetech.com>
> Cc: andrzej.hajda@...el.com; neil.armstrong@...aro.org; rfoss@...nel.org;
> Laurent.pinchart@...asonboard.com; jonas@...boo.se;
> jernej.skrabec@...il.com; maarten.lankhorst@...ux.intel.com;
> mripard@...nel.org; tzimmermann@...e.de; airlied@...il.com;
> daniel@...ll.ch; robh@...nel.org; krzk+dt@...nel.org; conor+dt@...nel.org;
> hjc@...k-chips.com; heiko@...ech.de; andy.yan@...k-chips.com; Xingyu Wu
> <xingyu.wu@...rfivetech.com>; p.zabel@...gutronix.de; Jack Zhu
> <jack.zhu@...rfivetech.com>; Shengyang Chen
> <shengyang.chen@...rfivetech.com>; dri-devel@...ts.freedesktop.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org;
> linux-arm-kernel@...ts.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