[RFC PATCH 2/4] drm/panthor: Add parsed gpu properties

Karunika Choo posted 4 patches 12 months ago
[RFC PATCH 2/4] drm/panthor: Add parsed gpu properties
Posted by Karunika Choo 12 months ago
This patch adds parsing of GPU register fields on initialization instead of
parsing the fields each time it is needed.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/Makefile         |   1 +
 drivers/gpu/drm/panthor/panthor_device.c |   1 +
 drivers/gpu/drm/panthor/panthor_device.h |   4 +
 drivers/gpu/drm/panthor/panthor_fw.c     |   5 +-
 drivers/gpu/drm/panthor/panthor_gpu.c    | 105 ++--------------
 drivers/gpu/drm/panthor/panthor_heap.c   |   6 +-
 drivers/gpu/drm/panthor/panthor_mmu.c    |  21 +---
 drivers/gpu/drm/panthor/panthor_props.c  | 151 +++++++++++++++++++++++
 drivers/gpu/drm/panthor/panthor_props.h  |  70 +++++++++++
 drivers/gpu/drm/panthor/panthor_regs.h   |   5 +
 drivers/gpu/drm/panthor/panthor_sched.c  |   6 +-
 11 files changed, 252 insertions(+), 123 deletions(-)
 create mode 100644 drivers/gpu/drm/panthor/panthor_props.c
 create mode 100644 drivers/gpu/drm/panthor/panthor_props.h

diff --git a/drivers/gpu/drm/panthor/Makefile b/drivers/gpu/drm/panthor/Makefile
index 15294719b09c..ab297637d172 100644
--- a/drivers/gpu/drm/panthor/Makefile
+++ b/drivers/gpu/drm/panthor/Makefile
@@ -9,6 +9,7 @@ panthor-y := \
 	panthor_gpu.o \
 	panthor_heap.o \
 	panthor_mmu.o \
+	panthor_props.o \
 	panthor_sched.o
 
 obj-$(CONFIG_DRM_PANTHOR) += panthor.o
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 0a37cfeeb181..0b74dc628489 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -19,6 +19,7 @@
 #include "panthor_fw.h"
 #include "panthor_gpu.h"
 #include "panthor_mmu.h"
+#include "panthor_props.h"
 #include "panthor_regs.h"
 #include "panthor_sched.h"
 
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index da6574021664..60c9a67fb4a2 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -26,6 +26,7 @@ struct panthor_group_pool;
 struct panthor_heap_pool;
 struct panthor_job;
 struct panthor_mmu;
+struct panthor_props;
 struct panthor_fw;
 struct panthor_perfcnt;
 struct panthor_vm;
@@ -117,6 +118,9 @@ struct panthor_device {
 	/** @gpu_info: GPU information. */
 	struct drm_panthor_gpu_info gpu_info;
 
+	/** @props: Parsed GPU properties */
+	struct panthor_props *props;
+
 	/** @csif_info: Command stream interface information. */
 	struct drm_panthor_csif_info csif_info;
 
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 8f1b9eff66ef..51b63d258c7a 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -22,6 +22,7 @@
 #include "panthor_gem.h"
 #include "panthor_gpu.h"
 #include "panthor_mmu.h"
+#include "panthor_props.h"
 #include "panthor_regs.h"
 #include "panthor_sched.h"
 
@@ -746,8 +747,8 @@ static int panthor_fw_load(struct panthor_device *ptdev)
 	int ret;
 
 	snprintf(fw_path, sizeof(fw_path), "arm/mali/arch%d.%d/%s",
-		 (u32)GPU_ARCH_MAJOR(ptdev->gpu_info.gpu_id),
-		 (u32)GPU_ARCH_MINOR(ptdev->gpu_info.gpu_id),
+		 ptdev->props->gpu_id.arch_major,
+		 ptdev->props->gpu_id.arch_minor,
 		 CSF_FW_NAME);
 
 	ret = request_firmware(&fw, fw_path, ptdev->base.dev);
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index a7d5022d34be..ec1780fe2638 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -18,6 +18,7 @@
 
 #include "panthor_device.h"
 #include "panthor_gpu.h"
+#include "panthor_props.h"
 #include "panthor_regs.h"
 
 /**
@@ -37,40 +38,6 @@ struct panthor_gpu {
 	wait_queue_head_t reqs_acked;
 };
 
-/**
- * struct panthor_model - GPU model description
- */
-struct panthor_model {
-	/** @name: Model name. */
-	const char *name;
-
-	/** @arch_major: Major version number of architecture. */
-	u8 arch_major;
-
-	/** @product_major: Major version number of product. */
-	u8 product_major;
-};
-
-/**
- * GPU_MODEL() - Define a GPU model. A GPU product can be uniquely identified
- * by a combination of the major architecture version and the major product
- * version.
- * @_name: Name for the GPU model.
- * @_arch_major: Architecture major.
- * @_product_major: Product major.
- */
-#define GPU_MODEL(_name, _arch_major, _product_major) \
-{\
-	.name = __stringify(_name),				\
-	.arch_major = _arch_major,				\
-	.product_major = _product_major,			\
-}
-
-static const struct panthor_model gpu_models[] = {
-	GPU_MODEL(g610, 10, 7),
-	{},
-};
-
 #define GPU_INTERRUPTS_MASK	\
 	(GPU_IRQ_FAULT | \
 	 GPU_IRQ_PROTM_FAULT | \
@@ -83,66 +50,6 @@ static void panthor_gpu_coherency_set(struct panthor_device *ptdev)
 		ptdev->coherent ? GPU_COHERENCY_PROT_BIT(ACE_LITE) : GPU_COHERENCY_NONE);
 }
 
-static void panthor_gpu_init_info(struct panthor_device *ptdev)
-{
-	const struct panthor_model *model;
-	u32 arch_major, product_major;
-	u32 major, minor, status;
-	unsigned int i;
-
-	ptdev->gpu_info.gpu_id = gpu_read(ptdev, GPU_ID);
-	ptdev->gpu_info.csf_id = gpu_read(ptdev, GPU_CSF_ID);
-	ptdev->gpu_info.gpu_rev = gpu_read(ptdev, GPU_REVID);
-	ptdev->gpu_info.core_features = gpu_read(ptdev, GPU_CORE_FEATURES);
-	ptdev->gpu_info.l2_features = gpu_read(ptdev, GPU_L2_FEATURES);
-	ptdev->gpu_info.tiler_features = gpu_read(ptdev, GPU_TILER_FEATURES);
-	ptdev->gpu_info.mem_features = gpu_read(ptdev, GPU_MEM_FEATURES);
-	ptdev->gpu_info.mmu_features = gpu_read(ptdev, GPU_MMU_FEATURES);
-	ptdev->gpu_info.thread_features = gpu_read(ptdev, GPU_THREAD_FEATURES);
-	ptdev->gpu_info.max_threads = gpu_read(ptdev, GPU_THREAD_MAX_THREADS);
-	ptdev->gpu_info.thread_max_workgroup_size = gpu_read(ptdev, GPU_THREAD_MAX_WORKGROUP_SIZE);
-	ptdev->gpu_info.thread_max_barrier_size = gpu_read(ptdev, GPU_THREAD_MAX_BARRIER_SIZE);
-	ptdev->gpu_info.coherency_features = gpu_read(ptdev, GPU_COHERENCY_FEATURES);
-	for (i = 0; i < 4; i++)
-		ptdev->gpu_info.texture_features[i] = gpu_read(ptdev, GPU_TEXTURE_FEATURES(i));
-
-	ptdev->gpu_info.as_present = gpu_read(ptdev, GPU_AS_PRESENT);
-
-	ptdev->gpu_info.shader_present = gpu_read64(ptdev, GPU_SHADER_PRESENT_LO);
-	ptdev->gpu_info.tiler_present = gpu_read64(ptdev, GPU_TILER_PRESENT_LO);
-	ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO);
-
-	arch_major = GPU_ARCH_MAJOR(ptdev->gpu_info.gpu_id);
-	product_major = GPU_PROD_MAJOR(ptdev->gpu_info.gpu_id);
-	major = GPU_VER_MAJOR(ptdev->gpu_info.gpu_id);
-	minor = GPU_VER_MINOR(ptdev->gpu_info.gpu_id);
-	status = GPU_VER_STATUS(ptdev->gpu_info.gpu_id);
-
-	for (model = gpu_models; model->name; model++) {
-		if (model->arch_major == arch_major &&
-		    model->product_major == product_major)
-			break;
-	}
-
-	drm_info(&ptdev->base,
-		 "mali-%s id 0x%x major 0x%x minor 0x%x status 0x%x",
-		 model->name ?: "unknown", ptdev->gpu_info.gpu_id >> 16,
-		 major, minor, status);
-
-	drm_info(&ptdev->base,
-		 "Features: L2:%#x Tiler:%#x Mem:%#x MMU:%#x AS:%#x",
-		 ptdev->gpu_info.l2_features,
-		 ptdev->gpu_info.tiler_features,
-		 ptdev->gpu_info.mem_features,
-		 ptdev->gpu_info.mmu_features,
-		 ptdev->gpu_info.as_present);
-
-	drm_info(&ptdev->base,
-		 "shader_present=0x%0llx l2_present=0x%0llx tiler_present=0x%0llx",
-		 ptdev->gpu_info.shader_present, ptdev->gpu_info.l2_present,
-		 ptdev->gpu_info.tiler_present);
-}
-
 static void panthor_gpu_irq_handler(struct panthor_device *ptdev, u32 status)
 {
 	if (status & GPU_IRQ_FAULT) {
@@ -193,7 +100,6 @@ void panthor_gpu_unplug(struct panthor_device *ptdev)
 int panthor_gpu_init(struct panthor_device *ptdev)
 {
 	struct panthor_gpu *gpu;
-	u32 pa_bits;
 	int ret, irq;
 
 	gpu = drmm_kzalloc(&ptdev->base, sizeof(*gpu), GFP_KERNEL);
@@ -203,11 +109,14 @@ int panthor_gpu_init(struct panthor_device *ptdev)
 	spin_lock_init(&gpu->reqs_lock);
 	init_waitqueue_head(&gpu->reqs_acked);
 	ptdev->gpu = gpu;
-	panthor_gpu_init_info(ptdev);
+
+	ret = panthor_props_init(ptdev);
+	if (ret)
+		return ret;
 
 	dma_set_max_seg_size(ptdev->base.dev, UINT_MAX);
-	pa_bits = GPU_MMU_FEATURES_PA_BITS(ptdev->gpu_info.mmu_features);
-	ret = dma_set_mask_and_coherent(ptdev->base.dev, DMA_BIT_MASK(pa_bits));
+	ret = dma_set_mask_and_coherent(ptdev->base.dev,
+					DMA_BIT_MASK(ptdev->props->mmu_pa_bits));
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 3796a9eb22af..995649081a66 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -10,6 +10,7 @@
 #include "panthor_gem.h"
 #include "panthor_heap.h"
 #include "panthor_mmu.h"
+#include "panthor_props.h"
 #include "panthor_regs.h"
 
 /*
@@ -101,10 +102,7 @@ struct panthor_heap_pool {
 
 static int panthor_heap_ctx_stride(struct panthor_device *ptdev)
 {
-	u32 l2_features = ptdev->gpu_info.l2_features;
-	u32 gpu_cache_line_size = GPU_L2_FEATURES_LINE_SIZE(l2_features);
-
-	return ALIGN(HEAP_CONTEXT_SIZE, gpu_cache_line_size);
+	return ALIGN(HEAP_CONTEXT_SIZE, ptdev->props->l2_line_size);
 }
 
 static int panthor_get_heap_ctx_offset(struct panthor_heap_pool *pool, int id)
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index bed13089bbd4..2b6d147a2f0d 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -31,6 +31,7 @@
 #include "panthor_gem.h"
 #include "panthor_heap.h"
 #include "panthor_mmu.h"
+#include "panthor_props.h"
 #include "panthor_regs.h"
 #include "panthor_sched.h"
 
@@ -695,7 +696,6 @@ static void panthor_vm_release_as_locked(struct panthor_vm *vm)
 int panthor_vm_active(struct panthor_vm *vm)
 {
 	struct panthor_device *ptdev = vm->ptdev;
-	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
 	struct io_pgtable_cfg *cfg = &io_pgtable_ops_to_pgtable(vm->pgtbl_ops)->cfg;
 	int ret = 0, as, cookie;
 	u64 transtab, transcfg;
@@ -756,7 +756,7 @@ int panthor_vm_active(struct panthor_vm *vm)
 	transcfg = AS_TRANSCFG_PTW_MEMATTR_WB |
 		   AS_TRANSCFG_PTW_RA |
 		   AS_TRANSCFG_ADRMODE_AARCH64_4K |
-		   AS_TRANSCFG_INA_BITS(55 - va_bits);
+		   AS_TRANSCFG_INA_BITS(55 - ptdev->props->mmu_va_bits);
 	if (ptdev->coherent)
 		transcfg |= AS_TRANSCFG_PTW_SH_OS;
 
@@ -1456,8 +1456,7 @@ panthor_vm_create_check_args(const struct panthor_device *ptdev,
 			     const struct drm_panthor_vm_create *args,
 			     u64 *kernel_va_start, u64 *kernel_va_range)
 {
-	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
-	u64 full_va_range = 1ull << va_bits;
+	u64 full_va_range = 1ull << ptdev->props->mmu_va_bits;
 	u64 user_va_range;
 
 	if (args->flags & ~PANTHOR_VM_CREATE_FLAGS)
@@ -2258,8 +2257,8 @@ panthor_vm_create(struct panthor_device *ptdev, bool for_mcu,
 		  u64 kernel_va_start, u64 kernel_va_size,
 		  u64 auto_kernel_va_start, u64 auto_kernel_va_size)
 {
-	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
-	u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(ptdev->gpu_info.mmu_features);
+	u32 va_bits = ptdev->props->mmu_va_bits;
+	u32 pa_bits = ptdev->props->mmu_pa_bits;
 	u64 full_va_range = 1ull << va_bits;
 	struct drm_gem_object *dummy_gem;
 	struct drm_gpu_scheduler *sched;
@@ -2688,7 +2687,6 @@ static void panthor_mmu_release_wq(struct drm_device *ddev, void *res)
  */
 int panthor_mmu_init(struct panthor_device *ptdev)
 {
-	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
 	struct panthor_mmu *mmu;
 	int ret, irq;
 
@@ -2722,15 +2720,6 @@ int panthor_mmu_init(struct panthor_device *ptdev)
 	if (!mmu->vm.wq)
 		return -ENOMEM;
 
-	/* On 32-bit kernels, the VA space is limited by the io_pgtable_ops abstraction,
-	 * which passes iova as an unsigned long. Patch the mmu_features to reflect this
-	 * limitation.
-	 */
-	if (va_bits > BITS_PER_LONG) {
-		ptdev->gpu_info.mmu_features &= ~GENMASK(7, 0);
-		ptdev->gpu_info.mmu_features |= BITS_PER_LONG;
-	}
-
 	return drmm_add_action_or_reset(&ptdev->base, panthor_mmu_release_wq, mmu->vm.wq);
 }
 
diff --git a/drivers/gpu/drm/panthor/panthor_props.c b/drivers/gpu/drm/panthor/panthor_props.c
new file mode 100644
index 000000000000..0a379feaf12d
--- /dev/null
+++ b/drivers/gpu/drm/panthor/panthor_props.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+/* Copyright 2024 ARM Limited. All rights reserved. */
+
+#include <drm/drm_managed.h>
+
+#include "panthor_device.h"
+#include "panthor_props.h"
+#include "panthor_regs.h"
+
+static void panthor_props_arch_10_8_init_info(struct panthor_device *ptdev)
+{
+	unsigned int i;
+
+	ptdev->gpu_info.csf_id = gpu_read(ptdev, GPU_CSF_ID);
+	ptdev->gpu_info.gpu_rev = gpu_read(ptdev, GPU_REVID);
+	ptdev->gpu_info.core_features = gpu_read(ptdev, GPU_CORE_FEATURES);
+	ptdev->gpu_info.l2_features = gpu_read(ptdev, GPU_L2_FEATURES);
+	ptdev->gpu_info.tiler_features = gpu_read(ptdev, GPU_TILER_FEATURES);
+	ptdev->gpu_info.mem_features = gpu_read(ptdev, GPU_MEM_FEATURES);
+	ptdev->gpu_info.mmu_features = gpu_read(ptdev, GPU_MMU_FEATURES);
+	ptdev->gpu_info.thread_features = gpu_read(ptdev, GPU_THREAD_FEATURES);
+	ptdev->gpu_info.max_threads = gpu_read(ptdev, GPU_THREAD_MAX_THREADS);
+	ptdev->gpu_info.thread_max_workgroup_size = gpu_read(ptdev, GPU_THREAD_MAX_WORKGROUP_SIZE);
+	ptdev->gpu_info.thread_max_barrier_size = gpu_read(ptdev, GPU_THREAD_MAX_BARRIER_SIZE);
+	ptdev->gpu_info.coherency_features = gpu_read(ptdev, GPU_COHERENCY_FEATURES);
+	for (i = 0; i < 4; i++)
+		ptdev->gpu_info.texture_features[i] = gpu_read(ptdev, GPU_TEXTURE_FEATURES(i));
+}
+
+static void panthor_props_arch_10_8_parse_props(struct panthor_device *ptdev)
+{
+	struct panthor_props *props = ptdev->props;
+	struct drm_panthor_gpu_info *info = &ptdev->gpu_info;
+
+	props->shader_core_count = hweight64(info->shader_present);
+	props->mmu_va_bits = GPU_MMU_FEATURES_VA_BITS(info->mmu_features);
+	props->mmu_pa_bits = GPU_MMU_FEATURES_PA_BITS(info->mmu_features);
+	props->mmu_as_count = hweight32(info->as_present);
+	props->l2_line_size = GPU_L2_FEATURES_LINE_SIZE(info->l2_features);
+
+	/* On 32-bit kernels, the VA space is limited by the io_pgtable_ops abstraction,
+	 * which passes iova as an unsigned long. Patch the mmu_features to reflect this
+	 * limitation.
+	 */
+	if (props->mmu_va_bits > BITS_PER_LONG) {
+		props->mmu_va_bits = BITS_PER_LONG;
+		info->mmu_features &= ~GENMASK(7, 0);
+		info->mmu_features |= BITS_PER_LONG;
+	}
+}
+
+static void panthor_props_arch_10_8_get_present_regs(struct panthor_device *ptdev)
+{
+	ptdev->gpu_info.as_present = gpu_read(ptdev, GPU_AS_PRESENT);
+	ptdev->gpu_info.shader_present = gpu_read64(ptdev, GPU_SHADER_PRESENT_LO);
+	ptdev->gpu_info.tiler_present = gpu_read64(ptdev, GPU_TILER_PRESENT_LO);
+	ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO);
+}
+
+static char *panthor_props_get_gpu_name(struct panthor_device *ptdev)
+{
+	struct panthor_gpu_id_props *gpu_id = &ptdev->props->gpu_id;
+
+	switch (gpu_id->product_id) {
+	case GPU_PRODUCT_ID_MAKE(10, 2):
+		return "Mali-G710";
+	case GPU_PRODUCT_ID_MAKE(10, 7):
+		return "Mali-G610";
+	case GPU_PRODUCT_ID_MAKE(10, 3):
+		return "Mali-G510";
+	case GPU_PRODUCT_ID_MAKE(10, 4):
+		return "Mali-G310";
+	}
+
+	return "(Unknown Mali GPU)";
+}
+
+static void panthor_props_show_info(struct panthor_device *ptdev)
+{
+	struct panthor_gpu_id_props *gpu_id = &ptdev->props->gpu_id;
+
+	drm_info(&ptdev->base, "%s id 0x%x major 0x%x minor 0x%x status 0x%x",
+		 panthor_props_get_gpu_name(ptdev), gpu_id->arch_id,
+		 gpu_id->version_major, gpu_id->version_minor,
+		 gpu_id->version_status);
+
+	drm_info(&ptdev->base,
+		 "Features: L2:%#x Tiler:%#x Mem:%#x MMU:%#x AS:%#x",
+		 ptdev->gpu_info.l2_features,
+		 ptdev->gpu_info.tiler_features,
+		 ptdev->gpu_info.mem_features,
+		 ptdev->gpu_info.mmu_features,
+		 ptdev->gpu_info.as_present);
+
+	drm_info(&ptdev->base,
+		 "shader_present=0x%0llx l2_present=0x%0llx tiler_present=0x%0llx",
+		 ptdev->gpu_info.shader_present, ptdev->gpu_info.l2_present,
+		 ptdev->gpu_info.tiler_present);
+}
+
+int panthor_props_gpu_id_init(struct panthor_device *ptdev)
+{
+	struct panthor_gpu_id_props *gpu_id = &ptdev->props->gpu_id;
+	struct drm_panthor_gpu_info *info = &ptdev->gpu_info;
+
+	info->gpu_id = gpu_read(ptdev, GPU_ID);
+	if (!info->gpu_id)
+		return -ENXIO;
+
+	gpu_id->arch_major = GPU_ARCH_MAJOR(info->gpu_id);
+	gpu_id->arch_minor = GPU_ARCH_MINOR(info->gpu_id);
+	gpu_id->arch_rev = GPU_ARCH_REV(info->gpu_id);
+	gpu_id->product_major = GPU_PROD_MAJOR(info->gpu_id);
+	gpu_id->version_major = GPU_VER_MAJOR(info->gpu_id);
+	gpu_id->version_minor = GPU_VER_MINOR(info->gpu_id);
+	gpu_id->version_status = GPU_VER_STATUS(info->gpu_id);
+
+	gpu_id->arch_id = GPU_ARCH_ID_MAKE(
+		gpu_id->arch_major, gpu_id->arch_minor, gpu_id->arch_rev);
+	gpu_id->product_id =
+		GPU_PRODUCT_ID_MAKE(gpu_id->arch_major, gpu_id->product_major);
+
+	return 0;
+}
+
+void panthor_props_load(struct panthor_device *ptdev)
+{
+	panthor_props_arch_10_8_init_info(ptdev);
+	panthor_props_arch_10_8_get_present_regs(ptdev);
+	panthor_props_arch_10_8_parse_props(ptdev);
+
+	panthor_props_show_info(ptdev);
+}
+
+int panthor_props_init(struct panthor_device *ptdev)
+{
+	struct panthor_props *props;
+	int ret;
+
+	props = drmm_kzalloc(&ptdev->base, sizeof(*props), GFP_KERNEL);
+	if (!props)
+		return -ENOMEM;
+
+	ptdev->props = props;
+
+	ret = panthor_props_gpu_id_init(ptdev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/panthor/panthor_props.h b/drivers/gpu/drm/panthor/panthor_props.h
new file mode 100644
index 000000000000..af39a7c7433f
--- /dev/null
+++ b/drivers/gpu/drm/panthor/panthor_props.h
@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+/* Copyright 2024 ARM Limited. All rights reserved. */
+
+#ifndef __PANTHOR_PROPS_H__
+#define __PANTHOR_PROPS_H__
+
+struct panthor_device;
+
+/**
+ * struct panthor_gpu_id_props - Parsed GPU_ID properties
+ */
+struct panthor_gpu_id_props {
+	/** @arch_major: Architecture major revision */
+	u8 arch_major;
+
+	/** @arch_minor: Architecture minor revision */
+	u8 arch_minor;
+
+	/** @arch_rev: Architecture patch revision */
+	u8 arch_rev;
+
+	/** @product_major: Product identifier */
+	u8 product_major;
+
+	/** @version_major: Major release version number */
+	u8 version_major;
+
+	/** @version_minor: Minor release version number */
+	u8 version_minor;
+
+	/** @version_status: Status of the GPU release */
+	u8 version_status;
+
+	/** @arch_id: Composite ID of arch_major, arch_minor and arch_rev */
+	u32 arch_id;
+
+	/** @arch_id: Composite ID of arch_major and product_major */
+	u32 product_id;
+};
+
+/**
+ * struct panthor_props - Parsed GPU properties
+ */
+struct panthor_props {
+	/** @gpu_id: parsed GPU_ID properties */
+	struct panthor_gpu_id_props gpu_id;
+
+	/** @shader_core_count: Number of shader cores present */
+	u8 shader_core_count;
+
+	/** @mmu_va_bits: Number of bits supported in virtual addresses */
+	u8 mmu_va_bits;
+
+	/** @mmu_pa_bits: Number of bits supported in physical addresses */
+	u8 mmu_pa_bits;
+
+	/** @mmu_as_count: Number of address spaces present */
+	u8 mmu_as_count;
+
+	/** @l2_line_size: L2 cache line size */
+	u8 l2_line_size;
+};
+
+int panthor_props_gpu_id_init(struct panthor_device *ptdev);
+
+void panthor_props_load(struct panthor_device *ptdev);
+
+int panthor_props_init(struct panthor_device *ptdev);
+
+#endif /* __PANTHOR_PROPS_H__ */
diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h
index 269c2c68dde2..bad172b8af82 100644
--- a/drivers/gpu/drm/panthor/panthor_regs.h
+++ b/drivers/gpu/drm/panthor/panthor_regs.h
@@ -22,6 +22,11 @@
 #define   GPU_VER_MINOR(x)				(((x) & GENMASK(11, 4)) >> 4)
 #define   GPU_VER_STATUS(x)				((x) & GENMASK(3, 0))
 
+#define GPU_ARCH_ID_MAKE(major, minor, rev) \
+	(((major) << 16) | ((minor) << 8) | (rev))
+#define GPU_PRODUCT_ID_MAKE(arch_major, product_major) \
+	(((arch_major) << 24) | (product_major))
+
 #define GPU_L2_FEATURES					0x4
 #define  GPU_L2_FEATURES_LINE_SIZE(x)			(1 << ((x) & GENMASK(7, 0)))
 
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 77b184c3fb0c..209fd9576969 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -29,6 +29,7 @@
 #include "panthor_gpu.h"
 #include "panthor_heap.h"
 #include "panthor_mmu.h"
+#include "panthor_props.h"
 #include "panthor_regs.h"
 #include "panthor_sched.h"
 
@@ -3832,10 +3833,9 @@ int panthor_sched_init(struct panthor_device *ptdev)
 	num_groups = min_t(u32, MAX_CSG_PRIO + 1, num_groups);
 
 	/* We need at least one AS for the MCU and one for the GPU contexts. */
-	gpu_as_count = hweight32(ptdev->gpu_info.as_present & GENMASK(31, 1));
-	if (!gpu_as_count) {
+	if (ptdev->props->mmu_as_count < 2) {
 		drm_err(&ptdev->base, "Not enough AS (%d, expected at least 2)",
-			gpu_as_count + 1);
+			ptdev->props->mmu_as_count);
 		return -EINVAL;
 	}
 
-- 
2.47.1
Re: [RFC PATCH 2/4] drm/panthor: Add parsed gpu properties
Posted by Steven Price 12 months ago
On 19/12/2024 17:05, Karunika Choo wrote:
> This patch adds parsing of GPU register fields on initialization instead of
> parsing the fields each time it is needed.

Why? ;)

The commit message should ideally explain the reason behind a change
rather than the change itself (that should ideally be obvious from the
patch). (See below for more).

Also from a reviewer's perspective it's hard to review patches which
both move code between files and change it. So splitting into an initial
patch which just moves code into the new panthor_props.c and a follow up
patch would make the review easier.

> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/Makefile         |   1 +
>  drivers/gpu/drm/panthor/panthor_device.c |   1 +
>  drivers/gpu/drm/panthor/panthor_device.h |   4 +
>  drivers/gpu/drm/panthor/panthor_fw.c     |   5 +-
>  drivers/gpu/drm/panthor/panthor_gpu.c    | 105 ++--------------
>  drivers/gpu/drm/panthor/panthor_heap.c   |   6 +-
>  drivers/gpu/drm/panthor/panthor_mmu.c    |  21 +---
>  drivers/gpu/drm/panthor/panthor_props.c  | 151 +++++++++++++++++++++++
>  drivers/gpu/drm/panthor/panthor_props.h  |  70 +++++++++++
>  drivers/gpu/drm/panthor/panthor_regs.h   |   5 +
>  drivers/gpu/drm/panthor/panthor_sched.c  |   6 +-
>  11 files changed, 252 insertions(+), 123 deletions(-)
>  create mode 100644 drivers/gpu/drm/panthor/panthor_props.c
>  create mode 100644 drivers/gpu/drm/panthor/panthor_props.h
> 
[...]
> diff --git a/drivers/gpu/drm/panthor/panthor_props.c b/drivers/gpu/drm/panthor/panthor_props.c
> new file mode 100644
> index 000000000000..0a379feaf12d
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_props.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +/* Copyright 2024 ARM Limited. All rights reserved. */
> +
> +#include <drm/drm_managed.h>
> +
> +#include "panthor_device.h"
> +#include "panthor_props.h"
> +#include "panthor_regs.h"
> +
> +static void panthor_props_arch_10_8_init_info(struct panthor_device *ptdev)
> +{
> +	unsigned int i;
> +
> +	ptdev->gpu_info.csf_id = gpu_read(ptdev, GPU_CSF_ID);
> +	ptdev->gpu_info.gpu_rev = gpu_read(ptdev, GPU_REVID);
> +	ptdev->gpu_info.core_features = gpu_read(ptdev, GPU_CORE_FEATURES);
> +	ptdev->gpu_info.l2_features = gpu_read(ptdev, GPU_L2_FEATURES);
> +	ptdev->gpu_info.tiler_features = gpu_read(ptdev, GPU_TILER_FEATURES);
> +	ptdev->gpu_info.mem_features = gpu_read(ptdev, GPU_MEM_FEATURES);
> +	ptdev->gpu_info.mmu_features = gpu_read(ptdev, GPU_MMU_FEATURES);
> +	ptdev->gpu_info.thread_features = gpu_read(ptdev, GPU_THREAD_FEATURES);
> +	ptdev->gpu_info.max_threads = gpu_read(ptdev, GPU_THREAD_MAX_THREADS);
> +	ptdev->gpu_info.thread_max_workgroup_size = gpu_read(ptdev, GPU_THREAD_MAX_WORKGROUP_SIZE);
> +	ptdev->gpu_info.thread_max_barrier_size = gpu_read(ptdev, GPU_THREAD_MAX_BARRIER_SIZE);
> +	ptdev->gpu_info.coherency_features = gpu_read(ptdev, GPU_COHERENCY_FEATURES);
> +	for (i = 0; i < 4; i++)
> +		ptdev->gpu_info.texture_features[i] = gpu_read(ptdev, GPU_TEXTURE_FEATURES(i));
> +}
> +
> +static void panthor_props_arch_10_8_parse_props(struct panthor_device *ptdev)
> +{
> +	struct panthor_props *props = ptdev->props;
> +	struct drm_panthor_gpu_info *info = &ptdev->gpu_info;
> +
> +	props->shader_core_count = hweight64(info->shader_present);
> +	props->mmu_va_bits = GPU_MMU_FEATURES_VA_BITS(info->mmu_features);
> +	props->mmu_pa_bits = GPU_MMU_FEATURES_PA_BITS(info->mmu_features);
> +	props->mmu_as_count = hweight32(info->as_present);
> +	props->l2_line_size = GPU_L2_FEATURES_LINE_SIZE(info->l2_features);

From the function name I can guess that you want to future proof against
these registers being moved around. If so that should definitely be in
the commit message.

I'm also somewhat tempted to say we should "wait-and-see" whether this
abstraction is necessary.

> +
> +	/* On 32-bit kernels, the VA space is limited by the io_pgtable_ops abstraction,
> +	 * which passes iova as an unsigned long. Patch the mmu_features to reflect this
> +	 * limitation.
> +	 */
> +	if (props->mmu_va_bits > BITS_PER_LONG) {
> +		props->mmu_va_bits = BITS_PER_LONG;
> +		info->mmu_features &= ~GENMASK(7, 0);
> +		info->mmu_features |= BITS_PER_LONG;
> +	}
> +}
> +
> +static void panthor_props_arch_10_8_get_present_regs(struct panthor_device *ptdev)
> +{
> +	ptdev->gpu_info.as_present = gpu_read(ptdev, GPU_AS_PRESENT);
> +	ptdev->gpu_info.shader_present = gpu_read64(ptdev, GPU_SHADER_PRESENT_LO);
> +	ptdev->gpu_info.tiler_present = gpu_read64(ptdev, GPU_TILER_PRESENT_LO);
> +	ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO);
> +}
> +
> +static char *panthor_props_get_gpu_name(struct panthor_device *ptdev)
> +{
> +	struct panthor_gpu_id_props *gpu_id = &ptdev->props->gpu_id;
> +
> +	switch (gpu_id->product_id) {
> +	case GPU_PRODUCT_ID_MAKE(10, 2):
> +		return "Mali-G710";
> +	case GPU_PRODUCT_ID_MAKE(10, 7):
> +		return "Mali-G610";
> +	case GPU_PRODUCT_ID_MAKE(10, 3):
> +		return "Mali-G510";
> +	case GPU_PRODUCT_ID_MAKE(10, 4):
> +		return "Mali-G310";
> +	}

You've sneaked in a bunch of new product names - this definitely
shouldn't be in this patch.

> +
> +	return "(Unknown Mali GPU)";
> +}
> +
> +static void panthor_props_show_info(struct panthor_device *ptdev)
> +{
> +	struct panthor_gpu_id_props *gpu_id = &ptdev->props->gpu_id;
> +
> +	drm_info(&ptdev->base, "%s id 0x%x major 0x%x minor 0x%x status 0x%x",
> +		 panthor_props_get_gpu_name(ptdev), gpu_id->arch_id,
> +		 gpu_id->version_major, gpu_id->version_minor,
> +		 gpu_id->version_status);
> +
> +	drm_info(&ptdev->base,
> +		 "Features: L2:%#x Tiler:%#x Mem:%#x MMU:%#x AS:%#x",
> +		 ptdev->gpu_info.l2_features,
> +		 ptdev->gpu_info.tiler_features,
> +		 ptdev->gpu_info.mem_features,
> +		 ptdev->gpu_info.mmu_features,
> +		 ptdev->gpu_info.as_present);
> +
> +	drm_info(&ptdev->base,
> +		 "shader_present=0x%0llx l2_present=0x%0llx tiler_present=0x%0llx",
> +		 ptdev->gpu_info.shader_present, ptdev->gpu_info.l2_present,
> +		 ptdev->gpu_info.tiler_present);
> +}
> +
> +int panthor_props_gpu_id_init(struct panthor_device *ptdev)
> +{
> +	struct panthor_gpu_id_props *gpu_id = &ptdev->props->gpu_id;
> +	struct drm_panthor_gpu_info *info = &ptdev->gpu_info;
> +
> +	info->gpu_id = gpu_read(ptdev, GPU_ID);
> +	if (!info->gpu_id)
> +		return -ENXIO;
> +
> +	gpu_id->arch_major = GPU_ARCH_MAJOR(info->gpu_id);
> +	gpu_id->arch_minor = GPU_ARCH_MINOR(info->gpu_id);
> +	gpu_id->arch_rev = GPU_ARCH_REV(info->gpu_id);
> +	gpu_id->product_major = GPU_PROD_MAJOR(info->gpu_id);
> +	gpu_id->version_major = GPU_VER_MAJOR(info->gpu_id);
> +	gpu_id->version_minor = GPU_VER_MINOR(info->gpu_id);
> +	gpu_id->version_status = GPU_VER_STATUS(info->gpu_id);

Why do we need to store the GPU_ID twice (once as the gpu_id register
and once again broken down)? The bit masking/extraction is really cheap.

> +
> +	gpu_id->arch_id = GPU_ARCH_ID_MAKE(
> +		gpu_id->arch_major, gpu_id->arch_minor, gpu_id->arch_rev);
> +	gpu_id->product_id =
> +		GPU_PRODUCT_ID_MAKE(gpu_id->arch_major, gpu_id->product_major);

And here we're gluing it back together... All for the benefit of a
drm_info() - clearly not a performance path.

Steve

> +
> +	return 0;
> +}
> +
> +void panthor_props_load(struct panthor_device *ptdev)
> +{
> +	panthor_props_arch_10_8_init_info(ptdev);
> +	panthor_props_arch_10_8_get_present_regs(ptdev);
> +	panthor_props_arch_10_8_parse_props(ptdev);
> +
> +	panthor_props_show_info(ptdev);
> +}
> +
> +int panthor_props_init(struct panthor_device *ptdev)
> +{
> +	struct panthor_props *props;
> +	int ret;
> +
> +	props = drmm_kzalloc(&ptdev->base, sizeof(*props), GFP_KERNEL);
> +	if (!props)
> +		return -ENOMEM;
> +
> +	ptdev->props = props;
> +
> +	ret = panthor_props_gpu_id_init(ptdev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/panthor/panthor_props.h b/drivers/gpu/drm/panthor/panthor_props.h
> new file mode 100644
> index 000000000000..af39a7c7433f
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_props.h
> @@ -0,0 +1,70 @@
> +/* SPDX-License-Identifier: GPL-2.0 or MIT */
> +/* Copyright 2024 ARM Limited. All rights reserved. */
> +
> +#ifndef __PANTHOR_PROPS_H__
> +#define __PANTHOR_PROPS_H__
> +
> +struct panthor_device;
> +
> +/**
> + * struct panthor_gpu_id_props - Parsed GPU_ID properties
> + */
> +struct panthor_gpu_id_props {
> +	/** @arch_major: Architecture major revision */
> +	u8 arch_major;
> +
> +	/** @arch_minor: Architecture minor revision */
> +	u8 arch_minor;
> +
> +	/** @arch_rev: Architecture patch revision */
> +	u8 arch_rev;
> +
> +	/** @product_major: Product identifier */
> +	u8 product_major;
> +
> +	/** @version_major: Major release version number */
> +	u8 version_major;
> +
> +	/** @version_minor: Minor release version number */
> +	u8 version_minor;
> +
> +	/** @version_status: Status of the GPU release */
> +	u8 version_status;
> +
> +	/** @arch_id: Composite ID of arch_major, arch_minor and arch_rev */
> +	u32 arch_id;
> +
> +	/** @arch_id: Composite ID of arch_major and product_major */
> +	u32 product_id;
> +};
> +
> +/**
> + * struct panthor_props - Parsed GPU properties
> + */
> +struct panthor_props {
> +	/** @gpu_id: parsed GPU_ID properties */
> +	struct panthor_gpu_id_props gpu_id;
> +
> +	/** @shader_core_count: Number of shader cores present */
> +	u8 shader_core_count;
> +
> +	/** @mmu_va_bits: Number of bits supported in virtual addresses */
> +	u8 mmu_va_bits;
> +
> +	/** @mmu_pa_bits: Number of bits supported in physical addresses */
> +	u8 mmu_pa_bits;
> +
> +	/** @mmu_as_count: Number of address spaces present */
> +	u8 mmu_as_count;
> +
> +	/** @l2_line_size: L2 cache line size */
> +	u8 l2_line_size;
> +};
> +
> +int panthor_props_gpu_id_init(struct panthor_device *ptdev);
> +
> +void panthor_props_load(struct panthor_device *ptdev);
> +
> +int panthor_props_init(struct panthor_device *ptdev);
> +
> +#endif /* __PANTHOR_PROPS_H__ */
> diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h
> index 269c2c68dde2..bad172b8af82 100644
> --- a/drivers/gpu/drm/panthor/panthor_regs.h
> +++ b/drivers/gpu/drm/panthor/panthor_regs.h
> @@ -22,6 +22,11 @@
>  #define   GPU_VER_MINOR(x)				(((x) & GENMASK(11, 4)) >> 4)
>  #define   GPU_VER_STATUS(x)				((x) & GENMASK(3, 0))
>  
> +#define GPU_ARCH_ID_MAKE(major, minor, rev) \
> +	(((major) << 16) | ((minor) << 8) | (rev))
> +#define GPU_PRODUCT_ID_MAKE(arch_major, product_major) \
> +	(((arch_major) << 24) | (product_major))
> +
>  #define GPU_L2_FEATURES					0x4
>  #define  GPU_L2_FEATURES_LINE_SIZE(x)			(1 << ((x) & GENMASK(7, 0)))
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 77b184c3fb0c..209fd9576969 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -29,6 +29,7 @@
>  #include "panthor_gpu.h"
>  #include "panthor_heap.h"
>  #include "panthor_mmu.h"
> +#include "panthor_props.h"
>  #include "panthor_regs.h"
>  #include "panthor_sched.h"
>  
> @@ -3832,10 +3833,9 @@ int panthor_sched_init(struct panthor_device *ptdev)
>  	num_groups = min_t(u32, MAX_CSG_PRIO + 1, num_groups);
>  
>  	/* We need at least one AS for the MCU and one for the GPU contexts. */
> -	gpu_as_count = hweight32(ptdev->gpu_info.as_present & GENMASK(31, 1));
> -	if (!gpu_as_count) {
> +	if (ptdev->props->mmu_as_count < 2) {
>  		drm_err(&ptdev->base, "Not enough AS (%d, expected at least 2)",
> -			gpu_as_count + 1);
> +			ptdev->props->mmu_as_count);
>  		return -EINVAL;
>  	}
>