[PATCH v4 0/7] Enable GuC submission

[PATCH v4 0/7] Enable GuC submission

[Dave Gordon]

This patchset covers various GuC-related changes, but most important of
these are (1) splitting the original "enable_guc_submission" parameter
into separate flags for loading GuC firmware vs. using the GuC for job
submission [PATCH 3/7], and (2) actually enabling GuC submission by
default on suitable platforms [PATCH 7/7].

Dave Gordon (7):
  drm/i915/guc: rename loader entry points
  drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED
  drm/i915/guc: add enable_guc_loading parameter
  drm/i915/guc: pass request (not client) to
    i915_guc_{wq_check_space,submit}()
  drm/i915/guc: don't spinwait if the GuC's workqueue is full
  drm/i915/guc: rework guc_add_workqueue_item()
  drm/i915/guc: change default to using GuC submission if possible

 drivers/gpu/drm/i915/i915_debugfs.c        |   1 +
 drivers/gpu/drm/i915/i915_dma.c            |   6 +-
 drivers/gpu/drm/i915/i915_drv.h            |  10 ++-
 drivers/gpu/drm/i915/i915_gem.c            |   5 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 137 +++++++++++++++++------------
 drivers/gpu/drm/i915/i915_params.c         |  14 ++-
 drivers/gpu/drm/i915/i915_params.h         |   3 +-
 drivers/gpu/drm/i915/intel_guc.h           |  35 +++++---
 drivers/gpu/drm/i915/intel_guc_fwif.h      |   3 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 127 ++++++++++++++------------
 drivers/gpu/drm/i915/intel_lrc.c           |   9 +-
 drivers/gpu/drm/i915/intel_pm.c            |   2 +-
 drivers/gpu/drm/i915/intel_uncore.c        |   2 +-
 13 files changed, 207 insertions(+), 147 deletions(-)

-- 
1.9.1

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

[PATCH v4 1/7] drm/i915/guc: rename loader entry points

[Dave Gordon]

The GuC initialisation code could do other things apart from loading
firmware, so here we rename the three primary entry points to remove any
specific reference to "ucode" (no functional changes, just renaming).

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |  6 +++---
 drivers/gpu/drm/i915/i915_gem.c         |  2 +-
 drivers/gpu/drm/i915/intel_guc.h        |  6 +++---
 drivers/gpu/drm/i915/intel_guc_loader.c | 19 ++++++++++---------
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 547100f..7d93f8f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -507,7 +507,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 * working irqs for e.g. gmbus and dp aux transfers. */
 	intel_modeset_init(dev);
 
-	intel_guc_ucode_init(dev);
+	intel_guc_init(dev);
 
 	ret = i915_gem_init(dev);
 	if (ret)
@@ -547,7 +547,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 cleanup_gem:
 	i915_gem_fini(dev);
 cleanup_irq:
-	intel_guc_ucode_fini(dev);
+	intel_guc_fini(dev);
 	drm_irq_uninstall(dev);
 	intel_teardown_gmbus(dev);
 cleanup_csr:
@@ -1528,7 +1528,7 @@ int i915_driver_unload(struct drm_device *dev)
 	/* Flush any outstanding unpin_work. */
 	flush_workqueue(dev_priv->wq);
 
-	intel_guc_ucode_fini(dev);
+	intel_guc_fini(dev);
 	i915_gem_fini(dev);
 	intel_fbc_cleanup_cfb(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a3d826b..43e45b7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4867,7 +4867,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 
 	/* We can't enable contexts until all firmware is loaded */
 	if (HAS_GUC_UCODE(dev)) {
-		ret = intel_guc_ucode_load(dev);
+		ret = intel_guc_setup(dev);
 		if (ret) {
 			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
 			ret = -EIO;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9d79c4c..3984136 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -138,9 +138,9 @@ struct intel_guc {
 };
 
 /* intel_guc_loader.c */
-extern void intel_guc_ucode_init(struct drm_device *dev);
-extern int intel_guc_ucode_load(struct drm_device *dev);
-extern void intel_guc_ucode_fini(struct drm_device *dev);
+extern void intel_guc_init(struct drm_device *dev);
+extern int intel_guc_setup(struct drm_device *dev);
+extern void intel_guc_fini(struct drm_device *dev);
 extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
 extern int intel_guc_suspend(struct drm_device *dev);
 extern int intel_guc_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 23345e1..b14a3a3 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -375,18 +375,19 @@ static int i915_reset_guc(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_guc_ucode_load() - load GuC uCode into the device
+ * intel_guc_setup() - finish preparing the GuC for activity
  * @dev:	drm device
  *
  * Called from gem_init_hw() during driver loading and also after a GPU reset.
  *
+ * The main action required here it to load the GuC uCode into the device.
  * The firmware image should have already been fetched into memory by the
- * earlier call to intel_guc_ucode_init(), so here we need only check that
- * is succeeded, and then transfer the image to the h/w.
+ * earlier call to intel_guc_init(), so here we need only check that worked,
+ * and then transfer the image to the h/w.
  *
  * Return:	non-zero code on error
  */
-int intel_guc_ucode_load(struct drm_device *dev)
+int intel_guc_setup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
@@ -620,15 +621,15 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 }
 
 /**
- * intel_guc_ucode_init() - define parameters and fetch firmware
+ * intel_guc_init() - define parameters and fetch firmware
  * @dev:	drm device
  *
  * Called early during driver load, but after GEM is initialised.
  *
  * The firmware will be transferred to the GuC's memory later,
- * when intel_guc_ucode_load() is called.
+ * when intel_guc_setup() is called.
  */
-void intel_guc_ucode_init(struct drm_device *dev)
+void intel_guc_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
@@ -676,10 +677,10 @@ void intel_guc_ucode_init(struct drm_device *dev)
 }
 
 /**
- * intel_guc_ucode_fini() - clean up all allocated resources
+ * intel_guc_fini() - clean up all allocated resources
  * @dev:	drm device
  */
-void intel_guc_ucode_fini(struct drm_device *dev)
+void intel_guc_fini(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
-- 
1.9.1

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

Re: [PATCH v4 1/7] drm/i915/guc: rename loader entry points

[Tvrtko Ursulin]

On 13/05/16 15:36, Dave Gordon wrote:
> The GuC initialisation code could do other things apart from loading
> firmware, so here we rename the three primary entry points to remove any
> specific reference to "ucode" (no functional changes, just renaming).
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_dma.c         |  6 +++---
>   drivers/gpu/drm/i915/i915_gem.c         |  2 +-
>   drivers/gpu/drm/i915/intel_guc.h        |  6 +++---
>   drivers/gpu/drm/i915/intel_guc_loader.c | 19 ++++++++++---------
>   4 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 547100f..7d93f8f 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -507,7 +507,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>   	 * working irqs for e.g. gmbus and dp aux transfers. */
>   	intel_modeset_init(dev);
>
> -	intel_guc_ucode_init(dev);
> +	intel_guc_init(dev);
>
>   	ret = i915_gem_init(dev);
>   	if (ret)
> @@ -547,7 +547,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>   cleanup_gem:
>   	i915_gem_fini(dev);
>   cleanup_irq:
> -	intel_guc_ucode_fini(dev);
> +	intel_guc_fini(dev);
>   	drm_irq_uninstall(dev);
>   	intel_teardown_gmbus(dev);
>   cleanup_csr:
> @@ -1528,7 +1528,7 @@ int i915_driver_unload(struct drm_device *dev)
>   	/* Flush any outstanding unpin_work. */
>   	flush_workqueue(dev_priv->wq);
>
> -	intel_guc_ucode_fini(dev);
> +	intel_guc_fini(dev);
>   	i915_gem_fini(dev);
>   	intel_fbc_cleanup_cfb(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a3d826b..43e45b7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4867,7 +4867,7 @@ int i915_gem_init_engines(struct drm_device *dev)
>
>   	/* We can't enable contexts until all firmware is loaded */
>   	if (HAS_GUC_UCODE(dev)) {
> -		ret = intel_guc_ucode_load(dev);
> +		ret = intel_guc_setup(dev);
>   		if (ret) {
>   			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
>   			ret = -EIO;
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 9d79c4c..3984136 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -138,9 +138,9 @@ struct intel_guc {
>   };
>
>   /* intel_guc_loader.c */
> -extern void intel_guc_ucode_init(struct drm_device *dev);
> -extern int intel_guc_ucode_load(struct drm_device *dev);
> -extern void intel_guc_ucode_fini(struct drm_device *dev);
> +extern void intel_guc_init(struct drm_device *dev);
> +extern int intel_guc_setup(struct drm_device *dev);
> +extern void intel_guc_fini(struct drm_device *dev);
>   extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
>   extern int intel_guc_suspend(struct drm_device *dev);
>   extern int intel_guc_resume(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 23345e1..b14a3a3 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -375,18 +375,19 @@ static int i915_reset_guc(struct drm_i915_private *dev_priv)
>   }
>
>   /**
> - * intel_guc_ucode_load() - load GuC uCode into the device
> + * intel_guc_setup() - finish preparing the GuC for activity
>    * @dev:	drm device
>    *
>    * Called from gem_init_hw() during driver loading and also after a GPU reset.
>    *
> + * The main action required here it to load the GuC uCode into the device.
>    * The firmware image should have already been fetched into memory by the
> - * earlier call to intel_guc_ucode_init(), so here we need only check that
> - * is succeeded, and then transfer the image to the h/w.
> + * earlier call to intel_guc_init(), so here we need only check that worked,
> + * and then transfer the image to the h/w.
>    *
>    * Return:	non-zero code on error
>    */
> -int intel_guc_ucode_load(struct drm_device *dev)
> +int intel_guc_setup(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> @@ -620,15 +621,15 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>   }
>
>   /**
> - * intel_guc_ucode_init() - define parameters and fetch firmware
> + * intel_guc_init() - define parameters and fetch firmware
>    * @dev:	drm device
>    *
>    * Called early during driver load, but after GEM is initialised.
>    *
>    * The firmware will be transferred to the GuC's memory later,
> - * when intel_guc_ucode_load() is called.
> + * when intel_guc_setup() is called.
>    */
> -void intel_guc_ucode_init(struct drm_device *dev)
> +void intel_guc_init(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> @@ -676,10 +677,10 @@ void intel_guc_ucode_init(struct drm_device *dev)
>   }
>
>   /**
> - * intel_guc_ucode_fini() - clean up all allocated resources
> + * intel_guc_fini() - clean up all allocated resources
>    * @dev:	drm device
>    */
> -void intel_guc_ucode_fini(struct drm_device *dev)
> +void intel_guc_fini(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

[PATCH v4 2/7] drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED

[Dave Gordon]

For now, anything with a GuC requires uCode loading, and then supports
command submission once loaded. But these are logically distinct from
simply "having a GuC", so we need a separate macro for the latter. Then,
various tests should use this new macro rather than HAS_GUC_UCODE() or
testing enable_guc_submission.

v4:
    Added a couple more uses of the new macro.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     | 10 ++++++++--
 drivers/gpu/drm/i915/i915_gem.c     |  2 +-
 drivers/gpu/drm/i915/intel_pm.c     |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c |  2 +-
 4 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d9d07b7..3b9ee51 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2726,8 +2726,14 @@ struct drm_i915_cmd_table {
 
 #define HAS_CSR(dev)	(IS_GEN9(dev))
 
-#define HAS_GUC_UCODE(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
-#define HAS_GUC_SCHED(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
+/*
+ * For now, anything with a GuC requires uCode loading, and then supports
+ * command submission once loaded. But these are logically independent
+ * properties, so we have separate macros to test them.
+ */
+#define HAS_GUC(dev)		(IS_GEN9(dev) && !IS_KABYLAKE(dev))
+#define HAS_GUC_UCODE(dev)	(HAS_GUC(dev))
+#define HAS_GUC_SCHED(dev)	(HAS_GUC(dev))
 
 #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
 				    INTEL_INFO(dev)->gen >= 8)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 43e45b7..2a7be71 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4866,7 +4866,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 	intel_mocs_init_l3cc_table(dev);
 
 	/* We can't enable contexts until all firmware is loaded */
-	if (HAS_GUC_UCODE(dev)) {
+	if (HAS_GUC(dev)) {
 		ret = intel_guc_setup(dev);
 		if (ret) {
 			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1bb0f9a..c584282 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4844,7 +4844,7 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
 	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
-	if (HAS_GUC_UCODE(dev_priv))
+	if (HAS_GUC(dev_priv))
 		I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
 
 	I915_WRITE(GEN6_RC_SLEEP, 0);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 385114b..c1ca458 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1715,7 +1715,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
 	int ret;
 	unsigned long irqflags;
 
-	if (!i915.enable_guc_submission)
+	if (!HAS_GUC(dev_priv))
 		return -EINVAL;
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
-- 
1.9.1

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

Re: [PATCH v4 2/7] drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED

[Tvrtko Ursulin]

On 13/05/16 15:36, Dave Gordon wrote:
> For now, anything with a GuC requires uCode loading, and then supports
> command submission once loaded. But these are logically distinct from
> simply "having a GuC", so we need a separate macro for the latter. Then,
> various tests should use this new macro rather than HAS_GUC_UCODE() or
> testing enable_guc_submission.
>
> v4:
>      Added a couple more uses of the new macro.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h     | 10 ++++++++--
>   drivers/gpu/drm/i915/i915_gem.c     |  2 +-
>   drivers/gpu/drm/i915/intel_pm.c     |  2 +-
>   drivers/gpu/drm/i915/intel_uncore.c |  2 +-
>   4 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d9d07b7..3b9ee51 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2726,8 +2726,14 @@ struct drm_i915_cmd_table {
>
>   #define HAS_CSR(dev)	(IS_GEN9(dev))
>
> -#define HAS_GUC_UCODE(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
> -#define HAS_GUC_SCHED(dev)	(IS_GEN9(dev) && !IS_KABYLAKE(dev))
> +/*
> + * For now, anything with a GuC requires uCode loading, and then supports
> + * command submission once loaded. But these are logically independent
> + * properties, so we have separate macros to test them.
> + */
> +#define HAS_GUC(dev)		(IS_GEN9(dev) && !IS_KABYLAKE(dev))
> +#define HAS_GUC_UCODE(dev)	(HAS_GUC(dev))
> +#define HAS_GUC_SCHED(dev)	(HAS_GUC(dev))
>
>   #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
>   				    INTEL_INFO(dev)->gen >= 8)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 43e45b7..2a7be71 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4866,7 +4866,7 @@ int i915_gem_init_engines(struct drm_device *dev)
>   	intel_mocs_init_l3cc_table(dev);
>
>   	/* We can't enable contexts until all firmware is loaded */
> -	if (HAS_GUC_UCODE(dev)) {
> +	if (HAS_GUC(dev)) {
>   		ret = intel_guc_setup(dev);
>   		if (ret) {
>   			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1bb0f9a..c584282 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4844,7 +4844,7 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
>   	for_each_engine(engine, dev_priv)
>   		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
>
> -	if (HAS_GUC_UCODE(dev_priv))
> +	if (HAS_GUC(dev_priv))
>   		I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
>
>   	I915_WRITE(GEN6_RC_SLEEP, 0);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 385114b..c1ca458 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1715,7 +1715,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
>   	int ret;
>   	unsigned long irqflags;
>
> -	if (!i915.enable_guc_submission)
> +	if (!HAS_GUC(dev_priv))
>   		return -EINVAL;
>
>   	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

[PATCH v4 3/7] drm/i915/guc: add enable_guc_loading parameter

[Dave Gordon]

Split the function of "enable_guc_submission" into two separate
options.  The new one ("enable_guc_loading") controls only the
*fetching and loading* of the GuC firmware image. The existing
one is redefined to control only the *use* of the GuC for batch
submission once the firmware is loaded.

In addition, the degree of control has been refined from a simple
bool to an integer key, allowing several options:
-1 (default)     whatever the platform default is
 0  DISABLE      don't load/use the GuC
 1  BEST EFFORT  try to load/use the GuC, fallback if not available
 2  REQUIRE      must load/use the GuC, else leave the GPU wedged

The new platform default (as coded here) will be to attempt to
load the GuC iff the device has a GuC that requires firmware,
but not yet to use it for submission. A later patch will change
to enable it if appropriate.

v4:
    Changed some error-message levels, mostly ERROR->INFO, per
    review comments by Tvrtko Ursulin.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   1 -
 drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
 drivers/gpu/drm/i915/i915_params.c         |  14 +++-
 drivers/gpu/drm/i915/i915_params.h         |   3 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 108 ++++++++++++++++-------------
 5 files changed, 76 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2a7be71..2bf8743 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4870,7 +4870,6 @@ int i915_gem_init_engines(struct drm_device *dev)
 		ret = intel_guc_setup(dev);
 		if (ret) {
 			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
-			ret = -EIO;
 			goto out;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 169242a..916cd67 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
@@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 383c076..6a5578c 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_submission = false,
+	.enable_guc_loading = -1,
+	.enable_guc_submission = 0,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
@@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = {
 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
 		 "2=default swing(400mV))");
 
-module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400);
-MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
+module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
+MODULE_PARM_DESC(enable_guc_loading,
+		"Enable GuC firmware loading "
+		"(-1=auto [default], 0=never, 1=if available, 2=required)");
+
+module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
+MODULE_PARM_DESC(enable_guc_submission,
+		"Enable GuC submission "
+		"(-1=auto, 0=never [default], 1=if available, 2=required)");
 
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 65e73dd..1323261 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,6 +45,8 @@ struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
+	int enable_guc_loading;
+	int enable_guc_submission;
 	int guc_log_level;
 	int use_mmio_flip;
 	int mmio_debug;
@@ -57,7 +59,6 @@ struct i915_params {
 	bool load_detect_test;
 	bool reset;
 	bool disable_display;
-	bool enable_guc_submission;
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
 	bool enable_dp_mst;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index b14a3a3..3d33944 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
-	int retries, err = 0;
+	const char *fw_path = guc_fw->guc_fw_path;
+	int retries, ret, err = 0;
 
-	if (!i915.enable_guc_submission)
-		return 0;
-
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
+		fw_path,
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	direct_interrupts_to_host(dev_priv);
+	/* Loading forbidden, or no firmware to load? */
+	if (!i915.enable_guc_loading)
+		goto fail;
+	if (fw_path == NULL)
+		goto fail;
+	if (*fw_path == '\0') {
+		DRM_INFO("No GuC firmware known for this platform\n");
+		goto fail;
+	}
 
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
-		return 0;
+	/* Fetch failed, or already fetched but failed to load? */
+	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS)
+		goto fail;
+	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
+		goto fail;
 
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
-	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
-		return -ENOEXEC;
+	direct_interrupts_to_host(dev_priv);
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
 
-	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
-
-	switch (guc_fw->guc_fw_fetch_status) {
-	case GUC_FIRMWARE_FAIL:
-		/* something went wrong :( */
-		err = -EIO;
-		goto fail;
-
-	case GUC_FIRMWARE_NONE:
-	case GUC_FIRMWARE_PENDING:
-	default:
-		/* "can't happen" */
-		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
-			guc_fw->guc_fw_path,
-			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-			guc_fw->guc_fw_fetch_status);
-		err = -ENXIO;
-		goto fail;
-
-	case GUC_FIRMWARE_SUCCESS:
-		break;
-	}
+	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
+		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	err = i915_guc_submission_init(dev);
 	if (err)
@@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev)
 	return 0;
 
 fail:
-	DRM_ERROR("GuC firmware load failed, err %d\n", err);
 	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
 		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
 
@@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev)
 	i915_guc_submission_disable(dev);
 	i915_guc_submission_fini(dev);
 
-	return err;
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	if (i915.enable_guc_loading > 1) {
+		ret = -EIO;
+	} else if (HAS_GUC_SCHED(dev) && !HAS_GUC_UCODE(dev)) {
+		/* This GuC works even without firmware :) */
+		return 0;
+	} else if (i915.enable_guc_submission > 1) {
+		ret = -EIO;
+	} else {
+		ret = 0;
+	}
+
+	if (ret == -EIO)
+		DRM_ERROR("GuC firmware load failed, err %d\n", err);
+	else
+		DRM_INFO("GuC firmware load failed, err %d\n", err);
+
+	if (i915.enable_guc_submission)
+		DRM_INFO("falling back to execlist mode, ret %d\n", ret);
+	i915.enable_guc_submission = 0;
+
+	return ret;
 }
 
 static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
@@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	if (!HAS_GUC_SCHED(dev))
-		i915.enable_guc_submission = false;
+	/* A negative value means "use platform default" */
+	if (i915.enable_guc_loading < 0)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	if (i915.enable_guc_submission < 0)
+		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
@@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev)
 		guc_fw->guc_fw_major_wanted = 8;
 		guc_fw->guc_fw_minor_wanted = 7;
 	} else {
-		i915.enable_guc_submission = false;
 		fw_path = "";	/* unknown device */
 	}
 
-	if (!i915.enable_guc_submission)
-		return;
-
 	guc_fw->guc_dev = dev;
 	guc_fw->guc_fw_path = fw_path;
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
+	/* Early (and silent) return if GuC loading is disabled */
+	if (!i915.enable_guc_loading)
+		return;
 	if (fw_path == NULL)
 		return;
-
-	if (*fw_path == '\0') {
-		DRM_ERROR("No GuC firmware known for this platform\n");
-		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
+	if (*fw_path == '\0')
 		return;
-	}
 
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-- 
1.9.1

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

Re: [PATCH v4 3/7] drm/i915/guc: add enable_guc_loading parameter

[Tvrtko Ursulin]

On 13/05/16 15:36, Dave Gordon wrote:
> Split the function of "enable_guc_submission" into two separate
> options.  The new one ("enable_guc_loading") controls only the
> *fetching and loading* of the GuC firmware image. The existing
> one is redefined to control only the *use* of the GuC for batch
> submission once the firmware is loaded.
>
> In addition, the degree of control has been refined from a simple
> bool to an integer key, allowing several options:
> -1 (default)     whatever the platform default is
>   0  DISABLE      don't load/use the GuC
>   1  BEST EFFORT  try to load/use the GuC, fallback if not available
>   2  REQUIRE      must load/use the GuC, else leave the GPU wedged
>
> The new platform default (as coded here) will be to attempt to
> load the GuC iff the device has a GuC that requires firmware,
> but not yet to use it for submission. A later patch will change
> to enable it if appropriate.
>
> v4:
>      Changed some error-message levels, mostly ERROR->INFO, per
>      review comments by Tvrtko Ursulin.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c            |   1 -
>   drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
>   drivers/gpu/drm/i915/i915_params.c         |  14 +++-
>   drivers/gpu/drm/i915/i915_params.h         |   3 +-
>   drivers/gpu/drm/i915/intel_guc_loader.c    | 108 ++++++++++++++++-------------
>   5 files changed, 76 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2a7be71..2bf8743 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4870,7 +4870,6 @@ int i915_gem_init_engines(struct drm_device *dev)
>   		ret = intel_guc_setup(dev);
>   		if (ret) {
>   			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);

This error msg should go as well I think.

> -			ret = -EIO;
>   			goto out;
>   		}
>   	}
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 169242a..916cd67 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
>   	ctx = dev_priv->kernel_context;
> @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;

Will the above two do the right thing for the fw_path == NULL case? That 
is !HAS_GUC_UCODE && HAS_GUC_SCHED? Looks like load status will bt NONE 
in that case, GuC submission will be enabled and suspend and resume 
hooks will be skipped?

Maybe fetch and load status should be set to success on such platforms?

>
>   	ctx = dev_priv->kernel_context;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 383c076..6a5578c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = {
>   	.verbose_state_checks = 1,
>   	.nuclear_pageflip = 0,
>   	.edp_vswing = 0,
> -	.enable_guc_submission = false,
> +	.enable_guc_loading = -1,
> +	.enable_guc_submission = 0,
>   	.guc_log_level = -1,
>   	.enable_dp_mst = true,
>   	.inject_load_failure = 0,
> @@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = {
>   		 "(0=use value from vbt [default], 1=low power swing(200mV),"
>   		 "2=default swing(400mV))");
>
> -module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400);
> -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
> +module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
> +MODULE_PARM_DESC(enable_guc_loading,
> +		"Enable GuC firmware loading "
> +		"(-1=auto [default], 0=never, 1=if available, 2=required)");
> +
> +module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
> +MODULE_PARM_DESC(enable_guc_submission,
> +		"Enable GuC submission "
> +		"(-1=auto, 0=never [default], 1=if available, 2=required)");
>
>   module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>   MODULE_PARM_DESC(guc_log_level,
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 65e73dd..1323261 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -45,6 +45,8 @@ struct i915_params {
>   	int enable_ips;
>   	int invert_brightness;
>   	int enable_cmd_parser;
> +	int enable_guc_loading;
> +	int enable_guc_submission;
>   	int guc_log_level;
>   	int use_mmio_flip;
>   	int mmio_debug;
> @@ -57,7 +59,6 @@ struct i915_params {
>   	bool load_detect_test;
>   	bool reset;
>   	bool disable_display;
> -	bool enable_guc_submission;
>   	bool verbose_state_checks;
>   	bool nuclear_pageflip;
>   	bool enable_dp_mst;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b14a3a3..3d33944 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	int retries, err = 0;
> +	const char *fw_path = guc_fw->guc_fw_path;
> +	int retries, ret, err = 0;
>
> -	if (!i915.enable_guc_submission)
> -		return 0;
> -
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> +		fw_path,
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	direct_interrupts_to_host(dev_priv);
> +	/* Loading forbidden, or no firmware to load? */
> +	if (!i915.enable_guc_loading)
> +		goto fail;
> +	if (fw_path == NULL)
> +		goto fail;
> +	if (*fw_path == '\0') {
> +		DRM_INFO("No GuC firmware known for this platform\n");
> +		goto fail;
> +	}
>
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
> -		return 0;
> +	/* Fetch failed, or already fetched but failed to load? */
> +	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS)
> +		goto fail;
> +	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
> +		goto fail;
>
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
> -	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
> -		return -ENOEXEC;
> +	direct_interrupts_to_host(dev_priv);
>
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> -	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> -
> -	switch (guc_fw->guc_fw_fetch_status) {
> -	case GUC_FIRMWARE_FAIL:
> -		/* something went wrong :( */
> -		err = -EIO;
> -		goto fail;
> -
> -	case GUC_FIRMWARE_NONE:
> -	case GUC_FIRMWARE_PENDING:
> -	default:
> -		/* "can't happen" */
> -		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
> -			guc_fw->guc_fw_path,
> -			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -			guc_fw->guc_fw_fetch_status);
> -		err = -ENXIO;
> -		goto fail;
> -
> -	case GUC_FIRMWARE_SUCCESS:
> -		break;
> -	}
> +	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>   	err = i915_guc_submission_init(dev);
>   	if (err)
> @@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev)
>   	return 0;
>
>   fail:
> -	DRM_ERROR("GuC firmware load failed, err %d\n", err);
>   	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
>   		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>
> @@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev)
>   	i915_guc_submission_disable(dev);
>   	i915_guc_submission_fini(dev);
>
> -	return err;
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1) {
> +		ret = -EIO;
> +	} else if (HAS_GUC_SCHED(dev) && !HAS_GUC_UCODE(dev)) {
> +		/* This GuC works even without firmware :) */
> +		return 0;
> +	} else if (i915.enable_guc_submission > 1) {
> +		ret = -EIO;
> +	} else {
> +		ret = 0;
> +	}
> +
> +	if (ret == -EIO)
> +		DRM_ERROR("GuC firmware load failed, err %d\n", err);
> +	else
> +		DRM_INFO("GuC firmware load failed, err %d\n", err);
> +
> +	if (i915.enable_guc_submission)
> +		DRM_INFO("falling back to execlist mode, ret %d\n", ret);

ret is always zero in this msg?

> +	i915.enable_guc_submission = 0;
> +
> +	return ret;
>   }
>
>   static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
> @@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev)
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>   	const char *fw_path;
>
> -	if (!HAS_GUC_SCHED(dev))
> -		i915.enable_guc_submission = false;
> +	/* A negative value means "use platform default" */
> +	if (i915.enable_guc_loading < 0)
> +		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
> +	if (i915.enable_guc_submission < 0)
> +		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
>
>   	if (!HAS_GUC_UCODE(dev)) {
>   		fw_path = NULL;
> @@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev)
>   		guc_fw->guc_fw_major_wanted = 8;
>   		guc_fw->guc_fw_minor_wanted = 7;
>   	} else {
> -		i915.enable_guc_submission = false;
>   		fw_path = "";	/* unknown device */
>   	}
>
> -	if (!i915.enable_guc_submission)
> -		return;
> -
>   	guc_fw->guc_dev = dev;
>   	guc_fw->guc_fw_path = fw_path;
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> +	/* Early (and silent) return if GuC loading is disabled */
> +	if (!i915.enable_guc_loading)
> +		return;
>   	if (fw_path == NULL)
>   		return;
> -
> -	if (*fw_path == '\0') {
> -		DRM_ERROR("No GuC firmware known for this platform\n");
> -		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> +	if (*fw_path == '\0')
>   		return;
> -	}
>
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
>   	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
>

Regards,

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

Re: [PATCH v4 3/7] drm/i915/guc: add enable_guc_loading parameter

[Dave Gordon]

On 13/05/16 16:31, Tvrtko Ursulin wrote:
>
> On 13/05/16 15:36, Dave Gordon wrote:
>> Split the function of "enable_guc_submission" into two separate
>> options.  The new one ("enable_guc_loading") controls only the
>> *fetching and loading* of the GuC firmware image. The existing
>> one is redefined to control only the *use* of the GuC for batch
>> submission once the firmware is loaded.
>>
>> In addition, the degree of control has been refined from a simple
>> bool to an integer key, allowing several options:
>> -1 (default)     whatever the platform default is
>>   0  DISABLE      don't load/use the GuC
>>   1  BEST EFFORT  try to load/use the GuC, fallback if not available
>>   2  REQUIRE      must load/use the GuC, else leave the GPU wedged
>>
>> The new platform default (as coded here) will be to attempt to
>> load the GuC iff the device has a GuC that requires firmware,
>> but not yet to use it for submission. A later patch will change
>> to enable it if appropriate.
>>
>> v4:
>>      Changed some error-message levels, mostly ERROR->INFO, per
>>      review comments by Tvrtko Ursulin.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c            |   1 -
>>   drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
>>   drivers/gpu/drm/i915/i915_params.c         |  14 +++-
>>   drivers/gpu/drm/i915/i915_params.h         |   3 +-
>>   drivers/gpu/drm/i915/intel_guc_loader.c    | 108
>> ++++++++++++++++-------------
>>   5 files changed, 76 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 2a7be71..2bf8743 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4870,7 +4870,6 @@ int i915_gem_init_engines(struct drm_device *dev)
>>           ret = intel_guc_setup(dev);
>>           if (ret) {
>>               DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
>
> This error msg should go as well I think.

Yes, if we reach this we'll have just printed a message in 
intel_guc_setup() so we don't really need both.

>> -            ret = -EIO;
>>               goto out;
>>           }
>>       }
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 169242a..916cd67 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
>>       struct intel_context *ctx;
>>       u32 data[3];
>>
>> -    if (!i915.enable_guc_submission)
>> +    if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>>           return 0;
>>
>>       ctx = dev_priv->kernel_context;
>> @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
>>       struct intel_context *ctx;
>>       u32 data[3];
>>
>> -    if (!i915.enable_guc_submission)
>> +    if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>>           return 0;
>
> Will the above two do the right thing for the fw_path == NULL case? That
> is !HAS_GUC_UCODE && HAS_GUC_SCHED? Looks like load status will bt NONE
> in that case, GuC submission will be enabled and suspend and resume
> hooks will be skipped?
>
> Maybe fetch and load status should be set to success on such platforms?

I think probably fetch==NONE but load==SUCCESS, in which case the code 
above will be correct already. OTOH there aren't actually any such 
platforms yet, and intel_guc_setup() doesn't really support this fully; 
for instance, we don't know whether the correct behaviour of _setup() on 
such a hypothetical platform would be to reset the GuC and just skip the 
DMA, or to skip the reset as well. Certainly some of the setup would 
still be required.

So for now I've forced GuC submission off for any such platform, so the 
code above should be OK for the four possibilities (no GuC, GuC not 
loaded, GuC loaded but not used for submission, GuC loaded and in use).

We can revisit this in the event that a firmware-free GuC ever appears!

[snip]

>> @@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev)
>>       return 0;
>>
>>   fail:
>> -    DRM_ERROR("GuC firmware load failed, err %d\n", err);
>>       if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
>>           guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>>
>> @@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev)
>>       i915_guc_submission_disable(dev);
>>       i915_guc_submission_fini(dev);
>>
>> -    return err;
>> +    /*
>> +     * We've failed to load the firmware :(
>> +     *
>> +     * Decide whether to disable GuC submission and fall back to
>> +     * execlist mode, and whether to hide the error by returning
>> +     * zero or to return -EIO, which the caller will treat as a
>> +     * nonfatal error (i.e. it doesn't prevent driver load, but
>> +     * marks the GPU as wedged until reset).
>> +     */
>> +    if (i915.enable_guc_loading > 1) {
>> +        ret = -EIO;
>> +    } else if (HAS_GUC_SCHED(dev) && !HAS_GUC_UCODE(dev)) {
>> +        /* This GuC works even without firmware :) */
>> +        return 0;
>> +    } else if (i915.enable_guc_submission > 1) {
>> +        ret = -EIO;
>> +    } else {
>> +        ret = 0;
>> +    }
>> +
>> +    if (ret == -EIO)
>> +        DRM_ERROR("GuC firmware load failed, err %d\n", err);
>> +    else
>> +        DRM_INFO("GuC firmware load failed, err %d\n", err);
>> +
>> +    if (i915.enable_guc_submission)
>> +        DRM_INFO("falling back to execlist mode, ret %d\n", ret);
>
> ret is always zero in this msg?

I shouldn't think so; AFAICS it could also be -EIO. This message can be 
printed in conjunction with either variant of the message above.

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

[PATCH v5 3/7] drm/i915/guc: add enable_guc_loading parameter

[Dave Gordon]

Split the function of "enable_guc_submission" into two separate
options.  The new one ("enable_guc_loading") controls only the
*fetching and loading* of the GuC firmware image. The existing
one is redefined to control only the *use* of the GuC for batch
submission once the firmware is loaded.

In addition, the degree of control has been refined from a simple
bool to an integer key, allowing several options:
-1 (default)     whatever the platform default is
 0  DISABLE      don't load/use the GuC
 1  BEST EFFORT  try to load/use the GuC, fallback if not available
 2  REQUIRE      must load/use the GuC, else leave the GPU wedged

The new platform default (as coded here) will be to attempt to
load the GuC iff the device has a GuC that requires firmware,
but not yet to use it for submission. A later patch will change
to enable it if appropriate.

v4:
    Changed some error-message levels, mostly ERROR->INFO, per
    review comments by Tvrtko Ursulin.

v5:
    Dropped one more error message, disabled GuC submission on
    hypothetical firmware-free devices [Tvrtko Ursulin].

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   5 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
 drivers/gpu/drm/i915/i915_params.c         |  14 +++-
 drivers/gpu/drm/i915/i915_params.h         |   3 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 108 ++++++++++++++++-------------
 5 files changed, 77 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2cc8c24..1e63744 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4868,11 +4868,8 @@ int i915_gem_init_engines(struct drm_device *dev)
 	/* We can't enable contexts until all firmware is loaded */
 	if (HAS_GUC(dev)) {
 		ret = intel_guc_setup(dev);
-		if (ret) {
-			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
-			ret = -EIO;
+		if (ret)
 			goto out;
-		}
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 169242a..916cd67 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
@@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 383c076..6a5578c 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_submission = false,
+	.enable_guc_loading = -1,
+	.enable_guc_submission = 0,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
@@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = {
 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
 		 "2=default swing(400mV))");
 
-module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400);
-MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
+module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
+MODULE_PARM_DESC(enable_guc_loading,
+		"Enable GuC firmware loading "
+		"(-1=auto [default], 0=never, 1=if available, 2=required)");
+
+module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
+MODULE_PARM_DESC(enable_guc_submission,
+		"Enable GuC submission "
+		"(-1=auto, 0=never [default], 1=if available, 2=required)");
 
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 65e73dd..1323261 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,6 +45,8 @@ struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
+	int enable_guc_loading;
+	int enable_guc_submission;
 	int guc_log_level;
 	int use_mmio_flip;
 	int mmio_debug;
@@ -57,7 +59,6 @@ struct i915_params {
 	bool load_detect_test;
 	bool reset;
 	bool disable_display;
-	bool enable_guc_submission;
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
 	bool enable_dp_mst;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index b14a3a3..7e67f73 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
-	int retries, err = 0;
+	const char *fw_path = guc_fw->guc_fw_path;
+	int retries, ret, err = 0;
 
-	if (!i915.enable_guc_submission)
-		return 0;
-
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
+		fw_path,
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	direct_interrupts_to_host(dev_priv);
+	/* Loading forbidden, or no firmware to load? */
+	if (!i915.enable_guc_loading)
+		goto fail;
+	if (fw_path == NULL)
+		goto fail;
+	if (*fw_path == '\0') {
+		DRM_INFO("No GuC firmware known for this platform\n");
+		goto fail;
+	}
 
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
-		return 0;
+	/* Fetch failed, or already fetched but failed to load? */
+	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS)
+		goto fail;
+	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
+		goto fail;
 
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
-	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
-		return -ENOEXEC;
+	direct_interrupts_to_host(dev_priv);
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
 
-	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
-
-	switch (guc_fw->guc_fw_fetch_status) {
-	case GUC_FIRMWARE_FAIL:
-		/* something went wrong :( */
-		err = -EIO;
-		goto fail;
-
-	case GUC_FIRMWARE_NONE:
-	case GUC_FIRMWARE_PENDING:
-	default:
-		/* "can't happen" */
-		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
-			guc_fw->guc_fw_path,
-			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-			guc_fw->guc_fw_fetch_status);
-		err = -ENXIO;
-		goto fail;
-
-	case GUC_FIRMWARE_SUCCESS:
-		break;
-	}
+	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
+		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	err = i915_guc_submission_init(dev);
 	if (err)
@@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev)
 	return 0;
 
 fail:
-	DRM_ERROR("GuC firmware load failed, err %d\n", err);
 	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
 		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
 
@@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev)
 	i915_guc_submission_disable(dev);
 	i915_guc_submission_fini(dev);
 
-	return err;
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	if (i915.enable_guc_loading > 1) {
+		ret = -EIO;
+	} else if (i915.enable_guc_submission > 1) {
+		ret = -EIO;
+	} else {
+		ret = 0;
+	}
+
+	if (ret == -EIO)
+		DRM_ERROR("GuC firmware load failed, err %d\n", err);
+	else
+		DRM_INFO("GuC firmware load failed, err %d\n", err);
+
+	if (i915.enable_guc_submission) {
+		if (fw_path == NULL)
+			DRM_INFO("GuC submission without firmware not supported\n");
+		DRM_INFO("falling back to execlist mode, ret %d\n", ret);
+	}
+	i915.enable_guc_submission = 0;
+
+	return ret;
 }
 
 static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
@@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	if (!HAS_GUC_SCHED(dev))
-		i915.enable_guc_submission = false;
+	/* A negative value means "use platform default" */
+	if (i915.enable_guc_loading < 0)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	if (i915.enable_guc_submission < 0)
+		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
@@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev)
 		guc_fw->guc_fw_major_wanted = 8;
 		guc_fw->guc_fw_minor_wanted = 7;
 	} else {
-		i915.enable_guc_submission = false;
 		fw_path = "";	/* unknown device */
 	}
 
-	if (!i915.enable_guc_submission)
-		return;
-
 	guc_fw->guc_dev = dev;
 	guc_fw->guc_fw_path = fw_path;
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
+	/* Early (and silent) return if GuC loading is disabled */
+	if (!i915.enable_guc_loading)
+		return;
 	if (fw_path == NULL)
 		return;
-
-	if (*fw_path == '\0') {
-		DRM_ERROR("No GuC firmware known for this platform\n");
-		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
+	if (*fw_path == '\0')
 		return;
-	}
 
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-- 
1.9.1

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

Re: [PATCH v5 3/7] drm/i915/guc: add enable_guc_loading parameter

[Tvrtko Ursulin]

On 16/05/16 20:12, Dave Gordon wrote:
> Split the function of "enable_guc_submission" into two separate
> options.  The new one ("enable_guc_loading") controls only the
> *fetching and loading* of the GuC firmware image. The existing
> one is redefined to control only the *use* of the GuC for batch
> submission once the firmware is loaded.
>
> In addition, the degree of control has been refined from a simple
> bool to an integer key, allowing several options:
> -1 (default)     whatever the platform default is
>   0  DISABLE      don't load/use the GuC
>   1  BEST EFFORT  try to load/use the GuC, fallback if not available
>   2  REQUIRE      must load/use the GuC, else leave the GPU wedged
>
> The new platform default (as coded here) will be to attempt to
> load the GuC iff the device has a GuC that requires firmware,
> but not yet to use it for submission. A later patch will change
> to enable it if appropriate.
>
> v4:
>      Changed some error-message levels, mostly ERROR->INFO, per
>      review comments by Tvrtko Ursulin.
>
> v5:
>      Dropped one more error message, disabled GuC submission on
>      hypothetical firmware-free devices [Tvrtko Ursulin].

Ok, hope it works. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Has someone at least done some manual testing on the various combination 
of new option values? Please have them splat a Tested-by just because 
the logic is not straight forward for me.

Regards,

Tvrtko

>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c            |   5 +-
>   drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
>   drivers/gpu/drm/i915/i915_params.c         |  14 +++-
>   drivers/gpu/drm/i915/i915_params.h         |   3 +-
>   drivers/gpu/drm/i915/intel_guc_loader.c    | 108 ++++++++++++++++-------------
>   5 files changed, 77 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2cc8c24..1e63744 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4868,11 +4868,8 @@ int i915_gem_init_engines(struct drm_device *dev)
>   	/* We can't enable contexts until all firmware is loaded */
>   	if (HAS_GUC(dev)) {
>   		ret = intel_guc_setup(dev);
> -		if (ret) {
> -			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
> -			ret = -EIO;
> +		if (ret)
>   			goto out;
> -		}
>   	}
>
>   	/*
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 169242a..916cd67 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
>   	ctx = dev_priv->kernel_context;
> @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
>   	ctx = dev_priv->kernel_context;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 383c076..6a5578c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = {
>   	.verbose_state_checks = 1,
>   	.nuclear_pageflip = 0,
>   	.edp_vswing = 0,
> -	.enable_guc_submission = false,
> +	.enable_guc_loading = -1,
> +	.enable_guc_submission = 0,
>   	.guc_log_level = -1,
>   	.enable_dp_mst = true,
>   	.inject_load_failure = 0,
> @@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = {
>   		 "(0=use value from vbt [default], 1=low power swing(200mV),"
>   		 "2=default swing(400mV))");
>
> -module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400);
> -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
> +module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
> +MODULE_PARM_DESC(enable_guc_loading,
> +		"Enable GuC firmware loading "
> +		"(-1=auto [default], 0=never, 1=if available, 2=required)");
> +
> +module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
> +MODULE_PARM_DESC(enable_guc_submission,
> +		"Enable GuC submission "
> +		"(-1=auto, 0=never [default], 1=if available, 2=required)");
>
>   module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>   MODULE_PARM_DESC(guc_log_level,
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 65e73dd..1323261 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -45,6 +45,8 @@ struct i915_params {
>   	int enable_ips;
>   	int invert_brightness;
>   	int enable_cmd_parser;
> +	int enable_guc_loading;
> +	int enable_guc_submission;
>   	int guc_log_level;
>   	int use_mmio_flip;
>   	int mmio_debug;
> @@ -57,7 +59,6 @@ struct i915_params {
>   	bool load_detect_test;
>   	bool reset;
>   	bool disable_display;
> -	bool enable_guc_submission;
>   	bool verbose_state_checks;
>   	bool nuclear_pageflip;
>   	bool enable_dp_mst;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b14a3a3..7e67f73 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	int retries, err = 0;
> +	const char *fw_path = guc_fw->guc_fw_path;
> +	int retries, ret, err = 0;
>
> -	if (!i915.enable_guc_submission)
> -		return 0;
> -
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> +		fw_path,
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	direct_interrupts_to_host(dev_priv);
> +	/* Loading forbidden, or no firmware to load? */
> +	if (!i915.enable_guc_loading)
> +		goto fail;
> +	if (fw_path == NULL)
> +		goto fail;
> +	if (*fw_path == '\0') {
> +		DRM_INFO("No GuC firmware known for this platform\n");
> +		goto fail;
> +	}
>
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
> -		return 0;
> +	/* Fetch failed, or already fetched but failed to load? */
> +	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS)
> +		goto fail;
> +	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
> +		goto fail;
>
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
> -	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
> -		return -ENOEXEC;
> +	direct_interrupts_to_host(dev_priv);
>
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> -	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> -
> -	switch (guc_fw->guc_fw_fetch_status) {
> -	case GUC_FIRMWARE_FAIL:
> -		/* something went wrong :( */
> -		err = -EIO;
> -		goto fail;
> -
> -	case GUC_FIRMWARE_NONE:
> -	case GUC_FIRMWARE_PENDING:
> -	default:
> -		/* "can't happen" */
> -		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
> -			guc_fw->guc_fw_path,
> -			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -			guc_fw->guc_fw_fetch_status);
> -		err = -ENXIO;
> -		goto fail;
> -
> -	case GUC_FIRMWARE_SUCCESS:
> -		break;
> -	}
> +	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>   	err = i915_guc_submission_init(dev);
>   	if (err)
> @@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev)
>   	return 0;
>
>   fail:
> -	DRM_ERROR("GuC firmware load failed, err %d\n", err);
>   	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
>   		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>
> @@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev)
>   	i915_guc_submission_disable(dev);
>   	i915_guc_submission_fini(dev);
>
> -	return err;
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1) {
> +		ret = -EIO;
> +	} else if (i915.enable_guc_submission > 1) {
> +		ret = -EIO;
> +	} else {
> +		ret = 0;
> +	}
> +
> +	if (ret == -EIO)
> +		DRM_ERROR("GuC firmware load failed, err %d\n", err);
> +	else
> +		DRM_INFO("GuC firmware load failed, err %d\n", err);
> +
> +	if (i915.enable_guc_submission) {
> +		if (fw_path == NULL)
> +			DRM_INFO("GuC submission without firmware not supported\n");
> +		DRM_INFO("falling back to execlist mode, ret %d\n", ret);
> +	}
> +	i915.enable_guc_submission = 0;
> +
> +	return ret;
>   }
>
>   static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
> @@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev)
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>   	const char *fw_path;
>
> -	if (!HAS_GUC_SCHED(dev))
> -		i915.enable_guc_submission = false;
> +	/* A negative value means "use platform default" */
> +	if (i915.enable_guc_loading < 0)
> +		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
> +	if (i915.enable_guc_submission < 0)
> +		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
>
>   	if (!HAS_GUC_UCODE(dev)) {
>   		fw_path = NULL;
> @@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev)
>   		guc_fw->guc_fw_major_wanted = 8;
>   		guc_fw->guc_fw_minor_wanted = 7;
>   	} else {
> -		i915.enable_guc_submission = false;
>   		fw_path = "";	/* unknown device */
>   	}
>
> -	if (!i915.enable_guc_submission)
> -		return;
> -
>   	guc_fw->guc_dev = dev;
>   	guc_fw->guc_fw_path = fw_path;
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> +	/* Early (and silent) return if GuC loading is disabled */
> +	if (!i915.enable_guc_loading)
> +		return;
>   	if (fw_path == NULL)
>   		return;
> -
> -	if (*fw_path == '\0') {
> -		DRM_ERROR("No GuC firmware known for this platform\n");
> -		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> +	if (*fw_path == '\0')
>   		return;
> -	}
>
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
>   	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 3/7] drm/i915/guc: add enable_guc_loading parameter

[Fiedorowicz, Lukasz]

Tested-by: Fiedorowicz, Lukasz <lukasz.fiedorowicz@intel.com>

I've done some manual testing on patches mentioned below. Results looks good. I did never encountered any kernel panic or hangs.
 GPU was wedged when scenario expected it. My testing scenario would look like this:

- boot with different parameters combination
- run gem_exec_nop
- check dmesg and guc debugfs files

When GuC was required (option 2) GPU was wedged as expected. When GuC wasn't required in successfully fall back to legacy submission. I've checked scenarios with and without valid GuC firmware present.

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Tvrtko Ursulin
Sent: Tuesday, May 17, 2016 11:09 AM
To: Gordon, David S <david.s.gordon@intel.com>; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v5 3/7] drm/i915/guc: add enable_guc_loading parameter


On 16/05/16 20:12, Dave Gordon wrote:
> Split the function of "enable_guc_submission" into two separate 
> options.  The new one ("enable_guc_loading") controls only the 
> *fetching and loading* of the GuC firmware image. The existing one is 
> redefined to control only the *use* of the GuC for batch submission 
> once the firmware is loaded.
>
> In addition, the degree of control has been refined from a simple bool 
> to an integer key, allowing several options:
> -1 (default)     whatever the platform default is
>   0  DISABLE      don't load/use the GuC
>   1  BEST EFFORT  try to load/use the GuC, fallback if not available
>   2  REQUIRE      must load/use the GuC, else leave the GPU wedged
>
> The new platform default (as coded here) will be to attempt to load 
> the GuC iff the device has a GuC that requires firmware, but not yet 
> to use it for submission. A later patch will change to enable it if 
> appropriate.
>
> v4:
>      Changed some error-message levels, mostly ERROR->INFO, per
>      review comments by Tvrtko Ursulin.
>
> v5:
>      Dropped one more error message, disabled GuC submission on
>      hypothetical firmware-free devices [Tvrtko Ursulin].

Ok, hope it works. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Has someone at least done some manual testing on the various combination of new option values? Please have them splat a Tested-by just because the logic is not straight forward for me.

Regards,

Tvrtko

>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c            |   5 +-
>   drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
>   drivers/gpu/drm/i915/i915_params.c         |  14 +++-
>   drivers/gpu/drm/i915/i915_params.h         |   3 +-
>   drivers/gpu/drm/i915/intel_guc_loader.c    | 108 ++++++++++++++++-------------
>   5 files changed, 77 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> b/drivers/gpu/drm/i915/i915_gem.c index 2cc8c24..1e63744 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4868,11 +4868,8 @@ int i915_gem_init_engines(struct drm_device *dev)
>   	/* We can't enable contexts until all firmware is loaded */
>   	if (HAS_GUC(dev)) {
>   		ret = intel_guc_setup(dev);
> -		if (ret) {
> -			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
> -			ret = -EIO;
> +		if (ret)
>   			goto out;
> -		}
>   	}
>
>   	/*
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 169242a..916cd67 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
>   	ctx = dev_priv->kernel_context;
> @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
>   	ctx = dev_priv->kernel_context;
> diff --git a/drivers/gpu/drm/i915/i915_params.c 
> b/drivers/gpu/drm/i915/i915_params.c
> index 383c076..6a5578c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = {
>   	.verbose_state_checks = 1,
>   	.nuclear_pageflip = 0,
>   	.edp_vswing = 0,
> -	.enable_guc_submission = false,
> +	.enable_guc_loading = -1,
> +	.enable_guc_submission = 0,
>   	.guc_log_level = -1,
>   	.enable_dp_mst = true,
>   	.inject_load_failure = 0,
> @@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = {
>   		 "(0=use value from vbt [default], 1=low power swing(200mV),"
>   		 "2=default swing(400mV))");
>
> -module_param_named_unsafe(enable_guc_submission, 
> i915.enable_guc_submission, bool, 0400); 
> -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission 
> (default:false)");
> +module_param_named_unsafe(enable_guc_loading, 
> +i915.enable_guc_loading, int, 0400); MODULE_PARM_DESC(enable_guc_loading,
> +		"Enable GuC firmware loading "
> +		"(-1=auto [default], 0=never, 1=if available, 2=required)");
> +
> +module_param_named_unsafe(enable_guc_submission, 
> +i915.enable_guc_submission, int, 0400); MODULE_PARM_DESC(enable_guc_submission,
> +		"Enable GuC submission "
> +		"(-1=auto, 0=never [default], 1=if available, 2=required)");
>
>   module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>   MODULE_PARM_DESC(guc_log_level,
> diff --git a/drivers/gpu/drm/i915/i915_params.h 
> b/drivers/gpu/drm/i915/i915_params.h
> index 65e73dd..1323261 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -45,6 +45,8 @@ struct i915_params {
>   	int enable_ips;
>   	int invert_brightness;
>   	int enable_cmd_parser;
> +	int enable_guc_loading;
> +	int enable_guc_submission;
>   	int guc_log_level;
>   	int use_mmio_flip;
>   	int mmio_debug;
> @@ -57,7 +59,6 @@ struct i915_params {
>   	bool load_detect_test;
>   	bool reset;
>   	bool disable_display;
> -	bool enable_guc_submission;
>   	bool verbose_state_checks;
>   	bool nuclear_pageflip;
>   	bool enable_dp_mst;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 
> b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b14a3a3..7e67f73 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	int retries, err = 0;
> +	const char *fw_path = guc_fw->guc_fw_path;
> +	int retries, ret, err = 0;
>
> -	if (!i915.enable_guc_submission)
> -		return 0;
> -
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> +		fw_path,
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	direct_interrupts_to_host(dev_priv);
> +	/* Loading forbidden, or no firmware to load? */
> +	if (!i915.enable_guc_loading)
> +		goto fail;
> +	if (fw_path == NULL)
> +		goto fail;
> +	if (*fw_path == '\0') {
> +		DRM_INFO("No GuC firmware known for this platform\n");
> +		goto fail;
> +	}
>
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
> -		return 0;
> +	/* Fetch failed, or already fetched but failed to load? */
> +	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS)
> +		goto fail;
> +	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
> +		goto fail;
>
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
> -	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
> -		return -ENOEXEC;
> +	direct_interrupts_to_host(dev_priv);
>
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> -	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> -
> -	switch (guc_fw->guc_fw_fetch_status) {
> -	case GUC_FIRMWARE_FAIL:
> -		/* something went wrong :( */
> -		err = -EIO;
> -		goto fail;
> -
> -	case GUC_FIRMWARE_NONE:
> -	case GUC_FIRMWARE_PENDING:
> -	default:
> -		/* "can't happen" */
> -		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
> -			guc_fw->guc_fw_path,
> -			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -			guc_fw->guc_fw_fetch_status);
> -		err = -ENXIO;
> -		goto fail;
> -
> -	case GUC_FIRMWARE_SUCCESS:
> -		break;
> -	}
> +	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>   	err = i915_guc_submission_init(dev);
>   	if (err)
> @@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev)
>   	return 0;
>
>   fail:
> -	DRM_ERROR("GuC firmware load failed, err %d\n", err);
>   	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
>   		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>
> @@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev)
>   	i915_guc_submission_disable(dev);
>   	i915_guc_submission_fini(dev);
>
> -	return err;
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1) {
> +		ret = -EIO;
> +	} else if (i915.enable_guc_submission > 1) {
> +		ret = -EIO;
> +	} else {
> +		ret = 0;
> +	}
> +
> +	if (ret == -EIO)
> +		DRM_ERROR("GuC firmware load failed, err %d\n", err);
> +	else
> +		DRM_INFO("GuC firmware load failed, err %d\n", err);
> +
> +	if (i915.enable_guc_submission) {
> +		if (fw_path == NULL)
> +			DRM_INFO("GuC submission without firmware not supported\n");
> +		DRM_INFO("falling back to execlist mode, ret %d\n", ret);
> +	}
> +	i915.enable_guc_submission = 0;
> +
> +	return ret;
>   }
>
>   static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw 
> *guc_fw) @@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev)
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>   	const char *fw_path;
>
> -	if (!HAS_GUC_SCHED(dev))
> -		i915.enable_guc_submission = false;
> +	/* A negative value means "use platform default" */
> +	if (i915.enable_guc_loading < 0)
> +		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
> +	if (i915.enable_guc_submission < 0)
> +		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
>
>   	if (!HAS_GUC_UCODE(dev)) {
>   		fw_path = NULL;
> @@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev)
>   		guc_fw->guc_fw_major_wanted = 8;
>   		guc_fw->guc_fw_minor_wanted = 7;
>   	} else {
> -		i915.enable_guc_submission = false;
>   		fw_path = "";	/* unknown device */
>   	}
>
> -	if (!i915.enable_guc_submission)
> -		return;
> -
>   	guc_fw->guc_dev = dev;
>   	guc_fw->guc_fw_path = fw_path;
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> +	/* Early (and silent) return if GuC loading is disabled */
> +	if (!i915.enable_guc_loading)
> +		return;
>   	if (fw_path == NULL)
>   		return;
> -
> -	if (*fw_path == '\0') {
> -		DRM_ERROR("No GuC firmware known for this platform\n");
> -		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> +	if (*fw_path == '\0')
>   		return;
> -	}
>
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
>   	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
>
_______________________________________________
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

[PATCH v6 3/7] drm/i915/guc: add enable_guc_loading parameter

[Tvrtko Ursulin]

From: Dave Gordon <david.s.gordon@intel.com>

Split the function of "enable_guc_submission" into two separate
options.  The new one ("enable_guc_loading") controls only the
*fetching and loading* of the GuC firmware image. The existing
one is redefined to control only the *use* of the GuC for batch
submission once the firmware is loaded.

In addition, the degree of control has been refined from a simple
bool to an integer key, allowing several options:
-1 (default)     whatever the platform default is
 0  DISABLE      don't load/use the GuC
 1  BEST EFFORT  try to load/use the GuC, fallback if not available
 2  REQUIRE      must load/use the GuC, else leave the GPU wedged

The new platform default (as coded here) will be to attempt to
load the GuC iff the device has a GuC that requires firmware,
but not yet to use it for submission. A later patch will change
to enable it if appropriate.

v4:
    Changed some error-message levels, mostly ERROR->INFO, per
    review comments by Tvrtko Ursulin.

v5:
    Dropped one more error message, disabled GuC submission on
    hypothetical firmware-free devices [Tvrtko Ursulin].

v6:
    Logging tidy by Tvrtko Ursulin:
     * Do not log falling back to execlists when wedging the GPU.
     * Do not log fw load errors when load was disabled by user.
     * Pass down some error code from fw load for log message to
       make more sense.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> (v5)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   5 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
 drivers/gpu/drm/i915/i915_params.c         |  14 +++-
 drivers/gpu/drm/i915/i915_params.h         |   3 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 123 +++++++++++++++++------------
 5 files changed, 89 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 88dce5482f2f..1a3a07eca0d0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4868,11 +4868,8 @@ i915_gem_init_hw(struct drm_device *dev)
 	/* We can't enable contexts until all firmware is loaded */
 	if (HAS_GUC(dev)) {
 		ret = intel_guc_setup(dev);
-		if (ret) {
-			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
-			ret = -EIO;
+		if (ret)
 			goto out;
-		}
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 169242a8adff..916cd6778cf3 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
@@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index cd74fb8e9387..21a323c01cdb 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -53,7 +53,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_submission = false,
+	.enable_guc_loading = -1,
+	.enable_guc_submission = 0,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
@@ -193,8 +194,15 @@ MODULE_PARM_DESC(edp_vswing,
 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
 		 "2=default swing(400mV))");
 
-module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400);
-MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
+module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
+MODULE_PARM_DESC(enable_guc_loading,
+		"Enable GuC firmware loading "
+		"(-1=auto [default], 0=never, 1=if available, 2=required)");
+
+module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
+MODULE_PARM_DESC(enable_guc_submission,
+		"Enable GuC submission "
+		"(-1=auto, 0=never [default], 1=if available, 2=required)");
 
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index dd0d0bbcc05b..658ce7379671 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,6 +45,8 @@ struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
+	int enable_guc_loading;
+	int enable_guc_submission;
 	int guc_log_level;
 	int mmio_debug;
 	int edp_vswing;
@@ -56,7 +58,6 @@ struct i915_params {
 	bool load_detect_test;
 	bool reset;
 	bool disable_display;
-	bool enable_guc_submission;
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
 	bool enable_dp_mst;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 30ec8208dbbc..29273e5fee22 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -402,50 +402,42 @@ int intel_guc_setup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
-	int retries, err = 0;
+	const char *fw_path = guc_fw->guc_fw_path;
+	int retries, ret, err;
 
-	if (!i915.enable_guc_submission)
-		return 0;
-
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
+		fw_path,
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	direct_interrupts_to_host(dev_priv);
-
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
-		return 0;
-
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
-	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
-		return -ENOEXEC;
-
-	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
-
-	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
+	/* Loading forbidden, or no firmware to load? */
+	if (!i915.enable_guc_loading) {
+		err = 0;
+		goto fail;
+	} else if (fw_path == NULL || *fw_path == '\0') {
+		if (*fw_path == '\0')
+			DRM_INFO("No GuC firmware known for this platform\n");
+		err = -ENODEV;
+		goto fail;
+	}
 
-	switch (guc_fw->guc_fw_fetch_status) {
-	case GUC_FIRMWARE_FAIL:
-		/* something went wrong :( */
+	/* Fetch failed, or already fetched but failed to load? */
+	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
 		err = -EIO;
 		goto fail;
-
-	case GUC_FIRMWARE_NONE:
-	case GUC_FIRMWARE_PENDING:
-	default:
-		/* "can't happen" */
-		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
-			guc_fw->guc_fw_path,
-			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-			guc_fw->guc_fw_fetch_status);
-		err = -ENXIO;
+	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
+		err = -ENOEXEC;
 		goto fail;
-
-	case GUC_FIRMWARE_SUCCESS:
-		break;
 	}
 
+	direct_interrupts_to_host(dev_priv);
+
+	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
+
+	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
+		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
+
 	err = i915_guc_submission_init(dev);
 	if (err)
 		goto fail;
@@ -463,7 +455,7 @@ int intel_guc_setup(struct drm_device *dev)
 		 */
 		err = i915_reset_guc(dev_priv);
 		if (err) {
-			DRM_ERROR("GuC reset failed, err %d\n", err);
+			DRM_ERROR("GuC reset failed: %d\n", err);
 			goto fail;
 		}
 
@@ -474,8 +466,8 @@ int intel_guc_setup(struct drm_device *dev)
 		if (--retries == 0)
 			goto fail;
 
-		DRM_INFO("GuC fw load failed, err %d; will reset and "
-			"retry %d more time(s)\n", err, retries);
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", err, retries);
 	}
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
@@ -497,7 +489,6 @@ int intel_guc_setup(struct drm_device *dev)
 	return 0;
 
 fail:
-	DRM_ERROR("GuC firmware load failed, err %d\n", err);
 	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
 		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
 
@@ -505,7 +496,41 @@ fail:
 	i915_guc_submission_disable(dev);
 	i915_guc_submission_fini(dev);
 
-	return err;
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	if (i915.enable_guc_loading > 1) {
+		ret = -EIO;
+	} else if (i915.enable_guc_submission > 1) {
+		ret = -EIO;
+	} else {
+		ret = 0;
+	}
+
+	if (err == 0)
+		DRM_INFO("GuC firmware load skipped\n");
+	else if (ret == -EIO)
+		DRM_ERROR("GuC firmware load failed: %d\n", err);
+	else
+		DRM_INFO("GuC firmware load failed: %d\n", err);
+
+	if (i915.enable_guc_submission) {
+		if (fw_path == NULL)
+			DRM_INFO("GuC submission without firmware not supported\n");
+		if (ret == 0)
+			DRM_INFO("Falling back to execlist mode\n");
+		else
+			DRM_ERROR("GuC init failed: %d\n", ret);
+	}
+	i915.enable_guc_submission = 0;
+
+	return ret;
 }
 
 static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
@@ -644,8 +669,11 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	if (!HAS_GUC_SCHED(dev))
-		i915.enable_guc_submission = false;
+	/* A negative value means "use platform default" */
+	if (i915.enable_guc_loading < 0)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	if (i915.enable_guc_submission < 0)
+		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
@@ -658,26 +686,21 @@ void intel_guc_init(struct drm_device *dev)
 		guc_fw->guc_fw_major_wanted = 8;
 		guc_fw->guc_fw_minor_wanted = 7;
 	} else {
-		i915.enable_guc_submission = false;
 		fw_path = "";	/* unknown device */
 	}
 
-	if (!i915.enable_guc_submission)
-		return;
-
 	guc_fw->guc_dev = dev;
 	guc_fw->guc_fw_path = fw_path;
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
+	/* Early (and silent) return if GuC loading is disabled */
+	if (!i915.enable_guc_loading)
+		return;
 	if (fw_path == NULL)
 		return;
-
-	if (*fw_path == '\0') {
-		DRM_ERROR("No GuC firmware known for this platform\n");
-		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
+	if (*fw_path == '\0')
 		return;
-	}
 
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
-- 
1.9.1

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

Re: [PATCH v6 3/7] drm/i915/guc: add enable_guc_loading parameter

[Fiedorowicz, Lukasz]

Tested-by: Fiedorowicz, Lukasz <lukasz.fiedorowicz@intel.com>

Run similar tests as for previous version of this patch. Results are the same and logs are cleaner. I'm happy with how it works.

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Tvrtko Ursulin
Sent: Friday, May 20, 2016 12:43 PM
To: Intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH v6 3/7] drm/i915/guc: add enable_guc_loading parameter

From: Dave Gordon <david.s.gordon@intel.com>

Split the function of "enable_guc_submission" into two separate options.  The new one ("enable_guc_loading") controls only the *fetching and loading* of the GuC firmware image. The existing one is redefined to control only the *use* of the GuC for batch submission once the firmware is loaded.

In addition, the degree of control has been refined from a simple bool to an integer key, allowing several options:
-1 (default)     whatever the platform default is
 0  DISABLE      don't load/use the GuC
 1  BEST EFFORT  try to load/use the GuC, fallback if not available
 2  REQUIRE      must load/use the GuC, else leave the GPU wedged

The new platform default (as coded here) will be to attempt to load the GuC iff the device has a GuC that requires firmware, but not yet to use it for submission. A later patch will change to enable it if appropriate.

v4:
    Changed some error-message levels, mostly ERROR->INFO, per
    review comments by Tvrtko Ursulin.

v5:
    Dropped one more error message, disabled GuC submission on
    hypothetical firmware-free devices [Tvrtko Ursulin].

v6:
    Logging tidy by Tvrtko Ursulin:
     * Do not log falling back to execlists when wedging the GPU.
     * Do not log fw load errors when load was disabled by user.
     * Pass down some error code from fw load for log message to
       make more sense.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> (v5)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   5 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
 drivers/gpu/drm/i915/i915_params.c         |  14 +++-
 drivers/gpu/drm/i915/i915_params.h         |   3 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 123 +++++++++++++++++------------
 5 files changed, 89 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 88dce5482f2f..1a3a07eca0d0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4868,11 +4868,8 @@ i915_gem_init_hw(struct drm_device *dev)
 	/* We can't enable contexts until all firmware is loaded */
 	if (HAS_GUC(dev)) {
 		ret = intel_guc_setup(dev);
-		if (ret) {
-			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
-			ret = -EIO;
+		if (ret)
 			goto out;
-		}
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 169242a8adff..916cd6778cf3 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
@@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
 	struct intel_context *ctx;
 	u32 data[3];
 
-	if (!i915.enable_guc_submission)
+	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
 	ctx = dev_priv->kernel_context;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index cd74fb8e9387..21a323c01cdb 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -53,7 +53,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_submission = false,
+	.enable_guc_loading = -1,
+	.enable_guc_submission = 0,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
@@ -193,8 +194,15 @@ MODULE_PARM_DESC(edp_vswing,
 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
 		 "2=default swing(400mV))");
 
-module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400); -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
+module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, 
+int, 0400); MODULE_PARM_DESC(enable_guc_loading,
+		"Enable GuC firmware loading "
+		"(-1=auto [default], 0=never, 1=if available, 2=required)");
+
+module_param_named_unsafe(enable_guc_submission, 
+i915.enable_guc_submission, int, 0400); MODULE_PARM_DESC(enable_guc_submission,
+		"Enable GuC submission "
+		"(-1=auto, 0=never [default], 1=if available, 2=required)");
 
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);  MODULE_PARM_DESC(guc_log_level, diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index dd0d0bbcc05b..658ce7379671 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,6 +45,8 @@ struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
+	int enable_guc_loading;
+	int enable_guc_submission;
 	int guc_log_level;
 	int mmio_debug;
 	int edp_vswing;
@@ -56,7 +58,6 @@ struct i915_params {
 	bool load_detect_test;
 	bool reset;
 	bool disable_display;
-	bool enable_guc_submission;
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
 	bool enable_dp_mst;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 30ec8208dbbc..29273e5fee22 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -402,50 +402,42 @@ int intel_guc_setup(struct drm_device *dev)  {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
-	int retries, err = 0;
+	const char *fw_path = guc_fw->guc_fw_path;
+	int retries, ret, err;
 
-	if (!i915.enable_guc_submission)
-		return 0;
-
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
+		fw_path,
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	direct_interrupts_to_host(dev_priv);
-
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
-		return 0;
-
-	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
-	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
-		return -ENOEXEC;
-
-	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
-
-	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
+	/* Loading forbidden, or no firmware to load? */
+	if (!i915.enable_guc_loading) {
+		err = 0;
+		goto fail;
+	} else if (fw_path == NULL || *fw_path == '\0') {
+		if (*fw_path == '\0')
+			DRM_INFO("No GuC firmware known for this platform\n");
+		err = -ENODEV;
+		goto fail;
+	}
 
-	switch (guc_fw->guc_fw_fetch_status) {
-	case GUC_FIRMWARE_FAIL:
-		/* something went wrong :( */
+	/* Fetch failed, or already fetched but failed to load? */
+	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
 		err = -EIO;
 		goto fail;
-
-	case GUC_FIRMWARE_NONE:
-	case GUC_FIRMWARE_PENDING:
-	default:
-		/* "can't happen" */
-		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
-			guc_fw->guc_fw_path,
-			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-			guc_fw->guc_fw_fetch_status);
-		err = -ENXIO;
+	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
+		err = -ENOEXEC;
 		goto fail;
-
-	case GUC_FIRMWARE_SUCCESS:
-		break;
 	}
 
+	direct_interrupts_to_host(dev_priv);
+
+	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
+
+	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
+		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
+
 	err = i915_guc_submission_init(dev);
 	if (err)
 		goto fail;
@@ -463,7 +455,7 @@ int intel_guc_setup(struct drm_device *dev)
 		 */
 		err = i915_reset_guc(dev_priv);
 		if (err) {
-			DRM_ERROR("GuC reset failed, err %d\n", err);
+			DRM_ERROR("GuC reset failed: %d\n", err);
 			goto fail;
 		}
 
@@ -474,8 +466,8 @@ int intel_guc_setup(struct drm_device *dev)
 		if (--retries == 0)
 			goto fail;
 
-		DRM_INFO("GuC fw load failed, err %d; will reset and "
-			"retry %d more time(s)\n", err, retries);
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", err, retries);
 	}
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS; @@ -497,7 +489,6 @@ int intel_guc_setup(struct drm_device *dev)
 	return 0;
 
 fail:
-	DRM_ERROR("GuC firmware load failed, err %d\n", err);
 	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
 		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
 
@@ -505,7 +496,41 @@ fail:
 	i915_guc_submission_disable(dev);
 	i915_guc_submission_fini(dev);
 
-	return err;
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	if (i915.enable_guc_loading > 1) {
+		ret = -EIO;
+	} else if (i915.enable_guc_submission > 1) {
+		ret = -EIO;
+	} else {
+		ret = 0;
+	}
+
+	if (err == 0)
+		DRM_INFO("GuC firmware load skipped\n");
+	else if (ret == -EIO)
+		DRM_ERROR("GuC firmware load failed: %d\n", err);
+	else
+		DRM_INFO("GuC firmware load failed: %d\n", err);
+
+	if (i915.enable_guc_submission) {
+		if (fw_path == NULL)
+			DRM_INFO("GuC submission without firmware not supported\n");
+		if (ret == 0)
+			DRM_INFO("Falling back to execlist mode\n");
+		else
+			DRM_ERROR("GuC init failed: %d\n", ret);
+	}
+	i915.enable_guc_submission = 0;
+
+	return ret;
 }
 
 static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) @@ -644,8 +669,11 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	if (!HAS_GUC_SCHED(dev))
-		i915.enable_guc_submission = false;
+	/* A negative value means "use platform default" */
+	if (i915.enable_guc_loading < 0)
+		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+	if (i915.enable_guc_submission < 0)
+		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
@@ -658,26 +686,21 @@ void intel_guc_init(struct drm_device *dev)
 		guc_fw->guc_fw_major_wanted = 8;
 		guc_fw->guc_fw_minor_wanted = 7;
 	} else {
-		i915.enable_guc_submission = false;
 		fw_path = "";	/* unknown device */
 	}
 
-	if (!i915.enable_guc_submission)
-		return;
-
 	guc_fw->guc_dev = dev;
 	guc_fw->guc_fw_path = fw_path;
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
+	/* Early (and silent) return if GuC loading is disabled */
+	if (!i915.enable_guc_loading)
+		return;
 	if (fw_path == NULL)
 		return;
-
-	if (*fw_path == '\0') {
-		DRM_ERROR("No GuC firmware known for this platform\n");
-		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
+	if (*fw_path == '\0')
 		return;
-	}
 
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
--
1.9.1

_______________________________________________
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 v6 3/7] drm/i915/guc: add enable_guc_loading parameter

[Nick Hoath]

On 20/05/2016 11:42, Tvrtko Ursulin wrote:
> From: Dave Gordon <david.s.gordon@intel.com>
>
> Split the function of "enable_guc_submission" into two separate
> options.  The new one ("enable_guc_loading") controls only the
> *fetching and loading* of the GuC firmware image. The existing
> one is redefined to control only the *use* of the GuC for batch
> submission once the firmware is loaded.
>
> In addition, the degree of control has been refined from a simple
> bool to an integer key, allowing several options:
> -1 (default)     whatever the platform default is
>   0  DISABLE      don't load/use the GuC
>   1  BEST EFFORT  try to load/use the GuC, fallback if not available
>   2  REQUIRE      must load/use the GuC, else leave the GPU wedged
>
> The new platform default (as coded here) will be to attempt to
> load the GuC iff the device has a GuC that requires firmware,
> but not yet to use it for submission. A later patch will change
> to enable it if appropriate.
>
> v4:
>      Changed some error-message levels, mostly ERROR->INFO, per
>      review comments by Tvrtko Ursulin.
>
> v5:
>      Dropped one more error message, disabled GuC submission on
>      hypothetical firmware-free devices [Tvrtko Ursulin].
>
> v6:
>      Logging tidy by Tvrtko Ursulin:
>       * Do not log falling back to execlists when wedging the GPU.
>       * Do not log fw load errors when load was disabled by user.
>       * Pass down some error code from fw load for log message to
>         make more sense.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> (v5)
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Nick Hoath <nicholas.hoath@intel.com> (v6)
> ---
>   drivers/gpu/drm/i915/i915_gem.c            |   5 +-
>   drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
>   drivers/gpu/drm/i915/i915_params.c         |  14 +++-
>   drivers/gpu/drm/i915/i915_params.h         |   3 +-
>   drivers/gpu/drm/i915/intel_guc_loader.c    | 123 +++++++++++++++++------------
>   5 files changed, 89 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 88dce5482f2f..1a3a07eca0d0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4868,11 +4868,8 @@ i915_gem_init_hw(struct drm_device *dev)
>   	/* We can't enable contexts until all firmware is loaded */
>   	if (HAS_GUC(dev)) {
>   		ret = intel_guc_setup(dev);
> -		if (ret) {
> -			DRM_ERROR("Failed to initialize GuC, error %d\n", ret);
> -			ret = -EIO;
> +		if (ret)
>   			goto out;
> -		}
>   	}
>
>   	/*
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 169242a8adff..916cd6778cf3 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
>   	ctx = dev_priv->kernel_context;
> @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev)
>   	struct intel_context *ctx;
>   	u32 data[3];
>
> -	if (!i915.enable_guc_submission)
> +	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
>   	ctx = dev_priv->kernel_context;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index cd74fb8e9387..21a323c01cdb 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -53,7 +53,8 @@ struct i915_params i915 __read_mostly = {
>   	.verbose_state_checks = 1,
>   	.nuclear_pageflip = 0,
>   	.edp_vswing = 0,
> -	.enable_guc_submission = false,
> +	.enable_guc_loading = -1,
> +	.enable_guc_submission = 0,
>   	.guc_log_level = -1,
>   	.enable_dp_mst = true,
>   	.inject_load_failure = 0,
> @@ -193,8 +194,15 @@ MODULE_PARM_DESC(edp_vswing,
>   		 "(0=use value from vbt [default], 1=low power swing(200mV),"
>   		 "2=default swing(400mV))");
>
> -module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400);
> -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)");
> +module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
> +MODULE_PARM_DESC(enable_guc_loading,
> +		"Enable GuC firmware loading "
> +		"(-1=auto [default], 0=never, 1=if available, 2=required)");
> +
> +module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
> +MODULE_PARM_DESC(enable_guc_submission,
> +		"Enable GuC submission "
> +		"(-1=auto, 0=never [default], 1=if available, 2=required)");
>
>   module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>   MODULE_PARM_DESC(guc_log_level,
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index dd0d0bbcc05b..658ce7379671 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -45,6 +45,8 @@ struct i915_params {
>   	int enable_ips;
>   	int invert_brightness;
>   	int enable_cmd_parser;
> +	int enable_guc_loading;
> +	int enable_guc_submission;
>   	int guc_log_level;
>   	int mmio_debug;
>   	int edp_vswing;
> @@ -56,7 +58,6 @@ struct i915_params {
>   	bool load_detect_test;
>   	bool reset;
>   	bool disable_display;
> -	bool enable_guc_submission;
>   	bool verbose_state_checks;
>   	bool nuclear_pageflip;
>   	bool enable_dp_mst;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 30ec8208dbbc..29273e5fee22 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -402,50 +402,42 @@ int intel_guc_setup(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	int retries, err = 0;
> +	const char *fw_path = guc_fw->guc_fw_path;
> +	int retries, ret, err;
>
> -	if (!i915.enable_guc_submission)
> -		return 0;
> -
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> +		fw_path,
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	direct_interrupts_to_host(dev_priv);
> -
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE)
> -		return 0;
> -
> -	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS &&
> -	    guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL)
> -		return -ENOEXEC;
> -
> -	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> -
> -	DRM_DEBUG_DRIVER("GuC fw fetch status %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status));
> +	/* Loading forbidden, or no firmware to load? */
> +	if (!i915.enable_guc_loading) {
> +		err = 0;
> +		goto fail;
> +	} else if (fw_path == NULL || *fw_path == '\0') {
> +		if (*fw_path == '\0')
> +			DRM_INFO("No GuC firmware known for this platform\n");
> +		err = -ENODEV;
> +		goto fail;
> +	}
>
> -	switch (guc_fw->guc_fw_fetch_status) {
> -	case GUC_FIRMWARE_FAIL:
> -		/* something went wrong :( */
> +	/* Fetch failed, or already fetched but failed to load? */
> +	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
>   		err = -EIO;
>   		goto fail;
> -
> -	case GUC_FIRMWARE_NONE:
> -	case GUC_FIRMWARE_PENDING:
> -	default:
> -		/* "can't happen" */
> -		WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n",
> -			guc_fw->guc_fw_path,
> -			intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -			guc_fw->guc_fw_fetch_status);
> -		err = -ENXIO;
> +	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> +		err = -ENOEXEC;
>   		goto fail;
> -
> -	case GUC_FIRMWARE_SUCCESS:
> -		break;
>   	}
>
> +	direct_interrupts_to_host(dev_priv);
> +
> +	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> +
> +	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> +		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> +
>   	err = i915_guc_submission_init(dev);
>   	if (err)
>   		goto fail;
> @@ -463,7 +455,7 @@ int intel_guc_setup(struct drm_device *dev)
>   		 */
>   		err = i915_reset_guc(dev_priv);
>   		if (err) {
> -			DRM_ERROR("GuC reset failed, err %d\n", err);
> +			DRM_ERROR("GuC reset failed: %d\n", err);
>   			goto fail;
>   		}
>
> @@ -474,8 +466,8 @@ int intel_guc_setup(struct drm_device *dev)
>   		if (--retries == 0)
>   			goto fail;
>
> -		DRM_INFO("GuC fw load failed, err %d; will reset and "
> -			"retry %d more time(s)\n", err, retries);
> +		DRM_INFO("GuC fw load failed: %d; will reset and "
> +			 "retry %d more time(s)\n", err, retries);
>   	}
>
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> @@ -497,7 +489,6 @@ int intel_guc_setup(struct drm_device *dev)
>   	return 0;
>
>   fail:
> -	DRM_ERROR("GuC firmware load failed, err %d\n", err);
>   	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
>   		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>
> @@ -505,7 +496,41 @@ fail:
>   	i915_guc_submission_disable(dev);
>   	i915_guc_submission_fini(dev);
>
> -	return err;
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1) {
> +		ret = -EIO;
> +	} else if (i915.enable_guc_submission > 1) {
> +		ret = -EIO;
> +	} else {
> +		ret = 0;
> +	}
> +
> +	if (err == 0)
> +		DRM_INFO("GuC firmware load skipped\n");
> +	else if (ret == -EIO)
> +		DRM_ERROR("GuC firmware load failed: %d\n", err);
> +	else
> +		DRM_INFO("GuC firmware load failed: %d\n", err);
> +
> +	if (i915.enable_guc_submission) {
> +		if (fw_path == NULL)
> +			DRM_INFO("GuC submission without firmware not supported\n");
> +		if (ret == 0)
> +			DRM_INFO("Falling back to execlist mode\n");
> +		else
> +			DRM_ERROR("GuC init failed: %d\n", ret);
> +	}
> +	i915.enable_guc_submission = 0;
> +
> +	return ret;
>   }
>
>   static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
> @@ -644,8 +669,11 @@ void intel_guc_init(struct drm_device *dev)
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>   	const char *fw_path;
>
> -	if (!HAS_GUC_SCHED(dev))
> -		i915.enable_guc_submission = false;
> +	/* A negative value means "use platform default" */
> +	if (i915.enable_guc_loading < 0)
> +		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
> +	if (i915.enable_guc_submission < 0)
> +		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
>
>   	if (!HAS_GUC_UCODE(dev)) {
>   		fw_path = NULL;
> @@ -658,26 +686,21 @@ void intel_guc_init(struct drm_device *dev)
>   		guc_fw->guc_fw_major_wanted = 8;
>   		guc_fw->guc_fw_minor_wanted = 7;
>   	} else {
> -		i915.enable_guc_submission = false;
>   		fw_path = "";	/* unknown device */
>   	}
>
> -	if (!i915.enable_guc_submission)
> -		return;
> -
>   	guc_fw->guc_dev = dev;
>   	guc_fw->guc_fw_path = fw_path;
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> +	/* Early (and silent) return if GuC loading is disabled */
> +	if (!i915.enable_guc_loading)
> +		return;
>   	if (fw_path == NULL)
>   		return;
> -
> -	if (*fw_path == '\0') {
> -		DRM_ERROR("No GuC firmware known for this platform\n");
> -		guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL;
> +	if (*fw_path == '\0')
>   		return;
> -	}
>
>   	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
>   	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
>

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

[PATCH v4 4/7] drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()

[Dave Gordon]

The knowledge of how to derive the relevant client from the request
should be localised within i915_guc_submission.c; the LRC code shouldn't
have to know about the internal details of the GuC submission process.
And all the information the GuC code needs should be encapsulated in (or
reachable from) the request.

v2:
    GEM_BUG_ON() for bad GuC client (Tvrtko Ursulin).
    Add/update kerneldoc explaining check_space/submit protocol

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 46 ++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_guc.h           |  5 ++--
 drivers/gpu/drm/i915/intel_lrc.c           |  9 ++----
 3 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 916cd67..87cb739 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -450,14 +450,30 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
 			     sizeof(desc) * client->ctx_index);
 }
 
-int i915_guc_wq_check_space(struct i915_guc_client *gc)
+/**
+ * i915_guc_wq_check_space() - check that the GuC can accept a request
+ * @request:	request associated with the commands
+ *
+ * Return:	0 if space is available
+ *		-EAGAIN if space is not currently available
+ *
+ * This function must be called (and must return 0) before a request
+ * is submitted to the GuC via i915_guc_submit() below. Once a result
+ * of 0 has been returned, it remains valid until (but only until)
+ * the next call to submit().
+ *
+ * This precheck allows the caller to determine in advance that space
+ * will be available for the next submission before committing resources
+ * to it, and helps avoid late failures with complicated recovery paths.
+ */
+int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
 {
+	const size_t size = sizeof(struct guc_wq_item);
+	struct i915_guc_client *gc = request->i915->guc.execbuf_client;
 	struct guc_process_desc *desc;
-	u32 size = sizeof(struct guc_wq_item);
 	int ret = -ETIMEDOUT, timeout_counter = 200;
 
-	if (!gc)
-		return 0;
+	GEM_BUG_ON(gc == NULL);
 
 	desc = gc->client_base + gc->proc_desc_offset;
 
@@ -531,16 +547,28 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 
 /**
  * i915_guc_submit() - Submit commands through GuC
- * @client:	the guc client where commands will go through
  * @rq:		request associated with the commands
  *
- * Return:	0 if succeed
+ * Return:	0 on success, otherwise an errno.
+ * 		(Note: nonzero really shouldn't happen!)
+ *
+ * The caller must have already called i915_guc_wq_check_space() above
+ * with a result of 0 (success) since the last request submission. This
+ * guarantees that there is space in the work queue for the new request,
+ * so enqueuing the item cannot fail.
+ *
+ * Bad Things Will Happen if the caller violates this protocol e.g. calls
+ * submit() when check() says there's no space, or calls submit() multiple
+ * times with no intervening check().
+ *
+ * The only error here arises if the doorbell hardware isn't functioning
+ * as expected, which really shouln't happen.
  */
-int i915_guc_submit(struct i915_guc_client *client,
-		    struct drm_i915_gem_request *rq)
+int i915_guc_submit(struct drm_i915_gem_request *rq)
 {
-	struct intel_guc *guc = client->guc;
 	unsigned int engine_id = rq->engine->guc_id;
+	struct intel_guc *guc = &rq->i915->guc;
+	struct i915_guc_client *client = guc->execbuf_client;
 	int q_ret, b_ret;
 
 	q_ret = guc_add_workqueue_item(client, rq);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 3984136..91f315c 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device *dev);
 /* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_device *dev);
 int i915_guc_submission_enable(struct drm_device *dev);
-int i915_guc_submit(struct i915_guc_client *client,
-		    struct drm_i915_gem_request *rq);
+int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
+int i915_guc_submit(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_device *dev);
 void i915_guc_submission_fini(struct drm_device *dev);
-int i915_guc_wq_check_space(struct i915_guc_client *client);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index db10c96..c0bd3db 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -698,9 +698,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 		 * going any further, as the i915_add_request() call
 		 * later on mustn't fail ...
 		 */
-		struct intel_guc *guc = &request->i915->guc;
-
-		ret = i915_guc_wq_check_space(guc->execbuf_client);
+		ret = i915_guc_wq_check_space(request);
 		if (ret)
 			return ret;
 	}
@@ -749,7 +747,6 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 {
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
-	struct drm_i915_private *dev_priv = request->i915;
 	struct intel_engine_cs *engine = request->engine;
 
 	intel_logical_ring_advance(ringbuf);
@@ -777,8 +774,8 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 	request->previous_context = engine->last_context;
 	engine->last_context = request->ctx;
 
-	if (dev_priv->guc.execbuf_client)
-		i915_guc_submit(dev_priv->guc.execbuf_client, request);
+	if (i915.enable_guc_submission)
+		i915_guc_submit(request);
 	else
 		execlists_context_queue(request);
 
-- 
1.9.1

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

[PATCH v4 5/7] drm/i915/guc: don't spinwait if the GuC's workqueue is full

[Dave Gordon]

Rather than wait to see whether more space becomes available in the GuC
submission workqueue, we can just return -EAGAIN and let the caller try
again in a little while. This gets rid of an uninterruptable sleep in
the polling code :)

We'll also add a counter to the GuC client statistics, to see how often
we find the WQ full.

v2:
    Flag the likely() code path (Tvtrko Ursulin).

v4:
    Add/update comments about failure counters (Tvtrko Ursulin).

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  1 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 18 +++++++-----------
 drivers/gpu/drm/i915/intel_guc.h           | 22 ++++++++++++++++------
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 24f4105..de05841 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2506,6 +2506,7 @@ static void i915_guc_client_info(struct seq_file *m,
 	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
 		client->wq_size, client->wq_offset, client->wq_tail);
 
+	seq_printf(m, "\tWork queue full: %u\n", client->no_wq_space);
 	seq_printf(m, "\tFailed to queue: %u\n", client->q_fail);
 	seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
 	seq_printf(m, "\tLast submission result: %d\n", client->retcode);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 87cb739..85b2b89 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -468,26 +468,22 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
  */
 int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
 {
-	const size_t size = sizeof(struct guc_wq_item);
+	const size_t wqi_size = sizeof(struct guc_wq_item);
 	struct i915_guc_client *gc = request->i915->guc.execbuf_client;
 	struct guc_process_desc *desc;
-	int ret = -ETIMEDOUT, timeout_counter = 200;
+	u32 freespace;
 
 	GEM_BUG_ON(gc == NULL);
 
 	desc = gc->client_base + gc->proc_desc_offset;
 
-	while (timeout_counter-- > 0) {
-		if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
-			ret = 0;
-			break;
-		}
+	freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
+	if (likely(freespace >= wqi_size))
+		return 0;
 
-		if (timeout_counter)
-			usleep_range(1000, 2000);
-	};
+	gc->no_wq_space += 1;
 
-	return ret;
+	return -EAGAIN;
 }
 
 static int guc_add_workqueue_item(struct i915_guc_client *gc,
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 91f315c..380a743 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -48,9 +48,18 @@ struct drm_i915_gem_request;
  * queue (a circular array of work items), again described in the process
  * descriptor. Work queue pages are mapped momentarily as required.
  *
- * Finally, we also keep a few statistics here, including the number of
- * submissions to each engine, and a record of the last submission failure
- * (if any).
+ * We also keep a few statistics on failures. Ideally, these should all
+ * be zero!
+ *   no_wq_space: times that the submission pre-check found no space was
+ *                available in the work queue (note, the queue is shared,
+ *                not per-engine). It is OK for this to be nonzero, but
+ *                it should not be huge!
+ *   q_fail: failed to enqueue a work item. This should never happen,
+ *           because we check for space beforehand.
+ *   b_fail: failed to ring the doorbell. This should never happen, unless
+ *           somehow the hardware misbehaves, or maybe if the GuC firmware
+ *           crashes? We probably need to reset the GPU to recover.
+ *   retcode: errno from last guc_submit()
  */
 struct i915_guc_client {
 	struct drm_i915_gem_object *client_obj;
@@ -71,12 +80,13 @@ struct i915_guc_client {
 	uint32_t wq_tail;
 	uint32_t unused;		/* Was 'wq_head'		*/
 
-	/* GuC submission statistics & status */
-	uint64_t submissions[GUC_MAX_ENGINES_NUM];
+	uint32_t no_wq_space;
 	uint32_t q_fail;
 	uint32_t b_fail;
 	int retcode;
-	int spare;			/* pad to 32 DWords		*/
+
+	/* Per-engine counts of GuC submissions */
+	uint64_t submissions[GUC_MAX_ENGINES_NUM];
 };
 
 enum intel_guc_fw_status {
-- 
1.9.1

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

Re: [PATCH v4 5/7] drm/i915/guc: don't spinwait if the GuC's workqueue is full

[Tvrtko Ursulin]

On 13/05/16 15:36, Dave Gordon wrote:
> Rather than wait to see whether more space becomes available in the GuC
> submission workqueue, we can just return -EAGAIN and let the caller try
> again in a little while. This gets rid of an uninterruptable sleep in
> the polling code :)
>
> We'll also add a counter to the GuC client statistics, to see how often
> we find the WQ full.
>
> v2:
>      Flag the likely() code path (Tvtrko Ursulin).
>
> v4:
>      Add/update comments about failure counters (Tvtrko Ursulin).
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        |  1 +
>   drivers/gpu/drm/i915/i915_guc_submission.c | 18 +++++++-----------
>   drivers/gpu/drm/i915/intel_guc.h           | 22 ++++++++++++++++------
>   3 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24f4105..de05841 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2506,6 +2506,7 @@ static void i915_guc_client_info(struct seq_file *m,
>   	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
>   		client->wq_size, client->wq_offset, client->wq_tail);
>
> +	seq_printf(m, "\tWork queue full: %u\n", client->no_wq_space);
>   	seq_printf(m, "\tFailed to queue: %u\n", client->q_fail);
>   	seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
>   	seq_printf(m, "\tLast submission result: %d\n", client->retcode);
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 87cb739..85b2b89 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -468,26 +468,22 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
>    */
>   int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
>   {
> -	const size_t size = sizeof(struct guc_wq_item);
> +	const size_t wqi_size = sizeof(struct guc_wq_item);
>   	struct i915_guc_client *gc = request->i915->guc.execbuf_client;
>   	struct guc_process_desc *desc;
> -	int ret = -ETIMEDOUT, timeout_counter = 200;
> +	u32 freespace;
>
>   	GEM_BUG_ON(gc == NULL);
>
>   	desc = gc->client_base + gc->proc_desc_offset;
>
> -	while (timeout_counter-- > 0) {
> -		if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
> -			ret = 0;
> -			break;
> -		}
> +	freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
> +	if (likely(freespace >= wqi_size))
> +		return 0;
>
> -		if (timeout_counter)
> -			usleep_range(1000, 2000);
> -	};
> +	gc->no_wq_space += 1;
>
> -	return ret;
> +	return -EAGAIN;
>   }
>
>   static int guc_add_workqueue_item(struct i915_guc_client *gc,
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 91f315c..380a743 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -48,9 +48,18 @@ struct drm_i915_gem_request;
>    * queue (a circular array of work items), again described in the process
>    * descriptor. Work queue pages are mapped momentarily as required.
>    *
> - * Finally, we also keep a few statistics here, including the number of
> - * submissions to each engine, and a record of the last submission failure
> - * (if any).
> + * We also keep a few statistics on failures. Ideally, these should all
> + * be zero!
> + *   no_wq_space: times that the submission pre-check found no space was
> + *                available in the work queue (note, the queue is shared,
> + *                not per-engine). It is OK for this to be nonzero, but
> + *                it should not be huge!
> + *   q_fail: failed to enqueue a work item. This should never happen,
> + *           because we check for space beforehand.
> + *   b_fail: failed to ring the doorbell. This should never happen, unless
> + *           somehow the hardware misbehaves, or maybe if the GuC firmware
> + *           crashes? We probably need to reset the GPU to recover.
> + *   retcode: errno from last guc_submit()
>    */
>   struct i915_guc_client {
>   	struct drm_i915_gem_object *client_obj;
> @@ -71,12 +80,13 @@ struct i915_guc_client {
>   	uint32_t wq_tail;
>   	uint32_t unused;		/* Was 'wq_head'		*/
>
> -	/* GuC submission statistics & status */
> -	uint64_t submissions[GUC_MAX_ENGINES_NUM];
> +	uint32_t no_wq_space;
>   	uint32_t q_fail;
>   	uint32_t b_fail;
>   	int retcode;
> -	int spare;			/* pad to 32 DWords		*/
> +
> +	/* Per-engine counts of GuC submissions */
> +	uint64_t submissions[GUC_MAX_ENGINES_NUM];
>   };
>
>   enum intel_guc_fw_status {
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

[PATCH v4 6/7] drm/i915/guc: rework guc_add_workqueue_item()

[Dave Gordon]

Mostly little optimisations and future-proofing against code breakage.
For instance, if the driver is correctly following the submission
protocol, the "out of space" condition is impossible, so the previous
runtime WARN_ON() is promoted to a GEM_BUG_ON() for a more dramatic
effect in development and less impact in end-user systems.

Similarly we can make alignment checking more stringent and replace
other WARN_ON() conditions that don't relate to the runtime hardware
state with either BUILD_BUG_ON() for compile-time-detectable issues, or
GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

v2:
    Note that we're now putting the request seqno in the "fence_id"
    field of each GuC-work-item, in case it turns up somewhere useful
    (e.g. in a GuC log) [Tvrtko Ursulin].

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 71 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_guc.h           |  2 +-
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  3 +-
 3 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 85b2b89..42a8508 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -486,23 +486,28 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
 	return -EAGAIN;
 }
 
-static int guc_add_workqueue_item(struct i915_guc_client *gc,
-				  struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+				   struct drm_i915_gem_request *rq)
 {
+	/* wqi_len is in DWords, and does not include the one-word header */
+	const size_t wqi_size = sizeof(struct guc_wq_item);
+	const u32 wqi_len = wqi_size/sizeof(u32) - 1;
 	struct guc_process_desc *desc;
 	struct guc_wq_item *wqi;
 	void *base;
-	u32 tail, wq_len, wq_off, space;
+	u32 freespace, tail, wq_off, wq_page;
 
 	desc = gc->client_base + gc->proc_desc_offset;
-	space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-	if (WARN_ON(space < sizeof(struct guc_wq_item)))
-		return -ENOSPC; /* shouldn't happen */
 
-	/* postincrement WQ tail for next time */
-	wq_off = gc->wq_tail;
-	gc->wq_tail += sizeof(struct guc_wq_item);
-	gc->wq_tail &= gc->wq_size - 1;
+	/* Free space is guaranteed, see i915_guc_wq_check_space() above */
+	freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
+	GEM_BUG_ON(freespace < wqi_size);
+
+	/* The GuC firmware wants the tail index in QWords, not bytes */
+	tail = rq->tail;
+	GEM_BUG_ON(tail & 7);
+	tail >>= 3;
+	GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);
 
 	/* For now workqueue item is 4 DWs; workqueue buffer is 2 pages. So we
 	 * should not have the case where structure wqi is across page, neither
@@ -511,19 +516,23 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	 * XXX: if not the case, we need save data to a temp wqi and copy it to
 	 * workqueue buffer dw by dw.
 	 */
-	WARN_ON(sizeof(struct guc_wq_item) != 16);
-	WARN_ON(wq_off & 3);
+	BUILD_BUG_ON(wqi_size != 16);
+
+	/* postincrement WQ tail for next time */
+	wq_off = gc->wq_tail;
+	gc->wq_tail += wqi_size;
+	gc->wq_tail &= gc->wq_size - 1;
+	GEM_BUG_ON(wq_off & (wqi_size - 1));
 
-	/* wq starts from the page after doorbell / process_desc */
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-			(wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
+	/* WQ starts from the page after doorbell / process_desc */
+	wq_page = (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT;
 	wq_off &= PAGE_SIZE - 1;
+	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, wq_page));
 	wqi = (struct guc_wq_item *)((char *)base + wq_off);
 
-	/* len does not include the header */
-	wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
+	/* Now fill in the 4-word work queue item */
 	wqi->header = WQ_TYPE_INORDER |
-			(wq_len << WQ_LEN_SHIFT) |
+			(wqi_len << WQ_LEN_SHIFT) |
 			(rq->engine->guc_id << WQ_TARGET_SHIFT) |
 			WQ_NO_WCFLUSH_WAIT;
 
@@ -531,14 +540,10 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
 	wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx,
 							     rq->engine);
 
-	/* The GuC firmware wants the tail index in QWords, not bytes */
-	tail = rq->ringbuf->tail >> 3;
 	wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
-	wqi->fence_id = 0; /*XXX: what fence to be here */
+	wqi->fence_id = rq->seqno;
 
 	kunmap_atomic(base);
-
-	return 0;
 }
 
 /**
@@ -565,26 +570,20 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
 	unsigned int engine_id = rq->engine->guc_id;
 	struct intel_guc *guc = &rq->i915->guc;
 	struct i915_guc_client *client = guc->execbuf_client;
-	int q_ret, b_ret;
+	int b_ret;
 
-	q_ret = guc_add_workqueue_item(client, rq);
-	if (q_ret == 0)
-		b_ret = guc_ring_doorbell(client);
+	guc_add_workqueue_item(client, rq);
+	b_ret = guc_ring_doorbell(client);
 
 	client->submissions[engine_id] += 1;
-	if (q_ret) {
-		client->q_fail += 1;
-		client->retcode = q_ret;
-	} else if (b_ret) {
+	client->retcode = b_ret;
+	if (b_ret)
 		client->b_fail += 1;
-		client->retcode = q_ret = b_ret;
-	} else {
-		client->retcode = 0;
-	}
+
 	guc->submissions[engine_id] += 1;
 	guc->last_seqno[engine_id] = rq->seqno;
 
-	return q_ret;
+	return b_ret;
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 380a743..7b1a5a3 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -81,7 +81,7 @@ struct i915_guc_client {
 	uint32_t unused;		/* Was 'wq_head'		*/
 
 	uint32_t no_wq_space;
-	uint32_t q_fail;
+	uint32_t q_fail;		/* No longer used		*/
 	uint32_t b_fail;
 	int retcode;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 2de57ff..944786d 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -71,7 +71,8 @@
 #define   WQ_WORKLOAD_TOUCH		(2 << WQ_WORKLOAD_SHIFT)
 
 #define WQ_RING_TAIL_SHIFT		20
-#define WQ_RING_TAIL_MASK		(0x7FF << WQ_RING_TAIL_SHIFT)
+#define WQ_RING_TAIL_MAX		0x7FF	/* 2^11 QWords */
+#define WQ_RING_TAIL_MASK		(WQ_RING_TAIL_MAX << WQ_RING_TAIL_SHIFT)
 
 #define GUC_DOORBELL_ENABLED		1
 #define GUC_DOORBELL_DISABLED		0
-- 
1.9.1

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

[PATCH v4 7/7] drm/i915/guc: change default to using GuC submission if possible

[Dave Gordon]

This patch simply changes the default value of "enable_guc_submission"
from 0 (never) to -1 (auto). This means that GuC submission will be
used if the platform has a GuC, the GuC supports the request submission
protocol, and any required GuC firmwware was successfully loaded. If any
of these conditions are not met, the driver will fall back to using
execlist mode.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 6a5578c..573e787 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -55,7 +55,7 @@ struct i915_params i915 __read_mostly = {
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
 	.enable_guc_loading = -1,
-	.enable_guc_submission = 0,
+	.enable_guc_submission = -1,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
@@ -207,7 +207,7 @@ struct i915_params i915 __read_mostly = {
 module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
 MODULE_PARM_DESC(enable_guc_submission,
 		"Enable GuC submission "
-		"(-1=auto, 0=never [default], 1=if available, 2=required)");
+		"(-1=auto [default], 0=never, 1=if available, 2=required)");
 
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
-- 
1.9.1

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

Re: [PATCH v4 7/7] drm/i915/guc: change default to using GuC submission if possible

[Chris Wilson]

On Fri, May 13, 2016 at 03:36:35PM +0100, Dave Gordon wrote:
> This patch simply changes the default value of "enable_guc_submission"
> from 0 (never) to -1 (auto). This means that GuC submission will be
> used if the platform has a GuC, the GuC supports the request submission
> protocol, and any required GuC firmwware was successfully loaded. If any
> of these conditions are not met, the driver will fall back to using
> execlist mode.

Why? This is the commit that people will bisect to, it should explain
why guc is preferred. At the moment, with the exception of reduced
context-switch overhead, I don't see any compelling advantage for
enabling guc, just lots of disadvantages.

Enabing a major feature like this, the commit should tell us the
advantages and disadvantages; and future roadmap (but really it should
be compelling in and of itself).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for Enable GuC submission

[Patchwork]

== Series Details ==

Series: Enable GuC submission
URL   : https://patchwork.freedesktop.org/series/7153/
State : failure

== Summary ==

Series 7153v1 Enable GuC submission
http://patchwork.freedesktop.org/api/1.0/series/7153/revisions/1/mbox

Test drv_hangman:
        Subgroup error-state-basic:
                fail       -> PASS       (ro-ilk1-i5-650)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (ro-ivb2-i7-3770)

ro-bdw-i5-5250u  total:219  pass:181  dwarn:0   dfail:0   fail:0   skip:38 
ro-bdw-i7-5557U  total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
ro-byt-n2820     total:218  pass:175  dwarn:0   dfail:0   fail:2   skip:41 
ro-hsw-i3-4010u  total:218  pass:193  dwarn:0   dfail:0   fail:0   skip:25 
ro-hsw-i7-4770r  total:219  pass:194  dwarn:0   dfail:0   fail:0   skip:25 
ro-ilk-i7-620lm  total:219  pass:151  dwarn:0   dfail:0   fail:1   skip:67 
ro-ilk1-i5-650   total:214  pass:152  dwarn:0   dfail:0   fail:1   skip:61 
ro-ivb-i7-3770   total:219  pass:183  dwarn:0   dfail:0   fail:0   skip:36 
ro-ivb2-i7-3770  total:219  pass:186  dwarn:0   dfail:0   fail:1   skip:32 
ro-skl-i7-6700hq total:214  pass:189  dwarn:0   dfail:0   fail:0   skip:25 
ro-snb-i7-2620M  total:219  pass:177  dwarn:0   dfail:0   fail:1   skip:41 

Results at /archive/results/CI_IGT_test/RO_Patchwork_894/

accd824 drm-intel-nightly: 2016y-05m-13d-14h-38m-15s UTC integration manifest
9f94194 drm/i915/guc: change default to using GuC submission if possible
1d395e6 drm/i915/guc: rework guc_add_workqueue_item()
0800e1c drm/i915/guc: don't spinwait if the GuC's workqueue is full
befef39 drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()
6fd5f03 drm/i915/guc: add enable_guc_loading parameter
35a330e drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED
d0009d1 drm/i915/guc: rename loader entry points

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

✗ Ro.CI.BAT: failure for Enable GuC submission (rev2)

[Patchwork]

== Series Details ==

Series: Enable GuC submission (rev2)
URL   : https://patchwork.freedesktop.org/series/7153/
State : failure

== Summary ==

Series 7153v2 Enable GuC submission
http://patchwork.freedesktop.org/api/1.0/series/7153/revisions/2/mbox

Test drv_module_reload_basic:
                pass       -> SKIP       (ro-ivb-i7-3770)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (ro-byt-n2820)
                fail       -> PASS       (ro-hsw-i3-4010u)

ro-bdw-i5-5250u  total:219  pass:181  dwarn:0   dfail:0   fail:0   skip:38 
ro-bdw-i7-5557U  total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
ro-bsw-n3050     total:219  pass:174  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:218  pass:173  dwarn:0   dfail:0   fail:4   skip:41 
ro-hsw-i3-4010u  total:218  pass:193  dwarn:0   dfail:0   fail:0   skip:25 
ro-hsw-i7-4770r  total:219  pass:194  dwarn:0   dfail:0   fail:0   skip:25 
ro-ilk-i7-620lm  total:219  pass:151  dwarn:0   dfail:0   fail:1   skip:67 
ro-ilk1-i5-650   total:214  pass:151  dwarn:0   dfail:0   fail:2   skip:61 
ro-ivb-i7-3770   total:219  pass:182  dwarn:0   dfail:0   fail:0   skip:37 
ro-ivb2-i7-3770  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
ro-skl-i7-6700hq total:214  pass:189  dwarn:0   dfail:0   fail:0   skip:25 
ro-snb-i7-2620M  total:219  pass:177  dwarn:0   dfail:0   fail:1   skip:41 
fi-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_912/

d381724 drm-intel-nightly: 2016y-05m-16d-12h-14m-04s UTC integration manifest
9870d88 drm/i915/guc: change default to using GuC submission if possible
e2ed92b drm/i915/guc: rework guc_add_workqueue_item()
0b04a38 drm/i915/guc: don't spinwait if the GuC's workqueue is full
6cbef76 drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()
1a50673 drm/i915/guc: add enable_guc_loading parameter
4461c44 drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED
1996f53 drm/i915/guc: rename loader entry points

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

✗ Ro.CI.BAT: failure for Enable GuC submission (rev3)

[Patchwork]

== Series Details ==

Series: Enable GuC submission (rev3)
URL   : https://patchwork.freedesktop.org/series/7153/
State : failure

== Summary ==

Series 7153v3 Enable GuC submission
http://patchwork.freedesktop.org/api/1.0/series/7153/revisions/3/mbox

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (ro-byt-n2820)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (ro-ivb-i7-3770)

fi-bdw-i7-5557u  total:217  pass:204  dwarn:0   dfail:0   fail:0   skip:13 
fi-bsw-n3050     total:216  pass:172  dwarn:0   dfail:0   fail:2   skip:42 
fi-byt-n2820     total:216  pass:173  dwarn:0   dfail:0   fail:2   skip:41 
fi-hsw-i7-4770r  total:217  pass:191  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-i7-6700k  total:217  pass:189  dwarn:0   dfail:0   fail:0   skip:28 
ro-bdw-i5-5250u  total:217  pass:179  dwarn:0   dfail:0   fail:0   skip:38 
ro-bdw-i7-5557U  total:217  pass:204  dwarn:0   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:217  pass:185  dwarn:0   dfail:0   fail:1   skip:31 
ro-bsw-n3050     total:217  pass:172  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:216  pass:171  dwarn:0   dfail:0   fail:4   skip:41 
ro-hsw-i3-4010u  total:216  pass:191  dwarn:0   dfail:0   fail:0   skip:25 
ro-hsw-i7-4770r  total:217  pass:191  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk-i7-620lm  total:217  pass:148  dwarn:0   dfail:0   fail:2   skip:67 
ro-ilk1-i5-650   total:212  pass:150  dwarn:0   dfail:0   fail:1   skip:61 
ro-ivb-i7-3770   total:217  pass:181  dwarn:0   dfail:0   fail:0   skip:36 
ro-ivb2-i7-3770  total:217  pass:185  dwarn:0   dfail:0   fail:0   skip:32 
ro-skl-i7-6700hq total:212  pass:188  dwarn:0   dfail:0   fail:0   skip:24 
ro-snb-i7-2620M  total:217  pass:175  dwarn:0   dfail:0   fail:1   skip:41 
fi-snb-i7-2600 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_948/

9d15199 drm-intel-nightly: 2016y-05m-20d-07h-54m-59s UTC integration manifest
c1bdfec drm/i915/guc: change default to using GuC submission if possible
429ed03 drm/i915/guc: rework guc_add_workqueue_item()
7e031c7 drm/i915/guc: don't spinwait if the GuC's workqueue is full
274cb6c drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()
25aa845 drm/i915/guc: add enable_guc_loading parameter
b9dc454 drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED
7bdfe0d drm/i915/guc: rename loader entry points

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

Re: ✗ Ro.CI.BAT: failure for Enable GuC submission (rev3)

[Tvrtko Ursulin]

On 20/05/16 12:15, Patchwork wrote:
> == Series Details ==
>
> Series: Enable GuC submission (rev3)
> URL   : https://patchwork.freedesktop.org/series/7153/
> State : failure
>
> == Summary ==
>
> Series 7153v3 Enable GuC submission
> http://patchwork.freedesktop.org/api/1.0/series/7153/revisions/3/mbox
>
> Test drv_module_reload_basic:
>                  dmesg-warn -> PASS       (ro-bdw-i7-5600u)
> Test kms_flip:
>          Subgroup basic-flip-vs-wf_vblank:
>                  pass       -> FAIL       (ro-byt-n2820)

Unrelated https://bugs.freedesktop.org/show_bug.cgi?id=94294

> Test kms_pipe_crc_basic:
>          Subgroup suspend-read-crc-pipe-b:
>                  dmesg-warn -> PASS       (ro-ivb-i7-3770)
>
> fi-bdw-i7-5557u  total:217  pass:204  dwarn:0   dfail:0   fail:0   skip:13
> fi-bsw-n3050     total:216  pass:172  dwarn:0   dfail:0   fail:2   skip:42
> fi-byt-n2820     total:216  pass:173  dwarn:0   dfail:0   fail:2   skip:41
> fi-hsw-i7-4770r  total:217  pass:191  dwarn:0   dfail:0   fail:0   skip:26
> fi-skl-i7-6700k  total:217  pass:189  dwarn:0   dfail:0   fail:0   skip:28
> ro-bdw-i5-5250u  total:217  pass:179  dwarn:0   dfail:0   fail:0   skip:38
> ro-bdw-i7-5557U  total:217  pass:204  dwarn:0   dfail:0   fail:0   skip:13
> ro-bdw-i7-5600u  total:217  pass:185  dwarn:0   dfail:0   fail:1   skip:31
> ro-bsw-n3050     total:217  pass:172  dwarn:0   dfail:0   fail:3   skip:42
> ro-byt-n2820     total:216  pass:171  dwarn:0   dfail:0   fail:4   skip:41
> ro-hsw-i3-4010u  total:216  pass:191  dwarn:0   dfail:0   fail:0   skip:25
> ro-hsw-i7-4770r  total:217  pass:191  dwarn:0   dfail:0   fail:0   skip:26
> ro-ilk-i7-620lm  total:217  pass:148  dwarn:0   dfail:0   fail:2   skip:67
> ro-ilk1-i5-650   total:212  pass:150  dwarn:0   dfail:0   fail:1   skip:61
> ro-ivb-i7-3770   total:217  pass:181  dwarn:0   dfail:0   fail:0   skip:36
> ro-ivb2-i7-3770  total:217  pass:185  dwarn:0   dfail:0   fail:0   skip:32
> ro-skl-i7-6700hq total:212  pass:188  dwarn:0   dfail:0   fail:0   skip:24
> ro-snb-i7-2620M  total:217  pass:175  dwarn:0   dfail:0   fail:1   skip:41
> fi-snb-i7-2600 failed to connect after reboot
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_948/
>
> 9d15199 drm-intel-nightly: 2016y-05m-20d-07h-54m-59s UTC integration manifest
> c1bdfec drm/i915/guc: change default to using GuC submission if possible
> 429ed03 drm/i915/guc: rework guc_add_workqueue_item()
> 7e031c7 drm/i915/guc: don't spinwait if the GuC's workqueue is full
> 274cb6c drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()
> 25aa845 drm/i915/guc: add enable_guc_loading parameter
> b9dc454 drm/i915/guc: distinguish HAS_GUC() from HAS_GUC_UCODE/HAS_GUC_SCHED
> 7bdfe0d drm/i915/guc: rename loader entry points

Merged up the next to last patch - so not enabling GuC by default yet. 
Thanks for pthe atches and review!

Regards,

Tvrtko

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