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

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

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.

>
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> index 2ae693b01b49..5475e95d61c6 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> @@ -9,8 +9,9 @@
>>   #include "gt/intel_gt_print.h"
>>   #include "intel_gsc_uc.h"
> alan: nit: not this patch's fault, alphabetically, intel_gsc_uc.h is after intel_gsc_proxy.h

will fix

>>   #include "intel_gsc_fw.h"
>> -#include "i915_drv.h"
>>   #include "intel_gsc_proxy.h"
>> +#include "i915_drv.h"
>> +#include "i915_reg.h"
>>   
>>   static void gsc_work(struct work_struct *work)
>>   {
>> @@ -301,3 +302,46 @@ void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc)
>>    	queue_work(gsc->wq, &gsc->work);
>>   }
>> +
> alan: btw, why are we putting intel_gsc_uc_load_status in intel_gsc_uc.c if the only caller is gsc_uc's debugfs?
> why not just make it a static in there? unless u plan to call it from "err_print_uc" - then can we add that in next rev?

I do indeed plan to follow up and add this to the error state, but I'll 
do that as a separate patch as I also want to add the GSC logs to the 
error state at the same time.

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

>> +		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:snip
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c
>> new file mode 100644
>> index 000000000000..da9f96b72291
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c
>> @@ -0,0 +1,38 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2020 Intel Corporation
> alan:2023?

D'oh!

Daniele

>
> alan:snip
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h
>> new file mode 100644
>> index 000000000000..c405e5574253
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2020 Intel Corporation
> alan:2023?
> alan:snip
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> alan:snip
>
>   
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
>> index 2f93cc4e408a..6d541c866edb 100644
> alan:snip
>
>
>