Re: [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw

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

On 5/25/2023 4:48 PM, Teres Alexis, Alan Previn wrote:
> Considering the only request i have below is touching up of existing comments (as
> far as this patch is concerned), and since the rest of the code looks good, here is
> my R-b - but i hope you can anwser my newbie question at the bottom:
>
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>
> On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote:
>> Add FW definition and the matching override modparam.
>>
>> The GSC FW has both a release version, based on platform and a rolling
>> counter, and a compatibility version, which is the one tracking
>> interface changes. Since what we care about is the interface, we use
>> the compatibility version in the buinary names.
> alan :s/buinary/binary
>
>> Same as with the GuC, a major version bump indicate a
>> backward-incompatible change, while a minor version bump indicates a
>> backward-compatible one, so we use only the former in the file name.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 32 ++++++++++++++++++------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> index 36ee96c02d74..531cd172151d 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -124,6 +124,18 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>>   	fw_def(BROXTON,      0, huc_mmp(bxt,  2, 0, 0)) \
>>   	fw_def(SKYLAKE,      0, huc_mmp(skl,  2, 0, 0))
>>   
>> +/*
>> + * The GSC FW has both a release version, based on platform and a rolling
>> + * counter, and a compatibility version, which is the one tracking
>> + * interface changes. Since what we care about is the interface, we use
>> + * the compatibility version in the buinary names.
> alan:s/buinary/binary
> also, since we will (i hope) be adding several comments (alongside the new
> version objects under intel_gsc_uc structure) in the patch #3 about what
> their differences are and which one we care about and when they get populated,
> perhaps we can minimize the information here and redirect to that other
> comment... OR ... we can minimize the comments in patch #3 and redirect here
> (will be good to have a single location with detailed explaination in the
> comments and a redirect-ptr from the other location since a reader would
> most likely stumble onto those questions from either of these locations).

I'll redirect, that makes more sense

>
>> + * Same as with the GuC, a major version bump indicate a
>> + * backward-incompatible change, while a minor version bump indicates a
>> + * backward-compatible one, so we use only the former in the file name.
>> + */
>> +#define INTEL_GSC_FIRMWARE_DEFS(fw_def, gsc_def) \
>> +	fw_def(METEORLAKE,   0, gsc_def(mtl, 1, 0))
>> +
>>   /*
>>   
>>
> alan:snip
>
>> @@ -257,14 +281,6 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>>   	int i;
>>   	bool found;
>>   
>> -	/*
>> -	 * GSC FW support is still not fully in place, so we're not defining
>> -	 * the FW blob yet because we don't want the driver to attempt to load
>> -	 * it until we're ready for it.
>> -	 */
>> -	if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
>> -		return;
>> -
> alan: more of a newbie question from myself: considering this is a new firmware
> we are adding, is there some kind of requirement to provide a link to the patch
> targetting the linux firmware repo that is a dependency of this series?
> or perhaps we should mention in the series that merge will only happen after
> that patch gets merged (with a final rev that includes the patch on
> the fw-repo side?). Just trying to understand the process.

We usually merge the kernel patch first and do the pull-request to 
linux-firmware immediately after. We did have cases where we change 
firmware version between the first rev and the one that gets merged, so 
we only go to linux-firmware once we're sure it's not going to change 
anymore. Case in point, the GSC team has asked me to hold off in sending 
the current blob to linux-firmware because they have some pending fixes 
they want to include in the first "official" release.

Daniele

>
>
>>   	/*
>>   	 * The only difference between the ADL GuC FWs is the HWConfig support.
>>   	 * ADL-N does not support HWConfig, so we should use the same binary as