Re: [Intel-xe] [PATCH] drm/xe/mtl: Use 16.67 Mhz freq scale factor to get rpX

["Nilawar, Badal" <badal.nilawar@intel.com>]

Hi Ashutosh,

On 01-11-2023 20:53, Dixit, Ashutosh wrote:
> On Wed, 01 Nov 2023 08:17:56 -0700, Badal Nilawar wrote:
>>
> 
> Hi Badal,
> 
>> For mtl and above 16.67 Mhz is the scale factor to calculate
>> rpX frequencies.
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_pc.c | 25 ++++++++++++++-----------
>>   1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 74247e0d3674..90e3e1b56c27 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -306,14 +306,15 @@ static int pc_set_max_freq(struct xe_guc_pc *pc, u32 freq)
>>   static void mtl_update_rpe_value(struct xe_guc_pc *pc)
>>   {
>> 	struct xe_gt *gt = pc_to_gt(pc);
>> -	u32 reg;
>> +	u32 freq;
>>
>> 	if (xe_gt_is_media_type(gt))
>> -		reg = xe_mmio_read32(gt, MTL_MPE_FREQUENCY);
>> +		freq = xe_mmio_read32(gt, MTL_MPE_FREQUENCY);
>> 	else
>> -		reg = xe_mmio_read32(gt, MTL_GT_RPE_FREQUENCY);
>> +		freq = xe_mmio_read32(gt, MTL_GT_RPE_FREQUENCY);
> 
> Generally I think we should avoid making these kind of variable name
> changes, if only to keep the patch small and reduce the review
> effort.
> 
> In this case, isn't 'reg' more appropriate anyway? If we retained reg this
> would be a single line change in this function.
Sure I will retain 'reg' and make the change this way

pc->rpe_freq = decode_freq(REG_FIELD_GET(MTL_RPE_MASK, freq));

for here and other two places.

Regards,
Badal
> 
>>
>> -	pc->rpe_freq = REG_FIELD_GET(MTL_RPE_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
>> +	freq = REG_FIELD_GET(MTL_RPE_MASK, freq);
>> +	pc->rpe_freq = decode_freq(freq);
>>   }
>>
>>   static void tgl_update_rpe_value(struct xe_guc_pc *pc)
>> @@ -645,18 +646,20 @@ static const struct attribute *pc_attrs[] = {
>>   static void mtl_init_fused_rp_values(struct xe_guc_pc *pc)
>>   {
>> 	struct xe_gt *gt = pc_to_gt(pc);
>> -	u32 reg;
>> +	u32 freq;
>>
>> 	xe_device_assert_mem_access(pc_to_xe(pc));
>>
>> 	if (xe_gt_is_media_type(gt))
>> -		reg = xe_mmio_read32(gt, MTL_MEDIAP_STATE_CAP);
>> +		freq = xe_mmio_read32(gt, MTL_MEDIAP_STATE_CAP);
>> 	else
>> -		reg = xe_mmio_read32(gt, MTL_RP_STATE_CAP);
>> -	pc->rp0_freq = REG_FIELD_GET(MTL_RP0_CAP_MASK, reg) *
>> -		GT_FREQUENCY_MULTIPLIER;
>> -	pc->rpn_freq = REG_FIELD_GET(MTL_RPN_CAP_MASK, reg) *
>> -		GT_FREQUENCY_MULTIPLIER;
>> +		freq = xe_mmio_read32(gt, MTL_RP_STATE_CAP);
>> +
>> +	freq = REG_FIELD_GET(MTL_RP0_CAP_MASK, freq);
>> +	pc->rp0_freq = decode_freq(freq);
>> +
>> +	freq = REG_FIELD_GET(MTL_RPN_CAP_MASK, freq);
>> +	pc->rpn_freq = decode_freq(freq);
> 
> Similar comments here too.
> 
>>   }
>>
>>   static void tgl_init_fused_rp_values(struct xe_guc_pc *pc)
>> --
>> 2.25.1
>>
> 
> Thanks.
> --
> Ashutosh