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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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

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


>  	/*
>  	 * 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