Re: [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW

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

On 11/29/2022 3:48 PM, Teres Alexis, Alan Previn wrote:
> Besides the nit below, just would like to echo the same thing Nikula said about not including the type definition in the
> main uc header (which i know can be a bit more work especially if we go with allocation of the structure at init time
> and using a ptr in the uc structure).

I had a stab at this and it is quite complicated due to the 
inter-dependencies between the headers. Even if I split out the type to 
its own header, I'd have to include it in the current one for the status 
checker functions to work, so there would be no gain. The whole 
infrastructure needs to be reworked so that the headers can be fully 
decoupled. I have a few ideas on how to go at that and I'll try to send 
out a few patches on the side to get that effort going.

> That said,
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>
> On Mon, 2022-11-21 at 15:16 -0800, Ceraolo Spurio, Daniele wrote:
>> On MTL the GSC FW needs to be loaded on the media GT by the graphics
>>
>>
>> @@ -246,6 +253,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>>   	int i;
>>   	bool found;
>>   
>> +	/* FW is not defined until all the support is in place */
> Nit: perhaps a little bit more explanation would be better here for folks reading thru the codes

Not sure what extra explanation I can provide here. The reason we're not 
defining the blob is because the support code is not fully there, there 
is no need to go into details of what's actually missing as that will 
evolve with time. I can however rephrase this if you think the current 
sentence is not clear, would something like this work:
"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".

Daniele

>> +	if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
>> +		return;
>> +
>