Re: [PATCH v4 1/3] drm/xe/guc_pc: Add _locked variant for min/max freq

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

On Sun, Jun 15, 2025 at 11:17:34PM -0700, Lucas De Marchi wrote:
> There are places in which the getters/setters are called one after the
> other causing a multiple lock()/unlock(). These are not currently a
> problem since they are all happening from the same thread, but there's a
> race possibility as calls are added outside of the early init when the
> max/min and stashed values need to be correlated.
> 
> Add the _locked() variants to prepare for that.
> 
> 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 | 124 +++++++++++++++++++++++------------------
>  1 file changed, 70 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 39d2acb2f30f6..53aaf937d4bec 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -5,6 +5,7 @@
>  
>  #include "xe_guc_pc.h"
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/ktime.h>
>  
> @@ -554,6 +555,25 @@ u32 xe_guc_pc_get_rpn_freq(struct xe_guc_pc *pc)
>  	return pc->rpn_freq;
>  }
>  
> +static int xe_guc_pc_get_min_freq_locked(struct xe_guc_pc *pc, u32 *freq)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&pc->freq_lock);
> +
> +	/* Might be in the middle of a gt reset */
> +	if (!pc->freq_ready)
> +		return -EAGAIN;
> +
> +	ret = pc_action_query_task_state(pc);
> +	if (ret)
> +		return ret;
> +
> +	*freq = pc_get_min_freq(pc);
> +
> +	return 0;
> +}
> +
>  /**
>   * xe_guc_pc_get_min_freq - Get the min operational frequency
>   * @pc: The GuC PC
> @@ -563,27 +583,29 @@ u32 xe_guc_pc_get_rpn_freq(struct xe_guc_pc *pc)
>   *         -EAGAIN if GuC PC not ready (likely in middle of a reset).
>   */
>  int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq)
> +{
> +	guard(mutex)(&pc->freq_lock);
> +
> +	return xe_guc_pc_get_min_freq_locked(pc, freq);
> +}
> +
> +static int xe_guc_pc_set_min_freq_locked(struct xe_guc_pc *pc, u32 freq)
>  {
>  	int ret;
>  
> -	xe_device_assert_mem_access(pc_to_xe(pc));
> +	lockdep_assert_held(&pc->freq_lock);
>  
> -	mutex_lock(&pc->freq_lock);
> -	if (!pc->freq_ready) {
> -		/* Might be in the middle of a gt reset */
> -		ret = -EAGAIN;
> -		goto out;
> -	}
> +	/* Might be in the middle of a gt reset */
> +	if (!pc->freq_ready)
> +		return -EAGAIN;
>  
> -	ret = pc_action_query_task_state(pc);
> +	ret = pc_set_min_freq(pc, freq);
>  	if (ret)
> -		goto out;
> +		return ret;
>  
> -	*freq = pc_get_min_freq(pc);
> +	pc->user_requested_min = freq;
>  
> -out:
> -	mutex_unlock(&pc->freq_lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> @@ -596,25 +618,30 @@ int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq)
>   *         -EINVAL if value out of bounds.
>   */
>  int xe_guc_pc_set_min_freq(struct xe_guc_pc *pc, u32 freq)
> +{
> +	guard(mutex)(&pc->freq_lock);
> +
> +	return xe_guc_pc_set_min_freq_locked(pc, freq);
> +}
> +
> +
> +static int xe_guc_pc_get_max_freq_locked(struct xe_guc_pc *pc, u32 *freq)
>  {
>  	int ret;
>  
> -	mutex_lock(&pc->freq_lock);
> -	if (!pc->freq_ready) {
> -		/* Might be in the middle of a gt reset */
> -		ret = -EAGAIN;
> -		goto out;
> -	}
> +	lockdep_assert_held(&pc->freq_lock);
>  
> -	ret = pc_set_min_freq(pc, freq);
> +	/* Might be in the middle of a gt reset */
> +	if (!pc->freq_ready)
> +		return -EAGAIN;
> +
> +	ret = pc_action_query_task_state(pc);
>  	if (ret)
> -		goto out;
> +		return ret;
>  
> -	pc->user_requested_min = freq;
> +	*freq = pc_get_max_freq(pc);
>  
> -out:
> -	mutex_unlock(&pc->freq_lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> @@ -626,25 +653,29 @@ int xe_guc_pc_set_min_freq(struct xe_guc_pc *pc, u32 freq)
>   *         -EAGAIN if GuC PC not ready (likely in middle of a reset).
>   */
>  int xe_guc_pc_get_max_freq(struct xe_guc_pc *pc, u32 *freq)
> +{
> +	guard(mutex)(&pc->freq_lock);
> +
> +	return xe_guc_pc_get_max_freq_locked(pc, freq);
> +}
> +
> +static int xe_guc_pc_set_max_freq_locked(struct xe_guc_pc *pc, u32 freq)
>  {
>  	int ret;
>  
> -	mutex_lock(&pc->freq_lock);
> -	if (!pc->freq_ready) {
> -		/* Might be in the middle of a gt reset */
> -		ret = -EAGAIN;
> -		goto out;
> -	}
> +	lockdep_assert_held(&pc->freq_lock);
>  
> -	ret = pc_action_query_task_state(pc);
> +	/* Might be in the middle of a gt reset */
> +	if (!pc->freq_ready)
> +		return -EAGAIN;
> +
> +	ret = pc_set_max_freq(pc, freq);
>  	if (ret)
> -		goto out;
> +		return ret;
>  
> -	*freq = pc_get_max_freq(pc);
> +	pc->user_requested_max = freq;
>  
> -out:
> -	mutex_unlock(&pc->freq_lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> @@ -658,24 +689,9 @@ int xe_guc_pc_get_max_freq(struct xe_guc_pc *pc, u32 *freq)
>   */
>  int xe_guc_pc_set_max_freq(struct xe_guc_pc *pc, u32 freq)
>  {
> -	int ret;
> -
> -	mutex_lock(&pc->freq_lock);
> -	if (!pc->freq_ready) {
> -		/* Might be in the middle of a gt reset */
> -		ret = -EAGAIN;
> -		goto out;
> -	}
> -
> -	ret = pc_set_max_freq(pc, freq);
> -	if (ret)
> -		goto out;
> +	guard(mutex)(&pc->freq_lock);
>  
> -	pc->user_requested_max = freq;
> -
> -out:
> -	mutex_unlock(&pc->freq_lock);
> -	return ret;
> +	return xe_guc_pc_set_max_freq_locked(pc, freq);
>  }
>  
>  /**
> 
> -- 
> 2.49.0
>