Re: [PATCH 09/10] drm/i915/step: Add intel_step_name() helper

Lucas De Marchi <lucas.demarchi@xxxxxxxxx> · Fri, 9 Jul 2021 11:43:48 -0700

On Fri, Jul 09, 2021 at 11:36:09AM -0700, Anusha Srivatsa wrote:

-----Original Message-----
From: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
Sent: Friday, July 9, 2021 10:53 AM
To: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
Cc: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel-
gfx@xxxxxxxxxxxxxxxxxxxxx; Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
Subject: Re:  [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.

both scale the same from what I can see. So, in the end I think the
consideration would be:

	- how much magic do they bring? (less is more... and subjective)
	- .ko size increase considering the new tables for new
	  platforms (may be negligible)

Lucas De Marchi

Anusha
Lucas De Marchi

>
>
>Matt
>
>>
>> Suggested-by: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> ---
>>  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
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> 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
>Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx