> -----Original Message-----
> From: De Marchi, Lucas <[email protected]>
> Sent: Friday, July 9, 2021 10:53 AM
> To: Roper, Matthew D <[email protected]>
> Cc: Srivatsa, Anusha <[email protected]>; intel-
> [email protected]; Jani Nikula <[email protected]>
> Subject: Re: [Intel-gfx] [PATCH 09/10] drm/i915/step: Add intel_step_name()
> helper
>
> On Thu, Jul 08, 2021 at 09:16:16PM -0700, Matt Roper wrote:
> >On Thu, Jul 08, 2021 at 04:18:20PM -0700, Anusha Srivatsa wrote:
> >> Add a helper to convert the step info to string.
> >> This is specifically useful when we want to load a specific firmware
> >> for a given stepping/substepping combination.
> >
> >What if we use macros to generate the per-stepping code here as well as
> >the stepping values in the enum?
> >
> >In intel_step.h:
> >
> > #define STEPPING_NAME_LIST(func) \
> > func(A0)
> > func(A1)
> > func(A2)
> > func(B0)
> > ...
> >
> > #define STEPPING_ENUM_VAL(name) STEP_##name,
> >
> > enum intel_step {
> > STEP_NONE = 0,
> > STEPPING_NAME_LIST(STEPPING_ENUM_VAL)
> > STEP_FUTURE,
> > STEP_FOREVER,
> > };
> >
> >and in intel_step.c:
> >
> > #define STEPPING_NAME_CASE(name) \
> > case STEP_##name: \
> > return #name; \
> > break;
> >
> > const char *intel_step_name(enum intel_step step) {
> > switch(step) {
> > STEPPING_NAME_LIST(STEPPING_NAME_CASE)
> >
> > default:
> > return "**";
> > }
> > }
> >
> >This has the advantage that anytime a new stepping is added (in
> >STEPPING_NAME_LIST) it will generate a new "STEP_XX" enum value and a
> >new case statement to return "XX" as the name; we won't have to
> >remember to update two separate places in the code.
>
> my other idea in the first iterations of this patch was to turn the stepping
> into
> u16 and then do something like (untested crap code below):
>
> #define make_step(a, b) ((a - 'A') << 8, (b - '0'))
>
> #define intel_step_name(s) ({
> char ret[3];
> ret[0] = ((s) >> 8) + 'A';
> ret[1] = ((s) & 0xff) + '0';
> ret[2] = '\0';
> ret;
> })
>
> enum intel_step {
> STEP_NONE = -1,
> STEP_A0 = make_step('A', '0'),
> ...
> }
>
> Or even not bother with the 'A'/'0' addition/subraction since 8 bits is enough
> for all the letters and numbers.
>
> If we keep it u8, then we are limited to step P7 (assuming we have 2
> reserved entries at the end),. It may or may not be sufficient (it currently
> is)
>
> better? worse?
I feel If Matt's solution is more scalable, better to go with it.
Anusha
> Lucas De Marchi
>
> >
> >
> >Matt
> >
> >>
> >> Suggested-by: Jani Nikula <[email protected]>
> >> Signed-off-by: Anusha Srivatsa <[email protected]>
> >> ---
> >> drivers/gpu/drm/i915/intel_step.c | 58
> >> +++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_step.h |
> >> 1 +
> >> 2 files changed, 59 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_step.c
> >> b/drivers/gpu/drm/i915/intel_step.c
> >> index 99c0d3df001b..9af7f30b777e 100644
> >> --- a/drivers/gpu/drm/i915/intel_step.c
> >> +++ b/drivers/gpu/drm/i915/intel_step.c
> >> @@ -182,3 +182,61 @@ void intel_step_init(struct drm_i915_private
> >> *i915)
> >>
> >> RUNTIME_INFO(i915)->step = step;
> >> }
> >> +
> >> +const char *intel_step_name(enum intel_step step) {
> >> + switch (step) {
> >> + case STEP_A0:
> >> + return "A0";
> >> + break;
> >> + case STEP_A1:
> >> + return "A1";
> >> + break;
> >> + case STEP_A2:
> >> + return "A2";
> >> + break;
> >> + case STEP_B0:
> >> + return "B0";
> >> + break;
> >> + case STEP_B1:
> >> + return "B1";
> >> + break;
> >> + case STEP_B2:
> >> + return "B2";
> >> + break;
> >> + case STEP_C0:
> >> + return "C0";
> >> + break;
> >> + case STEP_C1:
> >> + return "C1";
> >> + break;
> >> + case STEP_D0:
> >> + return "D0";
> >> + break;
> >> + case STEP_D1:
> >> + return "D1";
> >> + break;
> >> + case STEP_E0:
> >> + return "E0";
> >> + break;
> >> + case STEP_F0:
> >> + return "F0";
> >> + break;
> >> + case STEP_G0:
> >> + return "G0";
> >> + break;
> >> + case STEP_H0:
> >> + return "H0";
> >> + break;
> >> + case STEP_I0:
> >> + return "I0";
> >> + break;
> >> + case STEP_I1:
> >> + return "I1";
> >> + break;
> >> + case STEP_J0:
> >> + return "J0";
> >> + break;
> >> + default:
> >> + return "**";
> >> + }
> >> +}
> >> diff --git a/drivers/gpu/drm/i915/intel_step.h
> >> b/drivers/gpu/drm/i915/intel_step.h
> >> index 3e8b2babd9da..2fbe51483472 100644
> >> --- a/drivers/gpu/drm/i915/intel_step.h
> >> +++ b/drivers/gpu/drm/i915/intel_step.h
> >> @@ -43,5 +43,6 @@ enum intel_step {
> >> };
> >>
> >> void intel_step_init(struct drm_i915_private *i915);
> >> +const char *intel_step_name(enum intel_step step);
> >>
> >> #endif /* __INTEL_STEP_H__ */
> >> --
> >> 2.32.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >--
> >Matt Roper
> >Graphics Software Engineer
> >VTT-OSGC Platform Enablement
> >Intel Corporation
> >(916) 356-2795
> >_______________________________________________
> >Intel-gfx mailing list
> >[email protected]
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
