Re: [PATCH v4 2/3] drm/xe/xe_guc_pc: Lock once to update stashed frequencies

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Sun, Jun 15, 2025 at 11:17:35PM -0700, Lucas De Marchi wrote:
> pc_set_mert_freq_cap() currently lock()/unlock() the mutex multiple times
> to stash the current frequencies. It's not a problem since
> xe_guc_pc_restore_stashed_freq() is guaranteed to be called only later
> in the init sequence. However, now that we have _locked() variants for
> this functions, use them and avoid potential issues when called from
> other places or using the same pattern.
> 
> While at it, prefer and early return for the WA check to reduce
> indentation.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_guc_pc.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 53aaf937d4bec..d449eb0e3e8af 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -891,27 +891,28 @@ static int pc_adjust_requested_freq(struct xe_guc_pc *pc)
>  
>  static int pc_set_mert_freq_cap(struct xe_guc_pc *pc)
>  {
> -	int ret = 0;
> +	int ret;
>  
> -	if (XE_WA(pc_to_gt(pc), 22019338487)) {
> -		/*
> -		 * Get updated min/max and stash them.
> -		 */
> -		ret = xe_guc_pc_get_min_freq(pc, &pc->stashed_min_freq);
> -		if (!ret)
> -			ret = xe_guc_pc_get_max_freq(pc, &pc->stashed_max_freq);
> -		if (ret)
> -			return ret;
> +	if (!XE_WA(pc_to_gt(pc), 22019338487))
> +		return 0;
>  
> -		/*
> -		 * Ensure min and max are bound by MERT_FREQ_CAP until driver loads.
> -		 */
> -		mutex_lock(&pc->freq_lock);
> -		ret = pc_set_min_freq(pc, min(pc->rpe_freq, pc_max_freq_cap(pc)));
> -		if (!ret)
> -			ret = pc_set_max_freq(pc, min(pc->rp0_freq, pc_max_freq_cap(pc)));
> -		mutex_unlock(&pc->freq_lock);
> -	}
> +	guard(mutex)(&pc->freq_lock);
> +
> +	/*
> +	 * Get updated min/max and stash them.
> +	 */
> +	ret = xe_guc_pc_get_min_freq_locked(pc, &pc->stashed_min_freq);
> +	if (!ret)
> +		ret = xe_guc_pc_get_max_freq_locked(pc, &pc->stashed_max_freq);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Ensure min and max are bound by MERT_FREQ_CAP until driver loads.
> +	 */
> +	ret = pc_set_min_freq(pc, min(pc->rpe_freq, pc_max_freq_cap(pc)));
> +	if (!ret)
> +		ret = pc_set_max_freq(pc, min(pc->rp0_freq, pc_max_freq_cap(pc)));
>  
>  	return ret;
>  }
> 
> -- 
> 2.49.0
>