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@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?
>
> 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@xxxxxxxxx>
>
> Daniele
>
> >
> > > }
> > > static void driver_flr(struct intel_uncore *uncore)
> > > --
> > > 2.37.3
> > >
>
