Re: [PATCH v9 6/8] drm/i915/guc : Introducing enable_guc module parameter

Sujaritha <sujaritha.sundaresan@xxxxxxxxx> · Wed, 22 Nov 2017 13:54:25 -0800

On 11/12/2017 08:52 AM, Michal Wajdeczko wrote:
On Sat, 11 Nov 2017 01:06:36 +0100, Sujaritha Sundaresan 
<sujaritha.sundaresan@xxxxxxxxx> wrote:

Replacing enable_guc_submission with enable_guc modparam.
In effect enable_guc is replacing enable_guc_loading and
enable_guc_submission.

Maybe it will be better if we replace guc_loading and guc_submission
modparams with single guc_enable in one patch, as now you are touching
almost the same places again...

I will merge this with removing enable_guc_loading :)

Suggested by : Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
---
 drivers/gpu/drm/i915/i915_drv.h         |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
 drivers/gpu/drm/i915/i915_irq.c         |  2 +-
 drivers/gpu/drm/i915/i915_params.c      |  7 ++++---
 drivers/gpu/drm/i915/i915_params.h      |  2 +-
 drivers/gpu/drm/i915/intel_guc.c        |  2 +-
 drivers/gpu/drm/i915/intel_uc.c         | 35 
+++++++++++++++++----------------
 7 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h 
b/drivers/gpu/drm/i915/i915_drv.h
index 798fa8a..ad73cee 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3225,7 +3225,7 @@ static inline unsigned int 
i915_sg_segment_size(void)
 #define HAS_HUC_UCODE(dev_priv)    ((dev_priv)->huc.fw.path != NULL)
#define NEEDS_GUC_FW(dev_priv) \
-        (HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)
+        (HAS_GUC(dev_priv) && i915_modparams.enable_guc)
#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/drm/i915/i915_gem_context.c
index 6a819c0..43210df 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -409,7 +409,7 @@ struct i915_gem_context *
     i915_gem_context_set_closed(ctx); /* not user accessible */
     i915_gem_context_clear_bannable(ctx);
     i915_gem_context_set_force_single_submission(ctx);
-    if (!i915_modparams.enable_guc_submission)
+    if (!i915_modparams.enable_guc)
         ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
    GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
diff --git a/drivers/gpu/drm/i915/i915_irq.c 
b/drivers/gpu/drm/i915/i915_irq.c
index a414bca..693b345 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1400,7 +1400,7 @@ static void snb_gt_irq_handler(struct 
drm_i915_private *dev_priv,
    if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
         notify_ring(engine);
-        tasklet |= i915_modparams.enable_guc_submission;
+        tasklet |= i915_modparams.enable_guc;
     }
    if (tasklet)
diff --git a/drivers/gpu/drm/i915/i915_params.c 
b/drivers/gpu/drm/i915/i915_params.c
index 1c25f45..51cf6bd 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -162,9 +162,10 @@ struct i915_params i915_modparams __read_mostly = {
     "(0=use value from vbt [default], 1=low power swing(200mV),"
     "2=default swing(400mV))");
-i915_param_named_unsafe(enable_guc_submission, int, 0400,
-    "Enable GuC submission "
-    "(-1=auto, 0=never [default], 1=if available, 2=required)");
+i915_param_named_unsafe(enable_guc, int, 0400,
+    "Enable GuC submission and loading "
+    "(-1=auto [default], 0=No GuC or HuC, 1=Load & use GuC, HuC on 
the side"
+    " 2=Load GuC only for HuC)");

What about direct description:

"Enable GuC"
-1=auto [default]
 0=disable GuC loading
 1=enable GuC submission
 2=enable HuC
 3=enable GuC submission and HuC

i915_param_named(guc_log_level, int, 0400,
     "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
diff --git a/drivers/gpu/drm/i915/i915_params.h 
b/drivers/gpu/drm/i915/i915_params.h
index 9e1e231..7bf4dce 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,7 +44,7 @@
     param(int, disable_power_well, -1) \
     param(int, enable_ips, 1) \
     param(int, invert_brightness, 0) \
-    param(int, enable_guc_submission, 0) \
+    param(int, enable_guc, -1) \
     param(int, guc_log_level, -1) \
     param(char *, guc_firmware_path, NULL) \
     param(char *, huc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c
index 823d0c2..629ef5d 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -128,7 +128,7 @@ void intel_guc_init_params(struct intel_guc *guc)
     }
    /* If GuC submission is enabled, set up additional parameters 
here */
-    if (i915_modparams.enable_guc_submission) {
+    if (i915_modparams.enable_guc) {
         u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
         u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
         u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
diff --git a/drivers/gpu/drm/i915/intel_uc.c 
b/drivers/gpu/drm/i915/intel_uc.c
index 320165a..980de7a 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -51,33 +51,34 @@ void intel_uc_sanitize_options(struct 
drm_i915_private *dev_priv)
 {
     /* Verify Hardware version */
     if (!HAS_GUC(dev_priv)) {
-        if (i915_modparams.enable_guc_submission > 0)
+        if (i915_modparams.enable_guc > 0)
-            DRM_INFO("Ignoring option %s - no hardware", 
"enable_guc_submission");
+            DRM_INFO("Ignoring option %s - no hardware", "enable_guc");
-        i915_modparams.enable_guc_submission = 0;
+        i915_modparams.enable_guc = 0;
         return;
     }
    /* Verify Firmware version */
     if (!HAS_HUC_UCODE(dev_priv)) {
-        if (i915_modparams.enable_guc_submission > 0) {
+        if (i915_modparams.enable_guc > 0) {
-            DRM_INFO("Ignoring option %s - no firmware", 
"enable_guc_submission");
+            DRM_INFO("Ignoring option %s - no firmware", "enable_guc");
-            i915_modparams.enable_guc_submission = 0;
+            i915_modparams.enable_guc = 0;
         return;
         }

-        if (i915_modparams.enable_guc_submission < 0) {
-            i915_modparams.enable_guc_submission = 0;
-        return;
+        if (i915_modparams.enable_guc < 0) {
+            i915_modparams.enable_guc = 0;
+            return;
         }
     }
+
    /*
      * A negative value means "use platform default" (enabled if we 
have
      * survived to get here)
      */
-    if (i915_modparams.enable_guc_submission == 1)
-        i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
+    if (i915_modparams.enable_guc < 0)
+        i915_modparams.enable_guc = 1;
 }
void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -169,7 +170,7 @@ int intel_uc_init_hw(struct drm_i915_private 
*dev_priv)
     /* We need to notify the guc whenever we change the GGTT */
     i915_ggtt_enable_guc(dev_priv);
-    if (i915_modparams.enable_guc_submission) {
+    if (i915_modparams.enable_guc) {
         /*
          * This is stuff we need to have available at fw load time
          * if we are planning to enable submission later
@@ -219,7 +220,7 @@ int intel_uc_init_hw(struct drm_i915_private 
*dev_priv)
         goto err_log_capture;
    intel_huc_auth(&dev_priv->huc);
-    if (i915_modparams.enable_guc_submission) {
+    if (i915_modparams.enable_guc) {
         if (i915_modparams.guc_log_level >= 0)
             gen9_enable_guc_interrupts(dev_priv);
@@ -229,7 +230,7 @@ int intel_uc_init_hw(struct drm_i915_private 
*dev_priv)
     }
    dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version 
%u.%u])\n",
-         i915_modparams.enable_guc_submission ? "submission enabled" :
+         i915_modparams.enable_guc ? "submission enabled" :
                             "loaded",
          guc->fw.path,
          guc->fw.major_ver_found, guc->fw.minor_ver_found);
@@ -251,15 +252,15 @@ int intel_uc_init_hw(struct drm_i915_private 
*dev_priv)
 err_log_capture:
     guc_capture_load_err_log(guc);
 err_submission:
-    if (i915_modparams.enable_guc_submission)
+    if (i915_modparams.enable_guc)
         intel_guc_submission_fini(guc);
 err_guc:
     i915_ggtt_disable_guc(dev_priv);
-    if (i915_modparams.enable_guc_submission > 1) {
+    if (i915_modparams.enable_guc > 1) {
         DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
         ret = -EIO;
-    } else if (i915_modparams.enable_guc_submission == 1) {
+    } else if (i915_modparams.enable_guc == 1) {
         DRM_NOTE("Falling back from GuC submission to execlist 
mode\n");
         ret = 0;
     } else {
@@ -276,12 +277,12 @@ void intel_uc_fini_hw(struct drm_i915_private 
*dev_priv)
     if (!NEEDS_GUC_FW(dev_priv))
         return;
-    if (i915_modparams.enable_guc_submission)
+    if (i915_modparams.enable_guc)
         intel_guc_submission_disable(&dev_priv->guc);
    guc_disable_communication(&dev_priv->guc);
-    if (i915_modparams.enable_guc_submission) {
+    if (i915_modparams.enable_guc) {
         gen9_disable_guc_interrupts(dev_priv);
         intel_guc_submission_fini(&dev_priv->guc);
     }

Thanks for the review.

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







