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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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@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.
>
> 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

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