[15/20] drm/i915: Debugfs support for GuC logging control

Submitted by Akash Goel on Aug. 12, 2016, 6:25 a.m.

Details

Message ID 1470983123-22127-16-git-send-email-akash.goel@intel.com
State New
Headers show
Series "Support for sustained capturing of GuC firmware logs" ( rev: 6 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Akash Goel Aug. 12, 2016, 6:25 a.m.
From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

This patch provides debugfs interface i915_guc_output_control for
on the fly enabling/disabling of logging in GuC firmware and controlling
the verbosity level of logs.
The value written to the file, should have bit 0 set to enable logging and
bits 4-7 should contain the verbosity info.

v2: Add a forceful flush, to collect left over logs, on disabling logging.
    Useful for Validation.

v3: Besides minor cleanup, implement read method for the debugfs file and
    set the guc_log_level to -1 when logging is disabled. (Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 44 ++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_guc_submission.c | 63 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 3 files changed, 107 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 14e0dcf..f472fbcd3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2674,6 +2674,47 @@  static int i915_guc_log_dump(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_guc_log_control_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (!dev_priv->guc.log.obj)
+		return -EINVAL;
+
+	*val = i915.guc_log_level;
+
+	return 0;
+}
+
+static int i915_guc_log_control_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	if (!dev_priv->guc.log.obj) {
+		ret = -EINVAL;
+		goto end;
+	}
+
+	intel_runtime_pm_get(dev_priv);
+	ret = i915_guc_log_control(dev_priv, val);
+	intel_runtime_pm_put(dev_priv);
+
+end:
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
+			i915_guc_log_control_get, i915_guc_log_control_set,
+			"%lld\n");
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -5477,7 +5518,8 @@  static const struct i915_debugfs_files {
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
 	{"i915_dp_test_data", &i915_displayport_test_data_fops},
 	{"i915_dp_test_type", &i915_displayport_test_type_fops},
-	{"i915_dp_test_active", &i915_displayport_test_active_fops}
+	{"i915_dp_test_active", &i915_displayport_test_active_fops},
+	{"i915_guc_log_control", &i915_guc_log_control_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 4a75c16..041cf68 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -195,6 +195,16 @@  static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
 	return host2guc_action(guc, data, 2);
 }
 
+static int host2guc_logging_control(struct intel_guc *guc, u32 control_val)
+{
+	u32 data[2];
+
+	data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING;
+	data[1] = control_val;
+
+	return host2guc_action(guc, data, 2);
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -1538,3 +1548,56 @@  void i915_guc_register(struct drm_i915_private *dev_priv)
 	guc_log_late_setup(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
+
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
+{
+	union guc_log_control log_param;
+	int ret;
+
+	log_param.logging_enabled = control_val & 0x1;
+	log_param.verbosity = (control_val >> 4) & 0xF;
+
+	if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
+	    log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
+		return -EINVAL;
+
+	/* This combination doesn't make sense & won't have any effect */
+	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
+		return 0;
+
+	ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
+	if (ret < 0) {
+		DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret);
+		return ret;
+	}
+
+	i915.guc_log_level = log_param.verbosity;
+
+	/* If log_level was set as -1 at boot time, then the relay channel file
+	 * wouldn't have been created by now and interrupts also would not have
+	 * been enabled.
+	 */
+	if (!dev_priv->guc.log.relay_chan) {
+		ret = guc_log_late_setup(&dev_priv->guc);
+		if (!ret)
+			gen9_enable_guc_interrupts(dev_priv);
+	} else if (!log_param.logging_enabled) {
+		/* Once logging is disabled, GuC won't generate logs & send an
+		 * interrupt. But there could be some data in the log buffer
+		 * which is yet to be captured. So request GuC to update the log
+		 * buffer state and then collect the left over logs.
+		 */
+		i915_guc_flush_logs(dev_priv);
+
+		/* GuC would have updated the log buffer by now, so capture it */
+		i915_guc_capture_logs(dev_priv);
+
+		/* As logging is disabled, update the log level to reflect that */
+		i915.guc_log_level = -1;
+	} else {
+		/* In case interrupts were disabled, enable them now */
+		gen9_enable_guc_interrupts(dev_priv);
+	}
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d3a5447..2f8c408 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -186,5 +186,6 @@  void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
 void i915_guc_flush_logs(struct drm_i915_private *dev_priv);
 void i915_guc_register(struct drm_i915_private *dev_priv);
 void i915_guc_unregister(struct drm_i915_private *dev_priv);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 
 #endif

Comments

On 12/08/16 07:25, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> This patch provides debugfs interface i915_guc_output_control for
> on the fly enabling/disabling of logging in GuC firmware and controlling
> the verbosity level of logs.
> The value written to the file, should have bit 0 set to enable logging and
> bits 4-7 should contain the verbosity info.
>
> v2: Add a forceful flush, to collect left over logs, on disabling logging.
>      Useful for Validation.
>
> v3: Besides minor cleanup, implement read method for the debugfs file and
>      set the guc_log_level to -1 when logging is disabled. (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        | 44 ++++++++++++++++++++-
>   drivers/gpu/drm/i915/i915_guc_submission.c | 63 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>   3 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 14e0dcf..f472fbcd3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2674,6 +2674,47 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
>   	return 0;
>   }
>
> +static int i915_guc_log_control_get(void *data, u64 *val)
> +{
> +	struct drm_device *dev = data;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (!dev_priv->guc.log.obj)
> +		return -EINVAL;
> +
> +	*val = i915.guc_log_level;
> +
> +	return 0;
> +}
> +
> +static int i915_guc_log_control_set(void *data, u64 val)
> +{
> +	struct drm_device *dev = data;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	if (!dev_priv->guc.log.obj) {
> +		ret = -EINVAL;
> +		goto end;
> +	}
> +
> +	intel_runtime_pm_get(dev_priv);
> +	ret = i915_guc_log_control(dev_priv, val);
> +	intel_runtime_pm_put(dev_priv);
> +
> +end:
> +	mutex_unlock(&dev->struct_mutex);
> +	return ret;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
> +			i915_guc_log_control_get, i915_guc_log_control_set,
> +			"%lld\n");
> +
>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>   {
>   	struct drm_info_node *node = m->private;
> @@ -5477,7 +5518,8 @@ static const struct i915_debugfs_files {
>   	{"i915_fbc_false_color", &i915_fbc_fc_fops},
>   	{"i915_dp_test_data", &i915_displayport_test_data_fops},
>   	{"i915_dp_test_type", &i915_displayport_test_type_fops},
> -	{"i915_dp_test_active", &i915_displayport_test_active_fops}
> +	{"i915_dp_test_active", &i915_displayport_test_active_fops},
> +	{"i915_guc_log_control", &i915_guc_log_control_fops}
>   };
>
>   void intel_display_crc_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 4a75c16..041cf68 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -195,6 +195,16 @@ static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
>   	return host2guc_action(guc, data, 2);
>   }
>
> +static int host2guc_logging_control(struct intel_guc *guc, u32 control_val)
> +{
> +	u32 data[2];
> +
> +	data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING;
> +	data[1] = control_val;
> +
> +	return host2guc_action(guc, data, 2);
> +}
> +
>   /*
>    * Initialise, update, or clear doorbell data shared with the GuC
>    *
> @@ -1538,3 +1548,56 @@ void i915_guc_register(struct drm_i915_private *dev_priv)
>   	guc_log_late_setup(&dev_priv->guc);
>   	mutex_unlock(&dev_priv->drm.struct_mutex);
>   }
> +
> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> +{
> +	union guc_log_control log_param;
> +	int ret;
> +
> +	log_param.logging_enabled = control_val & 0x1;
> +	log_param.verbosity = (control_val >> 4) & 0xF;

Maybe "log_param.value = control_val" would also work since 
guc_log_control is conveniently defined as an union. Doesn't matter though.

> +
> +	if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
> +	    log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
> +		return -EINVAL;
> +
> +	/* This combination doesn't make sense & won't have any effect */
> +	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
> +		return 0;

I wonder if it would work and maybe look nicer to generalize as:

	int guc_log_level;

	guc_log_level = log_param.logging_enabled ? log_param.verbosity : -1;
	if (i915.guc_log_level == guc_log_level)
		return 0;
> +
> +	ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
> +	if (ret < 0) {
> +		DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	i915.guc_log_level = log_param.verbosity;

This would then become i915.guc_log_level = guc_log_level.

> +
> +	/* If log_level was set as -1 at boot time, then the relay channel file
> +	 * wouldn't have been created by now and interrupts also would not have
> +	 * been enabled.
> +	 */
> +	if (!dev_priv->guc.log.relay_chan) {
> +		ret = guc_log_late_setup(&dev_priv->guc);
> +		if (!ret)
> +			gen9_enable_guc_interrupts(dev_priv);
> +	} else if (!log_param.logging_enabled) {
> +		/* Once logging is disabled, GuC won't generate logs & send an
> +		 * interrupt. But there could be some data in the log buffer
> +		 * which is yet to be captured. So request GuC to update the log
> +		 * buffer state and then collect the left over logs.
> +		 */
> +		i915_guc_flush_logs(dev_priv);
> +
> +		/* GuC would have updated the log buffer by now, so capture it */
> +		i915_guc_capture_logs(dev_priv);
> +
> +		/* As logging is disabled, update the log level to reflect that */
> +		i915.guc_log_level = -1;
> +	} else {
> +		/* In case interrupts were disabled, enable them now */
> +		gen9_enable_guc_interrupts(dev_priv);
> +	}

And this block would need some adjustments with my guc_log_level idea.

Well not sure, see what you think. I am just attracted to the idea of 
operating in the same value domain as much as possible for readability 
and simplicity. Maybe it would not improve anything, I did not bother 
with typing it all to check.

> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index d3a5447..2f8c408 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -186,5 +186,6 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
>   void i915_guc_flush_logs(struct drm_i915_private *dev_priv);
>   void i915_guc_register(struct drm_i915_private *dev_priv);
>   void i915_guc_unregister(struct drm_i915_private *dev_priv);
> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
>
>   #endif
>

Patch looks correct as is, so:

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

Although I would be happier though if my suggestion to use the same 
value domain as for the module parameter was used. In other words:

	{"i915_guc_log_level", &i915_guc_log_control_fops}

...

int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
...
	int guc_log_level = (int)control_val;
...
	log_param.logging_enabled = guc_log_level > -1;
	log_param.verbosity = guc_log_level > -1 ? guc_log_level : 0;
...

It think it would be simpler for the user and developer to only have to 
think about one set of values when dealing with guc logging.

But maybe extensions to guc_log_control are imminent and expected so it 
would not be worth it in the long run. No idea.

Regards,

Tvrtko
On 8/12/2016 9:27 PM, Tvrtko Ursulin wrote:
>
> On 12/08/16 07:25, akash.goel@intel.com wrote:
>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>
>> This patch provides debugfs interface i915_guc_output_control for
>> on the fly enabling/disabling of logging in GuC firmware and controlling
>> the verbosity level of logs.
>> The value written to the file, should have bit 0 set to enable logging
>> and
>> bits 4-7 should contain the verbosity info.
>>
>> v2: Add a forceful flush, to collect left over logs, on disabling
>> logging.
>>      Useful for Validation.
>>
>> v3: Besides minor cleanup, implement read method for the debugfs file and
>>      set the guc_log_level to -1 when logging is disabled. (Tvrtko)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c        | 44 ++++++++++++++++++++-
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 63
>> ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>   3 files changed, 107 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 14e0dcf..f472fbcd3 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2674,6 +2674,47 @@ static int i915_guc_log_dump(struct seq_file
>> *m, void *data)
>>       return 0;
>>   }
>>
>> +static int i915_guc_log_control_get(void *data, u64 *val)
>> +{
>> +    struct drm_device *dev = data;
>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> +    if (!dev_priv->guc.log.obj)
>> +        return -EINVAL;
>> +
>> +    *val = i915.guc_log_level;
>> +
>> +    return 0;
>> +}
>> +
>> +static int i915_guc_log_control_set(void *data, u64 val)
>> +{
>> +    struct drm_device *dev = data;
>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>> +    int ret;
>> +
>> +    ret = mutex_lock_interruptible(&dev->struct_mutex);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (!dev_priv->guc.log.obj) {
>> +        ret = -EINVAL;
>> +        goto end;
>> +    }
>> +
>> +    intel_runtime_pm_get(dev_priv);
>> +    ret = i915_guc_log_control(dev_priv, val);
>> +    intel_runtime_pm_put(dev_priv);
>> +
>> +end:
>> +    mutex_unlock(&dev->struct_mutex);
>> +    return ret;
>> +}
>> +
>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>> +            i915_guc_log_control_get, i915_guc_log_control_set,
>> +            "%lld\n");
>> +
>>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>>   {
>>       struct drm_info_node *node = m->private;
>> @@ -5477,7 +5518,8 @@ static const struct i915_debugfs_files {
>>       {"i915_fbc_false_color", &i915_fbc_fc_fops},
>>       {"i915_dp_test_data", &i915_displayport_test_data_fops},
>>       {"i915_dp_test_type", &i915_displayport_test_type_fops},
>> -    {"i915_dp_test_active", &i915_displayport_test_active_fops}
>> +    {"i915_dp_test_active", &i915_displayport_test_active_fops},
>> +    {"i915_guc_log_control", &i915_guc_log_control_fops}
>>   };
>>
>>   void intel_display_crc_init(struct drm_device *dev)
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 4a75c16..041cf68 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -195,6 +195,16 @@ static int host2guc_force_logbuffer_flush(struct
>> intel_guc *guc)
>>       return host2guc_action(guc, data, 2);
>>   }
>>
>> +static int host2guc_logging_control(struct intel_guc *guc, u32
>> control_val)
>> +{
>> +    u32 data[2];
>> +
>> +    data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING;
>> +    data[1] = control_val;
>> +
>> +    return host2guc_action(guc, data, 2);
>> +}
>> +
>>   /*
>>    * Initialise, update, or clear doorbell data shared with the GuC
>>    *
>> @@ -1538,3 +1548,56 @@ void i915_guc_register(struct drm_i915_private
>> *dev_priv)
>>       guc_log_late_setup(&dev_priv->guc);
>>       mutex_unlock(&dev_priv->drm.struct_mutex);
>>   }
>> +
>> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64
>> control_val)
>> +{
>> +    union guc_log_control log_param;
>> +    int ret;
>> +
>> +    log_param.logging_enabled = control_val & 0x1;
>> +    log_param.verbosity = (control_val >> 4) & 0xF;
>
> Maybe "log_param.value = control_val" would also work since
> guc_log_control is conveniently defined as an union. Doesn't matter though.
>
>> +
>> +    if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
>> +        log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
>> +        return -EINVAL;
>> +
>> +    /* This combination doesn't make sense & won't have any effect */
>> +    if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>> +        return 0;
>
> I wonder if it would work and maybe look nicer to generalize as:
>
>     int guc_log_level;
>
>     guc_log_level = log_param.logging_enabled ? log_param.verbosity : -1;
>     if (i915.guc_log_level == guc_log_level)
>         return 0;

Fine, will try to refactor the code as per your suggestions.
Thanks for the suggestions.

>> +
>> +    ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
>> +    if (ret < 0) {
>> +        DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    i915.guc_log_level = log_param.verbosity;
>
> This would then become i915.guc_log_level = guc_log_level.
>
>> +
>> +    /* If log_level was set as -1 at boot time, then the relay
>> channel file
>> +     * wouldn't have been created by now and interrupts also would
>> not have
>> +     * been enabled.
>> +     */
>> +    if (!dev_priv->guc.log.relay_chan) {
>> +        ret = guc_log_late_setup(&dev_priv->guc);
>> +        if (!ret)
>> +            gen9_enable_guc_interrupts(dev_priv);
>> +    } else if (!log_param.logging_enabled) {
>> +        /* Once logging is disabled, GuC won't generate logs & send an
>> +         * interrupt. But there could be some data in the log buffer
>> +         * which is yet to be captured. So request GuC to update the log
>> +         * buffer state and then collect the left over logs.
>> +         */
>> +        i915_guc_flush_logs(dev_priv);
>> +
>> +        /* GuC would have updated the log buffer by now, so capture
>> it */
>> +        i915_guc_capture_logs(dev_priv);
>> +
>> +        /* As logging is disabled, update the log level to reflect
>> that */
>> +        i915.guc_log_level = -1;
>> +    } else {
>> +        /* In case interrupts were disabled, enable them now */
>> +        gen9_enable_guc_interrupts(dev_priv);
>> +    }
>
> And this block would need some adjustments with my guc_log_level idea.
>
> Well not sure, see what you think. I am just attracted to the idea of
> operating in the same value domain as much as possible for readability
> and simplicity. Maybe it would not improve anything, I did not bother
> with typing it all to check.
>
>> +
>> +    return ret;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index d3a5447..2f8c408 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -186,5 +186,6 @@ void i915_guc_capture_logs(struct drm_i915_private
>> *dev_priv);
>>   void i915_guc_flush_logs(struct drm_i915_private *dev_priv);
>>   void i915_guc_register(struct drm_i915_private *dev_priv);
>>   void i915_guc_unregister(struct drm_i915_private *dev_priv);
>> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64
>> control_val);
>>
>>   #endif
>>
>
> Patch looks correct as is, so:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Although I would be happier though if my suggestion to use the same
> value domain as for the module parameter was used. In other words:
>
>     {"i915_guc_log_level", &i915_guc_log_control_fops}
>
> ...
>
> int i915_guc_log_control(struct drm_i915_private *dev_priv, u64
> control_val)
> ...
>     int guc_log_level = (int)control_val;
> ...
>     log_param.logging_enabled = guc_log_level > -1;
>     log_param.verbosity = guc_log_level > -1 ? guc_log_level : 0;
> ...
>
> It think it would be simpler for the user and developer to only have to
> think about one set of values when dealing with guc logging.
>
Really nice suggestion, but as you mentioned below this log control 
interface is most likely to get extended in near future.

Best regards
Akash

> But maybe extensions to guc_log_control are imminent and expected so it
> would not be worth it in the long run. No idea.
>

> Regards,
>
> Tvrtko