Re: [PATCH v10 06/11] drm/etnaviv: Add driver support for the PCI devices

[Sui Jingfeng <suijingfeng@xxxxxxxxxxx>]

Hi,

On 2023/6/21 17:39, Lucas Stach wrote:
> Am Dienstag, dem 20.06.2023 um 17:47 +0800 schrieb Sui Jingfeng:
> > From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> >
> > This patch adds PCI driver support on top of what we already have, take
> > the GC1000 in LS7A1000/LS2K1000 as the first instance which enjoy the PCI
> > device driver. There is only one GPU core for the GC1000 in the LS7A1000
> > and LS2K1000. Therefore, component frameworks can be avoided.
> >
> > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > Cc: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
> > Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/etnaviv/Kconfig           | 10 +++
> >   drivers/gpu/drm/etnaviv/Makefile          |  2 +
> >   drivers/gpu/drm/etnaviv/etnaviv_drv.c     | 13 +++-
> >   drivers/gpu/drm/etnaviv/etnaviv_drv.h     |  3 +
> >   drivers/gpu/drm/etnaviv/etnaviv_gpu.c     |  8 +--
> >   drivers/gpu/drm/etnaviv/etnaviv_gpu.h     |  6 ++
> >   drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 75 +++++++++++++++++++++++
> >   drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h | 18 ++++++
> >   8 files changed, 128 insertions(+), 7 deletions(-)
> >   create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
> >   create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
> >
> > diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
> > index faa7fc68b009..1b5b162efb61 100644
> > --- a/drivers/gpu/drm/etnaviv/Kconfig
> > +++ b/drivers/gpu/drm/etnaviv/Kconfig
> > @@ -15,6 +15,16 @@ config DRM_ETNAVIV
> >   	help
> >   	  DRM driver for Vivante GPUs.
> >   
> > +config DRM_ETNAVIV_PCI_DRIVER
> > +	bool "enable ETNAVIV PCI driver support"
> > +	depends on DRM_ETNAVIV
> > +	depends on PCI
> > +	default y
> Drop the default y. Lots of existing systems with Vivante GPUs on a
> platform bus also have a PCI bus, so this will activate the added code
> paths in a lot of configurations that don't need it.
This is OK,
> > +	help
> > +	  Compile in support for PCI GPUs of Vivante.
> Compile in support for Vivante GPUs attached via PCI(e).
OK, acceptable.
> > +	  For example, the GC1000 in LS7A1000 and LS2K1000.
> Not sure if the example is that helpful. Maybe drop this.
OK.
> > +	  Say Y if you have such a hardware.
> > +
> >   config DRM_ETNAVIV_THERMAL
> >   	bool "enable ETNAVIV thermal throttling"
> >   	depends on DRM_ETNAVIV
> > diff --git a/drivers/gpu/drm/etnaviv/Makefile b/drivers/gpu/drm/etnaviv/Makefile
> > index 46e5ffad69a6..6829e1ebf2db 100644
> > --- a/drivers/gpu/drm/etnaviv/Makefile
> > +++ b/drivers/gpu/drm/etnaviv/Makefile
> > @@ -16,4 +16,6 @@ etnaviv-y := \
> >   	etnaviv_perfmon.o \
> >   	etnaviv_sched.o
> >   
> > +etnaviv-$(CONFIG_DRM_ETNAVIV_PCI_DRIVER) += etnaviv_pci_drv.o
> > +
> >   obj-$(CONFIG_DRM_ETNAVIV)	+= etnaviv.o
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > index 93ca240cd4c0..0a365e96d371 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > @@ -21,6 +21,7 @@
> >   #include "etnaviv_gpu.h"
> >   #include "etnaviv_gem.h"
> >   #include "etnaviv_mmu.h"
> > +#include "etnaviv_pci_drv.h"
> >   #include "etnaviv_perfmon.h"
> >   
> >   /*
> > @@ -538,7 +539,7 @@ static const struct drm_driver etnaviv_drm_driver = {
> >   
> >   static struct etnaviv_drm_private *etna_private_ptr;
> >   
> > -static int etnaviv_drm_bind(struct device *dev, bool component)
> > +int etnaviv_drm_bind(struct device *dev, bool component)
> >   {
> >   	struct etnaviv_drm_private *priv;
> >   	struct drm_device *drm;
> > @@ -588,7 +589,7 @@ static int etnaviv_drm_bind(struct device *dev, bool component)
> >   	return ret;
> >   }
> >   
> > -static void etnaviv_drm_unbind(struct device *dev, bool component)
> > +void etnaviv_drm_unbind(struct device *dev, bool component)
> >   {
> >   	struct etnaviv_drm_private *priv = etna_private_ptr;
> >   	struct drm_device *drm = priv->drm;
> > @@ -746,6 +747,10 @@ static int __init etnaviv_init(void)
> >   	if (ret != 0)
> >   		goto unregister_gpu_driver;
> >   
> > +	ret = etnaviv_register_pci_driver();
> > +	if (ret != 0)
> > +		goto unregister_platform_driver;
> > +
> >   	/*
> >   	 * If the DT contains at least one available GPU device, instantiate
> >   	 * the DRM platform device.
> > @@ -763,7 +768,7 @@ static int __init etnaviv_init(void)
> >   		break;
> >   	}
> >   
> > -	return 0;
> > +	return ret;
> >   
> >   unregister_platform_driver:
> >   	platform_driver_unregister(&etnaviv_platform_driver);
> > @@ -778,6 +783,8 @@ static void __exit etnaviv_exit(void)
> >   	etnaviv_destroy_platform_device(&etnaviv_platform_device);
> >   	platform_driver_unregister(&etnaviv_platform_driver);
> >   	platform_driver_unregister(&etnaviv_gpu_driver);
> > +
> > +	etnaviv_unregister_pci_driver();
> >   }
> >   module_exit(etnaviv_exit);
> >   
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> > index e58f82e698de..9cd72948cfad 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> > @@ -83,6 +83,9 @@ bool etnaviv_cmd_validate_one(struct etnaviv_gpu *gpu,
> >   	u32 *stream, unsigned int size,
> >   	struct drm_etnaviv_gem_submit_reloc *relocs, unsigned int reloc_size);
> >   
> > +int etnaviv_drm_bind(struct device *dev, bool component);
> > +void etnaviv_drm_unbind(struct device *dev, bool component);
> > +
> >   #ifdef CONFIG_DEBUG_FS
> >   void etnaviv_gem_describe_objects(struct etnaviv_drm_private *priv,
> >   	struct seq_file *m);
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > index 059be8c89c5a..d6a21e97feb1 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > @@ -1866,8 +1866,8 @@ static int etnaviv_gpu_register_irq(struct etnaviv_gpu *gpu, int irq)
> >   
> >   /* platform independent */
> >   
> > -static int etnaviv_gpu_driver_create(struct device *dev, void __iomem *mmio,
> > -				     int irq, bool component, bool has_clk)
> > +int etnaviv_gpu_driver_create(struct device *dev, void __iomem *mmio,
> > +			      int irq, bool component, bool has_clk)
> >   {
> >   	struct etnaviv_gpu *gpu;
> >   	int err;
> > @@ -1916,7 +1916,7 @@ static int etnaviv_gpu_driver_create(struct device *dev, void __iomem *mmio,
> >   	return 0;
> >   }
> >   
> > -static void etnaviv_gpu_driver_destroy(struct device *dev, bool component)
> > +void etnaviv_gpu_driver_destroy(struct device *dev, bool component)
> >   {
> >   	if (component)
> >   		component_del(dev, &gpu_ops);
> > @@ -1967,7 +1967,7 @@ static int etnaviv_gpu_rpm_resume(struct device *dev)
> >   	return 0;
> >   }
> >   
> > -static const struct dev_pm_ops etnaviv_gpu_pm_ops = {
> > +const struct dev_pm_ops etnaviv_gpu_pm_ops = {
> >   	RUNTIME_PM_OPS(etnaviv_gpu_rpm_suspend, etnaviv_gpu_rpm_resume, NULL)
> >   };
> >   
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > index 1ec829a649b5..8d9833996ed7 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > @@ -209,6 +209,12 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch);
> >   int etnaviv_gpu_bind(struct device *dev, struct device *master, void *data);
> >   void etnaviv_gpu_unbind(struct device *dev, struct device *master, void *data);
> >   
> > +int etnaviv_gpu_driver_create(struct device *dev, void __iomem *mmio,
> > +			      int irq, bool component, bool has_clk);
> > +
> > +void etnaviv_gpu_driver_destroy(struct device *dev, bool component);
> > +
> >   extern struct platform_driver etnaviv_gpu_driver;
> > +extern const struct dev_pm_ops etnaviv_gpu_pm_ops;
> >   
> >   #endif /* __ETNAVIV_GPU_H__ */
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
> > new file mode 100644
> > index 000000000000..78e44a28d30c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
> > @@ -0,0 +1,75 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/pci.h>
> > +
> > +#include "etnaviv_drv.h"
> > +#include "etnaviv_gpu.h"
> > +#include "etnaviv_pci_drv.h"
> > +
> > +static int etnaviv_pci_probe(struct pci_dev *pdev,
> > +			     const struct pci_device_id *ent)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	void __iomem *mmio;
> > +	int ret;
> > +
> > +	ret = pcim_enable_device(pdev);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable\n");
> > +		return ret;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> Is your device really restricted to 32 bit dma addressing?
Yes, if I don't add this, bugs may happens.
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Map registers, assume the PCI bar 0 contain the registers */
> > +	mmio = pcim_iomap(pdev, 0, 0);
> > +	if (IS_ERR(mmio))
> > +		return PTR_ERR(mmio);
> > +
> > +	ret = etnaviv_gpu_driver_create(dev, mmio, pdev->irq, false, false);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return etnaviv_drm_bind(dev, false);
> > +}
> > +
> > +static void etnaviv_pci_remove(struct pci_dev *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +
> > +	etnaviv_drm_unbind(dev, false);
> > +
> > +	etnaviv_gpu_driver_destroy(dev, false);
> > +
> > +	pci_clear_master(pdev);
> > +}
> > +
> > +static const struct pci_device_id etnaviv_pci_id_lists[] = {
> > +	{PCI_VDEVICE(LOONGSON, 0x7a15)},
> > +	{PCI_VDEVICE(LOONGSON, 0x7a05)},
> > +	{ }
> > +};
> > +
> > +static struct pci_driver etnaviv_pci_driver = {
> > +	.name = "etnaviv",
> > +	.id_table = etnaviv_pci_id_lists,
> > +	.probe = etnaviv_pci_probe,
> > +	.remove = etnaviv_pci_remove,
> > +	.driver.pm = pm_ptr(&etnaviv_gpu_pm_ops),
> > +};
> > +
> > +int etnaviv_register_pci_driver(void)
> > +{
> > +	return pci_register_driver(&etnaviv_pci_driver);
> > +}
> > +
> > +void etnaviv_unregister_pci_driver(void)
> > +{
> > +	pci_unregister_driver(&etnaviv_pci_driver);
> > +}
> > +
> > +MODULE_DEVICE_TABLE(pci, etnaviv_pci_id_lists);
> Hm, I think the amount of code added here is small enough to go
> directly into etnaviv_drv.c behind a
> #ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER, which would allow to keep the
> referenced functions and pm ops static.
OK, this is acceptable.
> Regards,
> Lucas
>
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
> > new file mode 100644
> > index 000000000000..1db559ee5e9b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef __ETNAVIV_PCI_DRV_H__
> > +#define __ETNAVIV_PCI_DRV_H__
> > +
> > +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> > +
> > +int etnaviv_register_pci_driver(void);
> > +void etnaviv_unregister_pci_driver(void);
> > +
> > +#else
> > +
> > +static inline int etnaviv_register_pci_driver(void) { return 0; }
> > +static inline void etnaviv_unregister_pci_driver(void) { }
> > +
> > +#endif
> > +
> > +#endif

--
Jingfeng