Re: [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter

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

 




On 11/07/16 13:11, Goel, Akash wrote:


On 7/11/2016 5:20 PM, Tvrtko Ursulin wrote:

On 11/07/16 12:41, Goel, Akash wrote:
On 7/11/2016 3:07 PM, Tvrtko Ursulin wrote:

On 10/07/16 14:41, akash.goel@xxxxxxxxx wrote:
From: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>

b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..8a9a0cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..b211bd0 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,11 +175,13 @@ static void set_guc_init_params(struct
drm_i915_private *dev_priv)
      params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
              GUC_CTL_VCS2_ENABLED;

-    if (i915.guc_log_level >= 0) {
-        params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+    params[GUC_CTL_LOG_PARAMS] = guc->log_flags;

guc->log_flags will be zero when logging is not configured because guc
is a part of dev_priv. So it looks safe - although I reckon it would be
clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else
below?

If logging is not enabled at (due to guc_log_level < 0), then also
log_flags needs to be setup & passed to GuC firmware.
log_flags shall not be zero even when logging is not be enabled (at boot
time).
Actually log_flags will also contain the address of the log buffer.

Ah yes, I got confused by jumping between one file with your patch
applied and one without it.

+
+    if (i915.guc_log_level >= 0)
          params[GUC_CTL_DEBUG] =
              i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-    }
+    else
+        params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;

I also wonder how come GUC_LOG_DISABLED isn't set today when
i915.guc_log_level == -1, given that:

#define   GUC_LOG_DISABLED             (1 << 6)

Is that bit set by default somehow if i915 does not program it?


Yes currently GUC_LOG_DISABLED won't get set for guc_log_level = -1.
But then log buffer address will go as NULL and GUC_LOG_VALID flag will
go as 0, for guc_log_level = -1. So this way logging on GuC side will
not get enabled.
I hope I understood your concern correctly.

Yes, this clarifies it. Although I do have one more question then - what
happens if at boot i915.guc_log_level == -1 and then with later patches
logging gets enabled via debugfs - who and how sets
params[GUC_CTL_DEBUG]? Host2GuC overrides this parameter?


Yes through Host2GuC action type, UK_LOG_ENABLE_LOGGING, Host will
request GuC firmware to enable/disable logging and alter the verbosity
level.

The params[GUC_CTL_DEBUG] is just part of the firmware initialization
parameters and is not used after that.

Got it now, in that case:

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