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;
> }
>
