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

2017-10-21 Thread Sagar Arun Kamble 



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 
Cc: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Tvrtko Ursulin 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
---
  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

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

2017-10-19 Thread Tvrtko Ursulin 


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 
Cc: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Tvrtko Ursulin 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
---
  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.


Regards,

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

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

2017-10-18 Thread Sagar Arun Kamble 



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 
Cc: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Tvrtko Ursulin 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
---
  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.


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.

will change with enable_guc_loading change.


Does that make sense?

Yes :)



+    }
    /* 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 

Regards,

Tvrtko


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

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

2017-10-18 Thread Tvrtko Ursulin 


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 
Cc: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Tvrtko Ursulin 
Cc: Chris Wilson 
Cc: Joonas Lahtinen 
---
  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 

Regards,

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