[Intel-gfx] [PATCH v3 6/8] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Michal Wajdeczko]

On Tue, 05 Dec 2017 23:47:21 +0100, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-12-05 16:38:42)
>> -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>> +static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
>>  {
>> -       if (!HAS_GUC(dev_priv)) {
>> -               if (i915_modparams.enable_guc_loading > 0 ||
>> -                   i915_modparams.enable_guc_submission > 0)
>> -                       DRM_INFO("Ignoring GuC options, no hardware\n");
>> +       struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>> +       struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>> +       int enable_guc = 0;
>>
>> -               i915_modparams.enable_guc_loading = 0;
>> -               i915_modparams.enable_guc_submission = 0;
>> -               return;
>> -       }
>> +       /* Default is to enable GuC/HuC if we know their firmwares */
>> +       if (intel_uc_fw_is_selected(guc_fw))
>> +               enable_guc |= ENABLE_GUC_SUBMISSION;
>> +       if (intel_uc_fw_is_selected(huc_fw))
>> +               enable_guc |= ENABLE_GUC_LOAD_HUC;
>>
>> -       /* A negative value means "use platform default" */
>> -       if (i915_modparams.enable_guc_loading < 0)
>> -               i915_modparams.enable_guc_loading =  
>> HAS_GUC_UCODE(dev_priv);
>> +       /* Any platform specific fine-tuning can be done here */
>>
>> -       /* Verify firmware version */
>> -       if (i915_modparams.enable_guc_loading) {
>> -               if (!intel_uc_fw_is_selected(&dev_priv->guc.fw))
>> -                       i915_modparams.enable_guc_loading = 0;
>> -       }
>> +       return enable_guc;
>> +}
>>
>> -       /* Can't enable guc submission without guc loaded */
>> -       if (!i915_modparams.enable_guc_loading)
>> -               i915_modparams.enable_guc_submission = 0;
>> +/**
>> + * intel_uc_sanitize_options - sanitize uC related modparam options
>> + * @dev_priv: device private
>> + *
>> + * In case of "enable_guc" option this function will attempt to modify
>> + * it only if it was initially set to "auto(-1)". Default value for  
>> this
>> + * modparam varies between platforms and it is hardcoded in driver  
>> code.
>> + * Any other modparam value is only monitored against availability of  
>> the
>> + * related hardware or firmware definitions.
>> + */
>> +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>> +{
>> +       struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>> +       struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>>
>>         /* A negative value means "use platform default" */
>> -       if (i915_modparams.enable_guc_submission < 0)
>> -               i915_modparams.enable_guc_submission =  
>> HAS_GUC_SCHED(dev_priv);
>> +       if (i915_modparams.enable_guc < 0)
>> +               i915_modparams.enable_guc =  
>> __get_platform_enable_guc(dev_priv);
>> +
>> +       DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
>> +                        i915_modparams.enable_guc,
>> +                        yesno(intel_uc_is_using_guc_submission()),
>> +                        yesno(intel_uc_is_using_huc()));
>> +
>> +       /* Verify GuC firmware availability */
>> +       if (intel_uc_is_using_guc() &&  
>> !intel_uc_fw_is_selected(guc_fw)) {
>> +               DRM_WARN("Incompatible option detected: enable_guc=%d,  
>> %s!\n",
>> +                        i915_modparams.enable_guc,
>> +                        !HAS_GUC(dev_priv) ? "no GuC hardware" :
>> +                                             "no GuC firmware");
>> +       }
>> +
>> +       /* Verify HuC firmware availability */
>> +       if (intel_uc_is_using_huc() &&  
>> !intel_uc_fw_is_selected(huc_fw)) {
>> +               DRM_WARN("Incompatible option detected: enable_guc=%d,  
>> %s!\n",
>> +                        i915_modparams.enable_guc,
>> +                        !HAS_HUC(dev_priv) ? "no HuC hardware" :
>> +                                             "no HuC firmware");
>> +       }
>
> With the intent that after the warning, the uc load will fail and the
> module load will then abort? Just to be clear.
>

Yes. As I'm not sure that we should ignore explicit user options and
pretend that nothing really happen and continue with some fallback.

But I must admit that with this patch we can upset users when module is
loaded with enable_guc=-1(auto) and driver has corresponding GuC/HuC fw
definitions but firmware blobs are not present on the target machine.

Then we can wrongly read these options as explicit and abort module load
due to fw upload failure.

But impact of this issue can be minimized if we try to fetch GuC/HuC
firmwares much earlier (init_early). Then we can quickly update our
"preferred" auto options into "available". However, if we fail on any
later stage, after we decided to enable GuC/Huc, there still will be
no fallback and we will abort driver load.

Michal