Re: [Intel-xe] [PATCH] drm/xe/guc_pc: Reorder forcewake and xe_pm_runtime calls

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

On Wed, Apr 19, 2023 at 02:39:33PM +0530, Riana Tauro wrote:
> When the device is runtime suspended, reading some of the sysfs
> entries under device/gt#/ causes a resume error
> This is due to the ordering of pm_runtime and forcewake calls.
> Reorder to wake up using xe_pm_runtime_get and then forcewake
> 
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_pc.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 5a8d827ba770..f71f6b3df38a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -370,6 +370,7 @@ static ssize_t freq_act_show(struct device *dev,
>  	u32 freq;
>  	ssize_t ret;
>  
> +	xe_device_mem_access_get(gt_to_xe(gt));
>  	/*
>  	 * When in RC6, actual frequency is 0. Let's block RC6 so we are able
>  	 * to verify that our freq requests are really happening.
> @@ -378,8 +379,6 @@ static ssize_t freq_act_show(struct device *dev,
>  	if (ret)
>  		return ret;

you probably need to change these returns to some goto, no?!
(and others below as well...)

>  
> -	xe_device_mem_access_get(gt_to_xe(gt));
> -
>  	if (xe->info.platform == XE_METEORLAKE) {
>  		freq = xe_mmio_read32(gt, MTL_MIRROR_TARGET_WP1.reg);
>  		freq = REG_FIELD_GET(MTL_CAGF_MASK, freq);
> @@ -388,11 +387,10 @@ static ssize_t freq_act_show(struct device *dev,
>  		freq = REG_FIELD_GET(GEN12_CAGF_MASK, freq);
>  	}
>  
> -	xe_device_mem_access_put(gt_to_xe(gt));
> -
>  	ret = sysfs_emit(buf, "%d\n", decode_freq(freq));
>  
>  	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
> +	xe_device_mem_access_put(gt_to_xe(gt));
>  	return ret;
>  }
>  static DEVICE_ATTR_RO(freq_act);
> @@ -405,6 +403,7 @@ static ssize_t freq_cur_show(struct device *dev,
>  	u32 freq;
>  	ssize_t ret;
>  
> +	xe_device_mem_access_get(gt_to_xe(gt));
>  	/*
>  	 * GuC SLPC plays with cur freq request when GuCRC is enabled
>  	 * Block RC6 for a more reliable read.
> @@ -413,14 +412,13 @@ static ssize_t freq_cur_show(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	xe_device_mem_access_get(gt_to_xe(gt));
>  	freq = xe_mmio_read32(gt, GEN6_RPNSWREQ.reg);
> -	xe_device_mem_access_put(gt_to_xe(gt));
>  
>  	freq = REG_FIELD_GET(REQ_RATIO_MASK, freq);
>  	ret = sysfs_emit(buf, "%d\n", decode_freq(freq));
>  
>  	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
> +	xe_device_mem_access_put(gt_to_xe(gt));
>  	return ret;
>  }
>  static DEVICE_ATTR_RO(freq_cur);
> @@ -610,17 +608,16 @@ static ssize_t rc6_residency_show(struct device *dev,
>  	u32 reg;
>  	ssize_t ret;
>  
> +	xe_device_mem_access_get(pc_to_xe(pc));
>  	ret = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>  	if (ret)
>  		return ret;
>  
> -	xe_device_mem_access_get(pc_to_xe(pc));
>  	reg = xe_mmio_read32(gt, GEN6_GT_GFX_RC6.reg);
> -	xe_device_mem_access_put(pc_to_xe(pc));
> -
>  	ret = sysfs_emit(buff, "%u\n", reg);
>  
>  	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
> +	xe_device_mem_access_put(pc_to_xe(pc));
>  	return ret;
>  }
>  static DEVICE_ATTR_RO(rc6_residency);
> -- 
> 2.40.0
>