| Message ID | 20250320111741.1937892-6-karunika.choo@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Add GPU specific initialization framework to support new Mali GPUs | expand |
On Thu, 20 Mar 2025 11:17:37 +0000 Karunika Choo <karunika.choo@arm.com> wrote: > This patch replaces the previous panthor_model structure with a simple > switch case based on the product_id, which is in the format of: > ((arch_major << 24) | product_major) > > This not only simplifies the comparison, but also allows extending the > function to accommodate naming differences based on GPU features. > > Signed-off-by: Karunika Choo <karunika.choo@arm.com> > --- > drivers/gpu/drm/panthor/panthor_hw.c | 63 +++++++------------------- > drivers/gpu/drm/panthor/panthor_regs.h | 1 + > 2 files changed, 18 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c > index 4cc4b0d5382c..12183c04cd21 100644 > --- a/drivers/gpu/drm/panthor/panthor_hw.c > +++ b/drivers/gpu/drm/panthor/panthor_hw.c > @@ -5,40 +5,6 @@ > #include "panthor_hw.h" > #include "panthor_regs.h" > > -/** > - * 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), > - {}, > -}; > - > static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) > { > unsigned int i; > @@ -66,29 +32,34 @@ static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) > ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO); > } > > +static char *get_gpu_model_name(struct panthor_device *ptdev) > +{ > + const u32 gpu_id = ptdev->gpu_info.gpu_id; > + const u32 product_id = GPU_PROD_ID_MAKE(GPU_ARCH_MAJOR(gpu_id), > + GPU_PROD_MAJOR(gpu_id)); > + > + switch (product_id) { > + case GPU_PROD_ID_MAKE(10, 7): > + return "Mali-G610"; > + } I a big fan of these ever growing switch statements with nested conditionals. Could we instead add an optional ::get_variant() callback in panthor_model and have the following formatting: "Mali-%s%s%s", model->name, model->get_variant ? "-" : "", model->get_variant ? model->get_variant() : "" > + > + return "(Unknown Mali GPU)"; > +} > + > static void panthor_gpu_init_info(struct panthor_device *ptdev) > { > - const struct panthor_model *model; > - u32 arch_major, product_major; > + const char *gpu_model_name = get_gpu_model_name(ptdev); > u32 major, minor, status; > > ptdev->hw->ops.gpu_info_init(ptdev); > > - 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, > + "%s id 0x%x major 0x%x minor 0x%x status 0x%x", > + gpu_model_name, ptdev->gpu_info.gpu_id >> 16, > major, minor, status); > > drm_info(&ptdev->base, > diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h > index ba452c1dd644..d9e0769d6f1a 100644 > --- a/drivers/gpu/drm/panthor/panthor_regs.h > +++ b/drivers/gpu/drm/panthor/panthor_regs.h > @@ -20,6 +20,7 @@ > #define GPU_VER_STATUS(x) ((x) & GENMASK(3, 0)) > > #define GPU_ARCH_ID_MAKE(major, minor, rev) (((major) << 16) | ((minor) << 8) | (rev)) > +#define GPU_PROD_ID_MAKE(arch_major, prod_major) (((arch_major) << 24) | (prod_major)) > > #define GPU_L2_FEATURES 0x4 > #define GPU_L2_FEATURES_LINE_SIZE(x) (1 << ((x) & GENMASK(7, 0)))
On 21/03/2025 08:02, Boris Brezillon wrote: > On Thu, 20 Mar 2025 11:17:37 +0000 > Karunika Choo <karunika.choo@arm.com> wrote: > >> This patch replaces the previous panthor_model structure with a simple >> switch case based on the product_id, which is in the format of: >> ((arch_major << 24) | product_major) >> >> This not only simplifies the comparison, but also allows extending the >> function to accommodate naming differences based on GPU features. >> >> Signed-off-by: Karunika Choo <karunika.choo@arm.com> >> --- >> drivers/gpu/drm/panthor/panthor_hw.c | 63 +++++++------------------- >> drivers/gpu/drm/panthor/panthor_regs.h | 1 + >> 2 files changed, 18 insertions(+), 46 deletions(-) >> >> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c >> index 4cc4b0d5382c..12183c04cd21 100644 >> --- a/drivers/gpu/drm/panthor/panthor_hw.c >> +++ b/drivers/gpu/drm/panthor/panthor_hw.c >> @@ -5,40 +5,6 @@ >> #include "panthor_hw.h" >> #include "panthor_regs.h" >> >> -/** >> - * 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), >> - {}, >> -}; >> - >> static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) >> { >> unsigned int i; >> @@ -66,29 +32,34 @@ static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) >> ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO); >> } >> >> +static char *get_gpu_model_name(struct panthor_device *ptdev) >> +{ >> + const u32 gpu_id = ptdev->gpu_info.gpu_id; >> + const u32 product_id = GPU_PROD_ID_MAKE(GPU_ARCH_MAJOR(gpu_id), >> + GPU_PROD_MAJOR(gpu_id)); >> + >> + switch (product_id) { >> + case GPU_PROD_ID_MAKE(10, 7): >> + return "Mali-G610"; >> + } > > I a big fan of these ever growing switch statements with nested > conditionals. Could we instead add an optional ::get_variant() callback > in panthor_model and have the following formatting: > > "Mali-%s%s%s", model->name, > model->get_variant ? "-" : "", > model->get_variant ? model->get_variant() : "" > While that’s certainly an option, I wonder if it’s better to avoid additional string formatting when it’s not strictly necessary. The switch cases provide a straightforward GPU name without needing to handle conditional "-" separators or similar. Also, with the current approach, if a GPU is misconfigured with an incorrect product_major for its core count, the switch’s fallthrough helps ensure the correct name is still returned. A model->get_variant() callback wouldn’t give us that same flexibility to adjust the name based on such mismatches. Kind regards, Karunika Choo >> + >> + return "(Unknown Mali GPU)"; >> +} >> + >> static void panthor_gpu_init_info(struct panthor_device *ptdev) >> { >> - const struct panthor_model *model; >> - u32 arch_major, product_major; >> + const char *gpu_model_name = get_gpu_model_name(ptdev); >> u32 major, minor, status; >> >> ptdev->hw->ops.gpu_info_init(ptdev); >> >> - 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, >> + "%s id 0x%x major 0x%x minor 0x%x status 0x%x", >> + gpu_model_name, ptdev->gpu_info.gpu_id >> 16, >> major, minor, status); >> >> drm_info(&ptdev->base, >> diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h >> index ba452c1dd644..d9e0769d6f1a 100644 >> --- a/drivers/gpu/drm/panthor/panthor_regs.h >> +++ b/drivers/gpu/drm/panthor/panthor_regs.h >> @@ -20,6 +20,7 @@ >> #define GPU_VER_STATUS(x) ((x) & GENMASK(3, 0)) >> >> #define GPU_ARCH_ID_MAKE(major, minor, rev) (((major) << 16) | ((minor) << 8) | (rev)) >> +#define GPU_PROD_ID_MAKE(arch_major, prod_major) (((arch_major) << 24) | (prod_major)) >> >> #define GPU_L2_FEATURES 0x4 >> #define GPU_L2_FEATURES_LINE_SIZE(x) (1 << ((x) & GENMASK(7, 0))) >
On Thu, 10 Apr 2025 14:20:59 +0100 Karunika Choo <karunika.choo@arm.com> wrote: > On 21/03/2025 08:02, Boris Brezillon wrote: > > On Thu, 20 Mar 2025 11:17:37 +0000 > > Karunika Choo <karunika.choo@arm.com> wrote: > > > >> This patch replaces the previous panthor_model structure with a simple > >> switch case based on the product_id, which is in the format of: > >> ((arch_major << 24) | product_major) > >> > >> This not only simplifies the comparison, but also allows extending the > >> function to accommodate naming differences based on GPU features. > >> > >> Signed-off-by: Karunika Choo <karunika.choo@arm.com> > >> --- > >> drivers/gpu/drm/panthor/panthor_hw.c | 63 +++++++------------------- > >> drivers/gpu/drm/panthor/panthor_regs.h | 1 + > >> 2 files changed, 18 insertions(+), 46 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c > >> index 4cc4b0d5382c..12183c04cd21 100644 > >> --- a/drivers/gpu/drm/panthor/panthor_hw.c > >> +++ b/drivers/gpu/drm/panthor/panthor_hw.c > >> @@ -5,40 +5,6 @@ > >> #include "panthor_hw.h" > >> #include "panthor_regs.h" > >> > >> -/** > >> - * 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), > >> - {}, > >> -}; > >> - > >> static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) > >> { > >> unsigned int i; > >> @@ -66,29 +32,34 @@ static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) > >> ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO); > >> } > >> > >> +static char *get_gpu_model_name(struct panthor_device *ptdev) > >> +{ > >> + const u32 gpu_id = ptdev->gpu_info.gpu_id; > >> + const u32 product_id = GPU_PROD_ID_MAKE(GPU_ARCH_MAJOR(gpu_id), > >> + GPU_PROD_MAJOR(gpu_id)); > >> + > >> + switch (product_id) { > >> + case GPU_PROD_ID_MAKE(10, 7): > >> + return "Mali-G610"; > >> + } > > > > I a big fan of these ever growing switch statements with nested > > conditionals. Could we instead add an optional ::get_variant() callback > > in panthor_model and have the following formatting: > > > > "Mali-%s%s%s", model->name, > > model->get_variant ? "-" : "", > > model->get_variant ? model->get_variant() : "" > > > > While that’s certainly an option, I wonder if it’s better to avoid > additional string formatting when it’s not strictly necessary. The > switch cases provide a straightforward GPU name without needing to > handle conditional "-" separators or similar. > > Also, with the current approach, if a GPU is misconfigured with an > incorrect product_major for its core count, the switch’s fallthrough > helps ensure the correct name is still returned. A model->get_variant() > callback wouldn’t give us that same flexibility to adjust the name based > on such mismatches. Fair enough. I guess we can live with this sort of switch statement for the name selection. Hopefully the variants are rare enough that it doesn't go too wild.
diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c index 4cc4b0d5382c..12183c04cd21 100644 --- a/drivers/gpu/drm/panthor/panthor_hw.c +++ b/drivers/gpu/drm/panthor/panthor_hw.c @@ -5,40 +5,6 @@ #include "panthor_hw.h" #include "panthor_regs.h" -/** - * 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), - {}, -}; - static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) { unsigned int i; @@ -66,29 +32,34 @@ static void arch_10_8_gpu_info_init(struct panthor_device *ptdev) ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT_LO); } +static char *get_gpu_model_name(struct panthor_device *ptdev) +{ + const u32 gpu_id = ptdev->gpu_info.gpu_id; + const u32 product_id = GPU_PROD_ID_MAKE(GPU_ARCH_MAJOR(gpu_id), + GPU_PROD_MAJOR(gpu_id)); + + switch (product_id) { + case GPU_PROD_ID_MAKE(10, 7): + return "Mali-G610"; + } + + return "(Unknown Mali GPU)"; +} + static void panthor_gpu_init_info(struct panthor_device *ptdev) { - const struct panthor_model *model; - u32 arch_major, product_major; + const char *gpu_model_name = get_gpu_model_name(ptdev); u32 major, minor, status; ptdev->hw->ops.gpu_info_init(ptdev); - 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, + "%s id 0x%x major 0x%x minor 0x%x status 0x%x", + gpu_model_name, ptdev->gpu_info.gpu_id >> 16, major, minor, status); drm_info(&ptdev->base, diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h index ba452c1dd644..d9e0769d6f1a 100644 --- a/drivers/gpu/drm/panthor/panthor_regs.h +++ b/drivers/gpu/drm/panthor/panthor_regs.h @@ -20,6 +20,7 @@ #define GPU_VER_STATUS(x) ((x) & GENMASK(3, 0)) #define GPU_ARCH_ID_MAKE(major, minor, rev) (((major) << 16) | ((minor) << 8) | (rev)) +#define GPU_PROD_ID_MAKE(arch_major, prod_major) (((arch_major) << 24) | (prod_major)) #define GPU_L2_FEATURES 0x4 #define GPU_L2_FEATURES_LINE_SIZE(x) (1 << ((x) & GENMASK(7, 0)))
This patch replaces the previous panthor_model structure with a simple switch case based on the product_id, which is in the format of: ((arch_major << 24) | product_major) This not only simplifies the comparison, but also allows extending the function to accommodate naming differences based on GPU features. Signed-off-by: Karunika Choo <karunika.choo@arm.com> --- drivers/gpu/drm/panthor/panthor_hw.c | 63 +++++++------------------- drivers/gpu/drm/panthor/panthor_regs.h | 1 + 2 files changed, 18 insertions(+), 46 deletions(-)