Re: [PATCH] drm/xe/slpc: Remove unnecessary force wakes

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 17-12-2024 01:11, Vinay Belgaumkar wrote:
> FORCEWAKE_ALL is not needed when we are trying to read the cur_freq,
> we just need to wake up the GT domain. We also do not need a force wake
> to for a H2G query to obtain min frequency.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc_pc.c | 26 +++++++-------------------
>   1 file changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index e8b9faeaef64..5f9828ffc19c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -421,8 +421,8 @@ int xe_guc_pc_get_cur_freq(struct xe_guc_pc *pc, u32 *freq)
>   	 * GuC SLPC plays with cur freq request when GuCRC is enabled
>   	 * Block RC6 for a more reliable read.
>   	 */
> -	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> -	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
> +	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FW_GT)) {

Since you are getting forcewake for single domain.

if(!fw)
     return -ETIMEDOUT;

is sufficient.

>   		xe_force_wake_put(gt_to_fw(gt), fw_ref);
>   		return -ETIMEDOUT;
>   	}
> @@ -481,10 +481,10 @@ u32 xe_guc_pc_get_rpn_freq(struct xe_guc_pc *pc)
>    */
>   int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq)
>   {
> -	struct xe_gt *gt = pc_to_gt(pc);
> -	unsigned int fw_ref;
>   	int ret;
>   
> +	xe_device_assert_mem_access(pc_to_xe(pc));
> +
>   	mutex_lock(&pc->freq_lock);
>   	if (!pc->freq_ready) {
>   		/* Might be in the middle of a gt reset */
> @@ -492,24 +492,12 @@ int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq)
>   		goto out;
>   	}
>   
> -	/*
> -	 * GuC SLPC plays with min freq request when GuCRC is enabled
> -	 * Block RC6 for a more reliable read.
> -	 */
> -	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> -	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
> -		ret = -ETIMEDOUT;
> -		goto fw;
> -	}
> -
>   	ret = pc_action_query_task_state(pc);
>   	if (ret)
> -		goto fw;
> +		goto out;
>   
>   	*freq = pc_get_min_freq(pc);
>   
> -fw:
> -	xe_force_wake_put(gt_to_fw(gt), fw_ref);
>   out:
>   	mutex_unlock(&pc->freq_lock);
>   	return ret;
> @@ -969,8 +957,8 @@ int xe_guc_pc_start(struct xe_guc_pc *pc)
>   
>   	xe_gt_assert(gt, xe_device_uc_enabled(xe));
>   
> -	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> -	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
> +	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> +	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FW_GT)) {

same as above.

With these addressed. Patch Looks good to me.
Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>


>   		xe_force_wake_put(gt_to_fw(gt), fw_ref);
>   		return -ETIMEDOUT;
>   	}