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

[Sujaritha]

On 09/29/2017 12:10 AM, Michal Wajdeczko wrote:
> 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
Will do.
>
>>> 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

Yes, this is a good point.

Thanks for the review.

Regards,

Sujaritha