Re: [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected

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

On Mon, Nov 21, 2022 at 03:16:16PM -0800, Daniele Ceraolo Spurio wrote:
> From: Jonathan Cavitt <jonathan.cavitt@intel.com>
> 
> The GSC CS is only used for communicating with the GSC FW, so no need to
> initialize it if we're not going to use the FW. If we're not using
> neither the engine nor the microcontoller, then we can also disable the
> power well.
> 
> IMPORTANT: lack of GSC FW breaks media C6 due to opposing requirements
> between CS setup and forcewake idleness. See in-code comment for detail.
> 
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: John C Harrison <John.C.Harrison@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.c       |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index c33e0d72d670..99c4b866addd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -894,6 +894,24 @@ static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
>  	engine_mask_apply_compute_fuses(gt);
>  	engine_mask_apply_copy_fuses(gt);
>  
> +	/*
> +	 * The only use of the GSC CS is to load and communicate with the GSC
> +	 * FW, so we have no use for it if we don't have the FW.
> +	 *
> +	 * IMPORTANT: in cases where we don't have the GSC FW, we have a
> +	 * catch-22 situation that breaks media C6 due to 2 requirements:
> +	 * 1) once turned on, the GSC power well will not go to sleep unless the
> +	 *    GSC FW is loaded.
> +	 * 2) to enable idling (which is required for media C6) we need to
> +	 *    initialize the IDLE_MSG register for the GSC CS and do at least 1
> +	 *    submission, which will wake up the GSC power well.
> +	 */
> +	if (__HAS_ENGINE(info->engine_mask, GSC0) && !intel_uc_wants_gsc_uc(&gt->uc)) {
> +		drm_notice(&gt->i915->drm,
> +			   "No GSC FW selected, disabling GSC CS and media C6\n");
> +		info->engine_mask &= ~BIT(GSC0);
> +	}
> +
>  	return info->engine_mask;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c1befa33ff59..e63d957b59eb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2701,6 +2701,9 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
>  		if (fw_domains & BIT(domain_id))
>  			fw_domain_fini(uncore, domain_id);
>  	}
> +
> +	if ((fw_domains & BIT(FW_DOMAIN_ID_GSC)) && !HAS_ENGINE(gt, GSC0))
> +		fw_domain_fini(uncore, FW_DOMAIN_ID_GSC);

On a quick glace I was asking "why do you need this since it doesn't have the gsc0?
Then I remember that fw_domain got initialized and it will be skipped, right?
Then I though about at least have a comment here, but finally I got myself wondering
why we don't do this already in the if above, while we are cleaning the engine mask?

>  }
>  
>  static void driver_flr(struct intel_uncore *uncore)
> -- 
> 2.37.3
>