[PATCH v4 08/10] drm/vs: add vs drm master driver

[Keith Zhao]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> Sent: 2024年6月24日 5:08
> To: Keith Zhao <keith.zhao at starfivetech.com>
> Cc: andrzej.hajda at intel.com; neil.armstrong at linaro.org; rfoss at kernel.org;
> Laurent.pinchart at ideasonboard.com; jonas at kwiboo.se;
> jernej.skrabec at gmail.com; maarten.lankhorst at linux.intel.com;
> mripard at kernel.org; tzimmermann at suse.de; airlied at gmail.com;
> daniel at ffwll.ch; robh at kernel.org; krzk+dt at kernel.org; conor+dt at kernel.org;
> hjc at rock-chips.com; heiko at sntech.de; andy.yan at rock-chips.com; Xingyu Wu
> <xingyu.wu at starfivetech.com>; p.zabel at pengutronix.de; Jack Zhu
> <jack.zhu at starfivetech.com>; Shengyang Chen
> <shengyang.chen at starfivetech.com>; dri-devel at lists.freedesktop.org;
> devicetree at vger.kernel.org; linux-kernel at vger.kernel.org;
> linux-arm-kernel at lists.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 at starfivetech.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