On Mon, Nov 21, 2022 at 03:16:16PM -0800, Daniele Ceraolo Spurio wrote:
> From: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Cc: John C Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> ---
> 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(>->uc)) {
> + drm_notice(>->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
>
