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

[Sui Jingfeng <suijingfeng@loongson.cn>]

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@loongson.cn>
>>
>> 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@pengutronix.de>
>> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   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