Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Get rid of the enable_guc_loading module parameter

2017-05-31 Thread Oscar Mateo 




On 05/18/2017 05:41 PM, Michal Wajdeczko wrote:

On Fri, May 05, 2017 at 01:23:17PM +, Oscar Mateo wrote:

The decission to enable GuC loading shouldn't be left to the user.
Provided the HW supports the GuC, there are only two reasons to load it:

- The user has requested GuC submission.
- We have a HuC firmware available (so we need the GuC to validate it).

We leave the enable_guc_submission parameter untouched ("auto", "never",
"if supported", "required") but make its behavior a little bit more
consistent. Also, if not really required, we do not try to fetch any
firmware.

Cc: Anusha Srivatsa 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Signed-off-by: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_debugfs.c | 10 --
  drivers/gpu/drm/i915/i915_drv.c |  2 +-
  drivers/gpu/drm/i915/i915_drv.h | 16 +
  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
  drivers/gpu/drm/i915/i915_gem_gtt.c |  2 +-
  drivers/gpu/drm/i915/i915_irq.c |  2 +-
  drivers/gpu/drm/i915/i915_params.c  |  6 
  drivers/gpu/drm/i915/i915_params.h  |  2 --
  drivers/gpu/drm/i915/intel_guc_loader.c | 48 +++
  drivers/gpu/drm/i915/intel_huc.c|  5 +--
  drivers/gpu/drm/i915/intel_uc.c | 58 +
  drivers/gpu/drm/i915/intel_uc.h |  4 +--
  drivers/gpu/drm/i915/intel_uncore.c |  3 +-
  13 files changed, 82 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 870c470..e030b41 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2366,8 +2366,10 @@ static int i915_huc_load_status_info(struct seq_file *m, 
void *data)
struct drm_i915_private *dev_priv = node_to_i915(m->private);
struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
  
-	if (!HAS_HUC_UCODE(dev_priv))

+   if (!HAS_GUC(dev_priv)) {
+   seq_puts(m, "No HuC support in HW\n");
return 0;
+   }
  
  	seq_puts(m, "HuC firmware status:\n");

seq_printf(m, "\tpath: %s\n", huc_fw->path);
@@ -2399,8 +2401,10 @@ static int i915_guc_load_status_info(struct seq_file *m, 
void *data)
struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
u32 tmp, i;
  
-	if (!HAS_GUC_UCODE(dev_priv))

+   if (!HAS_GUC(dev_priv)) {
+   seq_puts(m, "No GuC support in HW\n");
return 0;
+   }
  
  	seq_printf(m, "GuC firmware status:\n");

seq_printf(m, "\tpath: %s\n",
@@ -2504,7 +2508,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
  
  	if (!guc->execbuf_client) {

seq_printf(m, "GuC submission %s\n",
-  HAS_GUC_SCHED(dev_priv) ?
+  HAS_GUC(dev_priv) ?
   "disabled" :
   "not supported");
return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 72fb47a..006ed91 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -996,7 +996,7 @@ static void intel_sanitize_options(struct drm_i915_private 
*dev_priv)
i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
DRM_DEBUG_DRIVER("use GPU semaphores? %s\n", yesno(i915.semaphores));
  
-	intel_uc_sanitize_options(dev_priv);

+   intel_guc_sanitize_submission(dev_priv);
  }
  
  /**

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b20ed16..5d00120 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2921,15 +2921,17 @@ static inline struct scatterlist *__sg_next(struct 
scatterlist *sg)
  #define HAS_RUNTIME_PM(dev_priv) ((dev_priv)->info.has_runtime_pm)
  #define HAS_64BIT_RELOC(dev_priv) ((dev_priv)->info.has_64bit_reloc)
  
+#define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)

+#define HAS_GUC_UCODE(dev_priv)((dev_priv)->guc.fw.path != NULL)
+#define HAS_HUC_UCODE(dev_priv)((dev_priv)->huc.fw.path != NULL)
+
  /*
- * 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.
+ * Only two things require us to load the GuC firmware: either we want
+ * to enable GuC submission or we need it to to validate a HuC firmware
   */
-#define HAS_GUC(dev_priv)  ((dev_priv)->info.has_guc)
-#define HAS_GUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
-#define HAS_GUC_SCHED(dev_priv)(HAS_GUC(dev_priv))
-#define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv))
+#define NEEDS_GUC_LOADING(dev_priv) \
+   (HAS_GUC(dev_priv) && \
+   (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
  
  #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
  
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/d

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Get rid of the enable_guc_loading module parameter

2017-05-18 Thread Michal Wajdeczko 
On Fri, May 05, 2017 at 01:23:17PM +, Oscar Mateo wrote:
> The decission to enable GuC loading shouldn't be left to the user.
> Provided the HW supports the GuC, there are only two reasons to load it:
> 
> - The user has requested GuC submission.
> - We have a HuC firmware available (so we need the GuC to validate it).
> 
> We leave the enable_guc_submission parameter untouched ("auto", "never",
> "if supported", "required") but make its behavior a little bit more
> consistent. Also, if not really required, we do not try to fetch any
> firmware.
> 
> Cc: Anusha Srivatsa 
> Cc: Daniele Ceraolo Spurio 
> Cc: Chris Wilson 
> Signed-off-by: Oscar Mateo 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 10 --
>  drivers/gpu/drm/i915/i915_drv.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h | 16 +
>  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c |  2 +-
>  drivers/gpu/drm/i915/i915_params.c  |  6 
>  drivers/gpu/drm/i915/i915_params.h  |  2 --
>  drivers/gpu/drm/i915/intel_guc_loader.c | 48 +++
>  drivers/gpu/drm/i915/intel_huc.c|  5 +--
>  drivers/gpu/drm/i915/intel_uc.c | 58 
> +
>  drivers/gpu/drm/i915/intel_uc.h |  4 +--
>  drivers/gpu/drm/i915/intel_uncore.c |  3 +-
>  13 files changed, 82 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 870c470..e030b41 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2366,8 +2366,10 @@ static int i915_huc_load_status_info(struct seq_file 
> *m, void *data)
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
>   struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>  
> - if (!HAS_HUC_UCODE(dev_priv))
> + if (!HAS_GUC(dev_priv)) {
> + seq_puts(m, "No HuC support in HW\n");
>   return 0;
> + }
>  
>   seq_puts(m, "HuC firmware status:\n");
>   seq_printf(m, "\tpath: %s\n", huc_fw->path);
> @@ -2399,8 +2401,10 @@ static int i915_guc_load_status_info(struct seq_file 
> *m, void *data)
>   struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>   u32 tmp, i;
>  
> - if (!HAS_GUC_UCODE(dev_priv))
> + if (!HAS_GUC(dev_priv)) {
> + seq_puts(m, "No GuC support in HW\n");
>   return 0;
> + }
>  
>   seq_printf(m, "GuC firmware status:\n");
>   seq_printf(m, "\tpath: %s\n",
> @@ -2504,7 +2508,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  
>   if (!guc->execbuf_client) {
>   seq_printf(m, "GuC submission %s\n",
> -HAS_GUC_SCHED(dev_priv) ?
> +HAS_GUC(dev_priv) ?
>  "disabled" :
>  "not supported");
>   return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 72fb47a..006ed91 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -996,7 +996,7 @@ static void intel_sanitize_options(struct 
> drm_i915_private *dev_priv)
>   i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
>   DRM_DEBUG_DRIVER("use GPU semaphores? %s\n", yesno(i915.semaphores));
>  
> - intel_uc_sanitize_options(dev_priv);
> + intel_guc_sanitize_submission(dev_priv);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b20ed16..5d00120 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2921,15 +2921,17 @@ static inline struct scatterlist *__sg_next(struct 
> scatterlist *sg)
>  #define HAS_RUNTIME_PM(dev_priv) ((dev_priv)->info.has_runtime_pm)
>  #define HAS_64BIT_RELOC(dev_priv) ((dev_priv)->info.has_64bit_reloc)
>  
> +#define HAS_GUC(dev_priv)((dev_priv)->info.has_guc)
> +#define HAS_GUC_UCODE(dev_priv)  ((dev_priv)->guc.fw.path != NULL)
> +#define HAS_HUC_UCODE(dev_priv)  ((dev_priv)->huc.fw.path != NULL)
> +
>  /*
> - * 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.
> + * Only two things require us to load the GuC firmware: either we want
> + * to enable GuC submission or we need it to to validate a HuC firmware
>   */
> -#define HAS_GUC(dev_priv)((dev_priv)->info.has_guc)
> -#define HAS_GUC_UCODE(dev_priv)  (HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv)  (HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv)  (HAS_GUC(dev_priv))
> +#define NEEDS_GUC_LOADING(dev_priv) \
> + (HAS_GUC(dev_priv) && \
> + (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>  
>  #define HAS_RESOURCE_STREAMER(dev_priv) 
> ((dev_pr

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Get rid of the enable_guc_loading module parameter

2017-05-17 Thread Srivatsa, Anusha 
I like the approach.

BR,
Anusha
>-Original Message-
>From: Mateo Lozano, Oscar
>Sent: Friday, May 5, 2017 6:23 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Mateo Lozano, Oscar ; Srivatsa, Anusha
>; Ceraolo Spurio, Daniele
>; Chris Wilson 
>Subject: [PATCH 1/2] drm/i915/guc: Get rid of the enable_guc_loading module
>parameter
>
>The decission to enable GuC loading shouldn't be left to the user.
>Provided the HW supports the GuC, there are only two reasons to load it:
>
>- The user has requested GuC submission.
>- We have a HuC firmware available (so we need the GuC to validate it).
>
>We leave the enable_guc_submission parameter untouched ("auto", "never", "if
>supported", "required") but make its behavior a little bit more consistent. 
>Also, if
>not really required, we do not try to fetch any firmware.
>
>Cc: Anusha Srivatsa 
>Cc: Daniele Ceraolo Spurio 
>Cc: Chris Wilson 
>Signed-off-by: Oscar Mateo 

Acked-by: Anusha Srivatsa 

> drivers/gpu/drm/i915/i915_debugfs.c | 10 --
> drivers/gpu/drm/i915/i915_drv.c |  2 +-
> drivers/gpu/drm/i915/i915_drv.h | 16 +
> drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c |  2 +-
> drivers/gpu/drm/i915/i915_irq.c |  2 +-
> drivers/gpu/drm/i915/i915_params.c  |  6 
> drivers/gpu/drm/i915/i915_params.h  |  2 --
> drivers/gpu/drm/i915/intel_guc_loader.c | 48 +++
> drivers/gpu/drm/i915/intel_huc.c|  5 +--
> drivers/gpu/drm/i915/intel_uc.c | 58 +
> drivers/gpu/drm/i915/intel_uc.h |  4 +--
> drivers/gpu/drm/i915/intel_uncore.c |  3 +-
> 13 files changed, 82 insertions(+), 78 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>b/drivers/gpu/drm/i915/i915_debugfs.c
>index 870c470..e030b41 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -2366,8 +2366,10 @@ static int i915_huc_load_status_info(struct seq_file
>*m, void *data)
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
>   struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>
>-  if (!HAS_HUC_UCODE(dev_priv))
>+  if (!HAS_GUC(dev_priv)) {
>+  seq_puts(m, "No HuC support in HW\n");
>   return 0;
>+  }
>
>   seq_puts(m, "HuC firmware status:\n");
>   seq_printf(m, "\tpath: %s\n", huc_fw->path); @@ -2399,8 +2401,10 @@
>static int i915_guc_load_status_info(struct seq_file *m, void *data)
>   struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>   u32 tmp, i;
>
>-  if (!HAS_GUC_UCODE(dev_priv))
>+  if (!HAS_GUC(dev_priv)) {
>+  seq_puts(m, "No GuC support in HW\n");
>   return 0;
>+  }
>
>   seq_printf(m, "GuC firmware status:\n");
>   seq_printf(m, "\tpath: %s\n",
>@@ -2504,7 +2508,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
>
>   if (!guc->execbuf_client) {
>   seq_printf(m, "GuC submission %s\n",
>- HAS_GUC_SCHED(dev_priv) ?
>+ HAS_GUC(dev_priv) ?
>  "disabled" :
>  "not supported");
>   return 0;
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index 72fb47a..006ed91 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -996,7 +996,7 @@ static void intel_sanitize_options(struct drm_i915_private
>*dev_priv)
>   i915.semaphores = intel_sanitize_semaphores(dev_priv,
>i915.semaphores);
>   DRM_DEBUG_DRIVER("use GPU semaphores? %s\n",
>yesno(i915.semaphores));
>
>-  intel_uc_sanitize_options(dev_priv);
>+  intel_guc_sanitize_submission(dev_priv);
> }
>
> /**
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index b20ed16..5d00120 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -2921,15 +2921,17 @@ static inline struct scatterlist *__sg_next(struct
>scatterlist *sg)  #define HAS_RUNTIME_PM(dev_priv) ((dev_priv)-
>>info.has_runtime_pm)  #define HAS_64BIT_RELOC(dev_priv) ((dev_priv)-
>>info.has_64bit_reloc)
>
>+#define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
>+#define HAS_GUC_UCODE(dev_priv)   ((dev_priv)->guc.fw.path != NULL)
>+#define HAS_HUC_UCODE(dev_priv)   ((dev_priv)->huc.fw.path != NULL)
>+
> /*
>- * 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.
>+ * Only two things require us to load the GuC firmware: either we want
>+ * to enable GuC submission or we need it to to validate a HuC firmware
>  */
>-#define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
>-#define HAS_GUC_UCODE(dev_priv)   (HAS_GUC(dev_priv))
>-#define HAS_GUC_SCHED(dev_priv)   (HAS_GUC(dev_priv))
>-#define HAS_HUC_UCODE(dev_priv)