Re: [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 6/5/2023 4:46 PM, Teres Alexis, Alan Previn wrote:
> 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?

Sure, but let's take this to a separate patch. This patch is not adding 
that code nor any calls to it (just updating the defines), so it isn't 
the right place to add that fix.

Daniele

>
> 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.