[PATCH v6 0/6] drm/etnaviv: add pci device driver support

[PATCH v6 0/6] drm/etnaviv: add pci device driver support

[Sui Jingfeng]

There is a Vivante GC1000 (v5037) in LS2K1000 and LS7A1000, this GPU is a
PCI device, and it has 2D and 3D cores in the same core. Thus, this patch
set is trying to add PCI device driver support to etnaviv.

v6:
	* Fix build issue on system without CONFIG_PCI enabled

Sui Jingfeng (6):
  drm/etnaviv: add a dedicated function to register an irq handler
  drm/etnaviv: add a dedicated function to get various clocks
  drm/etnaviv: add dedicated functions to create and destroy platform
    devices
  drm/etnaviv: add helpers for private data construction and destruction
  drm/etnaviv: add driver support for the PCI devices
  drm/etnaviv: allow usperspace create cached coherent bo

 drivers/gpu/drm/etnaviv/Kconfig             |   9 +
 drivers/gpu/drm/etnaviv/Makefile            |   2 +
 drivers/gpu/drm/etnaviv/etnaviv_drv.c       | 190 ++++++++++++++------
 drivers/gpu/drm/etnaviv/etnaviv_drv.h       |   7 +
 drivers/gpu/drm/etnaviv/etnaviv_gem.c       |  22 ++-
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |   9 +-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c       | 185 +++++++++++++------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h       |  13 ++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c   |  87 +++++++++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h   |  12 ++
 include/uapi/drm/etnaviv_drm.h              |  11 +-
 11 files changed, 433 insertions(+), 114 deletions(-)
 create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
 create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h

-- 
2.25.1

[PATCH v6 0/6] drm/etnaviv: add pci device driver support

[Sui Jingfeng]

There is a Vivante GC1000 (v5037) in LS2K1000 and LS7A1000, this GPU is a
PCI device, and it has 2D and 3D cores in the same core. Thus, this patch
set is trying to add PCI device driver support to etnaviv.

v6:
	* Fix build issue on system without CONFIG_PCI enabled

Sui Jingfeng (6):
  drm/etnaviv: add a dedicated function to register an irq handler
  drm/etnaviv: add a dedicated function to get various clocks
  drm/etnaviv: add dedicated functions to create and destroy platform
    devices
  drm/etnaviv: add helpers for private data construction and destruction
  drm/etnaviv: add driver support for the PCI devices
  drm/etnaviv: allow usperspace create cached coherent bo

 drivers/gpu/drm/etnaviv/Kconfig             |   9 +
 drivers/gpu/drm/etnaviv/Makefile            |   2 +
 drivers/gpu/drm/etnaviv/etnaviv_drv.c       | 190 ++++++++++++++------
 drivers/gpu/drm/etnaviv/etnaviv_drv.h       |   7 +
 drivers/gpu/drm/etnaviv/etnaviv_gem.c       |  22 ++-
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |   9 +-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c       | 185 +++++++++++++------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h       |  13 ++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c   |  87 +++++++++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h   |  12 ++
 include/uapi/drm/etnaviv_drm.h              |  11 +-
 11 files changed, 433 insertions(+), 114 deletions(-)
 create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
 create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h

-- 
2.25.1

[PATCH v6 1/6] drm/etnaviv: add a dedicated function to register an irq handler

[Sui Jingfeng]

Because getting IRQ from a device is platform-dependent, PCI devices have
different methods for getting an IRQ. This patch is a preparation patch to
extend support for the PCI device.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 34 ++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index de8c9894967c..636d3f39ddcb 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1817,6 +1817,29 @@ static const struct of_device_id etnaviv_gpu_match[] = {
 };
 MODULE_DEVICE_TABLE(of, etnaviv_gpu_match);
 
+static int etnaviv_gpu_register_irq(struct etnaviv_gpu *gpu, int irq)
+{
+	struct device *dev = gpu->dev;
+	int err;
+
+	if (irq < 0) {
+		dev_err(dev, "failed to get irq: %d\n", irq);
+		return irq;
+	}
+
+	err = devm_request_irq(dev, irq, irq_handler, 0, dev_name(dev), gpu);
+	if (err) {
+		dev_err(dev, "failed to request IRQ %u: %d\n", irq, err);
+		return err;
+	}
+
+	gpu->irq = irq;
+
+	dev_info(dev, "IRQ handler registered, irq = %d\n", irq);
+
+	return 0;
+}
+
 static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1837,16 +1860,9 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 		return PTR_ERR(gpu->mmio);
 
 	/* Get Interrupt: */
-	gpu->irq = platform_get_irq(pdev, 0);
-	if (gpu->irq < 0)
-		return gpu->irq;
-
-	err = devm_request_irq(&pdev->dev, gpu->irq, irq_handler, 0,
-			       dev_name(gpu->dev), gpu);
-	if (err) {
-		dev_err(dev, "failed to request IRQ%u: %d\n", gpu->irq, err);
+	err = etnaviv_gpu_register_irq(gpu,  platform_get_irq(pdev, 0));
+	if (err)
 		return err;
-	}
 
 	/* Get Clocks: */
 	gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
-- 
2.25.1

[PATCH v6 1/6] drm/etnaviv: add a dedicated function to register an irq handler

[Sui Jingfeng]

Because getting IRQ from a device is platform-dependent, PCI devices have
different methods for getting an IRQ. This patch is a preparation patch to
extend support for the PCI device.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 34 ++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index de8c9894967c..636d3f39ddcb 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1817,6 +1817,29 @@ static const struct of_device_id etnaviv_gpu_match[] = {
 };
 MODULE_DEVICE_TABLE(of, etnaviv_gpu_match);
 
+static int etnaviv_gpu_register_irq(struct etnaviv_gpu *gpu, int irq)
+{
+	struct device *dev = gpu->dev;
+	int err;
+
+	if (irq < 0) {
+		dev_err(dev, "failed to get irq: %d\n", irq);
+		return irq;
+	}
+
+	err = devm_request_irq(dev, irq, irq_handler, 0, dev_name(dev), gpu);
+	if (err) {
+		dev_err(dev, "failed to request IRQ %u: %d\n", irq, err);
+		return err;
+	}
+
+	gpu->irq = irq;
+
+	dev_info(dev, "IRQ handler registered, irq = %d\n", irq);
+
+	return 0;
+}
+
 static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1837,16 +1860,9 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 		return PTR_ERR(gpu->mmio);
 
 	/* Get Interrupt: */
-	gpu->irq = platform_get_irq(pdev, 0);
-	if (gpu->irq < 0)
-		return gpu->irq;
-
-	err = devm_request_irq(&pdev->dev, gpu->irq, irq_handler, 0,
-			       dev_name(gpu->dev), gpu);
-	if (err) {
-		dev_err(dev, "failed to request IRQ%u: %d\n", gpu->irq, err);
+	err = etnaviv_gpu_register_irq(gpu,  platform_get_irq(pdev, 0));
+	if (err)
 		return err;
-	}
 
 	/* Get Clocks: */
 	gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
-- 
2.25.1

[PATCH v6 2/6] drm/etnaviv: add a dedicated function to get various clocks

[Sui Jingfeng]

Because it is also platform-dependent, there are environments where don't
have CLK subsystem support, for example, discreted PCI gpu. So don't rage
quit if there is no CLK subsystem.

For the GPU in LS7a1000 and LS2k2000, the working frequency of the GPU is
tuned by configuring the PLL register directly.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 62 ++++++++++++++++++---------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  1 +
 2 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 636d3f39ddcb..4937580551a5 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1565,10 +1565,45 @@ static irqreturn_t irq_handler(int irq, void *data)
 	return ret;
 }
 
+static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
+{
+	struct device *dev = gpu->dev;
+
+	if (gpu->no_clk)
+		return 0;
+
+	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
+	DBG("clk_reg: %p", gpu->clk_reg);
+	if (IS_ERR(gpu->clk_reg))
+		return PTR_ERR(gpu->clk_reg);
+
+	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
+	DBG("clk_bus: %p", gpu->clk_bus);
+	if (IS_ERR(gpu->clk_bus))
+		return PTR_ERR(gpu->clk_bus);
+
+	gpu->clk_core = devm_clk_get(dev, "core");
+	DBG("clk_core: %p", gpu->clk_core);
+	if (IS_ERR(gpu->clk_core))
+		return PTR_ERR(gpu->clk_core);
+	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
+
+	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
+	DBG("clk_shader: %p", gpu->clk_shader);
+	if (IS_ERR(gpu->clk_shader))
+		return PTR_ERR(gpu->clk_shader);
+	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
+
+	return 0;
+}
+
 static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
 {
 	int ret;
 
+	if (gpu->no_clk)
+		return 0;
+
 	ret = clk_prepare_enable(gpu->clk_reg);
 	if (ret)
 		return ret;
@@ -1599,6 +1634,9 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
 
 static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
 {
+	if (gpu->no_clk)
+		return 0;
+
 	clk_disable_unprepare(gpu->clk_shader);
 	clk_disable_unprepare(gpu->clk_core);
 	clk_disable_unprepare(gpu->clk_bus);
@@ -1865,27 +1903,9 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 		return err;
 
 	/* Get Clocks: */
-	gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
-	DBG("clk_reg: %p", gpu->clk_reg);
-	if (IS_ERR(gpu->clk_reg))
-		return PTR_ERR(gpu->clk_reg);
-
-	gpu->clk_bus = devm_clk_get_optional(&pdev->dev, "bus");
-	DBG("clk_bus: %p", gpu->clk_bus);
-	if (IS_ERR(gpu->clk_bus))
-		return PTR_ERR(gpu->clk_bus);
-
-	gpu->clk_core = devm_clk_get(&pdev->dev, "core");
-	DBG("clk_core: %p", gpu->clk_core);
-	if (IS_ERR(gpu->clk_core))
-		return PTR_ERR(gpu->clk_core);
-	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
-
-	gpu->clk_shader = devm_clk_get_optional(&pdev->dev, "shader");
-	DBG("clk_shader: %p", gpu->clk_shader);
-	if (IS_ERR(gpu->clk_shader))
-		return PTR_ERR(gpu->clk_shader);
-	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
+	err = etnaviv_gpu_clk_get(gpu);
+	if (err)
+		return err;
 
 	/* TODO: figure out max mapped size */
 	dev_set_drvdata(dev, gpu);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 98c6f9c320fc..6da5209a7d64 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -148,6 +148,7 @@ struct etnaviv_gpu {
 	struct clk *clk_reg;
 	struct clk *clk_core;
 	struct clk *clk_shader;
+	bool no_clk;
 
 	unsigned int freq_scale;
 	unsigned long base_rate_core;
-- 
2.25.1

[PATCH v6 2/6] drm/etnaviv: add a dedicated function to get various clocks

[Sui Jingfeng]

Because it is also platform-dependent, there are environments where don't
have CLK subsystem support, for example, discreted PCI gpu. So don't rage
quit if there is no CLK subsystem.

For the GPU in LS7a1000 and LS2k2000, the working frequency of the GPU is
tuned by configuring the PLL register directly.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 62 ++++++++++++++++++---------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  1 +
 2 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 636d3f39ddcb..4937580551a5 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1565,10 +1565,45 @@ static irqreturn_t irq_handler(int irq, void *data)
 	return ret;
 }
 
+static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
+{
+	struct device *dev = gpu->dev;
+
+	if (gpu->no_clk)
+		return 0;
+
+	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
+	DBG("clk_reg: %p", gpu->clk_reg);
+	if (IS_ERR(gpu->clk_reg))
+		return PTR_ERR(gpu->clk_reg);
+
+	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
+	DBG("clk_bus: %p", gpu->clk_bus);
+	if (IS_ERR(gpu->clk_bus))
+		return PTR_ERR(gpu->clk_bus);
+
+	gpu->clk_core = devm_clk_get(dev, "core");
+	DBG("clk_core: %p", gpu->clk_core);
+	if (IS_ERR(gpu->clk_core))
+		return PTR_ERR(gpu->clk_core);
+	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
+
+	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
+	DBG("clk_shader: %p", gpu->clk_shader);
+	if (IS_ERR(gpu->clk_shader))
+		return PTR_ERR(gpu->clk_shader);
+	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
+
+	return 0;
+}
+
 static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
 {
 	int ret;
 
+	if (gpu->no_clk)
+		return 0;
+
 	ret = clk_prepare_enable(gpu->clk_reg);
 	if (ret)
 		return ret;
@@ -1599,6 +1634,9 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
 
 static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
 {
+	if (gpu->no_clk)
+		return 0;
+
 	clk_disable_unprepare(gpu->clk_shader);
 	clk_disable_unprepare(gpu->clk_core);
 	clk_disable_unprepare(gpu->clk_bus);
@@ -1865,27 +1903,9 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 		return err;
 
 	/* Get Clocks: */
-	gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
-	DBG("clk_reg: %p", gpu->clk_reg);
-	if (IS_ERR(gpu->clk_reg))
-		return PTR_ERR(gpu->clk_reg);
-
-	gpu->clk_bus = devm_clk_get_optional(&pdev->dev, "bus");
-	DBG("clk_bus: %p", gpu->clk_bus);
-	if (IS_ERR(gpu->clk_bus))
-		return PTR_ERR(gpu->clk_bus);
-
-	gpu->clk_core = devm_clk_get(&pdev->dev, "core");
-	DBG("clk_core: %p", gpu->clk_core);
-	if (IS_ERR(gpu->clk_core))
-		return PTR_ERR(gpu->clk_core);
-	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
-
-	gpu->clk_shader = devm_clk_get_optional(&pdev->dev, "shader");
-	DBG("clk_shader: %p", gpu->clk_shader);
-	if (IS_ERR(gpu->clk_shader))
-		return PTR_ERR(gpu->clk_shader);
-	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
+	err = etnaviv_gpu_clk_get(gpu);
+	if (err)
+		return err;
 
 	/* TODO: figure out max mapped size */
 	dev_set_drvdata(dev, gpu);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 98c6f9c320fc..6da5209a7d64 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -148,6 +148,7 @@ struct etnaviv_gpu {
 	struct clk *clk_reg;
 	struct clk *clk_core;
 	struct clk *clk_shader;
+	bool no_clk;
 
 	unsigned int freq_scale;
 	unsigned long base_rate_core;
-- 
2.25.1

[PATCH v6 3/6] drm/etnaviv: add dedicated functions to create and destroy platform devices

[Sui Jingfeng]

Also rename the virtual master platform device as etnaviv_platform_device,
for better reflection that it is a platform device, not a DRM device.

Another benefit is that we no longer need to call of_node_put() for three
different cases, Instead, we only need to call it once.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 56 +++++++++++++++++++--------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 31a7f59ccb49..0a9d90c18f2c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -656,12 +656,44 @@ static struct platform_driver etnaviv_platform_driver = {
 	},
 };
 
-static struct platform_device *etnaviv_drm;
+static struct platform_device *etnaviv_platform_device;
 
-static int __init etnaviv_init(void)
+static int etnaviv_create_platform_device(const char *name,
+					  struct platform_device **ppdev)
 {
 	struct platform_device *pdev;
 	int ret;
+
+	pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
+	if (!pdev)
+		return -ENOMEM;
+
+	ret = platform_device_add(pdev);
+	if (ret) {
+		platform_device_put(pdev);
+		return ret;
+	}
+
+	*ppdev = pdev;
+
+	return 0;
+}
+
+static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
+{
+	struct platform_device *pdev = *ppdev;
+
+	if (!pdev)
+		return;
+
+	*ppdev = NULL;
+
+	platform_device_unregister(pdev);
+}
+
+static int __init etnaviv_init(void)
+{
+	int ret;
 	struct device_node *np;
 
 	etnaviv_validate_init();
@@ -682,22 +714,12 @@ static int __init etnaviv_init(void)
 		if (!of_device_is_available(np))
 			continue;
 
-		pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE);
-		if (!pdev) {
-			ret = -ENOMEM;
-			of_node_put(np);
-			goto unregister_platform_driver;
-		}
-
-		ret = platform_device_add(pdev);
-		if (ret) {
-			platform_device_put(pdev);
-			of_node_put(np);
+		ret = etnaviv_create_platform_device("etnaviv",
+						     &etnaviv_platform_device);
+		of_node_put(np);
+		if (ret)
 			goto unregister_platform_driver;
-		}
 
-		etnaviv_drm = pdev;
-		of_node_put(np);
 		break;
 	}
 
@@ -713,7 +735,7 @@ module_init(etnaviv_init);
 
 static void __exit etnaviv_exit(void)
 {
-	platform_device_unregister(etnaviv_drm);
+	etnaviv_destroy_platform_device(&etnaviv_platform_device);
 	platform_driver_unregister(&etnaviv_platform_driver);
 	platform_driver_unregister(&etnaviv_gpu_driver);
 }
-- 
2.25.1

[PATCH v6 3/6] drm/etnaviv: add dedicated functions to create and destroy platform devices

[Sui Jingfeng]

Also rename the virtual master platform device as etnaviv_platform_device,
for better reflection that it is a platform device, not a DRM device.

Another benefit is that we no longer need to call of_node_put() for three
different cases, Instead, we only need to call it once.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 56 +++++++++++++++++++--------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 31a7f59ccb49..0a9d90c18f2c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -656,12 +656,44 @@ static struct platform_driver etnaviv_platform_driver = {
 	},
 };
 
-static struct platform_device *etnaviv_drm;
+static struct platform_device *etnaviv_platform_device;
 
-static int __init etnaviv_init(void)
+static int etnaviv_create_platform_device(const char *name,
+					  struct platform_device **ppdev)
 {
 	struct platform_device *pdev;
 	int ret;
+
+	pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
+	if (!pdev)
+		return -ENOMEM;
+
+	ret = platform_device_add(pdev);
+	if (ret) {
+		platform_device_put(pdev);
+		return ret;
+	}
+
+	*ppdev = pdev;
+
+	return 0;
+}
+
+static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
+{
+	struct platform_device *pdev = *ppdev;
+
+	if (!pdev)
+		return;
+
+	*ppdev = NULL;
+
+	platform_device_unregister(pdev);
+}
+
+static int __init etnaviv_init(void)
+{
+	int ret;
 	struct device_node *np;
 
 	etnaviv_validate_init();
@@ -682,22 +714,12 @@ static int __init etnaviv_init(void)
 		if (!of_device_is_available(np))
 			continue;
 
-		pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE);
-		if (!pdev) {
-			ret = -ENOMEM;
-			of_node_put(np);
-			goto unregister_platform_driver;
-		}
-
-		ret = platform_device_add(pdev);
-		if (ret) {
-			platform_device_put(pdev);
-			of_node_put(np);
+		ret = etnaviv_create_platform_device("etnaviv",
+						     &etnaviv_platform_device);
+		of_node_put(np);
+		if (ret)
 			goto unregister_platform_driver;
-		}
 
-		etnaviv_drm = pdev;
-		of_node_put(np);
 		break;
 	}
 
@@ -713,7 +735,7 @@ module_init(etnaviv_init);
 
 static void __exit etnaviv_exit(void)
 {
-	platform_device_unregister(etnaviv_drm);
+	etnaviv_destroy_platform_device(&etnaviv_platform_device);
 	platform_driver_unregister(&etnaviv_platform_driver);
 	platform_driver_unregister(&etnaviv_gpu_driver);
 }
-- 
2.25.1

[PATCH v6 4/6] drm/etnaviv: add helpers for private data construction and destruction

[Sui Jingfeng]

struct etnaviv_drm_private contains a lot of common resources that are
shared by all GPUs. This patch introduces two dedicated functions, which
is for the construction and destruction of instances of this structure.
    
The idea is to avoid leaking its members outside. The error handling code
can also be simplified.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 71 +++++++++++++++++----------
 drivers/gpu/drm/etnaviv/etnaviv_drv.h |  4 ++
 2 files changed, 50 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 0a9d90c18f2c..56c98711f8e1 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -498,28 +498,17 @@ static const struct drm_driver etnaviv_drm_driver = {
 	.minor              = 3,
 };
 
-/*
- * Platform driver:
- */
-static int etnaviv_bind(struct device *dev)
+static int etnaviv_alloc_private(struct device *dev,
+				 struct etnaviv_drm_private **ppriv)
 {
 	struct etnaviv_drm_private *priv;
-	struct drm_device *drm;
 	int ret;
 
-	drm = drm_dev_alloc(&etnaviv_drm_driver, dev);
-	if (IS_ERR(drm))
-		return PTR_ERR(drm);
-
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		dev_err(dev, "failed to allocate private data\n");
-		ret = -ENOMEM;
-		goto out_put;
+		return -ENOMEM;
 	}
-	drm->dev_private = priv;
-
-	dma_set_max_seg_size(dev, SZ_2G);
 
 	xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
 
@@ -528,18 +517,55 @@ static int etnaviv_bind(struct device *dev)
 	priv->num_gpus = 0;
 	priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
 
-	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
+	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(dev);
 	if (IS_ERR(priv->cmdbuf_suballoc)) {
-		dev_err(drm->dev, "Failed to create cmdbuf suballocator\n");
+		dev_err(dev, "Failed to create cmdbuf suballocator\n");
 		ret = PTR_ERR(priv->cmdbuf_suballoc);
-		goto out_free_priv;
+		kfree(priv);
+		return ret;
 	}
 
+	*ppriv = priv;
+
+	return 0;
+}
+
+static void etnaviv_free_private(struct etnaviv_drm_private *priv)
+{
+	etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
+
+	xa_destroy(&priv->active_contexts);
+
+	kfree(priv);
+}
+
+/*
+ * Platform driver:
+ */
+static int etnaviv_bind(struct device *dev)
+{
+	struct etnaviv_drm_private *priv;
+	struct drm_device *drm;
+	int ret;
+
+	drm = drm_dev_alloc(&etnaviv_drm_driver, dev);
+	if (IS_ERR(drm))
+		return PTR_ERR(drm);
+
+	ret = etnaviv_alloc_private(dev, &priv);
+	if (ret)
+		goto out_put;
+
+	priv->drm = drm;
+	drm->dev_private = priv;
+
+	dma_set_max_seg_size(dev, SZ_2G);
+
 	dev_set_drvdata(dev, drm);
 
 	ret = component_bind_all(dev, drm);
 	if (ret < 0)
-		goto out_destroy_suballoc;
+		goto out_free_priv;
 
 	load_gpu(drm);
 
@@ -551,10 +577,8 @@ static int etnaviv_bind(struct device *dev)
 
 out_unbind:
 	component_unbind_all(dev, drm);
-out_destroy_suballoc:
-	etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
 out_free_priv:
-	kfree(priv);
+	etnaviv_free_private(priv);
 out_put:
 	drm_dev_put(drm);
 
@@ -570,12 +594,9 @@ static void etnaviv_unbind(struct device *dev)
 
 	component_unbind_all(dev, drm);
 
-	etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
-
-	xa_destroy(&priv->active_contexts);
+	etnaviv_free_private(priv);
 
 	drm->dev_private = NULL;
-	kfree(priv);
 
 	drm_dev_put(drm);
 }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index b3eb1662e90c..87fb52c03c5e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -35,6 +35,7 @@ struct etnaviv_file_private {
 };
 
 struct etnaviv_drm_private {
+	struct drm_device *drm;
 	int num_gpus;
 	struct etnaviv_gpu *gpu[ETNA_MAX_PIPES];
 	gfp_t shm_gfp_mask;
@@ -45,6 +46,9 @@ struct etnaviv_drm_private {
 	struct xarray active_contexts;
 	u32 next_context_id;
 
+	/* hint for platform support cached coherent caching mode */
+	bool has_cached_coherent;
+
 	/* list of GEM objects: */
 	struct mutex gem_lock;
 	struct list_head gem_list;
-- 
2.25.1

[PATCH v6 4/6] drm/etnaviv: add helpers for private data construction and destruction

[Sui Jingfeng]

struct etnaviv_drm_private contains a lot of common resources that are
shared by all GPUs. This patch introduces two dedicated functions, which
is for the construction and destruction of instances of this structure.
    
The idea is to avoid leaking its members outside. The error handling code
can also be simplified.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 71 +++++++++++++++++----------
 drivers/gpu/drm/etnaviv/etnaviv_drv.h |  4 ++
 2 files changed, 50 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 0a9d90c18f2c..56c98711f8e1 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -498,28 +498,17 @@ static const struct drm_driver etnaviv_drm_driver = {
 	.minor              = 3,
 };
 
-/*
- * Platform driver:
- */
-static int etnaviv_bind(struct device *dev)
+static int etnaviv_alloc_private(struct device *dev,
+				 struct etnaviv_drm_private **ppriv)
 {
 	struct etnaviv_drm_private *priv;
-	struct drm_device *drm;
 	int ret;
 
-	drm = drm_dev_alloc(&etnaviv_drm_driver, dev);
-	if (IS_ERR(drm))
-		return PTR_ERR(drm);
-
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		dev_err(dev, "failed to allocate private data\n");
-		ret = -ENOMEM;
-		goto out_put;
+		return -ENOMEM;
 	}
-	drm->dev_private = priv;
-
-	dma_set_max_seg_size(dev, SZ_2G);
 
 	xa_init_flags(&priv->active_contexts, XA_FLAGS_ALLOC);
 
@@ -528,18 +517,55 @@ static int etnaviv_bind(struct device *dev)
 	priv->num_gpus = 0;
 	priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
 
-	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
+	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(dev);
 	if (IS_ERR(priv->cmdbuf_suballoc)) {
-		dev_err(drm->dev, "Failed to create cmdbuf suballocator\n");
+		dev_err(dev, "Failed to create cmdbuf suballocator\n");
 		ret = PTR_ERR(priv->cmdbuf_suballoc);
-		goto out_free_priv;
+		kfree(priv);
+		return ret;
 	}
 
+	*ppriv = priv;
+
+	return 0;
+}
+
+static void etnaviv_free_private(struct etnaviv_drm_private *priv)
+{
+	etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
+
+	xa_destroy(&priv->active_contexts);
+
+	kfree(priv);
+}
+
+/*
+ * Platform driver:
+ */
+static int etnaviv_bind(struct device *dev)
+{
+	struct etnaviv_drm_private *priv;
+	struct drm_device *drm;
+	int ret;
+
+	drm = drm_dev_alloc(&etnaviv_drm_driver, dev);
+	if (IS_ERR(drm))
+		return PTR_ERR(drm);
+
+	ret = etnaviv_alloc_private(dev, &priv);
+	if (ret)
+		goto out_put;
+
+	priv->drm = drm;
+	drm->dev_private = priv;
+
+	dma_set_max_seg_size(dev, SZ_2G);
+
 	dev_set_drvdata(dev, drm);
 
 	ret = component_bind_all(dev, drm);
 	if (ret < 0)
-		goto out_destroy_suballoc;
+		goto out_free_priv;
 
 	load_gpu(drm);
 
@@ -551,10 +577,8 @@ static int etnaviv_bind(struct device *dev)
 
 out_unbind:
 	component_unbind_all(dev, drm);
-out_destroy_suballoc:
-	etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
 out_free_priv:
-	kfree(priv);
+	etnaviv_free_private(priv);
 out_put:
 	drm_dev_put(drm);
 
@@ -570,12 +594,9 @@ static void etnaviv_unbind(struct device *dev)
 
 	component_unbind_all(dev, drm);
 
-	etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
-
-	xa_destroy(&priv->active_contexts);
+	etnaviv_free_private(priv);
 
 	drm->dev_private = NULL;
-	kfree(priv);
 
 	drm_dev_put(drm);
 }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index b3eb1662e90c..87fb52c03c5e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -35,6 +35,7 @@ struct etnaviv_file_private {
 };
 
 struct etnaviv_drm_private {
+	struct drm_device *drm;
 	int num_gpus;
 	struct etnaviv_gpu *gpu[ETNA_MAX_PIPES];
 	gfp_t shm_gfp_mask;
@@ -45,6 +46,9 @@ struct etnaviv_drm_private {
 	struct xarray active_contexts;
 	u32 next_context_id;
 
+	/* hint for platform support cached coherent caching mode */
+	bool has_cached_coherent;
+
 	/* list of GEM objects: */
 	struct mutex gem_lock;
 	struct list_head gem_list;
-- 
2.25.1

[PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

This patch adds PCI driver support on top of what already have. Take the
GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
Therefore, component frameworks can be avoided. Because we want to bind the
DRM driver service to the PCI driver manually.
    
We avoid using the component framework because the virtual master device
will not be used without a force override. X server and Mesa will try to
find the PCI device to use by default. Creating a virtual master device
for PCI GPUs cause unnecessary troubles.
    
Using the component framework with a PCI device is still possible; it is
just that the solo PCI device should be the master. A platform with a
single GPU core could also try the non-component code path.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/Kconfig           |  9 +++
 drivers/gpu/drm/etnaviv/Makefile          |  2 +
 drivers/gpu/drm/etnaviv/etnaviv_drv.c     | 69 +++++++++++++---
 drivers/gpu/drm/etnaviv/etnaviv_drv.h     |  3 +
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c     | 97 ++++++++++++++++-------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h     | 12 +++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 87 ++++++++++++++++++++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h | 12 +++
 8 files changed, 250 insertions(+), 41 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..dbf948f99976 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -15,6 +15,15 @@ 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
+	help
+	  Compile in support for PCI GPUs of Vivante.
+	  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 56c98711f8e1..052f745cecc0 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"
 
 /*
@@ -525,6 +526,16 @@ static int etnaviv_alloc_private(struct device *dev,
 		return ret;
 	}
 
+	/*
+	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
+	 * maintain cache coherency by hardware
+	 */
+	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
+		priv->has_cached_coherent = true;
+
+	dev_info(dev, "Cached coherent mode is %s\n",
+		 priv->has_cached_coherent ? "support" : "not support");
+
 	*ppriv = priv;
 
 	return 0;
@@ -539,10 +550,9 @@ static void etnaviv_free_private(struct etnaviv_drm_private *priv)
 	kfree(priv);
 }
 
-/*
- * Platform driver:
- */
-static int etnaviv_bind(struct device *dev)
+static struct etnaviv_drm_private *etna_private_s;
+
+int etnaviv_drm_bind(struct device *dev, bool component)
 {
 	struct etnaviv_drm_private *priv;
 	struct drm_device *drm;
@@ -558,12 +568,15 @@ static int etnaviv_bind(struct device *dev)
 
 	priv->drm = drm;
 	drm->dev_private = priv;
+	etna_private_s = priv;
 
 	dma_set_max_seg_size(dev, SZ_2G);
 
-	dev_set_drvdata(dev, drm);
+	if (component)
+		ret = component_bind_all(dev, drm);
+	else
+		ret = etnaviv_gpu_bind(dev, NULL, drm);
 
-	ret = component_bind_all(dev, drm);
 	if (ret < 0)
 		goto out_free_priv;
 
@@ -585,14 +598,17 @@ static int etnaviv_bind(struct device *dev)
 	return ret;
 }
 
-static void etnaviv_unbind(struct device *dev)
+void etnaviv_drm_unbind(struct device *dev, bool component)
 {
-	struct drm_device *drm = dev_get_drvdata(dev);
-	struct etnaviv_drm_private *priv = drm->dev_private;
+	struct etnaviv_drm_private *priv = etna_private_s;
+	struct drm_device *drm = priv->drm;
 
 	drm_dev_unregister(drm);
 
-	component_unbind_all(dev, drm);
+	if (component)
+		component_unbind_all(dev, drm);
+	else
+		etnaviv_gpu_unbind(dev, NULL, drm);
 
 	etnaviv_free_private(priv);
 
@@ -601,9 +617,22 @@ static void etnaviv_unbind(struct device *dev)
 	drm_dev_put(drm);
 }
 
+/*
+ * Platform driver:
+ */
+static int etnaviv_master_bind(struct device *dev)
+{
+	return etnaviv_drm_bind(dev, true);
+}
+
+static void etnaviv_master_unbind(struct device *dev)
+{
+	return etnaviv_drm_unbind(dev, true);
+}
+
 static const struct component_master_ops etnaviv_master_ops = {
-	.bind = etnaviv_bind,
-	.unbind = etnaviv_unbind,
+	.bind = etnaviv_master_bind,
+	.unbind = etnaviv_master_unbind,
 };
 
 static int etnaviv_pdev_probe(struct platform_device *pdev)
@@ -727,6 +756,12 @@ static int __init etnaviv_init(void)
 	if (ret != 0)
 		goto unregister_gpu_driver;
 
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+	ret = pci_register_driver(&etnaviv_pci_driver);
+#endif
+	if (ret != 0)
+		goto unregister_platform_driver;
+
 	/*
 	 * If the DT contains at least one available GPU device, instantiate
 	 * the DRM platform device.
@@ -739,13 +774,17 @@ static int __init etnaviv_init(void)
 						     &etnaviv_platform_device);
 		of_node_put(np);
 		if (ret)
-			goto unregister_platform_driver;
+			goto unregister_all_driver;
 
 		break;
 	}
 
 	return 0;
 
+unregister_all_driver:
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+	pci_unregister_driver(&etnaviv_pci_driver);
+#endif
 unregister_platform_driver:
 	platform_driver_unregister(&etnaviv_platform_driver);
 unregister_gpu_driver:
@@ -759,6 +798,10 @@ 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);
+
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+	pci_unregister_driver(&etnaviv_pci_driver);
+#endif
 }
 module_exit(etnaviv_exit);
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index 87fb52c03c5e..934fc3744389 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -86,6 +86,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 4937580551a5..700f2414b87d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1565,10 +1565,11 @@ static irqreturn_t irq_handler(int irq, void *data)
 	return ret;
 }
 
-static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
+static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu, bool no_clk)
 {
 	struct device *dev = gpu->dev;
 
+	gpu->no_clk = no_clk;
 	if (gpu->no_clk)
 		return 0;
 
@@ -1746,8 +1747,7 @@ static const struct thermal_cooling_device_ops cooling_ops = {
 	.set_cur_state = etnaviv_gpu_cooling_set_cur_state,
 };
 
-static int etnaviv_gpu_bind(struct device *dev, struct device *master,
-	void *data)
+int etnaviv_gpu_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
 	struct etnaviv_drm_private *priv = drm->dev_private;
@@ -1778,7 +1778,6 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 	if (ret < 0)
 		goto out_sched;
 
-
 	gpu->drm = drm;
 	gpu->fence_context = dma_fence_context_alloc(1);
 	xa_init_flags(&gpu->user_fences, XA_FLAGS_ALLOC);
@@ -1807,8 +1806,7 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 	return ret;
 }
 
-static void etnaviv_gpu_unbind(struct device *dev, struct device *master,
-	void *data)
+void etnaviv_gpu_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct etnaviv_gpu *gpu = dev_get_drvdata(dev);
 
@@ -1878,9 +1876,37 @@ static int etnaviv_gpu_register_irq(struct etnaviv_gpu *gpu, int irq)
 	return 0;
 }
 
-static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
+static int etnaviv_gpu_plat_drv_init(struct etnaviv_gpu *gpu, bool component)
+{
+	struct device *dev = gpu->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int err;
+
+	/* Map registers: */
+	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gpu->mmio))
+		return PTR_ERR(gpu->mmio);
+
+	if (component) {
+		err = component_add(dev, &gpu_ops);
+		if (err < 0) {
+			dev_err(dev, "failed to register component: %d\n", err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static void etnaviv_gpu_plat_drv_fini(struct etnaviv_gpu *gpu, bool component)
+{
+	if (component)
+		component_del(gpu->dev, &gpu_ops);
+}
+
+int etnaviv_gpu_driver_create(struct device *dev, int irq, bool component,
+			      bool no_clk, pfn_gpu_init_t gpu_post_init)
 {
-	struct device *dev = &pdev->dev;
 	struct etnaviv_gpu *gpu;
 	int err;
 
@@ -1888,22 +1914,16 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 	if (!gpu)
 		return -ENOMEM;
 
-	gpu->dev = &pdev->dev;
+	gpu->dev = dev;
 	mutex_init(&gpu->lock);
 	mutex_init(&gpu->sched_lock);
 
-	/* Map registers: */
-	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(gpu->mmio))
-		return PTR_ERR(gpu->mmio);
-
 	/* Get Interrupt: */
-	err = etnaviv_gpu_register_irq(gpu,  platform_get_irq(pdev, 0));
+	err = etnaviv_gpu_register_irq(gpu, irq);
 	if (err)
 		return err;
 
-	/* Get Clocks: */
-	err = etnaviv_gpu_clk_get(gpu);
+	err = etnaviv_gpu_clk_get(gpu, no_clk);
 	if (err)
 		return err;
 
@@ -1915,23 +1935,44 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 	 * autosuspend delay is rather arbitary: no measurements have
 	 * yet been performed to determine an appropriate value.
 	 */
-	pm_runtime_use_autosuspend(gpu->dev);
-	pm_runtime_set_autosuspend_delay(gpu->dev, 200);
-	pm_runtime_enable(gpu->dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 200);
+	pm_runtime_enable(dev);
 
-	err = component_add(&pdev->dev, &gpu_ops);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to register component: %d\n", err);
-		return err;
-	}
+	gpu_post_init(gpu, component);
 
 	return 0;
 }
 
+void etnaviv_gpu_driver_destroy(struct device *dev, bool component,
+				pfn_gpu_fini_t gpu_early_fini)
+{
+	struct etnaviv_gpu *gpu = dev_get_drvdata(dev);
+
+	if (!gpu) {
+		dev_err(dev, "device not initialized properly\n");
+		return;
+	}
+
+	gpu_early_fini(gpu, component);
+
+	pm_runtime_disable(dev);
+}
+
+static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int irq = platform_get_irq(pdev, 0);
+
+	return etnaviv_gpu_driver_create(dev, irq, true, false, etnaviv_gpu_plat_drv_init);
+}
+
 static int etnaviv_gpu_platform_remove(struct platform_device *pdev)
 {
-	component_del(&pdev->dev, &gpu_ops);
-	pm_runtime_disable(&pdev->dev);
+	struct device *dev = &pdev->dev;
+
+	etnaviv_gpu_driver_destroy(dev, true, etnaviv_gpu_plat_drv_fini);
+
 	return 0;
 }
 
@@ -1978,7 +2019,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 6da5209a7d64..cfcdc5dde9d9 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -207,6 +207,18 @@ void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu);
 int etnaviv_gpu_wait_idle(struct etnaviv_gpu *gpu, unsigned int timeout_ms);
 void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch);
 
+typedef int (*pfn_gpu_init_t)(struct etnaviv_gpu *gpu, bool component);
+typedef void (*pfn_gpu_fini_t)(struct etnaviv_gpu *gpu, bool component);
+
+int etnaviv_gpu_driver_create(struct device *dev, int irq, bool component,
+			      bool no_clk, pfn_gpu_init_t post_init);
+void etnaviv_gpu_driver_destroy(struct device *dev, bool component,
+				pfn_gpu_fini_t early_fini);
+
+int etnaviv_gpu_bind(struct device *dev, struct device *master, void *data);
+void etnaviv_gpu_unbind(struct device *dev, struct device *master, void *data);
+
 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..d0bb6615c434
--- /dev/null
+++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/pci.h>
+
+#include "etnaviv_drv.h"
+#include "etnaviv_gpu.h"
+#include "etnaviv_pci_drv.h"
+
+enum etnaviv_pci_gpu_family {
+	GC1000_IN_LS7A1000 = 0,
+	GC1000_IN_LS2K1000 = 1,
+};
+
+static int etnaviv_gpu_pci_init(struct etnaviv_gpu *gpu, bool component)
+{
+	struct pci_dev *pdev = to_pci_dev(gpu->dev);
+
+	/* Map registers, assume the PCI bar 0 contain the registers */
+	gpu->mmio = pcim_iomap(pdev, 0, 0);
+	if (IS_ERR(gpu->mmio))
+		return PTR_ERR(gpu->mmio);
+
+	gpu->no_clk = true;
+
+	return 0;
+}
+
+static void etnaviv_gpu_pci_fini(struct etnaviv_gpu *gpu, bool component)
+{
+	struct pci_dev *pdev = to_pci_dev(gpu->dev);
+
+	pci_clear_master(pdev);
+
+	dev_dbg(gpu->dev, "component %s\n", component ? "enabled" : "disabled");
+}
+
+static int etnaviv_pci_probe(struct pci_dev *pdev,
+			     const struct pci_device_id *ent)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = pcim_enable_device(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable\n");
+		return ret;
+	}
+
+	pci_set_master(pdev);
+
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret)
+		return ret;
+
+	/* Create a GPU driver instance for the PCI device itself */
+	ret = etnaviv_gpu_driver_create(dev, pdev->irq, false, true,
+					etnaviv_gpu_pci_init);
+	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, etnaviv_gpu_pci_fini);
+}
+
+static const struct pci_device_id etnaviv_pci_id_lists[] = {
+	{0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
+	{0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
+	{0, 0, 0, 0, 0, 0, 0}
+};
+
+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),
+};
+
+MODULE_DEVICE_TABLE(pci, etnaviv_pci_id_lists);
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..e4f8183c5558
--- /dev/null
+++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ETNAVIV_PCI_DRV_H__
+#define __ETNAVIV_PCI_DRV_H__
+
+#include <linux/pci.h>
+
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+extern struct pci_driver etnaviv_pci_driver;
+#endif
+
+#endif
-- 
2.25.1

[PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

This patch adds PCI driver support on top of what already have. Take the
GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
Therefore, component frameworks can be avoided. Because we want to bind the
DRM driver service to the PCI driver manually.
    
We avoid using the component framework because the virtual master device
will not be used without a force override. X server and Mesa will try to
find the PCI device to use by default. Creating a virtual master device
for PCI GPUs cause unnecessary troubles.
    
Using the component framework with a PCI device is still possible; it is
just that the solo PCI device should be the master. A platform with a
single GPU core could also try the non-component code path.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/Kconfig           |  9 +++
 drivers/gpu/drm/etnaviv/Makefile          |  2 +
 drivers/gpu/drm/etnaviv/etnaviv_drv.c     | 69 +++++++++++++---
 drivers/gpu/drm/etnaviv/etnaviv_drv.h     |  3 +
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c     | 97 ++++++++++++++++-------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h     | 12 +++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 87 ++++++++++++++++++++
 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h | 12 +++
 8 files changed, 250 insertions(+), 41 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..dbf948f99976 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -15,6 +15,15 @@ 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
+	help
+	  Compile in support for PCI GPUs of Vivante.
+	  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 56c98711f8e1..052f745cecc0 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"
 
 /*
@@ -525,6 +526,16 @@ static int etnaviv_alloc_private(struct device *dev,
 		return ret;
 	}
 
+	/*
+	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
+	 * maintain cache coherency by hardware
+	 */
+	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
+		priv->has_cached_coherent = true;
+
+	dev_info(dev, "Cached coherent mode is %s\n",
+		 priv->has_cached_coherent ? "support" : "not support");
+
 	*ppriv = priv;
 
 	return 0;
@@ -539,10 +550,9 @@ static void etnaviv_free_private(struct etnaviv_drm_private *priv)
 	kfree(priv);
 }
 
-/*
- * Platform driver:
- */
-static int etnaviv_bind(struct device *dev)
+static struct etnaviv_drm_private *etna_private_s;
+
+int etnaviv_drm_bind(struct device *dev, bool component)
 {
 	struct etnaviv_drm_private *priv;
 	struct drm_device *drm;
@@ -558,12 +568,15 @@ static int etnaviv_bind(struct device *dev)
 
 	priv->drm = drm;
 	drm->dev_private = priv;
+	etna_private_s = priv;
 
 	dma_set_max_seg_size(dev, SZ_2G);
 
-	dev_set_drvdata(dev, drm);
+	if (component)
+		ret = component_bind_all(dev, drm);
+	else
+		ret = etnaviv_gpu_bind(dev, NULL, drm);
 
-	ret = component_bind_all(dev, drm);
 	if (ret < 0)
 		goto out_free_priv;
 
@@ -585,14 +598,17 @@ static int etnaviv_bind(struct device *dev)
 	return ret;
 }
 
-static void etnaviv_unbind(struct device *dev)
+void etnaviv_drm_unbind(struct device *dev, bool component)
 {
-	struct drm_device *drm = dev_get_drvdata(dev);
-	struct etnaviv_drm_private *priv = drm->dev_private;
+	struct etnaviv_drm_private *priv = etna_private_s;
+	struct drm_device *drm = priv->drm;
 
 	drm_dev_unregister(drm);
 
-	component_unbind_all(dev, drm);
+	if (component)
+		component_unbind_all(dev, drm);
+	else
+		etnaviv_gpu_unbind(dev, NULL, drm);
 
 	etnaviv_free_private(priv);
 
@@ -601,9 +617,22 @@ static void etnaviv_unbind(struct device *dev)
 	drm_dev_put(drm);
 }
 
+/*
+ * Platform driver:
+ */
+static int etnaviv_master_bind(struct device *dev)
+{
+	return etnaviv_drm_bind(dev, true);
+}
+
+static void etnaviv_master_unbind(struct device *dev)
+{
+	return etnaviv_drm_unbind(dev, true);
+}
+
 static const struct component_master_ops etnaviv_master_ops = {
-	.bind = etnaviv_bind,
-	.unbind = etnaviv_unbind,
+	.bind = etnaviv_master_bind,
+	.unbind = etnaviv_master_unbind,
 };
 
 static int etnaviv_pdev_probe(struct platform_device *pdev)
@@ -727,6 +756,12 @@ static int __init etnaviv_init(void)
 	if (ret != 0)
 		goto unregister_gpu_driver;
 
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+	ret = pci_register_driver(&etnaviv_pci_driver);
+#endif
+	if (ret != 0)
+		goto unregister_platform_driver;
+
 	/*
 	 * If the DT contains at least one available GPU device, instantiate
 	 * the DRM platform device.
@@ -739,13 +774,17 @@ static int __init etnaviv_init(void)
 						     &etnaviv_platform_device);
 		of_node_put(np);
 		if (ret)
-			goto unregister_platform_driver;
+			goto unregister_all_driver;
 
 		break;
 	}
 
 	return 0;
 
+unregister_all_driver:
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+	pci_unregister_driver(&etnaviv_pci_driver);
+#endif
 unregister_platform_driver:
 	platform_driver_unregister(&etnaviv_platform_driver);
 unregister_gpu_driver:
@@ -759,6 +798,10 @@ 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);
+
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+	pci_unregister_driver(&etnaviv_pci_driver);
+#endif
 }
 module_exit(etnaviv_exit);
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index 87fb52c03c5e..934fc3744389 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -86,6 +86,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 4937580551a5..700f2414b87d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1565,10 +1565,11 @@ static irqreturn_t irq_handler(int irq, void *data)
 	return ret;
 }
 
-static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
+static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu, bool no_clk)
 {
 	struct device *dev = gpu->dev;
 
+	gpu->no_clk = no_clk;
 	if (gpu->no_clk)
 		return 0;
 
@@ -1746,8 +1747,7 @@ static const struct thermal_cooling_device_ops cooling_ops = {
 	.set_cur_state = etnaviv_gpu_cooling_set_cur_state,
 };
 
-static int etnaviv_gpu_bind(struct device *dev, struct device *master,
-	void *data)
+int etnaviv_gpu_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
 	struct etnaviv_drm_private *priv = drm->dev_private;
@@ -1778,7 +1778,6 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 	if (ret < 0)
 		goto out_sched;
 
-
 	gpu->drm = drm;
 	gpu->fence_context = dma_fence_context_alloc(1);
 	xa_init_flags(&gpu->user_fences, XA_FLAGS_ALLOC);
@@ -1807,8 +1806,7 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 	return ret;
 }
 
-static void etnaviv_gpu_unbind(struct device *dev, struct device *master,
-	void *data)
+void etnaviv_gpu_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct etnaviv_gpu *gpu = dev_get_drvdata(dev);
 
@@ -1878,9 +1876,37 @@ static int etnaviv_gpu_register_irq(struct etnaviv_gpu *gpu, int irq)
 	return 0;
 }
 
-static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
+static int etnaviv_gpu_plat_drv_init(struct etnaviv_gpu *gpu, bool component)
+{
+	struct device *dev = gpu->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int err;
+
+	/* Map registers: */
+	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gpu->mmio))
+		return PTR_ERR(gpu->mmio);
+
+	if (component) {
+		err = component_add(dev, &gpu_ops);
+		if (err < 0) {
+			dev_err(dev, "failed to register component: %d\n", err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static void etnaviv_gpu_plat_drv_fini(struct etnaviv_gpu *gpu, bool component)
+{
+	if (component)
+		component_del(gpu->dev, &gpu_ops);
+}
+
+int etnaviv_gpu_driver_create(struct device *dev, int irq, bool component,
+			      bool no_clk, pfn_gpu_init_t gpu_post_init)
 {
-	struct device *dev = &pdev->dev;
 	struct etnaviv_gpu *gpu;
 	int err;
 
@@ -1888,22 +1914,16 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 	if (!gpu)
 		return -ENOMEM;
 
-	gpu->dev = &pdev->dev;
+	gpu->dev = dev;
 	mutex_init(&gpu->lock);
 	mutex_init(&gpu->sched_lock);
 
-	/* Map registers: */
-	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(gpu->mmio))
-		return PTR_ERR(gpu->mmio);
-
 	/* Get Interrupt: */
-	err = etnaviv_gpu_register_irq(gpu,  platform_get_irq(pdev, 0));
+	err = etnaviv_gpu_register_irq(gpu, irq);
 	if (err)
 		return err;
 
-	/* Get Clocks: */
-	err = etnaviv_gpu_clk_get(gpu);
+	err = etnaviv_gpu_clk_get(gpu, no_clk);
 	if (err)
 		return err;
 
@@ -1915,23 +1935,44 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
 	 * autosuspend delay is rather arbitary: no measurements have
 	 * yet been performed to determine an appropriate value.
 	 */
-	pm_runtime_use_autosuspend(gpu->dev);
-	pm_runtime_set_autosuspend_delay(gpu->dev, 200);
-	pm_runtime_enable(gpu->dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 200);
+	pm_runtime_enable(dev);
 
-	err = component_add(&pdev->dev, &gpu_ops);
-	if (err < 0) {
-		dev_err(&pdev->dev, "failed to register component: %d\n", err);
-		return err;
-	}
+	gpu_post_init(gpu, component);
 
 	return 0;
 }
 
+void etnaviv_gpu_driver_destroy(struct device *dev, bool component,
+				pfn_gpu_fini_t gpu_early_fini)
+{
+	struct etnaviv_gpu *gpu = dev_get_drvdata(dev);
+
+	if (!gpu) {
+		dev_err(dev, "device not initialized properly\n");
+		return;
+	}
+
+	gpu_early_fini(gpu, component);
+
+	pm_runtime_disable(dev);
+}
+
+static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int irq = platform_get_irq(pdev, 0);
+
+	return etnaviv_gpu_driver_create(dev, irq, true, false, etnaviv_gpu_plat_drv_init);
+}
+
 static int etnaviv_gpu_platform_remove(struct platform_device *pdev)
 {
-	component_del(&pdev->dev, &gpu_ops);
-	pm_runtime_disable(&pdev->dev);
+	struct device *dev = &pdev->dev;
+
+	etnaviv_gpu_driver_destroy(dev, true, etnaviv_gpu_plat_drv_fini);
+
 	return 0;
 }
 
@@ -1978,7 +2019,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 6da5209a7d64..cfcdc5dde9d9 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -207,6 +207,18 @@ void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu);
 int etnaviv_gpu_wait_idle(struct etnaviv_gpu *gpu, unsigned int timeout_ms);
 void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch);
 
+typedef int (*pfn_gpu_init_t)(struct etnaviv_gpu *gpu, bool component);
+typedef void (*pfn_gpu_fini_t)(struct etnaviv_gpu *gpu, bool component);
+
+int etnaviv_gpu_driver_create(struct device *dev, int irq, bool component,
+			      bool no_clk, pfn_gpu_init_t post_init);
+void etnaviv_gpu_driver_destroy(struct device *dev, bool component,
+				pfn_gpu_fini_t early_fini);
+
+int etnaviv_gpu_bind(struct device *dev, struct device *master, void *data);
+void etnaviv_gpu_unbind(struct device *dev, struct device *master, void *data);
+
 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..d0bb6615c434
--- /dev/null
+++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/pci.h>
+
+#include "etnaviv_drv.h"
+#include "etnaviv_gpu.h"
+#include "etnaviv_pci_drv.h"
+
+enum etnaviv_pci_gpu_family {
+	GC1000_IN_LS7A1000 = 0,
+	GC1000_IN_LS2K1000 = 1,
+};
+
+static int etnaviv_gpu_pci_init(struct etnaviv_gpu *gpu, bool component)
+{
+	struct pci_dev *pdev = to_pci_dev(gpu->dev);
+
+	/* Map registers, assume the PCI bar 0 contain the registers */
+	gpu->mmio = pcim_iomap(pdev, 0, 0);
+	if (IS_ERR(gpu->mmio))
+		return PTR_ERR(gpu->mmio);
+
+	gpu->no_clk = true;
+
+	return 0;
+}
+
+static void etnaviv_gpu_pci_fini(struct etnaviv_gpu *gpu, bool component)
+{
+	struct pci_dev *pdev = to_pci_dev(gpu->dev);
+
+	pci_clear_master(pdev);
+
+	dev_dbg(gpu->dev, "component %s\n", component ? "enabled" : "disabled");
+}
+
+static int etnaviv_pci_probe(struct pci_dev *pdev,
+			     const struct pci_device_id *ent)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = pcim_enable_device(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable\n");
+		return ret;
+	}
+
+	pci_set_master(pdev);
+
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret)
+		return ret;
+
+	/* Create a GPU driver instance for the PCI device itself */
+	ret = etnaviv_gpu_driver_create(dev, pdev->irq, false, true,
+					etnaviv_gpu_pci_init);
+	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, etnaviv_gpu_pci_fini);
+}
+
+static const struct pci_device_id etnaviv_pci_id_lists[] = {
+	{0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
+	{0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
+	{0, 0, 0, 0, 0, 0, 0}
+};
+
+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),
+};
+
+MODULE_DEVICE_TABLE(pci, etnaviv_pci_id_lists);
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..e4f8183c5558
--- /dev/null
+++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ETNAVIV_PCI_DRV_H__
+#define __ETNAVIV_PCI_DRV_H__
+
+#include <linux/pci.h>
+
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+extern struct pci_driver etnaviv_pci_driver;
+#endif
+
+#endif
-- 
2.25.1

[PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Sui Jingfeng]

cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
snoop the CPU's cache. write-combine caching property is not suitiable for
us.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c       |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 22 +++++++++++++++++++--
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  9 ++++++++-
 include/uapi/drm/etnaviv_drm.h              | 11 ++++++-----
 4 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 052f745cecc0..2816c654c023 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -274,7 +274,7 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data,
 	struct drm_etnaviv_gem_new *args = data;
 
 	if (args->flags & ~(ETNA_BO_CACHED | ETNA_BO_WC | ETNA_BO_UNCACHED |
-			    ETNA_BO_FORCE_MMU))
+			    ETNA_BO_CACHED_COHERENT | ETNA_BO_FORCE_MMU))
 		return -EINVAL;
 
 	return etnaviv_gem_new_handle(dev, file, args->size,
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index b5f73502e3dd..d8b559bd33d3 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
 static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
 {
 	struct page **pages;
+	pgprot_t prot;
 
 	lockdep_assert_held(&obj->lock);
 
@@ -350,8 +351,20 @@ static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
 	if (IS_ERR(pages))
 		return NULL;
 
-	return vmap(pages, obj->base.size >> PAGE_SHIFT,
-			VM_MAP, pgprot_writecombine(PAGE_KERNEL));
+	switch (obj->flags) {
+	case ETNA_BO_CACHED_COHERENT:
+	case ETNA_BO_CACHED:
+		prot = PAGE_KERNEL;
+		break;
+	case ETNA_BO_UNCACHED:
+		prot = pgprot_noncached(PAGE_KERNEL);
+		break;
+	case ETNA_BO_WC:
+	default:
+		prot = pgprot_writecombine(PAGE_KERNEL);
+	}
+
+	return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
 }
 
 static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
@@ -545,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
 static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
 	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
 {
+	struct etnaviv_drm_private *priv = dev->dev_private;
 	struct etnaviv_gem_object *etnaviv_obj;
 	unsigned sz = sizeof(*etnaviv_obj);
 	bool valid = true;
@@ -555,6 +569,10 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
 	case ETNA_BO_CACHED:
 	case ETNA_BO_WC:
 		break;
+	case ETNA_BO_CACHED_COHERENT:
+		if (priv->has_cached_coherent)
+			break;
+		fallthrough;
 	default:
 		valid = false;
 	}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index 3524b5811682..671d91d8f1c6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -112,11 +112,18 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
 struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
 	struct dma_buf_attachment *attach, struct sg_table *sgt)
 {
+	struct etnaviv_drm_private *priv = dev->dev_private;
 	struct etnaviv_gem_object *etnaviv_obj;
 	size_t size = PAGE_ALIGN(attach->dmabuf->size);
+	u32 cache_flags;
 	int ret, npages;
 
-	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
+	if (priv->has_cached_coherent)
+		cache_flags = ETNA_BO_CACHED_COHERENT;
+	else
+		cache_flags = ETNA_BO_WC;
+
+	ret = etnaviv_gem_new_private(dev, size, cache_flags,
 				      &etnaviv_gem_prime_ops, &etnaviv_obj);
 	if (ret < 0)
 		return ERR_PTR(ret);
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index af024d90453d..474b0db286de 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -90,13 +90,14 @@ struct drm_etnaviv_param {
  * GEM buffers:
  */
 
-#define ETNA_BO_CACHE_MASK   0x000f0000
+#define ETNA_BO_CACHE_MASK              0x000f0000
 /* cache modes */
-#define ETNA_BO_CACHED       0x00010000
-#define ETNA_BO_WC           0x00020000
-#define ETNA_BO_UNCACHED     0x00040000
+#define ETNA_BO_CACHED                  0x00010000
+#define ETNA_BO_WC                      0x00020000
+#define ETNA_BO_UNCACHED                0x00040000
+#define ETNA_BO_CACHED_COHERENT         0x00080000
 /* map flags */
-#define ETNA_BO_FORCE_MMU    0x00100000
+#define ETNA_BO_FORCE_MMU               0x00100000
 
 struct drm_etnaviv_gem_new {
 	__u64 size;           /* in */
-- 
2.25.1

[PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Sui Jingfeng]

cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
snoop the CPU's cache. write-combine caching property is not suitiable for
us.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c       |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 22 +++++++++++++++++++--
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  9 ++++++++-
 include/uapi/drm/etnaviv_drm.h              | 11 ++++++-----
 4 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 052f745cecc0..2816c654c023 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -274,7 +274,7 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data,
 	struct drm_etnaviv_gem_new *args = data;
 
 	if (args->flags & ~(ETNA_BO_CACHED | ETNA_BO_WC | ETNA_BO_UNCACHED |
-			    ETNA_BO_FORCE_MMU))
+			    ETNA_BO_CACHED_COHERENT | ETNA_BO_FORCE_MMU))
 		return -EINVAL;
 
 	return etnaviv_gem_new_handle(dev, file, args->size,
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index b5f73502e3dd..d8b559bd33d3 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
 static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
 {
 	struct page **pages;
+	pgprot_t prot;
 
 	lockdep_assert_held(&obj->lock);
 
@@ -350,8 +351,20 @@ static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
 	if (IS_ERR(pages))
 		return NULL;
 
-	return vmap(pages, obj->base.size >> PAGE_SHIFT,
-			VM_MAP, pgprot_writecombine(PAGE_KERNEL));
+	switch (obj->flags) {
+	case ETNA_BO_CACHED_COHERENT:
+	case ETNA_BO_CACHED:
+		prot = PAGE_KERNEL;
+		break;
+	case ETNA_BO_UNCACHED:
+		prot = pgprot_noncached(PAGE_KERNEL);
+		break;
+	case ETNA_BO_WC:
+	default:
+		prot = pgprot_writecombine(PAGE_KERNEL);
+	}
+
+	return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
 }
 
 static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
@@ -545,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
 static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
 	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
 {
+	struct etnaviv_drm_private *priv = dev->dev_private;
 	struct etnaviv_gem_object *etnaviv_obj;
 	unsigned sz = sizeof(*etnaviv_obj);
 	bool valid = true;
@@ -555,6 +569,10 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
 	case ETNA_BO_CACHED:
 	case ETNA_BO_WC:
 		break;
+	case ETNA_BO_CACHED_COHERENT:
+		if (priv->has_cached_coherent)
+			break;
+		fallthrough;
 	default:
 		valid = false;
 	}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index 3524b5811682..671d91d8f1c6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -112,11 +112,18 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
 struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
 	struct dma_buf_attachment *attach, struct sg_table *sgt)
 {
+	struct etnaviv_drm_private *priv = dev->dev_private;
 	struct etnaviv_gem_object *etnaviv_obj;
 	size_t size = PAGE_ALIGN(attach->dmabuf->size);
+	u32 cache_flags;
 	int ret, npages;
 
-	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
+	if (priv->has_cached_coherent)
+		cache_flags = ETNA_BO_CACHED_COHERENT;
+	else
+		cache_flags = ETNA_BO_WC;
+
+	ret = etnaviv_gem_new_private(dev, size, cache_flags,
 				      &etnaviv_gem_prime_ops, &etnaviv_obj);
 	if (ret < 0)
 		return ERR_PTR(ret);
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index af024d90453d..474b0db286de 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -90,13 +90,14 @@ struct drm_etnaviv_param {
  * GEM buffers:
  */
 
-#define ETNA_BO_CACHE_MASK   0x000f0000
+#define ETNA_BO_CACHE_MASK              0x000f0000
 /* cache modes */
-#define ETNA_BO_CACHED       0x00010000
-#define ETNA_BO_WC           0x00020000
-#define ETNA_BO_UNCACHED     0x00040000
+#define ETNA_BO_CACHED                  0x00010000
+#define ETNA_BO_WC                      0x00020000
+#define ETNA_BO_UNCACHED                0x00040000
+#define ETNA_BO_CACHED_COHERENT         0x00080000
 /* map flags */
-#define ETNA_BO_FORCE_MMU    0x00100000
+#define ETNA_BO_FORCE_MMU               0x00100000
 
 struct drm_etnaviv_gem_new {
 	__u64 size;           /* in */
-- 
2.25.1

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Bjorn Helgaas]

s/usperspace/userspace/ (in subject)

On Wed, May 31, 2023 at 12:06:43AM +0800, Sui Jingfeng wrote:
> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
> snoop the CPU's cache. write-combine caching property is not suitiable for
> us.

s/allways/always/
s/suitiable/suitable/

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Bjorn Helgaas]

s/usperspace/userspace/ (in subject)

On Wed, May 31, 2023 at 12:06:43AM +0800, Sui Jingfeng wrote:
> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
> snoop the CPU's cache. write-combine caching property is not suitiable for
> us.

s/allways/always/
s/suitiable/suitable/

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Bjorn Helgaas]

On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
> This patch adds PCI driver support on top of what already have. Take the
> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
> Therefore, component frameworks can be avoided. Because we want to bind the
> DRM driver service to the PCI driver manually.

> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
> +	 * maintain cache coherency by hardware
> +	 */
> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
> +		priv->has_cached_coherent = true;

This looks like something that should be a runtime check, not a
compile-time check.

If it's possible to build a single kernel image that runs on Loongson
MIPS or LoongArch CPU and, in addition, runs on other platforms, you
cannot assume that all the others maintain this cache coherency.

> +static struct etnaviv_drm_private *etna_private_s;

A static pointer looks wrong because it probably limits you to a
single instance of something.

> @@ -727,6 +756,12 @@ static int __init etnaviv_init(void)
>  	if (ret != 0)
>  		goto unregister_gpu_driver;
>  
> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> +	ret = pci_register_driver(&etnaviv_pci_driver);
> +#endif
> +	if (ret != 0)
> +		goto unregister_platform_driver;

Why is this outside the #ifdef?  If CONFIG_DRM_ETNAVIV_PCI_DRIVER is
not set, you already tested "ret != 0" above and will never take this
goto.

> +static int etnaviv_gpu_plat_drv_init(struct etnaviv_gpu *gpu, bool component)
> +{
> +	struct device *dev = gpu->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int err;
> +
> +	/* Map registers: */
> +	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(gpu->mmio))
> +		return PTR_ERR(gpu->mmio);
> +
> +	if (component) {
> +		err = component_add(dev, &gpu_ops);
> +		if (err < 0) {
> +			dev_err(dev, "failed to register component: %d\n", err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

All this platform driver rearrangement looks like it should be a
separate patch so adding PCI support only adds PCI-related stuff.

> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/pci.h>
> +
> +#include "etnaviv_drv.h"
> +#include "etnaviv_gpu.h"
> +#include "etnaviv_pci_drv.h"
> +
> +enum etnaviv_pci_gpu_family {
> +	GC1000_IN_LS7A1000 = 0,
> +	GC1000_IN_LS2K1000 = 1,

Seems unused; why is this here?

> +static int etnaviv_pci_probe(struct pci_dev *pdev,
> +			     const struct pci_device_id *ent)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable\n");

Use "dev", no need for "&pdev->dev" since you already looked it up
above.  Also below for dma_set_mask_and_coherent().

> +		return ret;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

> +static const struct pci_device_id etnaviv_pci_id_lists[] = {
> +	{0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
> +	{0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
> +	{0, 0, 0, 0, 0, 0, 0}

Should probably use PCI_DEVICE_DATA().  Use PCI_VENDOR_ID_LOONGSON.
Only "{ }" required to terminate.

> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __ETNAVIV_PCI_DRV_H__
> +#define __ETNAVIV_PCI_DRV_H__
> +
> +#include <linux/pci.h>

This #include isn't required by this file.

> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> +extern struct pci_driver etnaviv_pci_driver;
> +#endif
> +
> +#endif

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Bjorn Helgaas]

On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
> This patch adds PCI driver support on top of what already have. Take the
> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
> Therefore, component frameworks can be avoided. Because we want to bind the
> DRM driver service to the PCI driver manually.

> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
> +	 * maintain cache coherency by hardware
> +	 */
> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
> +		priv->has_cached_coherent = true;

This looks like something that should be a runtime check, not a
compile-time check.

If it's possible to build a single kernel image that runs on Loongson
MIPS or LoongArch CPU and, in addition, runs on other platforms, you
cannot assume that all the others maintain this cache coherency.

> +static struct etnaviv_drm_private *etna_private_s;

A static pointer looks wrong because it probably limits you to a
single instance of something.

> @@ -727,6 +756,12 @@ static int __init etnaviv_init(void)
>  	if (ret != 0)
>  		goto unregister_gpu_driver;
>  
> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> +	ret = pci_register_driver(&etnaviv_pci_driver);
> +#endif
> +	if (ret != 0)
> +		goto unregister_platform_driver;

Why is this outside the #ifdef?  If CONFIG_DRM_ETNAVIV_PCI_DRIVER is
not set, you already tested "ret != 0" above and will never take this
goto.

> +static int etnaviv_gpu_plat_drv_init(struct etnaviv_gpu *gpu, bool component)
> +{
> +	struct device *dev = gpu->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int err;
> +
> +	/* Map registers: */
> +	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(gpu->mmio))
> +		return PTR_ERR(gpu->mmio);
> +
> +	if (component) {
> +		err = component_add(dev, &gpu_ops);
> +		if (err < 0) {
> +			dev_err(dev, "failed to register component: %d\n", err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

All this platform driver rearrangement looks like it should be a
separate patch so adding PCI support only adds PCI-related stuff.

> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/pci.h>
> +
> +#include "etnaviv_drv.h"
> +#include "etnaviv_gpu.h"
> +#include "etnaviv_pci_drv.h"
> +
> +enum etnaviv_pci_gpu_family {
> +	GC1000_IN_LS7A1000 = 0,
> +	GC1000_IN_LS2K1000 = 1,

Seems unused; why is this here?

> +static int etnaviv_pci_probe(struct pci_dev *pdev,
> +			     const struct pci_device_id *ent)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable\n");

Use "dev", no need for "&pdev->dev" since you already looked it up
above.  Also below for dma_set_mask_and_coherent().

> +		return ret;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

> +static const struct pci_device_id etnaviv_pci_id_lists[] = {
> +	{0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
> +	{0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
> +	{0, 0, 0, 0, 0, 0, 0}

Should probably use PCI_DEVICE_DATA().  Use PCI_VENDOR_ID_LOONGSON.
Only "{ }" required to terminate.

> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __ETNAVIV_PCI_DRV_H__
> +#define __ETNAVIV_PCI_DRV_H__
> +
> +#include <linux/pci.h>

This #include isn't required by this file.

> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> +extern struct pci_driver etnaviv_pci_driver;
> +#endif
> +
> +#endif

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Sui Jingfeng]

Hi,


Thanks a lot, will be fixed at next version.


On 2023/5/31 02:58, Bjorn Helgaas wrote:
> s/usperspace/userspace/ (in subject)
>
> On Wed, May 31, 2023 at 12:06:43AM +0800, Sui Jingfeng wrote:
>> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
>> snoop the CPU's cache. write-combine caching property is not suitiable for
>> us.
> s/allways/always/
> s/suitiable/suitable/

-- 
Jingfeng

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Sui Jingfeng]

Hi,


Thanks a lot, will be fixed at next version.


On 2023/5/31 02:58, Bjorn Helgaas wrote:
> s/usperspace/userspace/ (in subject)
>
> On Wed, May 31, 2023 at 12:06:43AM +0800, Sui Jingfeng wrote:
>> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
>> snoop the CPU's cache. write-combine caching property is not suitiable for
>> us.
> s/allways/always/
> s/suitiable/suitable/

-- 
Jingfeng

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

Hi,

On 2023/5/31 03:02, Bjorn Helgaas wrote:
> On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
>> This patch adds PCI driver support on top of what already have. Take the
>> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
>> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
>> Therefore, component frameworks can be avoided. Because we want to bind the
>> DRM driver service to the PCI driver manually.
>> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
>> +	 * maintain cache coherency by hardware
>> +	 */
>> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
>> +		priv->has_cached_coherent = true;
> This looks like something that should be a runtime check, not a
> compile-time check.
>
> If it's possible to build a single kernel image that runs on Loongson
> MIPS or LoongArch CPU and, in addition, runs on other platforms, you
> cannot assume that all the others maintain this cache coherency.

Nice catch! I don't even realize this!


LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,

instruction set, compiler, and binary interface are totally changed.

Therefore, it's impossible to build a single kernel image that runs on 
all Loongson CPUs.

Currently, I can guarantee that this works on the Loongson platform.

My initial intent here is to let priv->has_cached_coherent be *true* on 
the Loongson platform (both mips and loongarch).

I do know there are some other vendors who bought GPU IP from Vivante.

say GC7000, and integrate it into their discrete GPU product.

But it is also a PCI device, but this is another story; it deserves 
another patch.

I don't know if Etnaviv folk find some similar hardware on Arm Arch,

Some Arm CPUs do not maintain cached coherency on hardware.

The has_cached_coherent member can be set to false on such hardware.

For us, it seems that there is no need to do runtime checking,

because they are all cached coherent by default.


Can I improve this in the future, currently I don't have a good idea.

>> +static struct etnaviv_drm_private *etna_private_s;
> A static pointer looks wrong because it probably limits you to a
> single instance of something.

This structure is shared by all GPU cores.

Originally, Etnaviv was a component-based driver.

It's one driver wrangler for all GPU cores. (One piece of driver rules 
them all.)

This structure is shared by all GPU cores and does not have a copy per 
GPU core.

>> @@ -727,6 +756,12 @@ static int __init etnaviv_init(void)
>>   	if (ret != 0)
>>   		goto unregister_gpu_driver;
>>   
>> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
>> +	ret = pci_register_driver(&etnaviv_pci_driver);
>> +#endif
>> +	if (ret != 0)
>> +		goto unregister_platform_driver;
> Why is this outside the #ifdef?  If CONFIG_DRM_ETNAVIV_PCI_DRIVER is
> not set, you already tested "ret != 0" above and will never take this
> goto.

On arch/platform without CONFIG_PCI enabled,

CONFIG_DRM_ETNAVIV_PCI_DRIVER config option will not be enabled.


On such cases, GCC complains when compile with  W=1:


drivers/gpu/drm/etnaviv/etnaviv_drv.c: In function ‘etnaviv_init’:
drivers/gpu/drm/etnaviv/etnaviv_drv.c:787:1: warning: label 
‘unregister_platform_driver’ defined but not used [-Wunused-label]
   787 | unregister_platform_driver:
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~

This is the pain that #ifdefs and #endif bring to us.

We want to zero out compile warnings.

Otherwise, Intel's compiler test robot will come here and warn you!

Yet another test for "ret != 0" doesn't hurt much, probably could be 
optimized out by the compiler.

We are afraid of warnings.

>> +static int etnaviv_gpu_plat_drv_init(struct etnaviv_gpu *gpu, bool component)
>> +{
>> +	struct device *dev = gpu->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	int err;
>> +
>> +	/* Map registers: */
>> +	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(gpu->mmio))
>> +		return PTR_ERR(gpu->mmio);
>> +
>> +	if (component) {
>> +		err = component_add(dev, &gpu_ops);
>> +		if (err < 0) {
>> +			dev_err(dev, "failed to register component: %d\n", err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> All this platform driver rearrangement looks like it should be a
> separate patch so adding PCI support only adds PCI-related stuff.
>
Indeed, this is acceptable.
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
>> @@ -0,0 +1,87 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/pci.h>
>> +
>> +#include "etnaviv_drv.h"
>> +#include "etnaviv_gpu.h"
>> +#include "etnaviv_pci_drv.h"
>> +
>> +enum etnaviv_pci_gpu_family {
>> +	GC1000_IN_LS7A1000 = 0,
>> +	GC1000_IN_LS2K1000 = 1,
> Seems unused; why is this here?

Want to use it to provide device-specific information.

For example, LS2K1000 is an SoC, and LS7A1000 is a bridge chipset.

The GC1000 in those chips is a 32-bit GPU. even though the host is a 
64-bit system.

This GPU can only access memory below 4 GB.

This can be used to provide information known at compile time.

Attach the has_cached_coherent member to the chip model defined here？

>> +static int etnaviv_pci_probe(struct pci_dev *pdev,
>> +			     const struct pci_device_id *ent)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable\n");
> Use "dev", no need for "&pdev->dev" since you already looked it up
> above.  Also below for dma_set_mask_and_coherent().

Ok, acceptable.  You are a really serious man and quite professional.

>
>> +		return ret;
>> +	}
>> +
>> +	pci_set_master(pdev);
>> +
>> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> +static const struct pci_device_id etnaviv_pci_id_lists[] = {
>> +	{0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
>> +	{0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
>> +	{0, 0, 0, 0, 0, 0, 0}
> Should probably use PCI_DEVICE_DATA().  Use PCI_VENDOR_ID_LOONGSON.

Originally, I wanted to keep them on the same line, at the same time, 
avoiding complaints from checkpatch.pl.

PCI_VENDOR_ID_LOONGSON is too long. No problem, will be fixed in the next version.

> Only "{ }" required to terminate.
>
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __ETNAVIV_PCI_DRV_H__
>> +#define __ETNAVIV_PCI_DRV_H__
>> +
>> +#include <linux/pci.h>
> This #include isn't required by this file.

Let me give you a reason to do this:

My initial idea is that other source files only need to include 
"etnaviv_pci_drv.h",

OK, new.  will be fixed in the next version.

>> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
>> +extern struct pci_driver etnaviv_pci_driver;
>> +#endif
>> +
>> +#endif

-- 
Jingfeng

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

Hi,

On 2023/5/31 03:02, Bjorn Helgaas wrote:
> On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
>> This patch adds PCI driver support on top of what already have. Take the
>> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
>> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
>> Therefore, component frameworks can be avoided. Because we want to bind the
>> DRM driver service to the PCI driver manually.
>> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
>> +	 * maintain cache coherency by hardware
>> +	 */
>> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
>> +		priv->has_cached_coherent = true;
> This looks like something that should be a runtime check, not a
> compile-time check.
>
> If it's possible to build a single kernel image that runs on Loongson
> MIPS or LoongArch CPU and, in addition, runs on other platforms, you
> cannot assume that all the others maintain this cache coherency.

Nice catch! I don't even realize this!


LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,

instruction set, compiler, and binary interface are totally changed.

Therefore, it's impossible to build a single kernel image that runs on 
all Loongson CPUs.

Currently, I can guarantee that this works on the Loongson platform.

My initial intent here is to let priv->has_cached_coherent be *true* on 
the Loongson platform (both mips and loongarch).

I do know there are some other vendors who bought GPU IP from Vivante.

say GC7000, and integrate it into their discrete GPU product.

But it is also a PCI device, but this is another story; it deserves 
another patch.

I don't know if Etnaviv folk find some similar hardware on Arm Arch,

Some Arm CPUs do not maintain cached coherency on hardware.

The has_cached_coherent member can be set to false on such hardware.

For us, it seems that there is no need to do runtime checking,

because they are all cached coherent by default.


Can I improve this in the future, currently I don't have a good idea.

>> +static struct etnaviv_drm_private *etna_private_s;
> A static pointer looks wrong because it probably limits you to a
> single instance of something.

This structure is shared by all GPU cores.

Originally, Etnaviv was a component-based driver.

It's one driver wrangler for all GPU cores. (One piece of driver rules 
them all.)

This structure is shared by all GPU cores and does not have a copy per 
GPU core.

>> @@ -727,6 +756,12 @@ static int __init etnaviv_init(void)
>>   	if (ret != 0)
>>   		goto unregister_gpu_driver;
>>   
>> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
>> +	ret = pci_register_driver(&etnaviv_pci_driver);
>> +#endif
>> +	if (ret != 0)
>> +		goto unregister_platform_driver;
> Why is this outside the #ifdef?  If CONFIG_DRM_ETNAVIV_PCI_DRIVER is
> not set, you already tested "ret != 0" above and will never take this
> goto.

On arch/platform without CONFIG_PCI enabled,

CONFIG_DRM_ETNAVIV_PCI_DRIVER config option will not be enabled.


On such cases, GCC complains when compile with  W=1:


drivers/gpu/drm/etnaviv/etnaviv_drv.c: In function ‘etnaviv_init’:
drivers/gpu/drm/etnaviv/etnaviv_drv.c:787:1: warning: label 
‘unregister_platform_driver’ defined but not used [-Wunused-label]
   787 | unregister_platform_driver:
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~

This is the pain that #ifdefs and #endif bring to us.

We want to zero out compile warnings.

Otherwise, Intel's compiler test robot will come here and warn you!

Yet another test for "ret != 0" doesn't hurt much, probably could be 
optimized out by the compiler.

We are afraid of warnings.

>> +static int etnaviv_gpu_plat_drv_init(struct etnaviv_gpu *gpu, bool component)
>> +{
>> +	struct device *dev = gpu->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	int err;
>> +
>> +	/* Map registers: */
>> +	gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(gpu->mmio))
>> +		return PTR_ERR(gpu->mmio);
>> +
>> +	if (component) {
>> +		err = component_add(dev, &gpu_ops);
>> +		if (err < 0) {
>> +			dev_err(dev, "failed to register component: %d\n", err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> All this platform driver rearrangement looks like it should be a
> separate patch so adding PCI support only adds PCI-related stuff.
>
Indeed, this is acceptable.
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
>> @@ -0,0 +1,87 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/pci.h>
>> +
>> +#include "etnaviv_drv.h"
>> +#include "etnaviv_gpu.h"
>> +#include "etnaviv_pci_drv.h"
>> +
>> +enum etnaviv_pci_gpu_family {
>> +	GC1000_IN_LS7A1000 = 0,
>> +	GC1000_IN_LS2K1000 = 1,
> Seems unused; why is this here?

Want to use it to provide device-specific information.

For example, LS2K1000 is an SoC, and LS7A1000 is a bridge chipset.

The GC1000 in those chips is a 32-bit GPU. even though the host is a 
64-bit system.

This GPU can only access memory below 4 GB.

This can be used to provide information known at compile time.

Attach the has_cached_coherent member to the chip model defined here？

>> +static int etnaviv_pci_probe(struct pci_dev *pdev,
>> +			     const struct pci_device_id *ent)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable\n");
> Use "dev", no need for "&pdev->dev" since you already looked it up
> above.  Also below for dma_set_mask_and_coherent().

Ok, acceptable.  You are a really serious man and quite professional.

>
>> +		return ret;
>> +	}
>> +
>> +	pci_set_master(pdev);
>> +
>> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> +static const struct pci_device_id etnaviv_pci_id_lists[] = {
>> +	{0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
>> +	{0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
>> +	{0, 0, 0, 0, 0, 0, 0}
> Should probably use PCI_DEVICE_DATA().  Use PCI_VENDOR_ID_LOONGSON.

Originally, I wanted to keep them on the same line, at the same time, 
avoiding complaints from checkpatch.pl.

PCI_VENDOR_ID_LOONGSON is too long. No problem, will be fixed in the next version.

> Only "{ }" required to terminate.
>
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __ETNAVIV_PCI_DRV_H__
>> +#define __ETNAVIV_PCI_DRV_H__
>> +
>> +#include <linux/pci.h>
> This #include isn't required by this file.

Let me give you a reason to do this:

My initial idea is that other source files only need to include 
"etnaviv_pci_drv.h",

OK, new.  will be fixed in the next version.

>> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
>> +extern struct pci_driver etnaviv_pci_driver;
>> +#endif
>> +
>> +#endif

-- 
Jingfeng

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Lucas Stach]

Hi Sui Jingfeng,

Am Donnerstag, dem 01.06.2023 um 00:08 +0800 schrieb Sui Jingfeng:
> Hi,
> 
> On 2023/5/31 03:02, Bjorn Helgaas wrote:
> > On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
> > > This patch adds PCI driver support on top of what already have. Take the
> > > GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
> > > There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
> > > Therefore, component frameworks can be avoided. Because we want to bind the
> > > DRM driver service to the PCI driver manually.
> > > +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
> > > +	 * maintain cache coherency by hardware
> > > +	 */
> > > +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
> > > +		priv->has_cached_coherent = true;
> > This looks like something that should be a runtime check, not a
> > compile-time check.
> > 
> > If it's possible to build a single kernel image that runs on Loongson
> > MIPS or LoongArch CPU and, in addition, runs on other platforms, you
> > cannot assume that all the others maintain this cache coherency.
> 
> Nice catch! I don't even realize this!
> 
> 
> LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,
> 
> instruction set, compiler, and binary interface are totally changed.
> 
> Therefore, it's impossible to build a single kernel image that runs on 
> all Loongson CPUs.
> 
> Currently, I can guarantee that this works on the Loongson platform.
> 
> My initial intent here is to let priv->has_cached_coherent be *true* on 
> the Loongson platform (both mips and loongarch).
> 
> I do know there are some other vendors who bought GPU IP from Vivante.
> 
> say GC7000, and integrate it into their discrete GPU product.
> 
> But it is also a PCI device, but this is another story; it deserves 
> another patch.
> 
> I don't know if Etnaviv folk find some similar hardware on Arm Arch,
> 
> Some Arm CPUs do not maintain cached coherency on hardware.
> 
> The has_cached_coherent member can be set to false on such hardware.
> 
> For us, it seems that there is no need to do runtime checking,
> 
> because they are all cached coherent by default.
> 
> 
> Can I improve this in the future, currently I don't have a good idea.

I think I mentioned before that this needs to be a runtime check. What
does dev_is_dma_coherent() return for the Vivante GPU device on your
platform?

Regards,
Lucas

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Lucas Stach]

Hi Sui Jingfeng,

Am Donnerstag, dem 01.06.2023 um 00:08 +0800 schrieb Sui Jingfeng:
> Hi,
> 
> On 2023/5/31 03:02, Bjorn Helgaas wrote:
> > On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
> > > This patch adds PCI driver support on top of what already have. Take the
> > > GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
> > > There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
> > > Therefore, component frameworks can be avoided. Because we want to bind the
> > > DRM driver service to the PCI driver manually.
> > > +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
> > > +	 * maintain cache coherency by hardware
> > > +	 */
> > > +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
> > > +		priv->has_cached_coherent = true;
> > This looks like something that should be a runtime check, not a
> > compile-time check.
> > 
> > If it's possible to build a single kernel image that runs on Loongson
> > MIPS or LoongArch CPU and, in addition, runs on other platforms, you
> > cannot assume that all the others maintain this cache coherency.
> 
> Nice catch! I don't even realize this!
> 
> 
> LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,
> 
> instruction set, compiler, and binary interface are totally changed.
> 
> Therefore, it's impossible to build a single kernel image that runs on 
> all Loongson CPUs.
> 
> Currently, I can guarantee that this works on the Loongson platform.
> 
> My initial intent here is to let priv->has_cached_coherent be *true* on 
> the Loongson platform (both mips and loongarch).
> 
> I do know there are some other vendors who bought GPU IP from Vivante.
> 
> say GC7000, and integrate it into their discrete GPU product.
> 
> But it is also a PCI device, but this is another story; it deserves 
> another patch.
> 
> I don't know if Etnaviv folk find some similar hardware on Arm Arch,
> 
> Some Arm CPUs do not maintain cached coherency on hardware.
> 
> The has_cached_coherent member can be set to false on such hardware.
> 
> For us, it seems that there is no need to do runtime checking,
> 
> because they are all cached coherent by default.
> 
> 
> Can I improve this in the future, currently I don't have a good idea.

I think I mentioned before that this needs to be a runtime check. What
does dev_is_dma_coherent() return for the Vivante GPU device on your
platform?

Regards,
Lucas

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

Hi,

On 2023/6/1 00:23, Lucas Stach wrote:
> Hi Sui Jingfeng,
>
> Am Donnerstag, dem 01.06.2023 um 00:08 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/5/31 03:02, Bjorn Helgaas wrote:
>>> On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
>>>> This patch adds PCI driver support on top of what already have. Take the
>>>> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
>>>> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
>>>> Therefore, component frameworks can be avoided. Because we want to bind the
>>>> DRM driver service to the PCI driver manually.
>>>> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
>>>> +	 * maintain cache coherency by hardware
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
>>>> +		priv->has_cached_coherent = true;
>>> This looks like something that should be a runtime check, not a
>>> compile-time check.
>>>
>>> If it's possible to build a single kernel image that runs on Loongson
>>> MIPS or LoongArch CPU and, in addition, runs on other platforms, you
>>> cannot assume that all the others maintain this cache coherency.
>> Nice catch! I don't even realize this!
>>
>>
>> LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,
>>
>> instruction set, compiler, and binary interface are totally changed.
>>
>> Therefore, it's impossible to build a single kernel image that runs on
>> all Loongson CPUs.
>>
>> Currently, I can guarantee that this works on the Loongson platform.
>>
>> My initial intent here is to let priv->has_cached_coherent be *true* on
>> the Loongson platform (both mips and loongarch).
>>
>> I do know there are some other vendors who bought GPU IP from Vivante.
>>
>> say GC7000, and integrate it into their discrete GPU product.
>>
>> But it is also a PCI device, but this is another story; it deserves
>> another patch.
>>
>> I don't know if Etnaviv folk find some similar hardware on Arm Arch,
>>
>> Some Arm CPUs do not maintain cached coherency on hardware.
>>
>> The has_cached_coherent member can be set to false on such hardware.
>>
>> For us, it seems that there is no need to do runtime checking,
>>
>> because they are all cached coherent by default.
>>
>>
>> Can I improve this in the future, currently I don't have a good idea.
> I think I mentioned before that this needs to be a runtime check. What
> does dev_is_dma_coherent() return for the Vivante GPU device on your
> platform?

Yes, you have told me so.

I will try it, and I will answer your question tomorrow.

> Regards,
> Lucas

-- 
Jingfeng

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

Hi,

On 2023/6/1 00:23, Lucas Stach wrote:
> Hi Sui Jingfeng,
>
> Am Donnerstag, dem 01.06.2023 um 00:08 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/5/31 03:02, Bjorn Helgaas wrote:
>>> On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
>>>> This patch adds PCI driver support on top of what already have. Take the
>>>> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
>>>> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
>>>> Therefore, component frameworks can be avoided. Because we want to bind the
>>>> DRM driver service to the PCI driver manually.
>>>> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
>>>> +	 * maintain cache coherency by hardware
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
>>>> +		priv->has_cached_coherent = true;
>>> This looks like something that should be a runtime check, not a
>>> compile-time check.
>>>
>>> If it's possible to build a single kernel image that runs on Loongson
>>> MIPS or LoongArch CPU and, in addition, runs on other platforms, you
>>> cannot assume that all the others maintain this cache coherency.
>> Nice catch! I don't even realize this!
>>
>>
>> LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,
>>
>> instruction set, compiler, and binary interface are totally changed.
>>
>> Therefore, it's impossible to build a single kernel image that runs on
>> all Loongson CPUs.
>>
>> Currently, I can guarantee that this works on the Loongson platform.
>>
>> My initial intent here is to let priv->has_cached_coherent be *true* on
>> the Loongson platform (both mips and loongarch).
>>
>> I do know there are some other vendors who bought GPU IP from Vivante.
>>
>> say GC7000, and integrate it into their discrete GPU product.
>>
>> But it is also a PCI device, but this is another story; it deserves
>> another patch.
>>
>> I don't know if Etnaviv folk find some similar hardware on Arm Arch,
>>
>> Some Arm CPUs do not maintain cached coherency on hardware.
>>
>> The has_cached_coherent member can be set to false on such hardware.
>>
>> For us, it seems that there is no need to do runtime checking,
>>
>> because they are all cached coherent by default.
>>
>>
>> Can I improve this in the future, currently I don't have a good idea.
> I think I mentioned before that this needs to be a runtime check. What
> does dev_is_dma_coherent() return for the Vivante GPU device on your
> platform?

Yes, you have told me so.

I will try it, and I will answer your question tomorrow.

> Regards,
> Lucas

-- 
Jingfeng

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Lucas Stach]

Hi Sui Jingfeng,

Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
> snoop the CPU's cache. write-combine caching property is not suitiable for
> us.
> 
As previously mentioned in the Mesa MR, I don't think this is the right
approach.

ETNA_BO_CACHED already looks coherent to userspace, as all accesses are
bracketed via the ETNAVIV_GEM_CPU_PREP and ETNAVIV_GEM_CPU_FINI ioctls,
which will do the necessary cache maintenance on platforms where device
coherence isn't enforced by the hardware, so there is no need for a
separate ETNA_BO_CACHED_COHERENT.

Instead we just need a new ETNAVIV_PARAM to inform userspace about
hardware cache coherence being available for a specific GPU core, in
which case the userspace driver should switch to preferring
ETNA_BO_CACHED over ETNA_BO_WC.

Regards,
Lucas

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c       |  2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 22 +++++++++++++++++++--
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  9 ++++++++-
>  include/uapi/drm/etnaviv_drm.h              | 11 ++++++-----
>  4 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 052f745cecc0..2816c654c023 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -274,7 +274,7 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data,
>  	struct drm_etnaviv_gem_new *args = data;
>  
>  	if (args->flags & ~(ETNA_BO_CACHED | ETNA_BO_WC | ETNA_BO_UNCACHED |
> -			    ETNA_BO_FORCE_MMU))
> +			    ETNA_BO_CACHED_COHERENT | ETNA_BO_FORCE_MMU))
>  		return -EINVAL;
>  
>  	return etnaviv_gem_new_handle(dev, file, args->size,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index b5f73502e3dd..d8b559bd33d3 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
>  static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>  {
>  	struct page **pages;
> +	pgprot_t prot;
>  
>  	lockdep_assert_held(&obj->lock);
>  
> @@ -350,8 +351,20 @@ static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>  	if (IS_ERR(pages))
>  		return NULL;
>  
> -	return vmap(pages, obj->base.size >> PAGE_SHIFT,
> -			VM_MAP, pgprot_writecombine(PAGE_KERNEL));
> +	switch (obj->flags) {
> +	case ETNA_BO_CACHED_COHERENT:
> +	case ETNA_BO_CACHED:
> +		prot = PAGE_KERNEL;
> +		break;
> +	case ETNA_BO_UNCACHED:
> +		prot = pgprot_noncached(PAGE_KERNEL);
> +		break;
> +	case ETNA_BO_WC:
> +	default:
> +		prot = pgprot_writecombine(PAGE_KERNEL);
> +	}
> +
> +	return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
>  }
>  
>  static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
> @@ -545,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>  static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>  	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>  {
> +	struct etnaviv_drm_private *priv = dev->dev_private;
>  	struct etnaviv_gem_object *etnaviv_obj;
>  	unsigned sz = sizeof(*etnaviv_obj);
>  	bool valid = true;
> @@ -555,6 +569,10 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>  	case ETNA_BO_CACHED:
>  	case ETNA_BO_WC:
>  		break;
> +	case ETNA_BO_CACHED_COHERENT:
> +		if (priv->has_cached_coherent)
> +			break;
> +		fallthrough;
>  	default:
>  		valid = false;
>  	}
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index 3524b5811682..671d91d8f1c6 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -112,11 +112,18 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
>  struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>  	struct dma_buf_attachment *attach, struct sg_table *sgt)
>  {
> +	struct etnaviv_drm_private *priv = dev->dev_private;
>  	struct etnaviv_gem_object *etnaviv_obj;
>  	size_t size = PAGE_ALIGN(attach->dmabuf->size);
> +	u32 cache_flags;
>  	int ret, npages;
>  
> -	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
> +	if (priv->has_cached_coherent)
> +		cache_flags = ETNA_BO_CACHED_COHERENT;
> +	else
> +		cache_flags = ETNA_BO_WC;
> +
> +	ret = etnaviv_gem_new_private(dev, size, cache_flags,
>  				      &etnaviv_gem_prime_ops, &etnaviv_obj);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> index af024d90453d..474b0db286de 100644
> --- a/include/uapi/drm/etnaviv_drm.h
> +++ b/include/uapi/drm/etnaviv_drm.h
> @@ -90,13 +90,14 @@ struct drm_etnaviv_param {
>   * GEM buffers:
>   */
>  
> -#define ETNA_BO_CACHE_MASK   0x000f0000
> +#define ETNA_BO_CACHE_MASK              0x000f0000
>  /* cache modes */
> -#define ETNA_BO_CACHED       0x00010000
> -#define ETNA_BO_WC           0x00020000
> -#define ETNA_BO_UNCACHED     0x00040000
> +#define ETNA_BO_CACHED                  0x00010000
> +#define ETNA_BO_WC                      0x00020000
> +#define ETNA_BO_UNCACHED                0x00040000
> +#define ETNA_BO_CACHED_COHERENT         0x00080000
>  /* map flags */
> -#define ETNA_BO_FORCE_MMU    0x00100000
> +#define ETNA_BO_FORCE_MMU               0x00100000
>  
>  struct drm_etnaviv_gem_new {
>  	__u64 size;           /* in */

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Lucas Stach]

Hi Sui Jingfeng,

Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
> snoop the CPU's cache. write-combine caching property is not suitiable for
> us.
> 
As previously mentioned in the Mesa MR, I don't think this is the right
approach.

ETNA_BO_CACHED already looks coherent to userspace, as all accesses are
bracketed via the ETNAVIV_GEM_CPU_PREP and ETNAVIV_GEM_CPU_FINI ioctls,
which will do the necessary cache maintenance on platforms where device
coherence isn't enforced by the hardware, so there is no need for a
separate ETNA_BO_CACHED_COHERENT.

Instead we just need a new ETNAVIV_PARAM to inform userspace about
hardware cache coherence being available for a specific GPU core, in
which case the userspace driver should switch to preferring
ETNA_BO_CACHED over ETNA_BO_WC.

Regards,
Lucas

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c       |  2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 22 +++++++++++++++++++--
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  9 ++++++++-
>  include/uapi/drm/etnaviv_drm.h              | 11 ++++++-----
>  4 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 052f745cecc0..2816c654c023 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -274,7 +274,7 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data,
>  	struct drm_etnaviv_gem_new *args = data;
>  
>  	if (args->flags & ~(ETNA_BO_CACHED | ETNA_BO_WC | ETNA_BO_UNCACHED |
> -			    ETNA_BO_FORCE_MMU))
> +			    ETNA_BO_CACHED_COHERENT | ETNA_BO_FORCE_MMU))
>  		return -EINVAL;
>  
>  	return etnaviv_gem_new_handle(dev, file, args->size,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index b5f73502e3dd..d8b559bd33d3 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
>  static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>  {
>  	struct page **pages;
> +	pgprot_t prot;
>  
>  	lockdep_assert_held(&obj->lock);
>  
> @@ -350,8 +351,20 @@ static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>  	if (IS_ERR(pages))
>  		return NULL;
>  
> -	return vmap(pages, obj->base.size >> PAGE_SHIFT,
> -			VM_MAP, pgprot_writecombine(PAGE_KERNEL));
> +	switch (obj->flags) {
> +	case ETNA_BO_CACHED_COHERENT:
> +	case ETNA_BO_CACHED:
> +		prot = PAGE_KERNEL;
> +		break;
> +	case ETNA_BO_UNCACHED:
> +		prot = pgprot_noncached(PAGE_KERNEL);
> +		break;
> +	case ETNA_BO_WC:
> +	default:
> +		prot = pgprot_writecombine(PAGE_KERNEL);
> +	}
> +
> +	return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
>  }
>  
>  static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
> @@ -545,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>  static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>  	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>  {
> +	struct etnaviv_drm_private *priv = dev->dev_private;
>  	struct etnaviv_gem_object *etnaviv_obj;
>  	unsigned sz = sizeof(*etnaviv_obj);
>  	bool valid = true;
> @@ -555,6 +569,10 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>  	case ETNA_BO_CACHED:
>  	case ETNA_BO_WC:
>  		break;
> +	case ETNA_BO_CACHED_COHERENT:
> +		if (priv->has_cached_coherent)
> +			break;
> +		fallthrough;
>  	default:
>  		valid = false;
>  	}
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index 3524b5811682..671d91d8f1c6 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -112,11 +112,18 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
>  struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>  	struct dma_buf_attachment *attach, struct sg_table *sgt)
>  {
> +	struct etnaviv_drm_private *priv = dev->dev_private;
>  	struct etnaviv_gem_object *etnaviv_obj;
>  	size_t size = PAGE_ALIGN(attach->dmabuf->size);
> +	u32 cache_flags;
>  	int ret, npages;
>  
> -	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
> +	if (priv->has_cached_coherent)
> +		cache_flags = ETNA_BO_CACHED_COHERENT;
> +	else
> +		cache_flags = ETNA_BO_WC;
> +
> +	ret = etnaviv_gem_new_private(dev, size, cache_flags,
>  				      &etnaviv_gem_prime_ops, &etnaviv_obj);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> index af024d90453d..474b0db286de 100644
> --- a/include/uapi/drm/etnaviv_drm.h
> +++ b/include/uapi/drm/etnaviv_drm.h
> @@ -90,13 +90,14 @@ struct drm_etnaviv_param {
>   * GEM buffers:
>   */
>  
> -#define ETNA_BO_CACHE_MASK   0x000f0000
> +#define ETNA_BO_CACHE_MASK              0x000f0000
>  /* cache modes */
> -#define ETNA_BO_CACHED       0x00010000
> -#define ETNA_BO_WC           0x00020000
> -#define ETNA_BO_UNCACHED     0x00040000
> +#define ETNA_BO_CACHED                  0x00010000
> +#define ETNA_BO_WC                      0x00020000
> +#define ETNA_BO_UNCACHED                0x00040000
> +#define ETNA_BO_CACHED_COHERENT         0x00080000
>  /* map flags */
> -#define ETNA_BO_FORCE_MMU    0x00100000
> +#define ETNA_BO_FORCE_MMU               0x00100000
>  
>  struct drm_etnaviv_gem_new {
>  	__u64 size;           /* in */

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

Hi,

On 2023/6/1 00:23, Lucas Stach wrote:
> Hi Sui Jingfeng,
>
> Am Donnerstag, dem 01.06.2023 um 00:08 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/5/31 03:02, Bjorn Helgaas wrote:
>>> On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
>>>> This patch adds PCI driver support on top of what already have. Take the
>>>> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
>>>> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
>>>> Therefore, component frameworks can be avoided. Because we want to bind the
>>>> DRM driver service to the PCI driver manually.
>>>> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
>>>> +	 * maintain cache coherency by hardware
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
>>>> +		priv->has_cached_coherent = true;
>>> This looks like something that should be a runtime check, not a
>>> compile-time check.
>>>
>>> If it's possible to build a single kernel image that runs on Loongson
>>> MIPS or LoongArch CPU and, in addition, runs on other platforms, you
>>> cannot assume that all the others maintain this cache coherency.
>> Nice catch! I don't even realize this!
>>
>>
>> LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,
>>
>> instruction set, compiler, and binary interface are totally changed.
>>
>> Therefore, it's impossible to build a single kernel image that runs on
>> all Loongson CPUs.
>>
>> Currently, I can guarantee that this works on the Loongson platform.
>>
>> My initial intent here is to let priv->has_cached_coherent be *true* on
>> the Loongson platform (both mips and loongarch).
>>
>> I do know there are some other vendors who bought GPU IP from Vivante.
>>
>> say GC7000, and integrate it into their discrete GPU product.
>>
>> But it is also a PCI device, but this is another story; it deserves
>> another patch.
>>
>> I don't know if Etnaviv folk find some similar hardware on Arm Arch,
>>
>> Some Arm CPUs do not maintain cached coherency on hardware.
>>
>> The has_cached_coherent member can be set to false on such hardware.
>>
>> For us, it seems that there is no need to do runtime checking,
>>
>> because they are all cached coherent by default.
>>
>>
>> Can I improve this in the future, currently I don't have a good idea.
> I think I mentioned before that this needs to be a runtime check. What
> does dev_is_dma_coherent() return for the Vivante GPU device on your
> platform?

I have tested, it return *true*.

Yeah, out hardware is dma coherent.

Then,  we switching  to  dev_is_dma_coherent(dev) ?

> Regards,
> Lucas

-- 
Jingfeng

Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices

[Sui Jingfeng]

Hi,

On 2023/6/1 00:23, Lucas Stach wrote:
> Hi Sui Jingfeng,
>
> Am Donnerstag, dem 01.06.2023 um 00:08 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/5/31 03:02, Bjorn Helgaas wrote:
>>> On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
>>>> This patch adds PCI driver support on top of what already have. Take the
>>>> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
>>>> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
>>>> Therefore, component frameworks can be avoided. Because we want to bind the
>>>> DRM driver service to the PCI driver manually.
>>>> +	 * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
>>>> +	 * maintain cache coherency by hardware
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
>>>> +		priv->has_cached_coherent = true;
>>> This looks like something that should be a runtime check, not a
>>> compile-time check.
>>>
>>> If it's possible to build a single kernel image that runs on Loongson
>>> MIPS or LoongArch CPU and, in addition, runs on other platforms, you
>>> cannot assume that all the others maintain this cache coherency.
>> Nice catch! I don't even realize this!
>>
>>
>> LS3A4000 is mips64r2 with MSA SIMD, while LS3A5000 is LoongArch,
>>
>> instruction set, compiler, and binary interface are totally changed.
>>
>> Therefore, it's impossible to build a single kernel image that runs on
>> all Loongson CPUs.
>>
>> Currently, I can guarantee that this works on the Loongson platform.
>>
>> My initial intent here is to let priv->has_cached_coherent be *true* on
>> the Loongson platform (both mips and loongarch).
>>
>> I do know there are some other vendors who bought GPU IP from Vivante.
>>
>> say GC7000, and integrate it into their discrete GPU product.
>>
>> But it is also a PCI device, but this is another story; it deserves
>> another patch.
>>
>> I don't know if Etnaviv folk find some similar hardware on Arm Arch,
>>
>> Some Arm CPUs do not maintain cached coherency on hardware.
>>
>> The has_cached_coherent member can be set to false on such hardware.
>>
>> For us, it seems that there is no need to do runtime checking,
>>
>> because they are all cached coherent by default.
>>
>>
>> Can I improve this in the future, currently I don't have a good idea.
> I think I mentioned before that this needs to be a runtime check. What
> does dev_is_dma_coherent() return for the Vivante GPU device on your
> platform?

I have tested, it return *true*.

Yeah, out hardware is dma coherent.

Then,  we switching  to  dev_is_dma_coherent(dev) ?

> Regards,
> Lucas

-- 
Jingfeng

Re: [PATCH v6 2/6] drm/etnaviv: add a dedicated function to get various clocks

[Lucas Stach]

Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
> Because it is also platform-dependent, there are environments where don't
> have CLK subsystem support, for example, discreted PCI gpu. So don't rage
> quit if there is no CLK subsystem.
> 
> For the GPU in LS7a1000 and LS2k2000, the working frequency of the GPU is
> tuned by configuring the PLL register directly.
> 
Is this PLL under control of system firmware and invisible to Linux?

> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 62 ++++++++++++++++++---------
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  1 +
>  2 files changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 636d3f39ddcb..4937580551a5 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1565,10 +1565,45 @@ static irqreturn_t irq_handler(int irq, void *data)
>  	return ret;
>  }
>  
> +static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
> +{
> +	struct device *dev = gpu->dev;
> +
> +	if (gpu->no_clk)
> +		return 0;
> +
> +	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
> +	DBG("clk_reg: %p", gpu->clk_reg);
> +	if (IS_ERR(gpu->clk_reg))
> +		return PTR_ERR(gpu->clk_reg);
> +
> +	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
> +	DBG("clk_bus: %p", gpu->clk_bus);
> +	if (IS_ERR(gpu->clk_bus))
> +		return PTR_ERR(gpu->clk_bus);
> +
> +	gpu->clk_core = devm_clk_get(dev, "core");
> +	DBG("clk_core: %p", gpu->clk_core);
> +	if (IS_ERR(gpu->clk_core))
> +		return PTR_ERR(gpu->clk_core);
> +	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
> +
> +	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
> +	DBG("clk_shader: %p", gpu->clk_shader);
> +	if (IS_ERR(gpu->clk_shader))
> +		return PTR_ERR(gpu->clk_shader);
> +	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
> +
> +	return 0;
> +}
> +
>  static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>  {
>  	int ret;
>  
> +	if (gpu->no_clk)
> +		return 0;
> +
I don't see why this would be needed? If your platform doesn't provide
CONFIG_HAVE_CLK all those functions should be successful no-ops, so
there is no need to special case this in the driver.

Or does your platform in fact provide a clk subsystem, just the GPU
clocks are managed by it?

Also all those functions are fine with being called on a NULL clk, so
shouldn't it be enough to simply avoid calling etnaviv_gpu_clk_get() in
the PCI device case?

Regards,
Lucas

>  	ret = clk_prepare_enable(gpu->clk_reg);
>  	if (ret)
>  		return ret;
> @@ -1599,6 +1634,9 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>  
>  static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
>  {
> +	if (gpu->no_clk)
> +		return 0;
> +
>  	clk_disable_unprepare(gpu->clk_shader);
>  	clk_disable_unprepare(gpu->clk_core);
>  	clk_disable_unprepare(gpu->clk_bus);
> @@ -1865,27 +1903,9 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
>  		return err;
>  
>  	/* Get Clocks: */
> -	gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
> -	DBG("clk_reg: %p", gpu->clk_reg);
> -	if (IS_ERR(gpu->clk_reg))
> -		return PTR_ERR(gpu->clk_reg);
> -
> -	gpu->clk_bus = devm_clk_get_optional(&pdev->dev, "bus");
> -	DBG("clk_bus: %p", gpu->clk_bus);
> -	if (IS_ERR(gpu->clk_bus))
> -		return PTR_ERR(gpu->clk_bus);
> -
> -	gpu->clk_core = devm_clk_get(&pdev->dev, "core");
> -	DBG("clk_core: %p", gpu->clk_core);
> -	if (IS_ERR(gpu->clk_core))
> -		return PTR_ERR(gpu->clk_core);
> -	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
> -
> -	gpu->clk_shader = devm_clk_get_optional(&pdev->dev, "shader");
> -	DBG("clk_shader: %p", gpu->clk_shader);
> -	if (IS_ERR(gpu->clk_shader))
> -		return PTR_ERR(gpu->clk_shader);
> -	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
> +	err = etnaviv_gpu_clk_get(gpu);
> +	if (err)
> +		return err;
>  
>  	/* TODO: figure out max mapped size */
>  	dev_set_drvdata(dev, gpu);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 98c6f9c320fc..6da5209a7d64 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -148,6 +148,7 @@ struct etnaviv_gpu {
>  	struct clk *clk_reg;
>  	struct clk *clk_core;
>  	struct clk *clk_shader;
> +	bool no_clk;
>  
>  	unsigned int freq_scale;
>  	unsigned long base_rate_core;

Re: [PATCH v6 2/6] drm/etnaviv: add a dedicated function to get various clocks

[Sui Jingfeng]

Hi,

On 2023/6/1 02:07, Lucas Stach wrote:
> Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
>> Because it is also platform-dependent, there are environments where don't
>> have CLK subsystem support, for example, discreted PCI gpu. So don't rage
>> quit if there is no CLK subsystem.
>>
>> For the GPU in LS7a1000 and LS2k2000, the working frequency of the GPU is
>> tuned by configuring the PLL register directly.
>>
> Is this PLL under control of system firmware and invisible to Linux?
Yes, it is registers, both system firmware and kernel space driver can 
access it.
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 62 ++++++++++++++++++---------
>>   drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  1 +
>>   2 files changed, 42 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> index 636d3f39ddcb..4937580551a5 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> @@ -1565,10 +1565,45 @@ static irqreturn_t irq_handler(int irq, void *data)
>>   	return ret;
>>   }
>>   
>> +static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
>> +{
>> +	struct device *dev = gpu->dev;
>> +
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
>> +	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
>> +	DBG("clk_reg: %p", gpu->clk_reg);
>> +	if (IS_ERR(gpu->clk_reg))
>> +		return PTR_ERR(gpu->clk_reg);
>> +
>> +	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
>> +	DBG("clk_bus: %p", gpu->clk_bus);
>> +	if (IS_ERR(gpu->clk_bus))
>> +		return PTR_ERR(gpu->clk_bus);
>> +
>> +	gpu->clk_core = devm_clk_get(dev, "core");
>> +	DBG("clk_core: %p", gpu->clk_core);
>> +	if (IS_ERR(gpu->clk_core))
>> +		return PTR_ERR(gpu->clk_core);
>> +	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>> +
>> +	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
>> +	DBG("clk_shader: %p", gpu->clk_shader);
>> +	if (IS_ERR(gpu->clk_shader))
>> +		return PTR_ERR(gpu->clk_shader);
>> +	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>> +
>> +	return 0;
>> +}
>> +
>>   static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>>   {
>>   	int ret;
>>   
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
> I don't see why this would be needed? If your platform doesn't provide
> CONFIG_HAVE_CLK all those functions should be successful no-ops, so
> there is no need to special case this in the driver.
>
> Or does your platform in fact provide a clk subsystem, just the GPU
> clocks are managed by it?
>
> Also all those functions are fine with being called on a NULL clk,
right
> so
> shouldn't it be enough to simply avoid calling etnaviv_gpu_clk_get() in
> the PCI device case?

Yes, I just tried, your are right.

There also no need to add the 'no_clk' member into struct etnaviv_gpu

> Regards,
> Lucas
>
>>   	ret = clk_prepare_enable(gpu->clk_reg);
>>   	if (ret)
>>   		return ret;
>> @@ -1599,6 +1634,9 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>>   
>>   static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
>>   {
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
>>   	clk_disable_unprepare(gpu->clk_shader);
>>   	clk_disable_unprepare(gpu->clk_core);
>>   	clk_disable_unprepare(gpu->clk_bus);
>> @@ -1865,27 +1903,9 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
>>   		return err;
>>   
>>   	/* Get Clocks: */
>> -	gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
>> -	DBG("clk_reg: %p", gpu->clk_reg);
>> -	if (IS_ERR(gpu->clk_reg))
>> -		return PTR_ERR(gpu->clk_reg);
>> -
>> -	gpu->clk_bus = devm_clk_get_optional(&pdev->dev, "bus");
>> -	DBG("clk_bus: %p", gpu->clk_bus);
>> -	if (IS_ERR(gpu->clk_bus))
>> -		return PTR_ERR(gpu->clk_bus);
>> -
>> -	gpu->clk_core = devm_clk_get(&pdev->dev, "core");
>> -	DBG("clk_core: %p", gpu->clk_core);
>> -	if (IS_ERR(gpu->clk_core))
>> -		return PTR_ERR(gpu->clk_core);
>> -	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>> -
>> -	gpu->clk_shader = devm_clk_get_optional(&pdev->dev, "shader");
>> -	DBG("clk_shader: %p", gpu->clk_shader);
>> -	if (IS_ERR(gpu->clk_shader))
>> -		return PTR_ERR(gpu->clk_shader);
>> -	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>> +	err = etnaviv_gpu_clk_get(gpu);
>> +	if (err)
>> +		return err;
>>   
>>   	/* TODO: figure out max mapped size */
>>   	dev_set_drvdata(dev, gpu);
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
>> index 98c6f9c320fc..6da5209a7d64 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
>> @@ -148,6 +148,7 @@ struct etnaviv_gpu {
>>   	struct clk *clk_reg;
>>   	struct clk *clk_core;
>>   	struct clk *clk_shader;
>> +	bool no_clk;
>>   
>>   	unsigned int freq_scale;
>>   	unsigned long base_rate_core;

-- 
Jingfeng

Re: [PATCH v6 2/6] drm/etnaviv: add a dedicated function to get various clocks

[Sui Jingfeng]

Hi,

On 2023/6/1 02:07, Lucas Stach wrote:
> Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
>> Because it is also platform-dependent, there are environments where don't
>> have CLK subsystem support, for example, discreted PCI gpu. So don't rage
>> quit if there is no CLK subsystem.
>>
>> For the GPU in LS7a1000 and LS2k2000, the working frequency of the GPU is
>> tuned by configuring the PLL register directly.
>>
> Is this PLL under control of system firmware and invisible to Linux?
Yes, it is registers, both system firmware and kernel space driver can 
access it.
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 62 ++++++++++++++++++---------
>>   drivers/gpu/drm/etnaviv/etnaviv_gpu.h |  1 +
>>   2 files changed, 42 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> index 636d3f39ddcb..4937580551a5 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> @@ -1565,10 +1565,45 @@ static irqreturn_t irq_handler(int irq, void *data)
>>   	return ret;
>>   }
>>   
>> +static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
>> +{
>> +	struct device *dev = gpu->dev;
>> +
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
>> +	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
>> +	DBG("clk_reg: %p", gpu->clk_reg);
>> +	if (IS_ERR(gpu->clk_reg))
>> +		return PTR_ERR(gpu->clk_reg);
>> +
>> +	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
>> +	DBG("clk_bus: %p", gpu->clk_bus);
>> +	if (IS_ERR(gpu->clk_bus))
>> +		return PTR_ERR(gpu->clk_bus);
>> +
>> +	gpu->clk_core = devm_clk_get(dev, "core");
>> +	DBG("clk_core: %p", gpu->clk_core);
>> +	if (IS_ERR(gpu->clk_core))
>> +		return PTR_ERR(gpu->clk_core);
>> +	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>> +
>> +	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
>> +	DBG("clk_shader: %p", gpu->clk_shader);
>> +	if (IS_ERR(gpu->clk_shader))
>> +		return PTR_ERR(gpu->clk_shader);
>> +	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>> +
>> +	return 0;
>> +}
>> +
>>   static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>>   {
>>   	int ret;
>>   
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
> I don't see why this would be needed? If your platform doesn't provide
> CONFIG_HAVE_CLK all those functions should be successful no-ops, so
> there is no need to special case this in the driver.
>
> Or does your platform in fact provide a clk subsystem, just the GPU
> clocks are managed by it?
>
> Also all those functions are fine with being called on a NULL clk,
right
> so
> shouldn't it be enough to simply avoid calling etnaviv_gpu_clk_get() in
> the PCI device case?

Yes, I just tried, your are right.

There also no need to add the 'no_clk' member into struct etnaviv_gpu

> Regards,
> Lucas
>
>>   	ret = clk_prepare_enable(gpu->clk_reg);
>>   	if (ret)
>>   		return ret;
>> @@ -1599,6 +1634,9 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>>   
>>   static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
>>   {
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
>>   	clk_disable_unprepare(gpu->clk_shader);
>>   	clk_disable_unprepare(gpu->clk_core);
>>   	clk_disable_unprepare(gpu->clk_bus);
>> @@ -1865,27 +1903,9 @@ static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
>>   		return err;
>>   
>>   	/* Get Clocks: */
>> -	gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
>> -	DBG("clk_reg: %p", gpu->clk_reg);
>> -	if (IS_ERR(gpu->clk_reg))
>> -		return PTR_ERR(gpu->clk_reg);
>> -
>> -	gpu->clk_bus = devm_clk_get_optional(&pdev->dev, "bus");
>> -	DBG("clk_bus: %p", gpu->clk_bus);
>> -	if (IS_ERR(gpu->clk_bus))
>> -		return PTR_ERR(gpu->clk_bus);
>> -
>> -	gpu->clk_core = devm_clk_get(&pdev->dev, "core");
>> -	DBG("clk_core: %p", gpu->clk_core);
>> -	if (IS_ERR(gpu->clk_core))
>> -		return PTR_ERR(gpu->clk_core);
>> -	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>> -
>> -	gpu->clk_shader = devm_clk_get_optional(&pdev->dev, "shader");
>> -	DBG("clk_shader: %p", gpu->clk_shader);
>> -	if (IS_ERR(gpu->clk_shader))
>> -		return PTR_ERR(gpu->clk_shader);
>> -	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>> +	err = etnaviv_gpu_clk_get(gpu);
>> +	if (err)
>> +		return err;
>>   
>>   	/* TODO: figure out max mapped size */
>>   	dev_set_drvdata(dev, gpu);
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
>> index 98c6f9c320fc..6da5209a7d64 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
>> @@ -148,6 +148,7 @@ struct etnaviv_gpu {
>>   	struct clk *clk_reg;
>>   	struct clk *clk_core;
>>   	struct clk *clk_shader;
>> +	bool no_clk;
>>   
>>   	unsigned int freq_scale;
>>   	unsigned long base_rate_core;

-- 
Jingfeng

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Sui Jingfeng]

Hi,

On 2023/6/1 01:53, Lucas Stach wrote:
> Am Donnerstag, dem 01.06.2023 um 01:29 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/6/1 00:33, Lucas Stach wrote:
>>> Hi Sui Jingfeng,
>>>
>>> Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
>>>> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
>>>> snoop the CPU's cache. write-combine caching property is not suitiable for
>>>> us.
>>>>
>>> As previously mentioned in the Mesa MR, I don't think this is the right
>>> approach.
>>>
>>> ETNA_BO_CACHED already looks coherent to userspace, as all accesses are
>>> bracketed via the ETNAVIV_GEM_CPU_PREP and ETNAVIV_GEM_CPU_FINI ioctls,
>>> which will do the necessary cache maintenance on platforms where device
>>> coherence isn't enforced by the hardware, so there is no need for a
>>> separate ETNA_BO_CACHED_COHERENT.
>> As far as I can see,  ETNA_BO_CACHED_COHERENT could probably help to
>> bypass the overhead of
>>
>> dma_sync_sgtable_for_cpu() and dma_sync_sgtable_for_device() brings to us.
>>
>>
>> I have tested long time ago, there no need call this function on our
>> platform.
>>
>> The glmark2 works as before if I comment out thoes two function.
>>
>> Are you serious, sir?
>>
> The dma_sync* functions are more or less no-ops when the device is
> marked as being coherent. ce
> instance, you might need to propagate the coherent property from the
> GPU core device to the virtual DRM device, along the lines of how we
> propagate other DMA properties from the GPU device to the DRM device in
> etnaviv_pdev_probe.
>
> Other than that things should just work with minimal overhead.
>>> Instead we just need a new ETNAVIV_PARAM to inform userspace about
>>> hardware cache coherence being available for a specific GPU core,
>> Ok, let me think about for a while how to implement this.
>>
> Simple: add new ETNAVIV_PARAM_GPU_COHERENT to
> include/uapi/drm/etnaviv_drm.h, return the result from
> dev_is_dma_coherent in etnaviv_gpu_get_param().
Okay, agree
>> But How about we merge this first, I create another patch to improve it
>>
>> with a roughly working base first? I'm just asking if the answer is No :-)
>>
> The answer is a firm no.
>
> This impacts UAPI, so there is no chance to ever get rid of any wrong
> decisions here, as any added UAPI needs to be supported indefinitely.
> I'm not signing up for maintaining something I believe is implemented
> upside down.
>
> Please don't take this the wrong way: I'm pretty excited to see etnaviv
> used on more architectures and outside of the proven platform device
> paths, so I'm happy to assist in working out the design and help you
> get things merged in both kernel and Mesa. But I think we are still
> quite a few steps away from having things worked out enough to even
> think about merging those patchsets.
>
> Also please allow me to comment on the other patches of the series, so
> I can get a better understanding of your platform/integration, before
> sending another revision of those patches.

I go to sleep yesterday.

Okay, this sound fine.

> Regards,
> Lucas
>
>>>    in
>>> which case the userspace driver should switch to preferring
>>> ETNA_BO_CACHED over ETNA_BO_WC.
>> Yeah,  ETNA_BO_CACHED is enough.
>>
>> ETNA_BO_CACHED_COHERENT is actually a special case of ETNA_BO_CACHED.
>>
>>> Regards,
>>> Lucas
>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/gpu/drm/etnaviv/etnaviv_drv.c       |  2 +-
>>>>    drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 22 +++++++++++++++++++--
>>>>    drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  9 ++++++++-
>>>>    include/uapi/drm/etnaviv_drm.h              | 11 ++++++-----
>>>>    4 files changed, 35 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> index 052f745cecc0..2816c654c023 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> @@ -274,7 +274,7 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data,
>>>>    	struct drm_etnaviv_gem_new *args = data;
>>>>    
>>>>    	if (args->flags & ~(ETNA_BO_CACHED | ETNA_BO_WC | ETNA_BO_UNCACHED |
>>>> -			    ETNA_BO_FORCE_MMU))
>>>> +			    ETNA_BO_CACHED_COHERENT | ETNA_BO_FORCE_MMU))
>>>>    		return -EINVAL;
>>>>    
>>>>    	return etnaviv_gem_new_handle(dev, file, args->size,
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> index b5f73502e3dd..d8b559bd33d3 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> @@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
>>>>    static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>>>>    {
>>>>    	struct page **pages;
>>>> +	pgprot_t prot;
>>>>    
>>>>    	lockdep_assert_held(&obj->lock);
>>>>    
>>>> @@ -350,8 +351,20 @@ static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>>>>    	if (IS_ERR(pages))
>>>>    		return NULL;
>>>>    
>>>> -	return vmap(pages, obj->base.size >> PAGE_SHIFT,
>>>> -			VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>>>> +	switch (obj->flags) {
>>>> +	case ETNA_BO_CACHED_COHERENT:
>>>> +	case ETNA_BO_CACHED:
>>>> +		prot = PAGE_KERNEL;
>>>> +		break;
>>>> +	case ETNA_BO_UNCACHED:
>>>> +		prot = pgprot_noncached(PAGE_KERNEL);
>>>> +		break;
>>>> +	case ETNA_BO_WC:
>>>> +	default:
>>>> +		prot = pgprot_writecombine(PAGE_KERNEL);
>>>> +	}
>>>> +
>>>> +	return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
>>>>    }
>>>>    
>>>>    static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
>>>> @@ -545,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>>>    static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>>>    	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>>>>    {
>>>> +	struct etnaviv_drm_private *priv = dev->dev_private;
>>>>    	struct etnaviv_gem_object *etnaviv_obj;
>>>>    	unsigned sz = sizeof(*etnaviv_obj);
>>>>    	bool valid = true;
>>>> @@ -555,6 +569,10 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>>>    	case ETNA_BO_CACHED:
>>>>    	case ETNA_BO_WC:
>>>>    		break;
>>>> +	case ETNA_BO_CACHED_COHERENT:
>>>> +		if (priv->has_cached_coherent)
>>>> +			break;
>>>> +		fallthrough;
>>>>    	default:
>>>>    		valid = false;
>>>>    	}
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> index 3524b5811682..671d91d8f1c6 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> @@ -112,11 +112,18 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
>>>>    struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>>>>    	struct dma_buf_attachment *attach, struct sg_table *sgt)
>>>>    {
>>>> +	struct etnaviv_drm_private *priv = dev->dev_private;
>>>>    	struct etnaviv_gem_object *etnaviv_obj;
>>>>    	size_t size = PAGE_ALIGN(attach->dmabuf->size);
>>>> +	u32 cache_flags;
>>>>    	int ret, npages;
>>>>    
>>>> -	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
>>>> +	if (priv->has_cached_coherent)
>>>> +		cache_flags = ETNA_BO_CACHED_COHERENT;
>>>> +	else
>>>> +		cache_flags = ETNA_BO_WC;
>>>> +
>>>> +	ret = etnaviv_gem_new_private(dev, size, cache_flags,
>>>>    				      &etnaviv_gem_prime_ops, &etnaviv_obj);
>>>>    	if (ret < 0)
>>>>    		return ERR_PTR(ret);
>>>> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
>>>> index af024d90453d..474b0db286de 100644
>>>> --- a/include/uapi/drm/etnaviv_drm.h
>>>> +++ b/include/uapi/drm/etnaviv_drm.h
>>>> @@ -90,13 +90,14 @@ struct drm_etnaviv_param {
>>>>     * GEM buffers:
>>>>     */
>>>>    
>>>> -#define ETNA_BO_CACHE_MASK   0x000f0000
>>>> +#define ETNA_BO_CACHE_MASK              0x000f0000
>>>>    /* cache modes */
>>>> -#define ETNA_BO_CACHED       0x00010000
>>>> -#define ETNA_BO_WC           0x00020000
>>>> -#define ETNA_BO_UNCACHED     0x00040000
>>>> +#define ETNA_BO_CACHED                  0x00010000
>>>> +#define ETNA_BO_WC                      0x00020000
>>>> +#define ETNA_BO_UNCACHED                0x00040000
>>>> +#define ETNA_BO_CACHED_COHERENT         0x00080000
>>>>    /* map flags */
>>>> -#define ETNA_BO_FORCE_MMU    0x00100000
>>>> +#define ETNA_BO_FORCE_MMU               0x00100000
>>>>    
>>>>    struct drm_etnaviv_gem_new {
>>>>    	__u64 size;           /* in */

-- 
Jingfeng

Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo

[Sui Jingfeng]

Hi,

On 2023/6/1 01:53, Lucas Stach wrote:
> Am Donnerstag, dem 01.06.2023 um 01:29 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/6/1 00:33, Lucas Stach wrote:
>>> Hi Sui Jingfeng,
>>>
>>> Am Mittwoch, dem 31.05.2023 um 00:06 +0800 schrieb Sui Jingfeng:
>>>> cached system RAM is coherent on loongson CPUs, and the GPU and DC allways
>>>> snoop the CPU's cache. write-combine caching property is not suitiable for
>>>> us.
>>>>
>>> As previously mentioned in the Mesa MR, I don't think this is the right
>>> approach.
>>>
>>> ETNA_BO_CACHED already looks coherent to userspace, as all accesses are
>>> bracketed via the ETNAVIV_GEM_CPU_PREP and ETNAVIV_GEM_CPU_FINI ioctls,
>>> which will do the necessary cache maintenance on platforms where device
>>> coherence isn't enforced by the hardware, so there is no need for a
>>> separate ETNA_BO_CACHED_COHERENT.
>> As far as I can see,  ETNA_BO_CACHED_COHERENT could probably help to
>> bypass the overhead of
>>
>> dma_sync_sgtable_for_cpu() and dma_sync_sgtable_for_device() brings to us.
>>
>>
>> I have tested long time ago, there no need call this function on our
>> platform.
>>
>> The glmark2 works as before if I comment out thoes two function.
>>
>> Are you serious, sir?
>>
> The dma_sync* functions are more or less no-ops when the device is
> marked as being coherent. ce
> instance, you might need to propagate the coherent property from the
> GPU core device to the virtual DRM device, along the lines of how we
> propagate other DMA properties from the GPU device to the DRM device in
> etnaviv_pdev_probe.
>
> Other than that things should just work with minimal overhead.
>>> Instead we just need a new ETNAVIV_PARAM to inform userspace about
>>> hardware cache coherence being available for a specific GPU core,
>> Ok, let me think about for a while how to implement this.
>>
> Simple: add new ETNAVIV_PARAM_GPU_COHERENT to
> include/uapi/drm/etnaviv_drm.h, return the result from
> dev_is_dma_coherent in etnaviv_gpu_get_param().
Okay, agree
>> But How about we merge this first, I create another patch to improve it
>>
>> with a roughly working base first? I'm just asking if the answer is No :-)
>>
> The answer is a firm no.
>
> This impacts UAPI, so there is no chance to ever get rid of any wrong
> decisions here, as any added UAPI needs to be supported indefinitely.
> I'm not signing up for maintaining something I believe is implemented
> upside down.
>
> Please don't take this the wrong way: I'm pretty excited to see etnaviv
> used on more architectures and outside of the proven platform device
> paths, so I'm happy to assist in working out the design and help you
> get things merged in both kernel and Mesa. But I think we are still
> quite a few steps away from having things worked out enough to even
> think about merging those patchsets.
>
> Also please allow me to comment on the other patches of the series, so
> I can get a better understanding of your platform/integration, before
> sending another revision of those patches.

I go to sleep yesterday.

Okay, this sound fine.

> Regards,
> Lucas
>
>>>    in
>>> which case the userspace driver should switch to preferring
>>> ETNA_BO_CACHED over ETNA_BO_WC.
>> Yeah,  ETNA_BO_CACHED is enough.
>>
>> ETNA_BO_CACHED_COHERENT is actually a special case of ETNA_BO_CACHED.
>>
>>> Regards,
>>> Lucas
>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/gpu/drm/etnaviv/etnaviv_drv.c       |  2 +-
>>>>    drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 22 +++++++++++++++++++--
>>>>    drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c |  9 ++++++++-
>>>>    include/uapi/drm/etnaviv_drm.h              | 11 ++++++-----
>>>>    4 files changed, 35 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> index 052f745cecc0..2816c654c023 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> @@ -274,7 +274,7 @@ static int etnaviv_ioctl_gem_new(struct drm_device *dev, void *data,
>>>>    	struct drm_etnaviv_gem_new *args = data;
>>>>    
>>>>    	if (args->flags & ~(ETNA_BO_CACHED | ETNA_BO_WC | ETNA_BO_UNCACHED |
>>>> -			    ETNA_BO_FORCE_MMU))
>>>> +			    ETNA_BO_CACHED_COHERENT | ETNA_BO_FORCE_MMU))
>>>>    		return -EINVAL;
>>>>    
>>>>    	return etnaviv_gem_new_handle(dev, file, args->size,
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> index b5f73502e3dd..d8b559bd33d3 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> @@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
>>>>    static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>>>>    {
>>>>    	struct page **pages;
>>>> +	pgprot_t prot;
>>>>    
>>>>    	lockdep_assert_held(&obj->lock);
>>>>    
>>>> @@ -350,8 +351,20 @@ static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
>>>>    	if (IS_ERR(pages))
>>>>    		return NULL;
>>>>    
>>>> -	return vmap(pages, obj->base.size >> PAGE_SHIFT,
>>>> -			VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>>>> +	switch (obj->flags) {
>>>> +	case ETNA_BO_CACHED_COHERENT:
>>>> +	case ETNA_BO_CACHED:
>>>> +		prot = PAGE_KERNEL;
>>>> +		break;
>>>> +	case ETNA_BO_UNCACHED:
>>>> +		prot = pgprot_noncached(PAGE_KERNEL);
>>>> +		break;
>>>> +	case ETNA_BO_WC:
>>>> +	default:
>>>> +		prot = pgprot_writecombine(PAGE_KERNEL);
>>>> +	}
>>>> +
>>>> +	return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
>>>>    }
>>>>    
>>>>    static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
>>>> @@ -545,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>>>    static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>>>    	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>>>>    {
>>>> +	struct etnaviv_drm_private *priv = dev->dev_private;
>>>>    	struct etnaviv_gem_object *etnaviv_obj;
>>>>    	unsigned sz = sizeof(*etnaviv_obj);
>>>>    	bool valid = true;
>>>> @@ -555,6 +569,10 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>>>    	case ETNA_BO_CACHED:
>>>>    	case ETNA_BO_WC:
>>>>    		break;
>>>> +	case ETNA_BO_CACHED_COHERENT:
>>>> +		if (priv->has_cached_coherent)
>>>> +			break;
>>>> +		fallthrough;
>>>>    	default:
>>>>    		valid = false;
>>>>    	}
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> index 3524b5811682..671d91d8f1c6 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>>>> @@ -112,11 +112,18 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
>>>>    struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>>>>    	struct dma_buf_attachment *attach, struct sg_table *sgt)
>>>>    {
>>>> +	struct etnaviv_drm_private *priv = dev->dev_private;
>>>>    	struct etnaviv_gem_object *etnaviv_obj;
>>>>    	size_t size = PAGE_ALIGN(attach->dmabuf->size);
>>>> +	u32 cache_flags;
>>>>    	int ret, npages;
>>>>    
>>>> -	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
>>>> +	if (priv->has_cached_coherent)
>>>> +		cache_flags = ETNA_BO_CACHED_COHERENT;
>>>> +	else
>>>> +		cache_flags = ETNA_BO_WC;
>>>> +
>>>> +	ret = etnaviv_gem_new_private(dev, size, cache_flags,
>>>>    				      &etnaviv_gem_prime_ops, &etnaviv_obj);
>>>>    	if (ret < 0)
>>>>    		return ERR_PTR(ret);
>>>> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
>>>> index af024d90453d..474b0db286de 100644
>>>> --- a/include/uapi/drm/etnaviv_drm.h
>>>> +++ b/include/uapi/drm/etnaviv_drm.h
>>>> @@ -90,13 +90,14 @@ struct drm_etnaviv_param {
>>>>     * GEM buffers:
>>>>     */
>>>>    
>>>> -#define ETNA_BO_CACHE_MASK   0x000f0000
>>>> +#define ETNA_BO_CACHE_MASK              0x000f0000
>>>>    /* cache modes */
>>>> -#define ETNA_BO_CACHED       0x00010000
>>>> -#define ETNA_BO_WC           0x00020000
>>>> -#define ETNA_BO_UNCACHED     0x00040000
>>>> +#define ETNA_BO_CACHED                  0x00010000
>>>> +#define ETNA_BO_WC                      0x00020000
>>>> +#define ETNA_BO_UNCACHED                0x00040000
>>>> +#define ETNA_BO_CACHED_COHERENT         0x00080000
>>>>    /* map flags */
>>>> -#define ETNA_BO_FORCE_MMU    0x00100000
>>>> +#define ETNA_BO_FORCE_MMU               0x00100000
>>>>    
>>>>    struct drm_etnaviv_gem_new {
>>>>    	__u64 size;           /* in */

-- 
Jingfeng

Re: [PATCH v6 2/6] drm/etnaviv: add a dedicated function to get various clocks

[Sui Jingfeng]

Hi,

On 2023/6/1 02:07, Lucas Stach wrote:
>> +static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
>> +{
>> +	struct device *dev = gpu->dev;
>> +
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
>> +	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
>> +	DBG("clk_reg: %p", gpu->clk_reg);
>> +	if (IS_ERR(gpu->clk_reg))
>> +		return PTR_ERR(gpu->clk_reg);
>> +
>> +	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
>> +	DBG("clk_bus: %p", gpu->clk_bus);
>> +	if (IS_ERR(gpu->clk_bus))
>> +		return PTR_ERR(gpu->clk_bus);
>> +
>> +	gpu->clk_core = devm_clk_get(dev, "core");
>> +	DBG("clk_core: %p", gpu->clk_core);
>> +	if (IS_ERR(gpu->clk_core))
>> +		return PTR_ERR(gpu->clk_core);
>> +	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>> +
>> +	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
>> +	DBG("clk_shader: %p", gpu->clk_shader);
>> +	if (IS_ERR(gpu->clk_shader))
>> +		return PTR_ERR(gpu->clk_shader);
>> +	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>> +
>> +	return 0;
>> +}
>> +
>>   static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>>   {
>>   	int ret;
>>   
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
> I don't see why this would be needed?
I have just tested, this do not needed.
> If your platform doesn't provide
> CONFIG_HAVE_CLK all those functions should be successful no-ops, so
> there is no need to special case this in the driver.

My platform do enable CONFIG_HAVE_CLK,

for ls3a5000 + ls7a1000, my system do not provide device tree support,

that's is to say, there is no DT support.


For ls3a4000 + ls7a1000 platform, the system has DT support, but don't 
has CLK drivers implement toward the clock tree.

typically, our platform's firmware will do such thing(setting a default 
working frequency).


When I first saw etnaviv, I'm also astonishing.

I don't know why there so much clock controllable.

As far as I can understand, my system/hardware have only one clock,

It shall corresponding to the core clk.

> Or does your platform in fact provide a clk subsystem, just the GPU
> clocks are managed by it?
>
> Also all those functions are fine with being called on a NULL clk, so
> shouldn't it be enough to simply avoid calling etnaviv_gpu_clk_get() in
> the PCI device case?
>
> Regards,
> Lucas
>
-- 
Jingfeng

Re: [PATCH v6 2/6] drm/etnaviv: add a dedicated function to get various clocks

[Sui Jingfeng]

Hi,

On 2023/6/1 02:07, Lucas Stach wrote:
>> +static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
>> +{
>> +	struct device *dev = gpu->dev;
>> +
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
>> +	gpu->clk_reg = devm_clk_get_optional(dev, "reg");
>> +	DBG("clk_reg: %p", gpu->clk_reg);
>> +	if (IS_ERR(gpu->clk_reg))
>> +		return PTR_ERR(gpu->clk_reg);
>> +
>> +	gpu->clk_bus = devm_clk_get_optional(dev, "bus");
>> +	DBG("clk_bus: %p", gpu->clk_bus);
>> +	if (IS_ERR(gpu->clk_bus))
>> +		return PTR_ERR(gpu->clk_bus);
>> +
>> +	gpu->clk_core = devm_clk_get(dev, "core");
>> +	DBG("clk_core: %p", gpu->clk_core);
>> +	if (IS_ERR(gpu->clk_core))
>> +		return PTR_ERR(gpu->clk_core);
>> +	gpu->base_rate_core = clk_get_rate(gpu->clk_core);
>> +
>> +	gpu->clk_shader = devm_clk_get_optional(dev, "shader");
>> +	DBG("clk_shader: %p", gpu->clk_shader);
>> +	if (IS_ERR(gpu->clk_shader))
>> +		return PTR_ERR(gpu->clk_shader);
>> +	gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>> +
>> +	return 0;
>> +}
>> +
>>   static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
>>   {
>>   	int ret;
>>   
>> +	if (gpu->no_clk)
>> +		return 0;
>> +
> I don't see why this would be needed?
I have just tested, this do not needed.
> If your platform doesn't provide
> CONFIG_HAVE_CLK all those functions should be successful no-ops, so
> there is no need to special case this in the driver.

My platform do enable CONFIG_HAVE_CLK,

for ls3a5000 + ls7a1000, my system do not provide device tree support,

that's is to say, there is no DT support.


For ls3a4000 + ls7a1000 platform, the system has DT support, but don't 
has CLK drivers implement toward the clock tree.

typically, our platform's firmware will do such thing(setting a default 
working frequency).


When I first saw etnaviv, I'm also astonishing.

I don't know why there so much clock controllable.

As far as I can understand, my system/hardware have only one clock,

It shall corresponding to the core clk.

> Or does your platform in fact provide a clk subsystem, just the GPU
> clocks are managed by it?
>
> Also all those functions are fine with being called on a NULL clk, so
> shouldn't it be enough to simply avoid calling etnaviv_gpu_clk_get() in
> the PCI device case?
>
> Regards,
> Lucas
>
-- 
Jingfeng