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 Tue, Nov 22, 2022 at 02:58:37PM -0800, Ceraolo Spurio, Daniele wrote:
> 
> 
> On 11/22/2022 12:52 PM, Rodrigo Vivi wrote:
> > 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?
> 
> I've followed the existing code flows that we have in place for fused off
> VCS/VECS. Basically the existing code goes like this:
> 
> 1) All FW domains for the platform are initialized
> 2) We read the fuses and adjust the engine mask accordingly
> 3) We go back and prune the FW domains that are not applicable anymore due
> to the updated mask.
> 
> The idea is to have a single gt-level function doing all the mask adjusting
> and an uncore-level one doing all the domain pruning. I'm not against
> changing this approach, but in that case we should update the behavior for
> VCS/VECS as well (which might be complicated, because VCS/VECS engines share
> FW domains, so the pruning logic is ugly).

okay, then let's move with this...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

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