linux-kernel - RE: [PATCH v4 08/10] drm/vs: add vs drm master driver

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] [day] [month] [year] [list]
Date: Tue, 25 Jun 2024 08:34:08 +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 08/10] drm/vs: add vs drm master driver



> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> Sent: 2024年6月24日 5:08
> 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 08/10] drm/vs: add vs drm master driver
> 
> On Sun, Jun 23, 2024 at 07:16:57AM GMT, Keith Zhao wrote:
> > > On Tue, May 21, 2024 at 06:58:15PM +0800, keith wrote:
> > > > Add vs DRM master driver for JH7110 SoC ADD DMA GEM driver
> > > >
> > > > Signed-off-by: keith <keith.zhao@...rfivetech.com>
> > > > ---
> > > >  drivers/gpu/drm/verisilicon/Makefile |   3 +-
> > > >  drivers/gpu/drm/verisilicon/vs_drv.c | 718
> > > > +++++++++++++++++++++++++++
> > > >  2 files changed, 720 insertions(+), 1 deletion(-)  create mode
> > > > 100644 drivers/gpu/drm/verisilicon/vs_drv.c
> > > >
> 
> > > BIT(DRM_COLOR_YCBCR_BT2020),
> > > > +		.zpos			= 0,
> > >
> > > How are these zpos related to the zpos from drm_plane_state?
> > Zpos was added to drm_plane_state by calling
> > drm_plane_create_zpos_property funs,
> >
> > vs_plane_primary_create
> > ------> drm_plane_create_zpos_property(......vs_plane_primary_info->
> > ------> zpos )
> 
> Yes. But why do you need zpos here? Especially if it's set to 0.

if any plane has the zpos attribute (whether variable or immutable), 
then all planes should have the zpos attribute to ensure consistent stacking order and behavior.
Here is the initial value of zpos property
Usually min can be set to 0 and I set the zpos of primary plane to 0......

> 

> > >
> > > > +
> > > > +	drm_dev->mode_config.min_width = min_width;
> > > > +	drm_dev->mode_config.min_height = min_heigth;
> > > > +	drm_dev->mode_config.max_width = max_width;
> > > > +	drm_dev->mode_config.max_height = max_height;
> > >
> > > I thought that I saw mode_config.min/max being initialized.
> > Yes the mode_config.min/max has been initialized，
> > This place is doing an update according to detail info.
> 
> Then please drop previous initialisation. While looking at the code it's
> impossible to understand which one is correct.
Ok got it.
> 
> 
> > > > +
> > > > +static struct component_match *vs_add_external_components(struct
> > > > +device *dev) {
> > > > +	struct component_match *match = NULL;
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < ARRAY_SIZE(drm_sub_drivers); ++i) {
> > > > +		struct platform_driver *drv = drm_sub_drivers[i];
> > > > +		struct device *p = NULL, *d;
> > > > +
> > > > +		while ((d = platform_find_device_by_driver(p, &drv->driver))) {
> > > > +			put_device(p);
> > > > +
> > > > +			drm_of_component_match_add(dev, &match,
> > > component_compare_of,
> > > > +						   d->of_node);
> > > > +			p = d;
> > > > +		}
> > > > +		put_device(p);
> > >
> > > What about just going through the graph connections instead and adding
> them?
> >
> > The purpose of using components is to create encoder and connector to the
> drm subsystem by calling component_bind_all
> >
> > graph connection needs to be based on whether there is a bridge at present.
> > If the bridge has been added, it can be obtained through drm_of_get_bridge
> > Create a connector based on the obtained bridge and then attach the
> connector to the encoder.
> > Then do drm_dev_register.
> >
> > I don't know if my understanding is consistent with yours. Please help confirm
> it.
> > Thanks
> 
> Your code is looping over the subdrivers, locating devices and then
> adding them as components. Can you instead use device nodes which are
> connected to your master via the OF graph? If I understand examples in
> your DT bindings correctly, this approach should work.

Yes, The OF graph method appears to be more efficient and does not require traversal.
After find the device node , 
Let it start:
drm_of_component_match_add(dev, &match, component_compare_of, device->of_node);

> 
> > > > +static void __exit vs_drm_fini(void)
> > > > +{
> > > > +	platform_driver_unregister(&vs_drm_platform_driver);
> > > > +	platform_unregister_drivers(drm_sub_drivers,
> > > > +ARRAY_SIZE(drm_sub_drivers)); }
> > > > +
> > > > +late_initcall_sync(vs_drm_init);
> > >
> > > Why _sync?
> >
> > late_initcall_sync will make it success ,when do devm_drm_of_get_bridge.
> > Also it can use the " EPROBE_DEFER " to avoid it,
> 
> Why do you need this? It's perfectly fine to have DRM devices probe
> assynchronously.
Will replace it by module_init

> 
> > >
> > > > +module_exit(vs_drm_fini);
> > > > +
> > > > +MODULE_DESCRIPTION("VeriSilicon DRM Driver");
> > > MODULE_LICENSE("GPL");
> > > > --
> > > > 2.27.0
> > > >
> 
> --
> With best wishes
> Dmitry