Re: [PATCH] drm/i915/guc: Fix detection of GuC submission in use

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Thu, 05 Sep 2019 13:16:31 +0200, Janusz Krzysztofik  
<janusz.krzysztofik@linux.intel.com> wrote:

> The driver always assumes active GuC submission mode if it is
> supported.  That's not true if GuC initialization fails for some
> reason.  That may lead to kernel panics, caused e.g. by execlists
> fallback submission mode incorrectly detecting GuC submission in use.
>
> Fix it by also checking for GuC enabled status.
>
> Fixes: 356c484822e6 ("drm/i915/uc: Add explicit DISABLED state for  
> firmware")
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_uc.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 527995c21196..b28bab64a280 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -51,7 +51,8 @@ static inline bool  
> intel_uc_supports_guc_submission(struct intel_uc *uc)
> static inline bool intel_uc_uses_guc_submission(struct intel_uc *uc)
>  {
> -	return intel_guc_is_submission_supported(&uc->guc);
> +	return intel_guc_is_enabled(&uc->guc) &&
> +	       intel_guc_is_submission_supported(&uc->guc);

This wont fix your original problem (that btw is not possible to
repro on drm-tip) as after any GuC initialization failure we still
treat GuC as "enabled":

intel_guc_is_supported => H/W support (static)
intel_guc_is_enabled => aka not disabled by the user (config)
intel_guc_is_running => no major fw failure (runtime)

Note that we even s/intel_guc_is_enabled/intel_guc_is_running
won't help as GuC may be running but we may fail to correctly
initialize GuC submission.

Correct fix to original problem must be aligned with new GuC
submission model (coming soon) and it may look as this:

+static inline bool intel_guc_is_submission_active(struct intel_guc *guc)
+{
+	GEM_BUG_ON(guc->submission_active && !intel_guc_is_running(guc));
+	return guc->submission_active;
+}

and then

  static inline bool intel_uc_uses_guc_submission(struct intel_uc *uc)
  {
-	return intel_guc_is_submission_supported(&uc->guc);
+	return intel_guc_is_submission_active(&uc->guc);
  }

We may need to revisit all uses/supports/ macros to better
reflect configuration vs runtime differences.

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