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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.kamble@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
---
  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...

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?

I was just thinking that we should probably let the user know if they asked for logging, but it cannot happen due guc loading turned off, to have analogy with the same when GuC is not available in the first place.

Does that make sense?

+	}
/* A negative value means "use platform default" */
  	if (i915_modparams.enable_guc_submission < 0)


But since it looks like a cleanup even without the above:

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

Regards,

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux