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

From: Lucas Stach
Date: Wed Jun 21 2023 - 05:41:29 EST

Next message: Keiichi Watanabe: "Re: [PATCH 2/3] fuse: Add negative_dentry_timeout option"
Previous message: Sui Jingfeng: "Re: [PATCH v10 01/11] drm/etnaviv: Add a dedicated function to register an irq handler"
In reply to: Sui Jingfeng: "[PATCH v10 06/11] drm/etnaviv: Add driver support for the PCI devices"
Next in thread: Sui Jingfeng: "Re: [PATCH v10 06/11] drm/etnaviv: Add driver support for the PCI devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

> + help
> + Compile in support for PCI GPUs of Vivante.

Compile in support for Vivante GPUs attached via PCI(e).

> + For example, the GC1000 in LS7A1000 and LS2K1000.

Not sure if the example is that helpful. Maybe drop this.

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

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

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

Next message: Keiichi Watanabe: "Re: [PATCH 2/3] fuse: Add negative_dentry_timeout option"
Previous message: Sui Jingfeng: "Re: [PATCH v10 01/11] drm/etnaviv: Add a dedicated function to register an irq handler"
In reply to: Sui Jingfeng: "[PATCH v10 06/11] drm/etnaviv: Add driver support for the PCI devices"
Next in thread: Sui Jingfeng: "Re: [PATCH v10 06/11] drm/etnaviv: Add driver support for the PCI devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]