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

[Dmitry Baryshkov]

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.

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


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

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

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

-- 
With best wishes
Dmitry