[6/6] drm/msm/gpu: add ocmem init/cleanup functions

Submitted by Brian Masney on June 16, 2019, 1:29 p.m.

Details

Message ID 20190616132930.6942-7-masneyb@onstation.org
State New
Headers show
Series "qcom: add OCMEM support" ( rev: 1 ) in Freedreno

Not browsing as part of any series.

Commit Message

Brian Masney June 16, 2019, 1:29 p.m.
The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
now that OCMEM support is upstream.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 33 +++++++-------------
 drivers/gpu/drm/msm/adreno/a3xx_gpu.h   |  3 +-
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 30 ++++++------------
 drivers/gpu/drm/msm/adreno/a4xx_gpu.h   |  3 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 ++++++
 6 files changed, 74 insertions(+), 46 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index c3b4bc6e4155..72720bb2aca1 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -17,10 +17,6 @@ 
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifdef CONFIG_MSM_OCMEM
-#  include <mach/ocmem.h>
-#endif
-
 #include "a3xx_gpu.h"
 
 #define A3XX_INT0_MASK \
@@ -206,9 +202,9 @@  static int a3xx_hw_init(struct msm_gpu *gpu)
 		gpu_write(gpu, REG_A3XX_RBBM_GPR0_CTL, 0x00000000);
 
 	/* Set the OCMEM base address for A330, etc */
-	if (a3xx_gpu->ocmem_hdl) {
+	if (a3xx_gpu->ocmem.hdl) {
 		gpu_write(gpu, REG_A3XX_RB_GMEM_BASE_ADDR,
-			(unsigned int)(a3xx_gpu->ocmem_base >> 14));
+			(unsigned int)(a3xx_gpu->ocmem.base >> 14));
 	}
 
 	/* Turn on performance counters: */
@@ -329,10 +325,7 @@  static void a3xx_destroy(struct msm_gpu *gpu)
 
 	adreno_gpu_cleanup(adreno_gpu);
 
-#ifdef CONFIG_MSM_OCMEM
-	if (a3xx_gpu->ocmem_base)
-		ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
-#endif
+	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
 
 	kfree(a3xx_gpu);
 }
@@ -507,17 +500,10 @@  struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
 
 	/* if needed, allocate gmem: */
 	if (adreno_is_a330(adreno_gpu)) {
-#ifdef CONFIG_MSM_OCMEM
-		/* TODO this is different/missing upstream: */
-		struct ocmem_buf *ocmem_hdl =
-				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
-
-		a3xx_gpu->ocmem_hdl = ocmem_hdl;
-		a3xx_gpu->ocmem_base = ocmem_hdl->addr;
-		adreno_gpu->gmem = ocmem_hdl->len;
-		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
-				a3xx_gpu->ocmem_base);
-#endif
+		ret = adreno_gpu_ocmem_init(&adreno_gpu->base.pdev->dev,
+					    adreno_gpu, &a3xx_gpu->ocmem);
+		if (ret)
+			goto fail;
 	}
 
 	if (!gpu->aspace) {
@@ -530,11 +516,14 @@  struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
 		 */
 		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
 		ret = -ENXIO;
-		goto fail;
+		goto fail_cleanup_ocmem;
 	}
 
 	return gpu;
 
+fail_cleanup_ocmem:
+	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
+
 fail:
 	if (a3xx_gpu)
 		a3xx_destroy(&a3xx_gpu->base.base);
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
index ab60dc9e344e..727c34f38f9e 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
@@ -30,8 +30,7 @@  struct a3xx_gpu {
 	struct adreno_gpu base;
 
 	/* if OCMEM is used for GMEM: */
-	uint32_t ocmem_base;
-	void *ocmem_hdl;
+	struct adreno_ocmem ocmem;
 };
 #define to_a3xx_gpu(x) container_of(x, struct a3xx_gpu, base)
 
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index ab2b752566d8..b8f825107796 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -2,9 +2,6 @@ 
 /* Copyright (c) 2014 The Linux Foundation. All rights reserved.
  */
 #include "a4xx_gpu.h"
-#ifdef CONFIG_MSM_OCMEM
-#  include <soc/qcom/ocmem.h>
-#endif
 
 #define A4XX_INT0_MASK \
 	(A4XX_INT0_RBBM_AHB_ERROR |        \
@@ -188,7 +185,7 @@  static int a4xx_hw_init(struct msm_gpu *gpu)
 			(1 << 30) | 0xFFFF);
 
 	gpu_write(gpu, REG_A4XX_RB_GMEM_BASE_ADDR,
-			(unsigned int)(a4xx_gpu->ocmem_base >> 14));
+			(unsigned int)(a4xx_gpu->ocmem.base >> 14));
 
 	/* Turn on performance counters: */
 	gpu_write(gpu, REG_A4XX_RBBM_PERFCTR_CTL, 0x01);
@@ -318,10 +315,7 @@  static void a4xx_destroy(struct msm_gpu *gpu)
 
 	adreno_gpu_cleanup(adreno_gpu);
 
-#ifdef CONFIG_MSM_OCMEM
-	if (a4xx_gpu->ocmem_base)
-		ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
-#endif
+	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
 
 	kfree(a4xx_gpu);
 }
@@ -578,17 +572,10 @@  struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
 
 	/* if needed, allocate gmem: */
 	if (adreno_is_a4xx(adreno_gpu)) {
-#ifdef CONFIG_MSM_OCMEM
-		/* TODO this is different/missing upstream: */
-		struct ocmem_buf *ocmem_hdl =
-				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
-
-		a4xx_gpu->ocmem_hdl = ocmem_hdl;
-		a4xx_gpu->ocmem_base = ocmem_hdl->addr;
-		adreno_gpu->gmem = ocmem_hdl->len;
-		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
-				a4xx_gpu->ocmem_base);
-#endif
+		ret = adreno_gpu_ocmem_init(dev->dev, adreno_gpu,
+					    &a4xx_gpu->ocmem);
+		if (ret)
+			goto fail;
 	}
 
 	if (!gpu->aspace) {
@@ -601,11 +588,14 @@  struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
 		 */
 		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
 		ret = -ENXIO;
-		goto fail;
+		goto fail_cleanup_ocmem;
 	}
 
 	return gpu;
 
+fail_cleanup_ocmem:
+	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
+
 fail:
 	if (a4xx_gpu)
 		a4xx_destroy(&a4xx_gpu->base.base);
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
index d506311ee240..a01448cba2ea 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
@@ -16,8 +16,7 @@  struct a4xx_gpu {
 	struct adreno_gpu base;
 
 	/* if OCMEM is used for GMEM: */
-	uint32_t ocmem_base;
-	void *ocmem_hdl;
+	struct adreno_ocmem ocmem;
 };
 #define to_a4xx_gpu(x) container_of(x, struct a4xx_gpu, base)
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 6f7f4114afcf..e0a9409c8a32 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -29,6 +29,10 @@ 
 #include "msm_gem.h"
 #include "msm_mmu.h"
 
+#ifdef CONFIG_QCOM_OCMEM
+#  include <soc/qcom/ocmem.h>
+#endif
+
 static bool zap_available = true;
 
 static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
@@ -897,6 +901,43 @@  static int adreno_get_pwrlevels(struct device *dev,
 	return 0;
 }
 
+int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
+			  struct adreno_ocmem *adreno_ocmem)
+{
+#ifdef CONFIG_QCOM_OCMEM
+	struct ocmem_buf *ocmem_hdl;
+	struct ocmem *ocmem;
+
+	ocmem = of_get_ocmem(dev);
+	if (!ocmem) {
+		/* This is an optional property so return success. */
+		return 0;
+	}
+
+	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
+	if (IS_ERR(ocmem_hdl))
+		return PTR_ERR(ocmem_hdl);
+
+	adreno_ocmem->ocmem = ocmem;
+	adreno_ocmem->base = ocmem_hdl->addr;
+	adreno_ocmem->hdl = ocmem_hdl;
+	adreno_gpu->gmem = ocmem_hdl->len;
+	DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
+	    adreno_ocmem->base);
+#endif
+
+	return 0;
+}
+
+void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
+{
+#ifdef CONFIG_QCOM_OCMEM
+	if (adreno_ocmem->base)
+		ocmem_free(adreno_ocmem->ocmem, OCMEM_GRAPHICS,
+			   adreno_ocmem->hdl);
+#endif
+}
+
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 		struct adreno_gpu *adreno_gpu,
 		const struct adreno_gpu_funcs *funcs, int nr_rings)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 0925606ec9b5..1cd11570323b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -136,6 +136,12 @@  struct adreno_gpu {
 };
 #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
 
+struct adreno_ocmem {
+	struct ocmem *ocmem;
+	uint32_t base;
+	void *hdl;
+};
+
 /* platform config data (ie. from DT, or pdata) */
 struct adreno_platform_config {
 	struct adreno_rev rev;
@@ -241,6 +247,10 @@  void adreno_dump(struct msm_gpu *gpu);
 void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords);
 struct msm_ringbuffer *adreno_active_ring(struct msm_gpu *gpu);
 
+int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
+			  struct adreno_ocmem *ocmem);
+void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *ocmem);
+
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs,
 		int nr_rings);

Comments

On Sun 16 Jun 06:29 PDT 2019, Brian Masney wrote:

> The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
> that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
> and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
> need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
> now that OCMEM support is upstream.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 33 +++++++-------------
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 30 ++++++------------
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 ++++++
>  6 files changed, 74 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index c3b4bc6e4155..72720bb2aca1 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -17,10 +17,6 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <mach/ocmem.h>
> -#endif
> -
>  #include "a3xx_gpu.h"
>  
>  #define A3XX_INT0_MASK \
> @@ -206,9 +202,9 @@ static int a3xx_hw_init(struct msm_gpu *gpu)
>  		gpu_write(gpu, REG_A3XX_RBBM_GPR0_CTL, 0x00000000);
>  
>  	/* Set the OCMEM base address for A330, etc */
> -	if (a3xx_gpu->ocmem_hdl) {
> +	if (a3xx_gpu->ocmem.hdl) {
>  		gpu_write(gpu, REG_A3XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a3xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a3xx_gpu->ocmem.base >> 14));

This blindly requires that the ocmem allocator will return entries
allocated to 16kB. Please ensure that a future implementation of the
actual ocmem allocator maintains this (comments? checks?). 

>  	}
>  
>  	/* Turn on performance counters: */
> @@ -329,10 +325,7 @@ static void a3xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a3xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
>  
>  	kfree(a3xx_gpu);
>  }
> @@ -507,17 +500,10 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a330(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a3xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a3xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a3xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(&adreno_gpu->base.pdev->dev,
> +					    adreno_gpu, &a3xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -530,11 +516,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
> +
>  fail:
>  	if (a3xx_gpu)
>  		a3xx_destroy(&a3xx_gpu->base.base);
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> index ab60dc9e344e..727c34f38f9e 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> @@ -30,8 +30,7 @@ struct a3xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a3xx_gpu(x) container_of(x, struct a3xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index ab2b752566d8..b8f825107796 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -2,9 +2,6 @@
>  /* Copyright (c) 2014 The Linux Foundation. All rights reserved.
>   */
>  #include "a4xx_gpu.h"
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <soc/qcom/ocmem.h>
> -#endif
>  
>  #define A4XX_INT0_MASK \
>  	(A4XX_INT0_RBBM_AHB_ERROR |        \
> @@ -188,7 +185,7 @@ static int a4xx_hw_init(struct msm_gpu *gpu)
>  			(1 << 30) | 0xFFFF);
>  
>  	gpu_write(gpu, REG_A4XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a4xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a4xx_gpu->ocmem.base >> 14));
>  
>  	/* Turn on performance counters: */
>  	gpu_write(gpu, REG_A4XX_RBBM_PERFCTR_CTL, 0x01);
> @@ -318,10 +315,7 @@ static void a4xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a4xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
>  
>  	kfree(a4xx_gpu);
>  }
> @@ -578,17 +572,10 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a4xx(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a4xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a4xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a4xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(dev->dev, adreno_gpu,
> +					    &a4xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -601,11 +588,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
> +
>  fail:
>  	if (a4xx_gpu)
>  		a4xx_destroy(&a4xx_gpu->base.base);
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> index d506311ee240..a01448cba2ea 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> @@ -16,8 +16,7 @@ struct a4xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a4xx_gpu(x) container_of(x, struct a4xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 6f7f4114afcf..e0a9409c8a32 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -29,6 +29,10 @@
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
>  
> +#ifdef CONFIG_QCOM_OCMEM
> +#  include <soc/qcom/ocmem.h>
> +#endif

This file exists (after the previous patch), so no need to make its
inclusion conditional.

> +
>  static bool zap_available = true;
>  
>  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
>  	return 0;
>  }
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM

No need to make this conditional.

> +	struct ocmem_buf *ocmem_hdl;
> +	struct ocmem *ocmem;
> +
> +	ocmem = of_get_ocmem(dev);
> +	if (!ocmem) {
> +		/* This is an optional property so return success. */
> +		return 0;
> +	}
> +
> +	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
> +	if (IS_ERR(ocmem_hdl))
> +		return PTR_ERR(ocmem_hdl);
> +
> +	adreno_ocmem->ocmem = ocmem;
> +	adreno_ocmem->base = ocmem_hdl->addr;
> +	adreno_ocmem->hdl = ocmem_hdl;
> +	adreno_gpu->gmem = ocmem_hdl->len;
> +	DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> +	    adreno_ocmem->base);
> +#endif
> +
> +	return 0;
> +}
> +
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM
> +	if (adreno_ocmem->base)

It would be nice to have ocmem_free() accept NULL, similar to kfree() et
al.

> +		ocmem_free(adreno_ocmem->ocmem, OCMEM_GRAPHICS,
> +			   adreno_ocmem->hdl);
> +#endif
> +}
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *adreno_gpu,
>  		const struct adreno_gpu_funcs *funcs, int nr_rings)
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 0925606ec9b5..1cd11570323b 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -136,6 +136,12 @@ struct adreno_gpu {
>  };
>  #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
>  
> +struct adreno_ocmem {
> +	struct ocmem *ocmem;
> +	uint32_t base;

By ocmem being physically fixed this is sufficient, but unsigned long is
a nicer type for carrying memory addresses.

Regards,
Bjorn

> +	void *hdl;
> +};
> +
>  /* platform config data (ie. from DT, or pdata) */
>  struct adreno_platform_config {
>  	struct adreno_rev rev;
> @@ -241,6 +247,10 @@ void adreno_dump(struct msm_gpu *gpu);
>  void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords);
>  struct msm_ringbuffer *adreno_active_ring(struct msm_gpu *gpu);
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *ocmem);
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *ocmem);
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs,
>  		int nr_rings);
> -- 
> 2.20.1
>
Hi Bjorn,

On Sun, Jun 16, 2019 at 11:06:33AM -0700, Bjorn Andersson wrote:
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 6f7f4114afcf..e0a9409c8a32 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -29,6 +29,10 @@
> >  #include "msm_gem.h"
> >  #include "msm_mmu.h"
> >  
> > +#ifdef CONFIG_QCOM_OCMEM
> > +#  include <soc/qcom/ocmem.h>
> > +#endif
> 
> This file exists (after the previous patch), so no need to make its
> inclusion conditional.
> 
> > +
> >  static bool zap_available = true;
> >  
> >  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> > @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
> >  	return 0;
> >  }
> >  
> > +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> > +			  struct adreno_ocmem *adreno_ocmem)
> > +{
> > +#ifdef CONFIG_QCOM_OCMEM
> 
> No need to make this conditional.

I have these #ifdefs for the a5xx and a6xx GPUs that don't have ocmem
in the SoC. Without the #ifdefs, those systems would need to have the
ocmem driver in their kernel.

Thanks for the quick review on the patch set!

Brian
On Sun 16 Jun 17:18 PDT 2019, Brian Masney wrote:

> Hi Bjorn,
> 
> On Sun, Jun 16, 2019 at 11:06:33AM -0700, Bjorn Andersson wrote:
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > index 6f7f4114afcf..e0a9409c8a32 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > @@ -29,6 +29,10 @@
> > >  #include "msm_gem.h"
> > >  #include "msm_mmu.h"
> > >  
> > > +#ifdef CONFIG_QCOM_OCMEM
> > > +#  include <soc/qcom/ocmem.h>
> > > +#endif
> > 
> > This file exists (after the previous patch), so no need to make its
> > inclusion conditional.
> > 
> > > +
> > >  static bool zap_available = true;
> > >  
> > >  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> > > @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
> > >  	return 0;
> > >  }
> > >  
> > > +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> > > +			  struct adreno_ocmem *adreno_ocmem)
> > > +{
> > > +#ifdef CONFIG_QCOM_OCMEM
> > 
> > No need to make this conditional.
> 
> I have these #ifdefs for the a5xx and a6xx GPUs that don't have ocmem
> in the SoC. Without the #ifdefs, those systems would need to have the
> ocmem driver in their kernel.
> 

In order to provide the means for compiling a kernel for a[56]xx without
having to compile ocmem you should move these #ifdef to the ocmem
header file and provide static inline dummies for the case when it's
not.

(and use #if IS_ENABLED(CONFIG_FOO))

Don't forget to add
	depends on QCOM_OCMEM || QCOM_OCMEM=n

to the DRM_MSM config option, to allow the driver pair to be selected in
all possible ways.

> Thanks for the quick review on the patch set!
> 

Regards,
Bjorn
On Sun, Jun 16, 2019 at 09:29:30AM -0400, Brian Masney wrote:
> The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
> that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
> and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
> need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
> now that OCMEM support is upstream.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 33 +++++++-------------
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 30 ++++++------------
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.h   |  3 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 ++++++
>  6 files changed, 74 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index c3b4bc6e4155..72720bb2aca1 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -17,10 +17,6 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <mach/ocmem.h>
> -#endif
> -
>  #include "a3xx_gpu.h"
>  
>  #define A3XX_INT0_MASK \
> @@ -206,9 +202,9 @@ static int a3xx_hw_init(struct msm_gpu *gpu)
>  		gpu_write(gpu, REG_A3XX_RBBM_GPR0_CTL, 0x00000000);
>  
>  	/* Set the OCMEM base address for A330, etc */
> -	if (a3xx_gpu->ocmem_hdl) {
> +	if (a3xx_gpu->ocmem.hdl) {
>  		gpu_write(gpu, REG_A3XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a3xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a3xx_gpu->ocmem.base >> 14));
>  	}
>  
>  	/* Turn on performance counters: */
> @@ -329,10 +325,7 @@ static void a3xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a3xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
>  
>  	kfree(a3xx_gpu);
>  }
> @@ -507,17 +500,10 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a330(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a3xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a3xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a3xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(&adreno_gpu->base.pdev->dev,
> +					    adreno_gpu, &a3xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -530,11 +516,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a3xx_gpu->ocmem);
> +
>  fail:
>  	if (a3xx_gpu)
>  		a3xx_destroy(&a3xx_gpu->base.base);

a3xx_destroy is going to have to be able to cleanup the ocmem anyway, so you
might as well stick it in there instead of having a second goto label.

> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> index ab60dc9e344e..727c34f38f9e 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.h
> @@ -30,8 +30,7 @@ struct a3xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a3xx_gpu(x) container_of(x, struct a3xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index ab2b752566d8..b8f825107796 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -2,9 +2,6 @@
>  /* Copyright (c) 2014 The Linux Foundation. All rights reserved.
>   */
>  #include "a4xx_gpu.h"
> -#ifdef CONFIG_MSM_OCMEM
> -#  include <soc/qcom/ocmem.h>
> -#endif
>  
>  #define A4XX_INT0_MASK \
>  	(A4XX_INT0_RBBM_AHB_ERROR |        \
> @@ -188,7 +185,7 @@ static int a4xx_hw_init(struct msm_gpu *gpu)
>  			(1 << 30) | 0xFFFF);
>  
>  	gpu_write(gpu, REG_A4XX_RB_GMEM_BASE_ADDR,
> -			(unsigned int)(a4xx_gpu->ocmem_base >> 14));
> +			(unsigned int)(a4xx_gpu->ocmem.base >> 14));
>  
>  	/* Turn on performance counters: */
>  	gpu_write(gpu, REG_A4XX_RBBM_PERFCTR_CTL, 0x01);
> @@ -318,10 +315,7 @@ static void a4xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a4xx_gpu->ocmem_base)
> -		ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
> -#endif
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
>  
>  	kfree(a4xx_gpu);
>  }
> @@ -578,17 +572,10 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  
>  	/* if needed, allocate gmem: */
>  	if (adreno_is_a4xx(adreno_gpu)) {
> -#ifdef CONFIG_MSM_OCMEM
> -		/* TODO this is different/missing upstream: */
> -		struct ocmem_buf *ocmem_hdl =
> -				ocmem_allocate(OCMEM_GRAPHICS, adreno_gpu->gmem);
> -
> -		a4xx_gpu->ocmem_hdl = ocmem_hdl;
> -		a4xx_gpu->ocmem_base = ocmem_hdl->addr;
> -		adreno_gpu->gmem = ocmem_hdl->len;
> -		DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> -				a4xx_gpu->ocmem_base);
> -#endif
> +		ret = adreno_gpu_ocmem_init(dev->dev, adreno_gpu,
> +					    &a4xx_gpu->ocmem);
> +		if (ret)
> +			goto fail;
>  	}
>  
>  	if (!gpu->aspace) {
> @@ -601,11 +588,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>  		 */
>  		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
>  		ret = -ENXIO;
> -		goto fail;
> +		goto fail_cleanup_ocmem;
>  	}
>  
>  	return gpu;
>  
> +fail_cleanup_ocmem:
> +	adreno_gpu_ocmem_cleanup(&a4xx_gpu->ocmem);
> +
>  fail:
>  	if (a4xx_gpu)
>  		a4xx_destroy(&a4xx_gpu->base.base);

And same.

> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> index d506311ee240..a01448cba2ea 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.h
> @@ -16,8 +16,7 @@ struct a4xx_gpu {
>  	struct adreno_gpu base;
>  
>  	/* if OCMEM is used for GMEM: */
> -	uint32_t ocmem_base;
> -	void *ocmem_hdl;
> +	struct adreno_ocmem ocmem;
>  };
>  #define to_a4xx_gpu(x) container_of(x, struct a4xx_gpu, base)
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 6f7f4114afcf..e0a9409c8a32 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -29,6 +29,10 @@
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
>  
> +#ifdef CONFIG_QCOM_OCMEM

You won't need a ifdef here if the rest of the support is merged too.

> +#  include <soc/qcom/ocmem.h>
> +#endif
> +
>  static bool zap_available = true;
>  
>  static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> @@ -897,6 +901,43 @@ static int adreno_get_pwrlevels(struct device *dev,
>  	return 0;
>  }
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM

Same. It should be assumed that the generic ocmem functions will be properly
stubbed. I suppose you have an argument for wanting to not compile in this
extra code if it isn't needed, but if so you'll want to #ifdef the entire
function(s) and use stubs in the include file instead.

> +	struct ocmem_buf *ocmem_hdl;
> +	struct ocmem *ocmem;
> +
> +	ocmem = of_get_ocmem(dev);
> +	if (!ocmem) {
> +		/* This is an optional property so return success. */
> +		return 0;
> +	}
> +
> +	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
> +	if (IS_ERR(ocmem_hdl))
> +		return PTR_ERR(ocmem_hdl);
> +
> +	adreno_ocmem->ocmem = ocmem;
> +	adreno_ocmem->base = ocmem_hdl->addr;
> +	adreno_ocmem->hdl = ocmem_hdl;
> +	adreno_gpu->gmem = ocmem_hdl->len;
> +	DBG("using %dK of OCMEM at 0x%08x", adreno_gpu->gmem / 1024,
> +	    adreno_ocmem->base);
> +#endif
> +
> +	return 0;
> +}
> +
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
> +{
> +#ifdef CONFIG_QCOM_OCMEM
> +	if (adreno_ocmem->base)
> +		ocmem_free(adreno_ocmem->ocmem, OCMEM_GRAPHICS,
> +			   adreno_ocmem->hdl);
> +#endif
> +}
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *adreno_gpu,
>  		const struct adreno_gpu_funcs *funcs, int nr_rings)
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 0925606ec9b5..1cd11570323b 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -136,6 +136,12 @@ struct adreno_gpu {
>  };
>  #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
>  
> +struct adreno_ocmem {
> +	struct ocmem *ocmem;
> +	uint32_t base;
> +	void *hdl;
> +};
> +
>  /* platform config data (ie. from DT, or pdata) */
>  struct adreno_platform_config {
>  	struct adreno_rev rev;
> @@ -241,6 +247,10 @@ void adreno_dump(struct msm_gpu *gpu);
>  void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords);
>  struct msm_ringbuffer *adreno_active_ring(struct msm_gpu *gpu);
>  
> +int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> +			  struct adreno_ocmem *ocmem);
> +void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *ocmem);
> +
>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs,
>  		int nr_rings);
> -- 
> 2.20.1
On Wed, Jun 19, 2019 at 12:15:26PM -0600, Jordan Crouse wrote:
> On Sun, Jun 16, 2019 at 09:29:30AM -0400, Brian Masney wrote:
> > The files a3xx_gpu.c and a4xx_gpu.c have ifdefs for the OCMEM support
> > that was missing upstream. Add two new functions (adreno_gpu_ocmem_init
> > and adreno_gpu_ocmem_cleanup) that removes some duplicated code. We also
> > need to change the ifdef check for CONFIG_MSM_OCMEM to CONFIG_QCOM_OCMEM
> > now that OCMEM support is upstream.

Sorry for reviewing v1 when there was a v2 in flight. That will teach me to not
keep up on my email. I think you caught most of this, but a few things I still
saw.

<snip>

Jordan