Re: [PATCH v2] drm/xe: Check skip_guc_pc before setting SLPC flag

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 1/8/2024 10:14 AM, Lucas De Marchi wrote:
> On Thu, Jan 04, 2024 at 04:33:41PM -0800, Vinay Belgaumkar wrote:
>> Don't set SLPC GuC feature ctl flag if skip_guc_pc is true.
>>
>> v2: Skip the freq related sysfs creation as well (Badal)
>>
>> Fixes: 975e4a3795d4 ("drm/xe: Manually setup C6 when skip_guc_pc is 
>> set")
>> Fixes: bef52b5c7a19 ("drm/xe: Create a xe_gt_freq component for raw 
>> management and sysfs")
>> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_gt_freq.c | 3 +++
>> drivers/gpu/drm/xe/xe_guc.c     | 7 ++++++-
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_freq.c 
>> b/drivers/gpu/drm/xe/xe_gt_freq.c
>> index 3adfa6686e7c..e5b0f4ecdbe8 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_freq.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_freq.c
>> @@ -196,6 +196,9 @@ void xe_gt_freq_init(struct xe_gt *gt)
>>     struct xe_device *xe = gt_to_xe(gt);
>>     int err;
>>
>> +    if (xe->info.skip_guc_pc)
>> +        return;
>> +
>>     gt->freq = kobject_create_and_add("freq0", gt->sysfs);
>>     if (!gt->freq) {
>>         drm_warn(&xe->drm, "failed to add freq0 directory to %s\n",
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 811e8b201270..0e112b144ec8 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -62,7 +62,12 @@ static u32 guc_ctl_debug_flags(struct xe_guc *guc)
>>
>> static u32 guc_ctl_feature_flags(struct xe_guc *guc)
>> {
>> -    return GUC_CTL_ENABLE_SLPC;
>> +    u32 flags = 0;
>> +
>> +    if (!(guc_to_xe(guc))->info.skip_guc_pc)
>
> I was about to apply this, but then noticed this weird parenthesis.
> Did you intend to do !(guc_to_xe(guc)->info.skip_guc_pc) to clarify the
> operator precedence? Anyway, the parenthesis is not really needed and
> also not used anywhere else in the code base
> (git grep -e '_to_xe.*->info')... it's actually confusing.

Ok, we can remove the parenthesis in that case. Should I resend with that?

Thanks,

Vinay.

>
> Lucas De Marchi
>
>> +        flags |= GUC_CTL_ENABLE_SLPC;
>> +
>> +    return flags;
>> }
>>
>> static u32 guc_ctl_log_params_flags(struct xe_guc *guc)
>> -- 
>> 2.38.1
>>