Re: [Intel-gfx] [PATCH v3] drm/i915/guc/slpc: Provide sysfs for efficient freq

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

On 4/14/2023 4:49 PM, Dixit, Ashutosh wrote:
> On Fri, 14 Apr 2023 15:34:15 -0700, Vinay Belgaumkar wrote:
>> @@ -457,6 +458,34 @@ int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val)
>> 	return ret;
>>   }
>>
>> +int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val)
>> +{
>> +	struct drm_i915_private *i915 = slpc_to_i915(slpc);
>> +	intel_wakeref_t wakeref;
>> +	int ret = 0;
>> +
>> +	/* Need a lock now since waitboost can be modifying min as well */
> Delete comment.
ok.
>> +	mutex_lock(&slpc->lock);
> Actually, don't need the lock itself now so delete the lock.
>
> Or, maybe the lock prevents the race if userspace writes to the sysfs when
> GuC reset is going on so let's retain the lock. But the comment is wrong.
yup, ok.
>
>> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>> +
>> +	/* Ignore efficient freq if lower min freq is requested */
> Delete comment, it's wrong.
ok.
>
>> +	ret = slpc_set_param(slpc,
>> +			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
>> +			     val);
>> +	if (ret) {
>> +		guc_probe_error(slpc_to_guc(slpc), "Failed to set efficient freq(%d): %pe\n",
>> +				val, ERR_PTR(ret));
>> +		goto out;
>> +	}
>> +
>> +	slpc->ignore_eff_freq = val;
>> +
> This extra line can also be deleted.
ok.
>
>> +out:
>> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>> +	mutex_unlock(&slpc->lock);
>> +	return ret;
>> +}
>> +
>>   /**
>>    * intel_guc_slpc_set_min_freq() - Set min frequency limit for SLPC.
>>    * @slpc: pointer to intel_guc_slpc.
>> @@ -482,16 +511,6 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
>> 	mutex_lock(&slpc->lock);
>> 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>>
>> -	/* Ignore efficient freq if lower min freq is requested */
>> -	ret = slpc_set_param(slpc,
>> -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
>> -			     val < slpc->rp1_freq);
>> -	if (ret) {
>> -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
>> -				ERR_PTR(ret));
>> -		goto out;
>> -	}
>> -
> Great, thanks!
>
> After taking care of the above, and seems there are also a couple of
> checkpatch errors, this is:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

Thanks,

Vinay.