[Intel-gfx] [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Michal Wajdeczko]

On Wed, 29 Nov 2017 14:40:05 +0100, Sagar Arun Kamble  
<sagar.a.kamble at intel.com> wrote:

>
>
> On 11/29/2017 5:44 PM, Michal Wajdeczko wrote:
>> On Tue, 28 Nov 2017 11:41:57 +0100, Sagar Arun Kamble  
>> <sagar.a.kamble at intel.com> wrote:
>>
>>>
>>>
>>> On 11/28/2017 1:24 AM, Sujaritha Sundaresan wrote:
>>>> We currently have two module parameters that control GuC:
>>>> "enable_guc_loading" and "enable_guc_submission". Whenever
>>>> we need submission=1, we also need loading=1.We also need
>>>> loading=1 when we want to want to verify the HuC, which
>>>> is every time we have a HuC (but all platforms with HuC
>>>> have a GuC and viceversa).
>>>>
>>
>> <SNIP>
>>
>>>> +
>>>> +    /* Making sure that our auto is well defined */
>>>> +    GEM_BUG_ON(auto_enable_guc < 0);
>>>> +    GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
>>>> +    GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
>>>> +
>>>> +    if (i915_modparams.enable_guc < 0)
>>>> +        i915_modparams.enable_guc = auto_enable_guc;
>>>> +
>>>> +    if (i915_modparams.enable_guc > 0) {
>>>> +        if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
>>>> +            DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>>>> +                     i915_modparams.enable_guc,
>>>> +                     !HAS_GUC(dev_priv) ? "no GuC hardware" :
>>>> +                     "no GuC firmware");
>>>> +            i915_modparams.enable_guc = 0;
>>>> +        }
>>>>       }
>>>>   -    /* A negative value means "use platform default" */
>>>> -    if (i915_modparams.enable_guc_loading < 0)
>>>> -        i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>>>> -
>>>> -    /* Verify firmware version */
>>>> -    if (i915_modparams.enable_guc_loading) {
>>>> -        if (HAS_HUC_UCODE(dev_priv))
>>>> -            intel_huc_select_fw(&dev_priv->huc);
>>>> -
>>>> -        if (intel_guc_fw_select(&dev_priv->guc))
>>>> -            i915_modparams.enable_guc_loading = 0;
>>>> +    if (i915_modparams.enable_guc & 2) {
>>>> +        if (!HAS_HUC_FW(dev_priv)) {
>>>> +            DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>>>> +                i915_modparams.enable_guc, "no HuC firmware");
>>>> +            i915_modparams.enable_guc = 0;
>>> Clearing only HuC status from enable_guc would be proper I guess.  
>>> Sorry I did not see this earlier.
>>
>> My understanding is that if user had specified non-zero positive value  
>> of
>> 'enable_guc' then it means that he requests *all* GuC options to be  
>> available
>> (something like our old '2=required' mode). If any of required option  
>> is not
>> available then we should not try to cherry-pick/drop single option.
> I think we wanted to enable HuC for some platforms but not enable GuC  
> submission. Anusha can you
> please confirm if such cherry-picking is needed through module parameter.

Cherry-picking through module parameter is fine.
And at the same time we should treat them as mandatory options.

I was referring to cherry-picking (or more precisely droping) during call
to sanitize_options(). So when user specify guc_enable as 3(submission+huc)
we should not convert it into 2(submission only).

>>
>> Note that if user don't care about specific option, then user should  
>> use -1(auto)
>> mode and rely on driver decision what is available and in which  
>> configuration.
>> And for 'auto' mode we try to make sure to do not select broken  
>> configurations.
> In that case we can just have 3 values for enable_guc (if cherry-picking  
> is not to be done)
> -1: auto (whatever is available)
> 0: all disabled
> 1: all enabled if available else all disabled
>>
>> Michal