[PATCH] drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload

[PATCH] drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload

[Michel Thierry]

The driver reloads the GuC firmware after full gpu reset or
suspend/resume, but it never disables the GuC beforehand.
This leads us to hit the assert inside i915_ggtt_enable_guc added
by commit 04f7b24eccdf ("drm/i915/guc: Assert that we switch between
known ggtt->invalidate functions").

As a workaround, don't call i915_ggtt_enable_guc if there is a GuC
execbuf_client; because if there isn't one we are either loading in a
fresh system or we called intel_uc_fini_hw.

I'm inclined to this approach because even if intel_uc_fini_hw could be
added to the suspend path, we will still need to keep this for the
full gpu reset case.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index a8930f2feacf..564b0d2b8842 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -339,8 +339,14 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	guc_disable_communication(guc);
 	gen9_reset_guc_interrupts(dev_priv);
 
-	/* We need to notify the guc whenever we change the GGTT */
-	i915_ggtt_enable_guc(dev_priv);
+	/*
+	 * We need to notify the guc whenever we change the GGTT; but if we
+	 * are reloading the firmware (after full gpu reset or suspend/resume),
+	 * we should skip this since gtt->invalidate was already set (or we hit
+	 * an assert).
+	 */
+	if (!dev_priv->guc.execbuf_client)
+		i915_ggtt_enable_guc(dev_priv);
 
 	if (i915.enable_guc_submission) {
 		/*
-- 
2.11.0

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

✓ Fi.CI.BAT: success for drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload
URL   : https://patchwork.freedesktop.org/series/26651/
State : success

== Summary ==

Series 26651v1 drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload
https://patchwork.freedesktop.org/api/1.0/series/26651/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-j1900) fdo#101516

fdo#101516 https://bugs.freedesktop.org/show_bug.cgi?id=101516

fi-bdw-5557u     total:279  pass:264  dwarn:0   dfail:0   fail:3   skip:11  time:436s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:429s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:354s
fi-bsw-n3050     total:279  pass:239  dwarn:0   dfail:0   fail:3   skip:36  time:516s
fi-bxt-j4205     total:279  pass:256  dwarn:0   dfail:0   fail:3   skip:19  time:499s
fi-byt-j1900     total:279  pass:250  dwarn:1   dfail:0   fail:3   skip:24  time:475s
fi-byt-n2820     total:279  pass:246  dwarn:1   dfail:0   fail:3   skip:28  time:481s
fi-glk-2a        total:279  pass:256  dwarn:0   dfail:0   fail:3   skip:19  time:591s
fi-hsw-4770      total:279  pass:259  dwarn:0   dfail:0   fail:3   skip:16  time:426s
fi-hsw-4770r     total:279  pass:259  dwarn:0   dfail:0   fail:3   skip:16  time:408s
fi-ilk-650       total:279  pass:225  dwarn:0   dfail:0   fail:3   skip:50  time:415s
fi-ivb-3520m     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:488s
fi-ivb-3770      total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:469s
fi-kbl-7500u     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:458s
fi-kbl-7560u     total:279  pass:264  dwarn:1   dfail:0   fail:3   skip:10  time:559s
fi-kbl-r         total:279  pass:256  dwarn:1   dfail:0   fail:3   skip:18  time:564s
fi-pnv-d510      total:279  pass:221  dwarn:3   dfail:0   fail:0   skip:55  time:554s
fi-skl-6260u     total:279  pass:265  dwarn:0   dfail:0   fail:3   skip:10  time:452s
fi-skl-6700hq    total:279  pass:219  dwarn:1   dfail:0   fail:33  skip:24  time:330s
fi-skl-6700k     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:461s
fi-skl-6770hq    total:279  pass:265  dwarn:0   dfail:0   fail:3   skip:10  time:471s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:437s
fi-snb-2520m     total:279  pass:247  dwarn:0   dfail:0   fail:3   skip:28  time:538s
fi-snb-2600      total:279  pass:246  dwarn:0   dfail:0   fail:3   skip:29  time:402s

6412bcb8e279d62b9d8ae2840712d6c85806369b drm-tip: 2017y-06m-30d-14h-56m-41s UTC integration manifest
9fa53c2 drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5082/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload

[Michel Thierry]

On 6/30/2017 10:41 AM, Michel Thierry wrote:
> The driver reloads the GuC firmware after full gpu reset or
> suspend/resume, but it never disables the GuC beforehand.
> This leads us to hit the assert inside i915_ggtt_enable_guc added
> by commit 04f7b24eccdf ("drm/i915/guc: Assert that we switch between
> known ggtt->invalidate functions").
> 

Ping...
I know this isn't important, since who is using guc? (I only know people 
who hates it).

Or we just remove the assert in i915_ggtt_enable_guc.

> As a workaround, don't call i915_ggtt_enable_guc if there is a GuC
> execbuf_client; because if there isn't one we are either loading in a
> fresh system or we called intel_uc_fini_hw.
> 
> I'm inclined to this approach because even if intel_uc_fini_hw could be
> added to the suspend path, we will still need to keep this for the
> full gpu reset case.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uc.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index a8930f2feacf..564b0d2b8842 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -339,8 +339,14 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>          guc_disable_communication(guc);
>          gen9_reset_guc_interrupts(dev_priv);
> 
> -       /* We need to notify the guc whenever we change the GGTT */
> -       i915_ggtt_enable_guc(dev_priv);
> +       /*
> +        * We need to notify the guc whenever we change the GGTT; but if we
> +        * are reloading the firmware (after full gpu reset or suspend/resume),
> +        * we should skip this since gtt->invalidate was already set (or we hit
> +        * an assert).
> +        */
> +       if (!dev_priv->guc.execbuf_client)
> +               i915_ggtt_enable_guc(dev_priv);
> 
>          if (i915.enable_guc_submission) {
>                  /*
> --
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Prevent ggtt->invalidate assert during GuC reload

[Chris Wilson]

Quoting Michel Thierry (2017-07-26 21:59:07)
> On 6/30/2017 10:41 AM, Michel Thierry wrote:
> > The driver reloads the GuC firmware after full gpu reset or
> > suspend/resume, but it never disables the GuC beforehand.
> > This leads us to hit the assert inside i915_ggtt_enable_guc added
> > by commit 04f7b24eccdf ("drm/i915/guc: Assert that we switch between
> > known ggtt->invalidate functions").
> > 
> 
> Ping...
> I know this isn't important, since who is using guc? (I only know people 
> who hates it).
> 
> Or we just remove the assert in i915_ggtt_enable_guc.

Kind of. This implies we've lost control over the guc-driver state. My
feeling is that we need a post-reset hook for the guc so that we can
throw away the lost state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx