[Intel-gfx] [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

[Michal Wajdeczko]

On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha  
<anusha.srivatsa at intel.com> wrote:

>
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha
>> Sent: Thursday, September 21, 2017 11:38 AM
>> To: intel-gfx at lists.freedesktop.org
>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>; Wajdeczko,  
>> Michal
>> <Michal.Wajdeczko at intel.com>; Srivatsa, Anusha  
>> <anusha.srivatsa at intel.com>;
>> Mateo Lozano, Oscar <oscar.mateo at intel.com>; Ceraolo Spurio, Daniele
>> <daniele.ceraolospurio at intel.com>
>> Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading  
>> module
>>
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission".
>> Whenever we need i915.enable_guc_submission=1, we also need
>> enable_guc_loading=1.
>> We also need enable_guc_loading=1 when we want to verify the HuC, which  
>> is
>> every time we have a HuC (but all platforms with HuC have a GuC and  
>> viceversa).
>>
>> v2: Clarifying the commit message (Anusha)
>> v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>> v4: Rebased
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
>> ---

  snip

>> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 6d7d871..bd583f7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3138,11 +3138,14 @@ static inline unsigned int
>> i915_sg_segment_size(void)
>>  * command submission once loaded. But these are logically independent
>>  * properties, so we have separate macros to test them.
>>  */
>> -#define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
>> #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
>> -#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>> -#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
>> -#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>> +#define HAS_GUC(dev_priv)			((dev_priv)->info.has_guc)
>> +#define HAS_GUC_UCODE(dev_priv)		((dev_priv)->guc.fw.path !=
>> NULL)
>> +#define HAS_HUC_UCODE(dev_priv)		((dev_priv)->huc.fw.path !=
>> NULL)
>> +
>> +#define NEEDS_GUC_LOADING(dev_priv) \
>> +	(HAS_GUC(dev_priv) && \
>> +	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>
> What if there is GuC and we don't want to use it?  The above  
> NEEDS_GUC_LOADING is still going to load it because it is available. So  
> maybe there should only be:
>
> #define NEEDS_GUC_LOADING(dev_priv) \
> 	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
> That is, load guc since guc submission is enabled or we need guc to  
> authenticate HuC.
>

Note that we used HAS_GUC as prerequisite that is then followed by logical
operator AND what guarantees us that we will try to load Guc only if its
available. In your modified macro we will try to load Guc just due to user
provided enable_guc_submission modparam which may not match hw caps.

On other hand we can try to rely on earlier modparam sanitization but I  
would
rather not trust that too much and keep our macro fully independent.

Michal