Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

["Srivatsa, Anusha" <anusha.srivatsa@intel.com>]

>-----Original Message-----
>From: Wajdeczko, Michal
>Sent: Friday, September 29, 2017 12:10 AM
>To: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; intel-
>gfx@lists.freedesktop.org; Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; Mateo Lozano, Oscar
><oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
><daniele.ceraolospurio@intel.com>
>Subject: Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading
>module
>
>On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha
><anusha.srivatsa@intel.com> wrote:
>
>>
>>
>>> -----Original Message-----
>>> From: Sundaresan, Sujaritha
>>> Sent: Thursday, September 21, 2017 11:38 AM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; Wajdeczko,
>>> Michal
>>> <Michal.Wajdeczko@intel.com>; Srivatsa, Anusha
>>> <anusha.srivatsa@intel.com>;
>>> Mateo Lozano, Oscar <oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
>>> <daniele.ceraolospurio@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@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@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.

Yes, you are right.

Anusha

>Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx