[PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs

[Teres Alexis, Alan Previn]

On Wed, 2023-05-31 at 17:25 -0700, Ceraolo Spurio, Daniele wrote:
> 
> On 5/26/2023 3:57 PM, Teres Alexis, Alan Previn wrote:
> > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote:
> > > Add a new debugfs to dump information about the GSC. This includes:
> > alan:snip
> > Actually everything looks good except for a couple of questions + asks - hope we can close on this patch in next rev.
> > 
> > > - the FW path and SW tracking status;
> > > - the release, security and compatibility versions;
> > > - the HECI1 status registers.
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > > index 0b6dcd982b14..3014e982aab2 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > > @@ -12,36 +12,31 @@
> > >   #include "intel_gsc_fw.h"
> > >   #include "intel_gsc_meu_headers.h"
> > >   #include "intel_gsc_uc_heci_cmd_submit.h"
> > > -
> > > -#define GSC_FW_STATUS_REG			_MMIO(0x116C40)
> > > -#define GSC_FW_CURRENT_STATE			REG_GENMASK(3, 0)
> > > -#define   GSC_FW_CURRENT_STATE_RESET		0
> > > -#define   GSC_FW_PROXY_STATE_NORMAL		5
> > > -#define GSC_FW_INIT_COMPLETE_BIT		REG_BIT(9)
> > > +#include "i915_reg.h"
> > >   
> > alan:snip
> >   
> > alan: btw, just to be consistent with other top-level "intel_foo_is..." checking functions,
> > why don't we take the runtime wakeref inside the following functions and make it easier for any callers?
> > (just like what we do for "intel_huc_is_authenticated"):
> >      static bool gsc_is_in_reset(struct intel_uncore *uncore)
> >      bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
> >      bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
> 
> The idea was that we shouldn't check the FW status if we're not planning 
> to do something with it, in which case we should already have a wakeref. 
> HuC is a special case because userspace can query that when the HW is 
> idle. This said, I have nothing against adding an extra wakeref , but I 
> don't think it should be in this patch.
alan: i believe intel_pxp_gsccs_is_ready_for_sessions is being used in a
similar way where one of the uses it to check huc-status and gsc-proxy
status without actually doing any operation. If u still wanna keep it
differently then I'll have to update the PXP code. Or perhaps you could
help me fix that on the PXP side?

alna:snip
> > 

> > > +void intel_gsc_uc_load_status(struct intel_gsc_uc *gsc, struct drm_printer *p)
> > > +{
> > > +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> > > +	struct intel_uncore *uncore = gt->uncore;
> > > +	intel_wakeref_t wakeref;
> > > +
> > > +	if (!intel_gsc_uc_is_supported(gsc)) {
> > alan: this was already checked in caller so we'll never get here. i think we should remove the check in the caller, let below msg appear.
> 
> I did the same as what we do for GuC and HuC. I'd prefer to be 
> consistent in behavior with those.
alan: okay - sounds good
> 
> > > +		drm_printf(p, "GSC not supported\n");
> > > +		return;
> > > +	}
> > alan:snip

> > > +			drm_printf(p, "HECI1 FWSTST%u = 0x%08x\n", i, status);
> > alan:nit: do you we could add those additional shim regs? (seemed useful in recent offline debugs).
> 
> Agreed that it would be useful; I'll try to get a complete list from 
> arch and/or the GSC FW team. Are you ok if we go ahead with this in the 
> meantime?
alan: yes sure.
> 
> > 
>