Re: [PATCH 2/8] drm/i915/dmc: Use RUNTIME_INFO->step for DMC

[Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>]

On Wed, 07 Jul 2021, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Tue, 06 Jul 2021, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> wrote:
>> Instead of adding new table for every new platform, lets ues
>> the stepping info from RUNTIME_INFO(dev_priv)->step
>> This patch uses RUNTIME_INFO->step only for recent
>> platforms.
>>
>> Patches that follow this will address this change for
>> remaining platforms + missing platforms.
>>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dmc.c | 61 +++++++++++++++++++++---
>>  1 file changed, 54 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>> index f8789d4543bf..a38720f25910 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>> @@ -266,10 +266,12 @@ static const struct stepping_info icl_stepping_info[] = {
>>  };
>>  
>>  static const struct stepping_info no_stepping_info = { '*', '*' };
>> +struct stepping_info *display_step;
>
> We can't have driver specific mutable data for this. Almost everything
> has to be either device specific or const. The above would be shared
> between all devices.

I think the solution to your problem is two-fold.

First, I think you should add a *generic* function in intel_step.c to
get the chars or a string for an enum intel_step. Maybe a string,
because it'll also be useful for logging?

const char *intel_step_name(enum intel_step step)
{
	switch (step) {
        case STEP_A0:
		return "A0";
	case STEP_B0;
                /* etc ... */
	default:
		return "??";
        }
}

Second, I think you should modify intel_get_stepping_info() to let you
pass in the struct stepping_info pointer to fill in. Then you can have a
local struct stepping_info si variable in parse_dmc_fw(). We don't need
to store the data anywhere, it's only used once.

static void intel_get_stepping_info(struct drm_i915_private *dev_priv,
       struct stepping_info *si)

There you'd do something like:

	const char *step_name = intel_step_name(step);

	si->stepping = step_name[0];
        si->stepping = step_name[1];

And potentially handle the ?? case separately. Something along those
lines.

BR,
Jani.


>
> BR,
> Jani.
>
>>  
>>  static const struct stepping_info *
>>  intel_get_stepping_info(struct drm_i915_private *dev_priv)
>>  {
>> +	struct intel_step_info step = RUNTIME_INFO(dev_priv)->step;
>>  	const struct stepping_info *si;
>>  	unsigned int size;
>>  
>> @@ -282,15 +284,60 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv)
>>  	} else if (IS_BROXTON(dev_priv)) {
>>  		size = ARRAY_SIZE(bxt_stepping_info);
>>  		si = bxt_stepping_info;
>> -	} else {
>> -		size = 0;
>> -		si = NULL;
>>  	}
>>  
>> -	if (INTEL_REVID(dev_priv) < size)
>> -		return si + INTEL_REVID(dev_priv);
>> -
>> -	return &no_stepping_info;
>> +	if (IS_ICELAKE(dev_priv) || IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
>> +		return INTEL_REVID(dev_priv) < size ? si + INTEL_REVID(dev_priv) : &no_stepping_info;
>> +
>> +	else {
>> +		switch (step.display_step) {
>> +		case STEP_A0:
>> +			display_step->stepping = 'A';
>> +			display_step->substepping = '0';
>> +			break;
>> +		case STEP_A2:
>> +			display_step->stepping = 'A';
>> +			display_step->substepping = '2';
>> +			break;
>> +		case STEP_B0:
>> +			display_step->stepping = 'B';
>> +			display_step->substepping = '0';
>> +			break;
>> +		case STEP_B1:
>> +			display_step->stepping = 'B';
>> +			display_step->substepping = '1';
>> +			break;
>> +		case STEP_C0:
>> +			display_step->stepping = 'C';
>> +			display_step->substepping = '0';
>> +			break;
>> +		case STEP_D0:
>> +			display_step->stepping = 'D';
>> +			display_step->substepping = '0';
>> +			break;
>> +		case STEP_D1:
>> +			display_step->stepping = 'D';
>> +			display_step->substepping = '1';
>> +			break;
>> +		case STEP_E0:
>> +			display_step->stepping = 'E';
>> +			display_step->substepping = '0';
>> +			break;
>> +		case STEP_F0:
>> +			display_step->stepping = 'F';
>> +			display_step->substepping = '0';
>> +			break;
>> +		case STEP_G0:
>> +			display_step->stepping = 'G';
>> +			display_step->substepping = '0';
>> +			break;
>> +		default:
>> +			display_step->stepping = '*';
>> +			display_step->substepping = '*';
>> +			break;
>> +		}
>> +	}
>> +	return display_step;
>>  }
>>  
>>  static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx