Re: [PATCH v5 2/2] drm/xe/guc: Eliminate RPa frequency caching

["Anirban, Sk" <sk.anirban@intel.com>]

Hi,

On 04-11-2025 20:17, Nilawar, Badal wrote:
>
> On 04-11-2025 16:12, Sk Anirban wrote:
>> Remove the cached pc->rpa_freq field and refactor RPA frequency handling
>> to fetch values directly from hardware registers on each request.
>>
>> v2: Check graphics version instead of platform (Rodrigo)
>>
>> Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Suggested-by: Badal Nilawar <badal.nilawar@intel.com>
>> Signed-off-by: Sk Anirban <sk.anirban@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_pc.c       | 55 +++++++++++++---------------
>>   drivers/gpu/drm/xe/xe_guc_pc_types.h |  2 -
>>   2 files changed, 26 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c 
>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index a9c29f123b37..01fc8ecb373d 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -362,7 +362,7 @@ static int pc_set_max_freq(struct xe_guc_pc *pc, 
>> u32 freq)
>>                      freq);
>>   }
>>   -static void mtl_update_rpa_value(struct xe_guc_pc *pc)
>> +static u32 mtl_get_rpa_freq(struct xe_guc_pc *pc)
>>   {
>>       struct xe_gt *gt = pc_to_gt(pc);
>>       u32 reg;
>> @@ -372,7 +372,7 @@ static void mtl_update_rpa_value(struct xe_guc_pc 
>> *pc)
>>       else
>>           reg = xe_mmio_read32(&gt->mmio, MTL_GT_RPA_FREQUENCY);
>>   -    pc->rpa_freq = decode_freq(REG_FIELD_GET(MTL_RPA_MASK, reg));
>> +    return decode_freq(REG_FIELD_GET(MTL_RPA_MASK, reg));
>>   }
>>     static u32 mtl_get_rpe_freq(struct xe_guc_pc *pc)
>> @@ -388,24 +388,28 @@ static u32 mtl_get_rpe_freq(struct xe_guc_pc *pc)
>>       return decode_freq(REG_FIELD_GET(MTL_RPE_MASK, reg));
>>   }
>>   -static void tgl_update_rpa_value(struct xe_guc_pc *pc)
>> +static u32 pvc_get_rpa_freq(struct xe_guc_pc *pc)
>>   {
>> -    struct xe_gt *gt = pc_to_gt(pc);
>> -    struct xe_device *xe = gt_to_xe(gt);
>> -    u32 reg;
>> -
>>       /*
>>        * For PVC we still need to use fused RP0 as the approximation 
>> for RPa
>>        * For other platforms than PVC we get the resolved RPa 
>> directly from
>>        * PCODE at a different register
>>        */
>> -    if (xe->info.platform == XE_PVC) {
>> -        reg = xe_mmio_read32(&gt->mmio, PVC_RP_STATE_CAP);
>> -        pc->rpa_freq = REG_FIELD_GET(RP0_MASK, reg) * 
>> GT_FREQUENCY_MULTIPLIER;
>> -    } else {
>> -        reg = xe_mmio_read32(&gt->mmio, FREQ_INFO_REC);
>> -        pc->rpa_freq = REG_FIELD_GET(RPA_MASK, reg) * 
>> GT_FREQUENCY_MULTIPLIER;
>> -    }
>> +
>> +    struct xe_gt *gt = pc_to_gt(pc);
>> +    u32 reg;
>> +
>> +    reg = xe_mmio_read32(&gt->mmio, PVC_RP_STATE_CAP);
>> +    return REG_FIELD_GET(RP0_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
>> +}
>> +
>> +static u32 tgl_get_rpa_freq(struct xe_guc_pc *pc)
>> +{
>> +    struct xe_gt *gt = pc_to_gt(pc);
>> +    u32 reg;
>> +
>> +    reg = xe_mmio_read32(&gt->mmio, FREQ_INFO_REC);
>> +    return REG_FIELD_GET(RPA_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
>>   }
>>     static u32 pvc_get_rpe_freq(struct xe_guc_pc *pc)
>> @@ -426,17 +430,6 @@ static u32 tgl_get_rpe_freq(struct xe_guc_pc *pc)
>>       return REG_FIELD_GET(RPE_MASK, reg) * GT_FREQUENCY_MULTIPLIER;
>>   }
>>   -static void pc_update_rp_values(struct xe_guc_pc *pc)
>> -{
>> -    struct xe_gt *gt = pc_to_gt(pc);
>> -    struct xe_device *xe = gt_to_xe(gt);
>> -
>> -    if (GRAPHICS_VERx100(xe) >= 1270)
>> -        mtl_update_rpa_value(pc);
>> -    else
>> -        tgl_update_rpa_value(pc);
>> -}
>> -
>>   /**
>>    * xe_guc_pc_get_act_freq - Get Actual running frequency
>>    * @pc: The GuC PC
>> @@ -535,9 +528,15 @@ u32 xe_guc_pc_get_rp0_freq(struct xe_guc_pc *pc)
>>    */
>>   u32 xe_guc_pc_get_rpa_freq(struct xe_guc_pc *pc)
>>   {
>> -    pc_update_rp_values(pc);
>> +    struct xe_gt *gt = pc_to_gt(pc);
>> +    struct xe_device *xe = gt_to_xe(gt);
>>   -    return pc->rpa_freq;
>> +    if (GRAPHICS_VERx100(xe) >= 1260)
>> +        return pvc_get_rpa_freq(pc);
>> +    else if (GRAPHICS_VERx100(xe) >= 1270)
>> +        return mtl_get_rpa_freq(pc);
>
> This code will never execute in its current position; it should be 
> checked first. The correct order is:
>
> if (GRAPHICS_VERx100(xe) >= 1270)
>     return mtl_get_rpa_freq(pc);
> else if (GRAPHICS_VERx100(xe) >= 1260)
>     return pvc_get_rpa_freq(pc);
> Regards, Badal
>
I had intended to check only for PVC here, my mistake. I've sent the 
corrected version.

Thanks,
Anirban
>> +    else
>> +        return tgl_get_rpa_freq(pc);
>>   }
>>     /**
>> @@ -1134,8 +1133,6 @@ static int pc_init_freqs(struct xe_guc_pc *pc)
>>       if (ret)
>>           goto out;
>>   -    pc_update_rp_values(pc);
>> -
>>       pc_init_pcode_freq(pc);
>>         /*
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc_types.h 
>> b/drivers/gpu/drm/xe/xe_guc_pc_types.h
>> index f27c05d81706..711bbcdcb0d3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc_types.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc_types.h
>> @@ -19,8 +19,6 @@ struct xe_guc_pc {
>>       atomic_t flush_freq_limit;
>>       /** @rp0_freq: HW RP0 frequency - The Maximum one */
>>       u32 rp0_freq;
>> -    /** @rpa_freq: HW RPa frequency - The Achievable one */
>> -    u32 rpa_freq;
>>       /** @rpn_freq: HW RPN frequency - The Minimum one */
>>       u32 rpn_freq;
>>       /** @user_requested_min: Stash the minimum requested freq by 
>> user */