Re: [PATCH v7 01/11] drm/i915/perf: Drop wakeref on GuC RC error

[Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>]

On Fri, 17 Mar 2023, Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx> wrote:
> From: Chris Wilson <chris.p.wilson@xxxxxxxxxxxxxxx>
>
> If we fail to adjust the GuC run-control on opening the perf stream,
> make sure we unwind the wakeref just taken.
>
> v2: Retain old goto label names (Ashutosh)
>
> Fixes: 01e742746785 ("drm/i915/guc: Support OA when Wa_16011777198 is enabled")
> Signed-off-by: Chris Wilson <chris.p.wilson@xxxxxxxxxxxxxxx>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_perf.c       | 14 +++++++++-----
>  drivers/gpu/drm/i915/i915_perf_types.h |  6 ++++++
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 824a34ec0b83..283a4a3c6862 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1592,9 +1592,7 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
>  	/*
>  	 * Wa_16011777198:dg2: Unset the override of GUCRC mode to enable rc6.
>  	 */
> -	if (intel_uc_uses_guc_rc(&gt->uc) &&
> -	    (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) ||
> -	     IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0)))
> +	if (stream->override_gucrc)
>  		drm_WARN_ON(&gt->i915->drm,
>  			    intel_guc_slpc_unset_gucrc_mode(&gt->uc.guc.slpc));
>  
> @@ -3305,8 +3303,10 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>  		if (ret) {
>  			drm_dbg(&stream->perf->i915->drm,
>  				"Unable to override gucrc mode\n");
> -			goto err_config;
> +			goto err_gucrc;
>  		}
> +
> +		stream->override_gucrc = true;
>  	}
>  
>  	ret = alloc_oa_buffer(stream);
> @@ -3345,11 +3345,15 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>  	free_oa_buffer(stream);
>  
>  err_oa_buf_alloc:
> -	free_oa_configs(stream);
> +	if (stream->override_gucrc)
> +		intel_guc_slpc_unset_gucrc_mode(&gt->uc.guc.slpc);
>  
> +err_gucrc:
>  	intel_uncore_forcewake_put(stream->uncore, FORCEWAKE_ALL);
>  	intel_engine_pm_put(stream->engine);
>  
> +	free_oa_configs(stream);
> +
>  err_config:
>  	free_noa_wait(stream);
>  
> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index ca150b7af3f2..e36f046fe2b6 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -316,6 +316,12 @@ struct i915_perf_stream {
>  	 * buffer should be checked for available data.
>  	 */
>  	u64 poll_oa_period;
> +
> +	/**
> +	 * @override_gucrc: GuC RC has been overridden for the perf stream,
> +	 * and we need to restore the default configuration on release.
> +	 */
> +	bool override_gucrc:1;

What do you gain by making this a bitfield? It's already a big struct,
and there already are other bool flags.

BR,
Jani.



>  };
>  
>  /**

-- 
Jani Nikula, Intel Open Source Graphics Center