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

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-06 12:10:15)
> 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.

That worries me. But not enough to not give a r-b
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

I have nightmares of users bricking their computers just because an
update goes wrong. Or more likely a kernel is updated without the
accompanying linux-firmware. I think we should definitely be testing
module-load with incorrect firmware and ensure we don't leave a system
with a blank-screen-of-doom.
-Chris