Re: [Intel-gfx] [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

Sat, 21 Oct 2017 01:12:46 -0700 


On 10/19/2017 12:52 PM, Tvrtko Ursulin wrote:


On 18/10/2017 16:50, Sagar Arun Kamble wrote:

On 10/18/2017 6:29 PM, Tvrtko Ursulin wrote:


On 18/10/2017 07:46, Sagar Arun Kamble wrote:

Parameter guc_log_level needs to be sanitized based on GuC support and
enable_guc_loading parameter since it depends on them like
enable_guc_submission. This will make GuC logging paths independent of
enable_guc_submission parameter in further patches.

v2: Added documentation to intel_guc_log.c and param description about
GuC loading dependency. (Michal Wajdeczko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kam...@intel.com>
Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
---
  drivers/gpu/drm/i915/i915_params.c   |  4 +++-
  drivers/gpu/drm/i915/intel_guc_log.c |  1 +
  drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
  3 files changed, 11 insertions(+), 4 deletions(-)


diff --git a/drivers/gpu/drm/i915/i915_params.c 
b/drivers/gpu/drm/i915/i915_params.c

index b4faeb6..774c56e 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c

@@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly 
= {

      "(-1=auto, 0=never [default], 1=if available, 2=required)");
    i915_param_named(guc_log_level, int, 0400,

-    "GuC firmware logging level (-1:disabled (default), 
0-3:enabled)");
+    "GuC firmware logging level. This takes effect only if GuC is 
to be "

+    "loaded (depends on enable_guc_loading) (-1:disabled (default), "
+    "0-3:enabled)");
    i915_param_named_unsafe(guc_firmware_path, charp, 0400,
      "GuC firmware path to use instead of the default one");

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
b/drivers/gpu/drm/i915/intel_guc_log.c

index f53c663..200f0a1 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -34,6 +34,7 @@
   * DOC: GuC firmware log
   *

   * Firmware log is enabled by setting i915.guc_log_level to 
non-negative level.
+ * This takes effect only if GuC is to be loaded based on 
enable_guc_loading.
   * Log data is printed out via reading debugfs i915_guc_log_dump. 
Reading from
   * i915_guc_load_status will print out firmware loading status 
and scratch

   * registers value.

diff --git a/drivers/gpu/drm/i915/intel_uc.c 
b/drivers/gpu/drm/i915/intel_uc.c

index 62738ad..8feefcd 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c

@@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct 
drm_i915_private *dev_priv)

  {
      if (!HAS_GUC(dev_priv)) {
          if (i915_modparams.enable_guc_loading > 0 ||
-            i915_modparams.enable_guc_submission > 0)
+            i915_modparams.enable_guc_submission > 0 ||
+            i915_modparams.guc_log_level > 0)
              DRM_INFO("Ignoring GuC options, no hardware\n");



Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
default to one? Or we can worry about that when we get there...

This will fire for <=gen8 for +ve values which is expected.



i915_modparams.enable_guc_loading = 0;
          i915_modparams.enable_guc_submission = 0;
+        i915_modparams.guc_log_level = -1;
          return;
      }

  @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct 
drm_i915_private *dev_priv)

              i915_modparams.enable_guc_loading = 0;
      }
  -    /* Can't enable guc submission without guc loaded */
-    if (!i915_modparams.enable_guc_loading)
+    /* Can't enable guc submission and logging without guc loaded */
+    if (!i915_modparams.enable_guc_loading) {
          i915_modparams.enable_guc_submission = 0;
+        i915_modparams.guc_log_level = -1;



Hm2, how does this interact with the changes which are removing 
enable_guc_loading? Or it is a race?

It interacts. Will race. I think I should pause this series till the 
other one gets merged.



Okay, it's your call (as in you guys who are refactoring GuC heavily 
at the moment).



Should I continue to the end of this one then? It is not that big so I 
just as well can.

Thanks for the review. Will address all the inputs and post after 
Sujaritha's series as there will be some conflict.
I had thought of having this review in parallel as patches are largely 
independent but still there is some conflict so I

will defer next rev. for some more time.


Regards,

Tvrtko


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























					Reply via email to