drm/msm/adreno: Move away from legacy revision matching

From: Rob Clark <[email protected]>

No real need to have marketing names in the kernel.

Signed-off-by Rob Clark <[email protected]>

---

drivers/gpu/drm/msm/adreno/adreno_gpu.c +2 -0

+	if (!gpu_name)
+		return -ENOMEM;
 
 	adreno_gpu_config.ioname = "kgsl_3d0_reg_memory";

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -1

index cf45007400c8..6830c3776c2d 100644
@@ -61,7 +61,6 @@ extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
 struct adreno_info {
 	struct adreno_rev rev;
 	uint32_t revn;
-	const char *name;
 	const char *fw[ADRENO_FW_MAX];
 	uint32_t gmem;
 	u64 quirks;

From: Rob Clark <[email protected]>

No real need to have marketing names in the kernel.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 24 ----------------------
  drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 13 +++++-------
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 -
  3 files changed, 5 insertions(+), 33 deletions(-)

From: Rob Clark <[email protected]>

Even in the ocmem case, the allocated ocmem buffer size should match the requested size.

Signed-off-by Rob Clark <[email protected]>

---

drivers/gpu/drm/msm/adreno/adreno_gpu.c +0 -1

 	return 0;
 }

@@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 
 	adreno_gpu->funcs = funcs;
 	adreno_gpu->info = adreno_info(config->rev);
-	adreno_gpu->gmem = adreno_gpu->info->gmem;
 	adreno_gpu->revn = adreno_gpu->info->revn;
 	adreno_gpu->rev = *rev;
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -1

index 6830c3776c2d..aaf09c642dc6 100644
@@ -77,7 +77,6 @@ struct adreno_gpu {
 	struct msm_gpu base;
 	struct adreno_rev rev;
 	const struct adreno_info *info;
-	uint32_t gmem;  /* actual gmem size */
 	uint32_t revn;  /* numeric revision name */
 	uint16_t speedbin;
 	const struct adreno_gpu_funcs *funcs;

Quoted 7 lines · Rob Clark

From: Rob Clark <[email protected]>

Even in the ocmem case, the allocated ocmem buffer size should match the requested size.

Signed-off-by Rob Clark <[email protected]>

---

[...]

drivers/gpu/drm/msm/adreno/adreno_gpu.c int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu, · line 1052

Quoted 2 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +2 -0

+
+	WARN_ON(ocmem_hdl->len != adreno_gpu->info->gmem);

I believe this should be an error condition. If the sizes are mismatched, best case scenario you get suboptimal perf and worst case scenario your system explodes.

Very nice cleanup though!

Konrad

Quoted 23 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +0 -1

 	return 0;
 }

@@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 
 	adreno_gpu->funcs = funcs;
 	adreno_gpu->info = adreno_info(config->rev);
-	adreno_gpu->gmem = adreno_gpu->info->gmem;
 	adreno_gpu->revn = adreno_gpu->info->revn;
 	adreno_gpu->rev = *rev;
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -1

index 6830c3776c2d..aaf09c642dc6 100644
@@ -77,7 +77,6 @@ struct adreno_gpu {
 	struct msm_gpu base;
 	struct adreno_rev rev;
 	const struct adreno_info *info;
-	uint32_t gmem;  /* actual gmem size */
 	uint32_t revn;  /* numeric revision name */
 	uint16_t speedbin;
 	const struct adreno_gpu_funcs *funcs;

From: Rob Clark <[email protected]>

Even in the ocmem case, the allocated ocmem buffer size should match the requested size.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a2xx_gpu.c      | 2 +-
  drivers/gpu/drm/msm/adreno/a5xx_gpu.c      | 2 +-
  drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 2 +-
  drivers/gpu/drm/msm/adreno/adreno_device.c | 1 -
  drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 8 ++++----
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    | 1 -
  6 files changed, 7 insertions(+), 9 deletions(-)

drivers/gpu/drm/msm/adreno/a2xx_gpu.c +1 -1

index c67089a7ebc1..50ee03bc94b4 100644
@@ -205,7 +205,7 @@ static int a2xx_hw_init(struct msm_gpu *gpu)
  		A2XX_MH_INTERRUPT_MASK_MMU_PAGE_FAULT);
 
  	for (i = 3; i <= 5; i++)
-		if ((SZ_16K << i) == adreno_gpu->gmem)
+		if ((SZ_16K << i) == adreno_gpu->info->gmem)
  			break;
  	gpu_write(gpu, REG_A2XX_RB_EDRAM_INFO, i);
 

drivers/gpu/drm/msm/adreno/a5xx_gpu.c +1 -1

index a99310b68793..f0803e94ebe5 100644
@@ -749,7 +749,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
  	gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MIN_LO, 0x00100000);
  	gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MIN_HI, 0x00000000);
  	gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_LO,
-		0x00100000 + adreno_gpu->gmem - 1);
+		0x00100000 + adreno_gpu->info->gmem - 1);
  	gpu_write(gpu, REG_A5XX_UCHE_GMEM_RANGE_MAX_HI, 0x00000000);
 
  	if (adreno_is_a506(adreno_gpu) || adreno_is_a508(adreno_gpu) ||

drivers/gpu/drm/msm/adreno/a6xx_gpu.c +1 -1

index b3ada1e7b598..edbade75020f 100644
@@ -1270,7 +1270,7 @@ static int hw_init(struct msm_gpu *gpu)
  		gpu_write64(gpu, REG_A6XX_UCHE_GMEM_RANGE_MIN, 0x00100000);
 
  		gpu_write64(gpu, REG_A6XX_UCHE_GMEM_RANGE_MAX,
-			0x00100000 + adreno_gpu->gmem - 1);
+			0x00100000 + adreno_gpu->info->gmem - 1);
  	}
 
  	gpu_write(gpu, REG_A6XX_UCHE_FILTER_CNTL, 0x804);

drivers/gpu/drm/msm/adreno/adreno_device.c +0 -0

index 5eba0ae5c9a7..326912284a95 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.c +4 -4

index e3cd9ff6ff1d..4f59682f585e 100644
@@ -320,7 +320,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
  		*value = adreno_gpu->info->revn;
  		return 0;
  	case MSM_PARAM_GMEM_SIZE:
-		*value = adreno_gpu->gmem;
+		*value = adreno_gpu->info->gmem;
  		return 0;
  	case MSM_PARAM_GMEM_BASE:
  		*value = !adreno_is_a650_family(adreno_gpu) ? 0x100000 : 0;

@@ -1041,14 +1041,15 @@ int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
  		return PTR_ERR(ocmem);
  	}
 
-	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->gmem);
+	ocmem_hdl = ocmem_allocate(ocmem, OCMEM_GRAPHICS, adreno_gpu->info->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;
+
+	WARN_ON(ocmem_hdl->len != adreno_gpu->info->gmem);
 
  	return 0;
  }

@@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 
  	adreno_gpu->funcs = funcs;
  	adreno_gpu->info = adreno_info(config->rev);
-	adreno_gpu->gmem = adreno_gpu->info->gmem;
  	adreno_gpu->revn = adreno_gpu->info->revn;
  	adreno_gpu->rev = *rev;
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -1

index 6830c3776c2d..aaf09c642dc6 100644
@@ -77,7 +77,6 @@ struct adreno_gpu {
  	struct msm_gpu base;
  	struct adreno_rev rev;
  	const struct adreno_info *info;
-	uint32_t gmem;  /* actual gmem size */
  	uint32_t revn;  /* numeric revision name */
  	uint16_t speedbin;
  	const struct adreno_gpu_funcs *funcs;

From: Rob Clark <[email protected]>

This just duplicates what is in adreno_info, and can cause confusion if used before it is set.

Signed-off-by Rob Clark <[email protected]>

---

drivers/gpu/drm/msm/adreno/adreno_gpu.h +7 -8

+	return gpu->info->revn == revn;
 }
 
 static inline bool adreno_has_gmu_wrapper(const struct adreno_gpu *gpu)

@@ -160,18 +158,16 @@ static inline bool adreno_has_gmu_wrapper(const struct adreno_gpu *gpu)
 
 static inline bool adreno_is_a2xx(const struct adreno_gpu *gpu)
 {
-	/* revn can be zero, but if not is set at same time as info */
-	WARN_ON_ONCE(!gpu->info);
-
-	return (gpu->revn < 300);
+	if (WARN_ON_ONCE(!gpu->info))
+		return false;
+	return (gpu->info->revn < 300);
 }
 
 static inline bool adreno_is_a20x(const struct adreno_gpu *gpu)
 {
-	/* revn can be zero, but if not is set at same time as info */
-	WARN_ON_ONCE(!gpu->info);
-
-	return (gpu->revn < 210);
+	if (WARN_ON_ONCE(!gpu->info))
+		return false;
+	return (gpu->info->revn < 210);
 }
 
 static inline bool adreno_is_a225(const struct adreno_gpu *gpu)

From: Rob Clark <[email protected]>

This just duplicates what is in adreno_info, and can cause confusion if used before it is set.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   |  2 --
  drivers/gpu/drm/msm/adreno/adreno_gpu.c |  1 -
  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 22 +++++++++-------------
  3 files changed, 9 insertions(+), 16 deletions(-)

From: Rob Clark <[email protected]>

Rather than just open coding a list of gpu-id matches.

Signed-off-by Rob Clark <[email protected]>

---

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 3 +--
 drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++++
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

drivers/gpu/drm/msm/adreno/a6xx_gpu.c +1 -2

index 5ba8b5aca502..6f8c4381fa4a 100644
@@ -2489,8 +2489,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 	/* Quirk data */
 	adreno_gpu->info = info;
 
-	if (adreno_is_a650(adreno_gpu) || adreno_is_a660_family(adreno_gpu))
-		adreno_gpu->base.hw_apriv = true;
+	adreno_gpu->base.hw_apriv = !!(info->quirks & ADRENO_QUIRK_HAS_HW_APRIV);
 
 	a6xx_llc_slices_init(pdev, a6xx_gpu);
 

drivers/gpu/drm/msm/adreno/adreno_device.c +4 -0

index 326912284a95..f469f951a907 100644
@@ -302,6 +302,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M + SZ_128K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a650_zap.mdt",
 		.hwcg = a650_hwcg,

@@ -315,6 +316,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M + SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a660_zap.mdt",
 		.hwcg = a660_hwcg,

@@ -327,6 +329,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.hwcg = a660_hwcg,
 		.address_space_size = SZ_16G,

@@ -350,6 +353,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_4M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a690_zap.mdt",
 		.hwcg = a690_hwcg,

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index d31e2d37c61b..a7c4a2c536e3 100644
@@ -32,6 +32,7 @@ enum {
 #define ADRENO_QUIRK_TWO_PASS_USE_WFI		BIT(0)
 #define ADRENO_QUIRK_FAULT_DETECT_MASK		BIT(1)
 #define ADRENO_QUIRK_LMLOADKILL_DISABLE		BIT(2)
+#define ADRENO_QUIRK_HAS_HW_APRIV		BIT(3)
 
 struct adreno_rev {
 	uint8_t  core;

From: Rob Clark <[email protected]>

Rather than just open coding a list of gpu-id matches.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 3 +--
  drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++++
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    | 1 +
  3 files changed, 6 insertions(+), 2 deletions(-)

From: Rob Clark <[email protected]>

It is better to explicitly list it. With the move to opaque chip-id's for future devices, we should avoid trying to infer things like generation from the numerical value.

Signed-off-by Rob Clark <[email protected]>

---

 drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++++++++++++++-------
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 2 files changed, 17 insertions(+), 7 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +16 -7

index f469f951a907..3c531da417b9 100644
@@ -256,6 +256,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 	}, {
 		.rev = ADRENO_REV(6, 1, 9, ANY_ID),

@@ -266,6 +267,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a615_zap.mdt",
 		.hwcg = a615_hwcg,

@@ -278,6 +280,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a630_zap.mdt",
 		.hwcg = a630_hwcg,

@@ -290,6 +293,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a640_zap.mdt",
 		.hwcg = a640_hwcg,

@@ -302,7 +306,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M + SZ_128K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a650_zap.mdt",
 		.hwcg = a650_hwcg,

@@ -316,7 +321,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M + SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a660_zap.mdt",
 		.hwcg = a660_hwcg,

@@ -329,7 +335,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.hwcg = a660_hwcg,
 		.address_space_size = SZ_16G,

@@ -342,6 +349,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_2M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a640_zap.mdt",
 		.hwcg = a640_hwcg,

@@ -353,7 +361,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_4M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a690_zap.mdt",
 		.hwcg = a690_hwcg,

@@ -565,9 +574,9 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	if (config.rev.core >= 6)
-		if (!adreno_has_gmu_wrapper(to_adreno_gpu(gpu)))
-			priv->has_cached_coherent = true;
+	priv->has_cached_coherent =
+		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
+		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
 
 	return 0;
 }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index a7c4a2c536e3..e08d41337169 100644
@@ -33,6 +33,7 @@ enum {
 #define ADRENO_QUIRK_FAULT_DETECT_MASK		BIT(1)
 #define ADRENO_QUIRK_LMLOADKILL_DISABLE		BIT(2)
 #define ADRENO_QUIRK_HAS_HW_APRIV		BIT(3)
+#define ADRENO_QUIRK_HAS_CACHED_COHERENT	BIT(4)
 
 struct adreno_rev {
 	uint8_t  core;

From: Rob Clark <[email protected]>

It is better to explicitly list it. With the move to opaque chip-id's for future devices, we should avoid trying to infer things like generation from the numerical value.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++++++++++++++-------
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
  2 files changed, 17 insertions(+), 7 deletions(-)

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

It is better to explicitly list it. With the move to opaque chip-id's for future devices, we should avoid trying to infer things like generation from the numerical value.

Would it be better to push this to DT? I mean, we already have a 'dma-cache-coherent' property for it.

Quoted 7 lines

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++++++++++++++-------
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
  2 files changed, 17 insertions(+), 7 deletions(-)

--
With best wishes
Dmitry

From: Rob Clark <[email protected]>

It is better to explicitly list it. With the move to opaque chip-id's for future devices, we should avoid trying to infer things like generation from the numerical value.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++++++++++++++-------
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 2 files changed, 17 insertions(+), 7 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +16 -7

index f469f951a907..3c531da417b9 100644
@@ -256,6 +256,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 	}, {
 		.rev = ADRENO_REV(6, 1, 9, ANY_ID),

@@ -266,6 +267,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a615_zap.mdt",
 		.hwcg = a615_hwcg,

@@ -278,6 +280,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a630_zap.mdt",
 		.hwcg = a630_hwcg,

@@ -290,6 +293,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a640_zap.mdt",
 		.hwcg = a640_hwcg,

@@ -302,7 +306,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M + SZ_128K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a650_zap.mdt",
 		.hwcg = a650_hwcg,

@@ -316,7 +321,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_1M + SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a660_zap.mdt",
 		.hwcg = a660_hwcg,

@@ -329,7 +335,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_512K,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.hwcg = a660_hwcg,
 		.address_space_size = SZ_16G,

@@ -342,6 +349,7 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_2M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
 		.zapfw = "a640_zap.mdt",
 		.hwcg = a640_hwcg,

@@ -353,7 +361,8 @@ static const struct adreno_info gpulist[] = {
 		},
 		.gmem = SZ_4M,
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
-		.quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+			ADRENO_QUIRK_HAS_HW_APRIV,
 		.init = a6xx_gpu_init,
 		.zapfw = "a690_zap.mdt",
 		.hwcg = a690_hwcg,

@@ -565,9 +574,9 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	if (config.rev.core >= 6)
-		if (!adreno_has_gmu_wrapper(to_adreno_gpu(gpu)))
-			priv->has_cached_coherent = true;
+	priv->has_cached_coherent =
+		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
+		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
 
 	return 0;
 }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index a7c4a2c536e3..e08d41337169 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

 struct adreno_rev {
 	uint8_t  core;

Signature · 1 line

2.41.0

Quoted 108 lines · Rob Clark

From: Rob Clark <[email protected]>

It is better to explicitly list it. With the move to opaque chip-id's for future devices, we should avoid trying to infer things like generation from the numerical value.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++++++++++++++-------
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 2 files changed, 17 insertions(+), 7 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +16 -7

index f469f951a907..3c531da417b9 100644
@@ -256,6 +256,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
      }, {
              .rev = ADRENO_REV(6, 1, 9, ANY_ID),

@@ -266,6 +267,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a615_zap.mdt",
              .hwcg = a615_hwcg,

@@ -278,6 +280,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a630_zap.mdt",
              .hwcg = a630_hwcg,

@@ -290,6 +293,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a640_zap.mdt",
              .hwcg = a640_hwcg,

@@ -302,7 +306,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M + SZ_128K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .zapfw = "a650_zap.mdt",
              .hwcg = a650_hwcg,

@@ -316,7 +321,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M + SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .zapfw = "a660_zap.mdt",
              .hwcg = a660_hwcg,

@@ -329,7 +335,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .hwcg = a660_hwcg,
              .address_space_size = SZ_16G,

@@ -342,6 +349,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_2M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a640_zap.mdt",
              .hwcg = a640_hwcg,

@@ -353,7 +361,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_4M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .zapfw = "a690_zap.mdt",
              .hwcg = a690_hwcg,

@@ -565,9 +574,9 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
      if (ret)
              return ret;
 
-     if (config.rev.core >= 6)
-             if (!adreno_has_gmu_wrapper(to_adreno_gpu(gpu)))
-                     priv->has_cached_coherent = true;
+     priv->has_cached_coherent =
+             !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
+             !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h enum { · line 36

Quoted 11 lines

drivers/gpu/drm/msm/adreno/adreno_device.c +0 -0

      return 0;
 }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index a7c4a2c536e3..e08d41337169 100644
@@ -33,6 +33,7 @@ enum {
 #define ADRENO_QUIRK_FAULT_DETECT_MASK               BIT(1)
 #define ADRENO_QUIRK_LMLOADKILL_DISABLE              BIT(2)
 #define ADRENO_QUIRK_HAS_HW_APRIV            BIT(3)
+#define ADRENO_QUIRK_HAS_CACHED_COHERENT     BIT(4)

a shorter ADRENO_IS_IOCOHERENT?

-Akhil

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -1

 struct adreno_rev {
      uint8_t  core;
--

2.41.0

Quoted 110 lines · Akhil P Oommen

Quoted 108 lines · Rob Clark

From: Rob Clark <[email protected]>

It is better to explicitly list it. With the move to opaque chip-id's for future devices, we should avoid trying to infer things like generation from the numerical value.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++++++++++++++-------
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 2 files changed, 17 insertions(+), 7 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +16 -7

index f469f951a907..3c531da417b9 100644
@@ -256,6 +256,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
      }, {
              .rev = ADRENO_REV(6, 1, 9, ANY_ID),

@@ -266,6 +267,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a615_zap.mdt",
              .hwcg = a615_hwcg,

@@ -278,6 +280,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a630_zap.mdt",
              .hwcg = a630_hwcg,

@@ -290,6 +293,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a640_zap.mdt",
              .hwcg = a640_hwcg,

@@ -302,7 +306,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M + SZ_128K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .zapfw = "a650_zap.mdt",
              .hwcg = a650_hwcg,

@@ -316,7 +321,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_1M + SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .zapfw = "a660_zap.mdt",
              .hwcg = a660_hwcg,

@@ -329,7 +335,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_512K,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .hwcg = a660_hwcg,
              .address_space_size = SZ_16G,

@@ -342,6 +349,7 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_2M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
              .init = a6xx_gpu_init,
              .zapfw = "a640_zap.mdt",
              .hwcg = a640_hwcg,

@@ -353,7 +361,8 @@ static const struct adreno_info gpulist[] = {
              },
              .gmem = SZ_4M,
              .inactive_period = DRM_MSM_INACTIVE_PERIOD,
-             .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
+             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
+                     ADRENO_QUIRK_HAS_HW_APRIV,
              .init = a6xx_gpu_init,
              .zapfw = "a690_zap.mdt",
              .hwcg = a690_hwcg,

@@ -565,9 +574,9 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
      if (ret)
              return ret;
 
-     if (config.rev.core >= 6)
-             if (!adreno_has_gmu_wrapper(to_adreno_gpu(gpu)))
-                     priv->has_cached_coherent = true;
+     priv->has_cached_coherent =
+             !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
+             !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
 

Quoted 12 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h enum { · line 36

Quoted 11 lines

drivers/gpu/drm/msm/adreno/adreno_device.c +0 -0

      return 0;
 }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index a7c4a2c536e3..e08d41337169 100644
@@ -33,6 +33,7 @@ enum {
 #define ADRENO_QUIRK_FAULT_DETECT_MASK               BIT(1)
 #define ADRENO_QUIRK_LMLOADKILL_DISABLE              BIT(2)
 #define ADRENO_QUIRK_HAS_HW_APRIV            BIT(3)
+#define ADRENO_QUIRK_HAS_CACHED_COHERENT     BIT(4)

a shorter ADRENO_IS_IOCOHERENT?

I prefer "HAS" to "IS".. maybe it is just me but "IS" sounds to me like all dma is coherent, while in fact gpu mappings can be either coherent or not.

I suppose it could be "HAS_IOCOHERENT" but we do use "CACHED_COHERENT" elsewhere.

BR,
-R

Quoted 9 lines

-Akhil

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -1

 struct adreno_rev {
      uint8_t  core;
--

2.41.0

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Signed-off-by Rob Clark <[email protected]>

---

 drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
 		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
+	}, {
+		.machine = "qcom,sm4350",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
+	}, {
+		.machine = "qcom,sm6375",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
 	}, {
 		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
 		.revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
 	/* identify gpu: */
 	for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
 		const struct adreno_info *info = &gpulist[i];
+		if (info->machine && !of_machine_is_compatible(info->machine))
+			continue;
 		if (adreno_cmp_rev(info->rev, rev))
 			return info;
 	}

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 		config.rev.minor, config.rev.patchid);
 
 	priv->is_a2xx = config.rev.core == 2;
+	priv->has_cached_coherent =
+		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
 
 	gpu = info->init(drm);
 	if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	priv->has_cached_coherent =
-		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
 	return 0;
 }
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index e08d41337169..d5335b99c64c 100644
@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
 extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
 
 struct adreno_info {
+	const char *machine;
 	struct adreno_rev rev;
 	uint32_t revn;
 	const char *fw[ADRENO_FW_MAX];

Quoted 8 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Signed-off-by Rob Clark <[email protected]>

---

of_machine_is_compatible is rather used in extremely desperate situations :/ I'm not sure this is the correct way to do this..

Especially since there's a direct correlation between GMU presence and ability to do cached coherent.

The GMU mandates presence of RPMh (as most of what the GMU does is talk to AOSS through its RSC).

To achieve I/O coherency, there must be some memory that both the CPU and GPU (and possibly others) can access through some sort of a negotiator/manager.

In our case, I believe that's LLC. And guess what that implies. MEMNOC instead of BIMC. And guess what that implies. RPMh!

Now, we know GMU => RPMh, but does it work the other way around?

Yes. GMU wrapper was a hack because probably nobody in the Adreno team would have imagined that somebody would be crazy enough to fork multiple year old designs multiple times and release them as new SoCs with updated arm cores and 5G..

(Except for A612 which has a "Reduced GMU" but that zombie still talks to RPMh. And A612 is IO-coherent. So I guess it works anyway.)

Konrad

Quoted 82 lines

 drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
 		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
 		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 		.init = a6xx_gpu_init,
+	}, {
+		.machine = "qcom,sm4350",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
+	}, {
+		.machine = "qcom,sm6375",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
 	}, {
 		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
 		.revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
 	/* identify gpu: */
 	for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
 		const struct adreno_info *info = &gpulist[i];
+		if (info->machine && !of_machine_is_compatible(info->machine))
+			continue;
 		if (adreno_cmp_rev(info->rev, rev))
 			return info;
 	}

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 		config.rev.minor, config.rev.patchid);
 
 	priv->is_a2xx = config.rev.core == 2;
+	priv->has_cached_coherent =
+		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
 
 	gpu = info->init(drm);
 	if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	priv->has_cached_coherent =
-		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
 	return 0;
 }
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index e08d41337169..d5335b99c64c 100644
@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
 extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
 
 struct adreno_info {
+	const char *machine;
 	struct adreno_rev rev;
 	uint32_t revn;
 	const char *fw[ADRENO_FW_MAX];

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
  2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
  		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
  		.init = a6xx_gpu_init,
+	}, {
+		.machine = "qcom,sm4350",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
+	}, {
+		.machine = "qcom,sm6375",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
  	}, {
  		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
  		.revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
  	/* identify gpu: */
  	for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
  		const struct adreno_info *info = &gpulist[i];
+		if (info->machine && !of_machine_is_compatible(info->machine))
+			continue;
  		if (adreno_cmp_rev(info->rev, rev))
  			return info;
  	}

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
  		config.rev.minor, config.rev.patchid);
 
  	priv->is_a2xx = config.rev.core == 2;
+	priv->has_cached_coherent =
+		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
 
  	gpu = info->init(drm);
  	if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
  	if (ret)
  		return ret;
 
-	priv->has_cached_coherent =
-		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
  	return 0;
  }
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index e08d41337169..d5335b99c64c 100644
@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
  extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
 
  struct adreno_info {
+	const char *machine;
  	struct adreno_rev rev;
  	uint32_t revn;
  	const char *fw[ADRENO_FW_MAX];

Quoted 2 lines · Rob Clark

From: Rob Clark <[email protected]>

Quoted 3 lines

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Quoted 80 lines

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
  2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
  		.quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
  		.init = a6xx_gpu_init,
+	}, {
+		.machine = "qcom,sm4350",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
+	}, {
+		.machine = "qcom,sm6375",
+		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
+		.revn = 619,
+		.fw = {
+			[ADRENO_FW_SQE] = "a630_sqe.fw",
+			[ADRENO_FW_GMU] = "a619_gmu.bin",
+		},
+		.gmem = SZ_512K,
+		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
+		.init = a6xx_gpu_init,
+		.zapfw = "a615_zap.mdt",
+		.hwcg = a615_hwcg,
  	}, {
  		.rev = ADRENO_REV(6, 1, 9, ANY_ID),
  		.revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
  	/* identify gpu: */
  	for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
  		const struct adreno_info *info = &gpulist[i];
+		if (info->machine && !of_machine_is_compatible(info->machine))
+			continue;
  		if (adreno_cmp_rev(info->rev, rev))
  			return info;
  	}

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
  		config.rev.minor, config.rev.patchid);
  	priv->is_a2xx = config.rev.core == 2;
+	priv->has_cached_coherent =
+		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
  	gpu = info->init(drm);
  	if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
  	if (ret)
  		return ret;
-	priv->has_cached_coherent =
-		!!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-		!adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
  	return 0;
  }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

index e08d41337169..d5335b99c64c 100644
@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
  extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
  struct adreno_info {
+	const char *machine;
  	struct adreno_rev rev;
  	uint32_t revn;
  	const char *fw[ADRENO_FW_MAX];

Signature · 2 lines

With best wishes
Dmitry

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
  2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
            .inactive_period = DRM_MSM_INACTIVE_PERIOD,
            .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
            .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
    }, {
            .rev = ADRENO_REV(6, 1, 9, ANY_ID),
            .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
    /* identify gpu: */
    for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
            const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
            if (adreno_cmp_rev(info->rev, rev))
                    return info;
    }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
            config.rev.minor, config.rev.patchid);
    priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
    gpu = info->init(drm);
    if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
    if (ret)
            return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
    return 0;
  }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
  extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
  struct adreno_info {
+   const char *machine;
    struct adreno_rev rev;
    uint32_t revn;
    const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Ok, I think we could do this, so something like:

compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
  2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
            .inactive_period = DRM_MSM_INACTIVE_PERIOD,
            .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
            .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
    }, {
            .rev = ADRENO_REV(6, 1, 9, ANY_ID),
            .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
    /* identify gpu: */
    for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
            const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
            if (adreno_cmp_rev(info->rev, rev))
                    return info;
    }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
            config.rev.minor, config.rev.patchid);
    priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
    gpu = info->init(drm);
    if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
    if (ret)
            return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
    return 0;
  }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
  extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
  struct adreno_info {
+   const char *machine;
    struct adreno_rev rev;
    uint32_t revn;
    const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
  2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
            .inactive_period = DRM_MSM_INACTIVE_PERIOD,
            .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
            .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
    }, {
            .rev = ADRENO_REV(6, 1, 9, ANY_ID),
            .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
    /* identify gpu: */
    for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
            const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
            if (adreno_cmp_rev(info->rev, rev))
                    return info;
    }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
            config.rev.minor, config.rev.patchid);
    priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
    gpu = info->init(drm);
    if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
    if (ret)
            return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
    return 0;
  }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
  extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
  struct adreno_info {
+   const char *machine;
    struct adreno_rev rev;
    uint32_t revn;
    const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 108 lines

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 50 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Quoted 9 lines

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Anyways, future things, it sounds like we'll be able to read the id from the hw/fw. I'm not really a fan of breaking dtb fwd/bk compat, so I don't want to change any of the existing compat strings.

BR,
-R

Quoted 113 lines

Quoted 108 lines

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 63 lines · Rob Clark

Quoted 59 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 4 lines

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

Quoted 4 lines

Anyways, future things, it sounds like we'll be able to read the id from the hw/fw. I'm not really a fan of breaking dtb fwd/bk compat, so I don't want to change any of the existing compat strings.

I think so too. Current compat strings should stay.

Quoted 117 lines

BR,
-R

Quoted 113 lines

Quoted 108 lines

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 65 lines · Dmitry Baryshkov

Quoted 63 lines · Rob Clark

Quoted 59 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 8 lines

Quoted 4 lines

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

I no longer have a n4 that boots.. but if I did both it and the later ones should work properly if they expose the appropriate chip id

I do still prefer parsing the chip-id out of the compatible. It avoids needing separate table entries just to have a different chip-id. Maybe the scheme that is used elsewhere makes sense when it is only the kernel that needs to be aware of the device-id. And maybe we could just done matching based on compat-id in userspace as well, but (a) msm and freedreno pre-date dt, and (b) that ship has already sailed.

Quoted 79 lines · Rob Clark

Quoted 65 lines · Dmitry Baryshkov

Quoted 63 lines · Rob Clark

Quoted 59 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 8 lines

Quoted 4 lines

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

I no longer have a n4 that boots.. but if I did both it and the later ones should work properly if they expose the appropriate chip id

Quoted 7 lines

I do still prefer parsing the chip-id out of the compatible. It avoids needing separate table entries just to have a different chip-id. Maybe the scheme that is used elsewhere makes sense when it is only the kernel that needs to be aware of the device-id. And maybe we could just done matching based on compat-id in userspace as well, but (a) msm and freedreno pre-date dt, and (b) that ship has already sailed.

I think a per-soc dt would be the better approach..

We could probably solve the revision issue with a socid readout of the silicon revision and override based on that?

Konrad

Quoted 63 lines · Rob Clark

Quoted 59 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 4 lines

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

Quoted 4 lines

Anyways, future things, it sounds like we'll be able to read the id from the hw/fw. I'm not really a fan of breaking dtb fwd/bk compat, so I don't want to change any of the existing compat strings.

I think so too. Current compat strings should stay.

Quoted 117 lines

BR,
-R

Quoted 113 lines

Quoted 108 lines

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 72 lines · Dmitry Baryshkov

Quoted 63 lines · Rob Clark

Quoted 50 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Quoted 9 lines

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 4 lines

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

Quoted 1 line

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

At this point, I'm failing to see why my original solution of just checking of_machine_is_compatible() is worse ;-)

I mean what is the difference between checking "qcom,apq8064-v1.1-adreno" and "qcom,apq8064-v1.1". I wouldn't really want to use of_match_node() in either case.

BR,
-R

Quoted 130 lines

Quoted 4 lines

Anyways, future things, it sounds like we'll be able to read the id from the hw/fw. I'm not really a fan of breaking dtb fwd/bk compat, so I don't want to change any of the existing compat strings.

I think so too. Current compat strings should stay.

Quoted 117 lines

BR,
-R

Quoted 113 lines

Quoted 108 lines

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 77 lines · Rob Clark

Quoted 73 lines · Dmitry Baryshkov

Quoted 63 lines · Rob Clark

Quoted 50 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Quoted 9 lines

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 4 lines

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

Quoted 6 lines

At this point, I'm failing to see why my original solution of just checking of_machine_is_compatible() is worse ;-)

I mean what is the difference between checking "qcom,apq8064-v1.1-adreno" and "qcom,apq8064-v1.1". I wouldn't really want to use of_match_node() in either case.

I have been proposing to use of_device_get_match_data().

Quoted 134 lines

BR,
-R

Quoted 130 lines

Quoted 4 lines

Anyways, future things, it sounds like we'll be able to read the id from the hw/fw. I'm not really a fan of breaking dtb fwd/bk compat, so I don't want to change any of the existing compat strings.

I think so too. Current compat strings should stay.

Quoted 117 lines

BR,
-R

Quoted 113 lines

Quoted 108 lines

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

Quoted 86 lines · Dmitry Baryshkov

Quoted 83 lines · Rob Clark

Quoted 65 lines · Dmitry Baryshkov

Quoted 63 lines · Rob Clark

Quoted 59 lines · Dmitry Baryshkov

Quoted 40 lines · Rob Clark

Quoted 38 lines · Dmitry Baryshkov

Quoted 19 lines · Rob Clark

Quoted 10 lines · Akhil P Oommen

Quoted 8 lines · Dmitry Baryshkov

Quoted 5 lines · Rob Clark

From: Rob Clark <[email protected]>

There are cases where there are differences due to SoC integration. Such as cache-coherency support, and (in the next patch) e-fuse to speedbin mappings.

Quoted 6 lines

Quoted 4 lines

I have the feeling that we are trying to circumvent the way DT works. I'd suggest adding explicit SoC-compatible strings to Adreno bindings and then using of_device_id::data and then of_device_get_match_data().

Just thinking, then how about a unique compatible string which we match to identify gpu->info and drop chip-id check completely here?

Quoted 8 lines

Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350 or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string. So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a part of SoC data:

Quoted 7 lines

Quoted 4 lines

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

I don't think we can do that, there are cases where the same SoC had multiple revisions of adreno.

Is that the case for the production versions of the SoC? In other subsystems what we usually do is that we add support only for the latest SoC revision (which would probably mean the latest GPU patch revision). Previous GPU revisions can be added in the following way (pure example):

qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial sample
qcom,sm4350-v1-adreno -> 6,1,9,0

Quoted 8 lines

Quoted 4 lines

My recollection was that nexus4 shipped with an early version of 8064 which needed userspace workarounds that later 8064 did not. Not sure if that is the only such example, but it is one that userspace needed to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

At this point, I'm failing to see why my original solution of just checking of_machine_is_compatible() is worse ;-)

I mean what is the difference between checking "qcom,apq8064-v1.1-adreno" and "qcom,apq8064-v1.1". I wouldn't really want to use of_match_node() in either case.

Quoted 1 line

I have been proposing to use of_device_get_match_data().

That has the same limitation when it comes to our needs.. our current setup lets us have a single table entry that matches multiple chip-id's. I don't really see the point of using of_match_node() or of_device_get_match_data() just for the sake of using them.

BR,
-R

Quoted 140 lines

Quoted 134 lines

BR,
-R

Quoted 130 lines

Quoted 4 lines

Anyways, future things, it sounds like we'll be able to read the id from the hw/fw. I'm not really a fan of breaking dtb fwd/bk compat, so I don't want to change any of the existing compat strings.

I think so too. Current compat strings should stay.

Quoted 117 lines

BR,
-R

Quoted 113 lines

Quoted 108 lines

We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a tiny bit more complex is something like:

    compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

Quoted 97 lines

Quoted 91 lines

BR,
-R

Quoted 87 lines

-Akhil

Quoted 85 lines

Quoted 74 lines

Signed-off-by Rob Clark <[email protected]>

---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++++++++++++++++++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +30 -4

index 3c531da417b9..e62bc895a31f 100644
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
             .inactive_period = DRM_MSM_INACTIVE_PERIOD,
             .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
             .init = a6xx_gpu_init,
+   }, {
+           .machine = "qcom,sm4350",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
+   }, {
+           .machine = "qcom,sm6375",
+           .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+           .revn = 619,
+           .fw = {
+                   [ADRENO_FW_SQE] = "a630_sqe.fw",
+                   [ADRENO_FW_GMU] = "a619_gmu.bin",
+           },
+           .gmem = SZ_512K,
+           .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+           .init = a6xx_gpu_init,
+           .zapfw = "a615_zap.mdt",
+           .hwcg = a615_hwcg,
     }, {
             .rev = ADRENO_REV(6, 1, 9, ANY_ID),
             .revn = 619,

@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
     /* identify gpu: */
     for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
             const struct adreno_info *info = &gpulist[i];
+           if (info->machine && !of_machine_is_compatible(info->machine))
+                   continue;
             if (adreno_cmp_rev(info->rev, rev))
                     return info;
     }

@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
             config.rev.minor, config.rev.patchid);
     priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
     gpu = info->init(drm);
     if (IS_ERR(gpu)) {

@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
     if (ret)
             return ret;
-   priv->has_cached_coherent =
-           !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-           !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
     return 0;
   }

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index e08d41337169..d5335b99c64c 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h · line 66

Quoted 6 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +1 -0

@@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
   struct adreno_info {
+   const char *machine;
     struct adreno_rev rev;
     uint32_t revn;
     const char *fw[ADRENO_FW_MAX];

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

--
With best wishes
Dmitry

From: Rob Clark <[email protected]>

This simplifies the code.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 171 ++-------------------
  drivers/gpu/drm/msm/adreno/adreno_device.c |  51 ++++++
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  25 +++
  3 files changed, 92 insertions(+), 155 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0

index d5335b99c64c..994ac26ce731 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h +18 -0

  };
 
+/*
+ * Helper to build a speedbin table, ie. the table:
+ *      fuse | speedbin
+ *      -----+---------
+ *        0  |   0
+ *       169 |   1
+ *       174 |   2
+ *
+ * would be declared as:
+ *
+ *     .speedbins = ADRENO_SPEEDBINS(
+ *                      0,   0,
+ *                      169, 1,
+ *                      174, 2
+ *                  ),
+ */
+#define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
+
  const struct adreno_info *adreno_info(struct adreno_rev rev);
 
  struct adreno_gpu {

Quoted 10 lines · Rob Clark

From: Rob Clark <[email protected]>

This simplifies the code.

Signed-off-by Rob Clark <[email protected]>

---
  drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 171 ++-------------------
  drivers/gpu/drm/msm/adreno/adreno_device.c |  51 ++++++
  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  25 +++
  3 files changed, 92 insertions(+), 155 deletions(-)

Interesting hack, I'd say.

Reviewed-by Dmitry Baryshkov <[email protected]>

Minor nit below.

Quoted 12 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +2 -0

index d5335b99c64c..994ac26ce731 100644
@@ -72,8 +72,33 @@ struct adreno_info {
      u32 inactive_period;
      const struct adreno_reglist *hwcg;
      u64 address_space_size;
+     /**
+      * @speedbins: Optional table of fuse to speedbin mappings

drivers/gpu/drm/msm/adreno/adreno_gpu.h struct adreno_info { · line 81

Quoted 5 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.h +5 -0

+      *
+      * Consists of pairs of fuse, index mappings, terminated with
+      * UINT_MAX sentinal.
+      */
+     uint32_t *speedbins;

Would it be better to explicitly list this as pairs of uint32_t? And then use braces in ADRENO_SPEEDBIN initialisation.