Quoting Michal Wajdeczko (2017-12-06 12:10:15)
> On Tue, 05 Dec 2017 23:47:21 +0100, Chris Wilson
> <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
