[09/17] drm/i915: Debugfs support for GuC logging control

Commit Message

[e-mail] July 10, 2016, 1:41 p.m. UTC

From: Sagar Arun Kamble <[e-mail]>

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.

Signed-off-by: Sagar Arun Kamble <[e-mail]>
Signed-off-by: Akash Goel <[e-mail]>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
 drivers/gpu/drm/i915/i915_guc_submission.c | 57 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 3 files changed, 89 insertions(+), 1 deletion(-)

Comments

kernel test robot July 10, 2016, 5:59 p.m. UTC | #1

Hi,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160708]
[cannot apply to v4.7-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/akash-goel-intel-com/Support-for-sustained-capturing-of-GuC-firmware-logs/20160710-213134
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s5-07102345 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `relay_reserve':
   i915_guc_submission.c:(.text+0x1db624): undefined reference to `relay_switch_subbuf'
   drivers/built-in.o: In function `create_buf_file_callback':
   i915_guc_submission.c:(.text+0x1db682): undefined reference to `relay_file_operations'
   drivers/built-in.o: In function `subbuf_start_callback':
   i915_guc_submission.c:(.text+0x1db69e): undefined reference to `relay_buf_full'
   drivers/built-in.o: In function `guc_log_cleanup':
   i915_guc_submission.c:(.text+0x1db792): undefined reference to `relay_close'
   drivers/built-in.o: In function `guc_log_late_setup':
>> i915_guc_submission.c:(.text+0x1db9a4): undefined reference to `relay_open'
   drivers/built-in.o: In function `i915_guc_capture_logs':
   (.text+0x1dcf44): undefined reference to `relay_flush'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Tvrtko Ursulin July 19, 2016, 11:24 a.m. UTC | #2

On 10/07/16 14:41, [e-mail] wrote:
> From: Sagar Arun Kamble <[e-mail]>
>
> 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.
>
> Signed-off-by: Sagar Arun Kamble <[e-mail]>
> Signed-off-by: Akash Goel <[e-mail]>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
>   drivers/gpu/drm/i915/i915_guc_submission.c | 57 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>   3 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5e35565..3c9c7f7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
>   	return 0;
>   }
>
> +static int
> +i915_guc_log_control_set(void *data, u64 val)
> +{
> +	struct drm_device *dev = data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;

to_i915 should be used.

> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {

Wouldn't guc.log.obj be enough?

> +		ret = -EINVAL;
> +		goto end;
> +	}
> +
> +	intel_runtime_pm_get(dev_priv);
> +	ret = i915_guc_log_control(dev, val);
> +	intel_runtime_pm_put(dev_priv);
> +
> +end:
> +	mutex_unlock(&dev->struct_mutex);
> +	return ret;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
> +			NULL, i915_guc_log_control_set,
> +			"0x%08llx\n");

Does the readback still work with no get method?

> +
>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>   {
>   	struct drm_info_node *node = m->private;
> @@ -5464,7 +5493,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 8cc31c6..2e3b723 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -193,6 +193,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
>    *
> @@ -1455,3 +1465,50 @@ void i915_guc_register(struct drm_device *dev)
>   	guc_log_late_setup(dev);
>   	mutex_unlock(&dev->struct_mutex);
>   }
> +
> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;

to_i915

Actually, function should take dev_priv if not even guc depending on the 
established convention in the file.

> +	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 -EINVAL;

Hm, disabling while already disabled - why should that return an error? 
Might be annoying in scripts.

> +
> +	ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
> +	if (ret < 0) {
> +		DRM_DEBUG_DRIVER("host2guc action failed\n");

Add ret to the log since it is easy?

> +		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);
> +		if (!ret)
> +			gen9_enable_guc_interrupts(dev_priv);

Hm, look at the above and below, do we need to create the relay channel 
if logging_enabled == false ?

> +	} 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 send the flush interrupt so that Host can
> +		 * collect the left over logs also.
> +		 */
> +		flush_work(&dev_priv->guc.events_work);
> +		host2guc_force_logbuffer_flush(&dev_priv->guc);
> +	}
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index ed773b5..d56bde6 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -178,5 +178,6 @@ void i915_guc_capture_logs(struct drm_device *dev);
>   void i915_guc_capture_logs_on_reset(struct drm_device *dev);
>   void i915_guc_register(struct drm_device *dev);
>   void i915_guc_unregister(struct drm_device *dev);
> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val);
>
>   #endif
>

Regards,

Tvrtko

[e-mail] July 20, 2016, 4:42 a.m. UTC | #3

On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
>
> On 10/07/16 14:41, [e-mail] wrote:
>> From: Sagar Arun Kamble <[e-mail]>
>>
>> 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.
>>
>> Signed-off-by: Sagar Arun Kamble <[e-mail]>
>> Signed-off-by: Akash Goel <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 57
>> ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>   3 files changed, 89 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 5e35565..3c9c7f7 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file
>> *m, void *data)
>>       return 0;
>>   }
>>
>> +static int
>> +i915_guc_log_control_set(void *data, u64 val)
>> +{
>> +    struct drm_device *dev = data;
>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>
> to_i915 should be used.
Sorry for missing this, need to use this at other places also.

>
>> +    int ret;
>> +
>> +    ret = mutex_lock_interruptible(&dev->struct_mutex);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
>
> Wouldn't guc.log.obj be enough?

Actually failure in allocation of log buffer, at boot time, is not 
considered fatal and submission through GuC is still done.
So i915.enable_guc_submission could be 1 with guc.log.obj as NULL.

>
>> +        ret = -EINVAL;
>> +        goto end;
>> +    }
>> +
>> +    intel_runtime_pm_get(dev_priv);
>> +    ret = i915_guc_log_control(dev, val);
>> +    intel_runtime_pm_put(dev_priv);
>> +
>> +end:
>> +    mutex_unlock(&dev->struct_mutex);
>> +    return ret;
>> +}
>> +
>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>> +            NULL, i915_guc_log_control_set,
>> +            "0x%08llx\n");
>
> Does the readback still work with no get method?

readback will give a 'Permission denied' error

>
>> +
>>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>>   {
>>       struct drm_info_node *node = m->private;
>> @@ -5464,7 +5493,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 8cc31c6..2e3b723 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -193,6 +193,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
>>    *
>> @@ -1455,3 +1465,50 @@ void i915_guc_register(struct drm_device *dev)
>>       guc_log_late_setup(dev);
>>       mutex_unlock(&dev->struct_mutex);
>>   }
>> +
>> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
>> +{
>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>
> to_i915
>
> Actually, function should take dev_priv if not even guc depending on the
> established convention in the file.
>
Ok for all the new logging related exported functions, will use dev_priv.

>> +    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 -EINVAL;
>
> Hm, disabling while already disabled - why should that return an error?
> Might be annoying in scripts.

Just to make the User aware. Ok will suppress this and return 0.
>
>> +
>> +    ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
>> +    if (ret < 0) {
>> +        DRM_DEBUG_DRIVER("host2guc action failed\n");
>
> Add ret to the log since it is easy?
>
fine will do that.
>> +        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);
>> +        if (!ret)
>> +            gen9_enable_guc_interrupts(dev_priv);
>
> Hm, look at the above and below, do we need to create the relay channel
> if logging_enabled == false ?

Can come here only if logging is enabled, by the virtue of above check,
        /* This combination doesn't make sense & won't have any effect */
        if (!log_param.logging_enabled && (i915.guc_log_level < 0))
	   return -EINVAL;

When guc_log_level < 0, first write on this file by User should be to 
enable logging.

Best regards
Akash

>
>> +    } 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 send the flush interrupt so that Host can
>> +         * collect the left over logs also.
>> +         */
>> +        flush_work(&dev_priv->guc.events_work);
>> +        host2guc_force_logbuffer_flush(&dev_priv->guc);
>> +    }
>> +
>> +    return ret;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index ed773b5..d56bde6 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -178,5 +178,6 @@ void i915_guc_capture_logs(struct drm_device *dev);
>>   void i915_guc_capture_logs_on_reset(struct drm_device *dev);
>>   void i915_guc_register(struct drm_device *dev);
>>   void i915_guc_unregister(struct drm_device *dev);
>> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val);
>>
>>   #endif
>>
>
> Regards,
>
> Tvrtko

Tvrtko Ursulin July 20, 2016, 9:08 a.m. UTC | #4

On 20/07/16 05:42, Goel, Akash wrote:
> On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
>>
>> On 10/07/16 14:41, [e-mail] wrote:
>>> From: Sagar Arun Kamble <[e-mail]>
>>>
>>> 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.
>>>
>>> Signed-off-by: Sagar Arun Kamble <[e-mail]>
>>> Signed-off-by: Akash Goel <[e-mail]>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 57
>>> ++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>>   3 files changed, 89 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 5e35565..3c9c7f7 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file
>>> *m, void *data)
>>>       return 0;
>>>   }
>>>
>>> +static int
>>> +i915_guc_log_control_set(void *data, u64 val)
>>> +{
>>> +    struct drm_device *dev = data;
>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> to_i915 should be used.
> Sorry for missing this, need to use this at other places also.
>
>>
>>> +    int ret;
>>> +
>>> +    ret = mutex_lock_interruptible(&dev->struct_mutex);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
>>
>> Wouldn't guc.log.obj be enough?
>
> Actually failure in allocation of log buffer, at boot time, is not
> considered fatal and submission through GuC is still done.
> So i915.enable_guc_submission could be 1 with guc.log.obj as NULL.

If guc.log.obj is NULL it will return -EINVAL without trying to create 
it here. If you intended for this function to try and create the log 
object if not already present, via i915_guc_log_control, in that case 
the condition above should only be if (!i915.enable_guc_submisison), no?

>>
>>> +        ret = -EINVAL;
>>> +        goto end;
>>> +    }
>>> +
>>> +    intel_runtime_pm_get(dev_priv);
>>> +    ret = i915_guc_log_control(dev, val);
>>> +    intel_runtime_pm_put(dev_priv);
>>> +
>>> +end:
>>> +    mutex_unlock(&dev->struct_mutex);
>>> +    return ret;
>>> +}
>>> +
>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>> +            NULL, i915_guc_log_control_set,
>>> +            "0x%08llx\n");
>>
>> Does the readback still work with no get method?
>
> readback will give a 'Permission denied' error

Is that what we want? I think it would be nice to allow read-back unless 
there is a specific reason why it shouldn't be allowed.

>>
>>> +
>>>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>>>   {
>>>       struct drm_info_node *node = m->private;
>>> @@ -5464,7 +5493,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 8cc31c6..2e3b723 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -193,6 +193,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
>>>    *
>>> @@ -1455,3 +1465,50 @@ void i915_guc_register(struct drm_device *dev)
>>>       guc_log_late_setup(dev);
>>>       mutex_unlock(&dev->struct_mutex);
>>>   }
>>> +
>>> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
>>> +{
>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> to_i915
>>
>> Actually, function should take dev_priv if not even guc depending on the
>> established convention in the file.
>>
> Ok for all the new logging related exported functions, will use dev_priv.

Or intel_guc where applicable, please look in guc code to see what is 
mostly used. There is also guc_to_i915 helper or something.

>
>>> +    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 -EINVAL;
>>
>> Hm, disabling while already disabled - why should that return an error?
>> Might be annoying in scripts.
>
> Just to make the User aware. Ok will suppress this and return 0.

Good, because it would be really annoying since you don't implement 
readback as well. For example:

echo 0x0 > guc_logging_control

= -EINVAL

"What's wrong? What's the current status?"

cat guc_logging_control

= -EACESS (or whatever)

"What?!?"

:)

>>
>>> +
>>> +    ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
>>> +    if (ret < 0) {
>>> +        DRM_DEBUG_DRIVER("host2guc action failed\n");
>>
>> Add ret to the log since it is easy?
>>
> fine will do that.
>>> +        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);
>>> +        if (!ret)
>>> +            gen9_enable_guc_interrupts(dev_priv);
>>
>> Hm, look at the above and below, do we need to create the relay channel
>> if logging_enabled == false ?
>
> Can come here only if logging is enabled, by the virtue of above check,
>         /* This combination doesn't make sense & won't have any effect */
>         if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>         return -EINVAL;
>
> When guc_log_level < 0, first write on this file by User should be to
> enable logging.

Okay just make sure that the relay channel is not created on repeated 
disabling.

Regards,

Tvrtko

[e-mail] July 20, 2016, 9:32 a.m. UTC | #5

On 7/20/2016 2:38 PM, Tvrtko Ursulin wrote:
>
> On 20/07/16 05:42, Goel, Akash wrote:
>> On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
>>>
>>> On 10/07/16 14:41, [e-mail] wrote:
>>>> From: Sagar Arun Kamble <[e-mail]>
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Sagar Arun Kamble <[e-mail]>
>>>> Signed-off-by: Akash Goel <[e-mail]>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 57
>>>> ++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>>>   3 files changed, 89 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>> index 5e35565..3c9c7f7 100644
>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>> @@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file
>>>> *m, void *data)
>>>>       return 0;
>>>>   }
>>>>
>>>> +static int
>>>> +i915_guc_log_control_set(void *data, u64 val)
>>>> +{
>>>> +    struct drm_device *dev = data;
>>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>>
>>> to_i915 should be used.
>> Sorry for missing this, need to use this at other places also.
>>
>>>
>>>> +    int ret;
>>>> +
>>>> +    ret = mutex_lock_interruptible(&dev->struct_mutex);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
>>>
>>> Wouldn't guc.log.obj be enough?
>>
>> Actually failure in allocation of log buffer, at boot time, is not
>> considered fatal and submission through GuC is still done.
>> So i915.enable_guc_submission could be 1 with guc.log.obj as NULL.
>
> If guc.log.obj is NULL it will return -EINVAL without trying to create
> it here. If you intended for this function to try and create the log
> object if not already present, via i915_guc_log_control, in that case
> the condition above should only be if (!i915.enable_guc_submisison), no?
>
If guc.log.obj is found to be NULL, we consider logging can't be enabled 
at run time. Allocation of log buffer is supposed to done
at boot time only, otherwise GuC would have to be reset & firmware to be 
reloaded to pass the log buffer address at run time, which is probably 
not desirable. That's why in the first patch decoupled the allocation of 
log buffer from log_level value.

>>>
>>>> +        ret = -EINVAL;
>>>> +        goto end;
>>>> +    }
>>>> +
>>>> +    intel_runtime_pm_get(dev_priv);
>>>> +    ret = i915_guc_log_control(dev, val);
>>>> +    intel_runtime_pm_put(dev_priv);
>>>> +
>>>> +end:
>>>> +    mutex_unlock(&dev->struct_mutex);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>>> +            NULL, i915_guc_log_control_set,
>>>> +            "0x%08llx\n");
>>>
>>> Does the readback still work with no get method?
>>
>> readback will give a 'Permission denied' error
>
> Is that what we want? I think it would be nice to allow read-back unless
> there is a specific reason why it shouldn't be allowed.
>

Ok can implement a dummy read back function but what should be 
shown/returned on read.

Should I show/return the guc_log_level value (which is also available 
from /sys/module/i915/parameters/) ?


>>>
>>>> +
>>>>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>>>>   {
>>>>       struct drm_info_node *node = m->private;
>>>> @@ -5464,7 +5493,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 8cc31c6..2e3b723 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -193,6 +193,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
>>>>    *
>>>> @@ -1455,3 +1465,50 @@ void i915_guc_register(struct drm_device *dev)
>>>>       guc_log_late_setup(dev);
>>>>       mutex_unlock(&dev->struct_mutex);
>>>>   }
>>>> +
>>>> +int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
>>>> +{
>>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>>
>>> to_i915
>>>
>>> Actually, function should take dev_priv if not even guc depending on the
>>> established convention in the file.
>>>
>> Ok for all the new logging related exported functions, will use dev_priv.
>
> Or intel_guc where applicable, please look in guc code to see what is
> mostly used. There is also guc_to_i915 helper or something.
>
for functions exported from i915_guc_submission.c file, dev_priv has 
been used.

>>
>>>> +    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 -EINVAL;
>>>
>>> Hm, disabling while already disabled - why should that return an error?
>>> Might be annoying in scripts.
>>
>> Just to make the User aware. Ok will suppress this and return 0.
>
> Good, because it would be really annoying since you don't implement
> readback as well. For example:
>
> echo 0x0 > guc_logging_control
>
> = -EINVAL
>
> "What's wrong? What's the current status?"
>
> cat guc_logging_control
>
> = -EACESS (or whatever)
>
> "What?!?"
>
> :)
>
>>>
>>>> +
>>>> +    ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
>>>> +    if (ret < 0) {
>>>> +        DRM_DEBUG_DRIVER("host2guc action failed\n");
>>>
>>> Add ret to the log since it is easy?
>>>
>> fine will do that.
>>>> +        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);
>>>> +        if (!ret)
>>>> +            gen9_enable_guc_interrupts(dev_priv);
>>>
>>> Hm, look at the above and below, do we need to create the relay channel
>>> if logging_enabled == false ?
>>
>> Can come here only if logging is enabled, by the virtue of above check,
>>         /* This combination doesn't make sense & won't have any effect */
>>         if (!log_param.logging_enabled && (i915.guc_log_level < 0))
>>         return -EINVAL;
>>
>> When guc_log_level < 0, first write on this file by User should be to
>> enable logging.
>
> Okay just make sure that the relay channel is not created on repeated
> disabling.
>
Yes channel will be created only once, when logging is enabled for first 
time.

Best regards
Akash
> Regards,
>
> Tvrtko

Tvrtko Ursulin July 20, 2016, 9:47 a.m. UTC | #6

On 20/07/16 10:32, Goel, Akash wrote:
>
>
> On 7/20/2016 2:38 PM, Tvrtko Ursulin wrote:
>>
>> On 20/07/16 05:42, Goel, Akash wrote:
>>> On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
>>>>
>>>> On 10/07/16 14:41, [e-mail] wrote:
>>>>> From: Sagar Arun Kamble <[e-mail]>
>>>>>
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Sagar Arun Kamble <[e-mail]>
>>>>> Signed-off-by: Akash Goel <[e-mail]>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
>>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 57
>>>>> ++++++++++++++++++++++++++++++
>>>>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>>>>   3 files changed, 89 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> index 5e35565..3c9c7f7 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> @@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file
>>>>> *m, void *data)
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> +static int
>>>>> +i915_guc_log_control_set(void *data, u64 val)
>>>>> +{
>>>>> +    struct drm_device *dev = data;
>>>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>>>
>>>> to_i915 should be used.
>>> Sorry for missing this, need to use this at other places also.
>>>
>>>>
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = mutex_lock_interruptible(&dev->struct_mutex);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
>>>>
>>>> Wouldn't guc.log.obj be enough?
>>>
>>> Actually failure in allocation of log buffer, at boot time, is not
>>> considered fatal and submission through GuC is still done.
>>> So i915.enable_guc_submission could be 1 with guc.log.obj as NULL.
>>
>> If guc.log.obj is NULL it will return -EINVAL without trying to create
>> it here. If you intended for this function to try and create the log
>> object if not already present, via i915_guc_log_control, in that case
>> the condition above should only be if (!i915.enable_guc_submisison), no?
>>
> If guc.log.obj is found to be NULL, we consider logging can't be enabled
> at run time. Allocation of log buffer is supposed to done
> at boot time only, otherwise GuC would have to be reset & firmware to be
> reloaded to pass the log buffer address at run time, which is probably
> not desirable. That's why in the first patch decoupled the allocation of
> log buffer from log_level value.

Okay so why then the check above shouldn't just be;

	if (!dev_priv->guc.log.obj)

as I originally suggested?

>
>>>>
>>>>> +        ret = -EINVAL;
>>>>> +        goto end;
>>>>> +    }
>>>>> +
>>>>> +    intel_runtime_pm_get(dev_priv);
>>>>> +    ret = i915_guc_log_control(dev, val);
>>>>> +    intel_runtime_pm_put(dev_priv);
>>>>> +
>>>>> +end:
>>>>> +    mutex_unlock(&dev->struct_mutex);
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>>>> +            NULL, i915_guc_log_control_set,
>>>>> +            "0x%08llx\n");
>>>>
>>>> Does the readback still work with no get method?
>>>
>>> readback will give a 'Permission denied' error
>>
>> Is that what we want? I think it would be nice to allow read-back unless
>> there is a specific reason why it shouldn't be allowed.
>>
>
> Ok can implement a dummy read back function but what should be
> shown/returned on read.
>
> Should I show/return the guc_log_level value (which is also available
> from /sys/module/i915/parameters/) ?

I would return the same value that was written in. Is the problem that 
it is not stored anywhere? Maybe reconstruct it from
i915.guc_log_level ?

Although it is not ideal that we got two formats for the same thing. 
Thinking about that, why not use the same format in debugfs as for the 
module param?

And I forgot, i915.guc_log_level == 0 is logging enabled with minimum 
verbosity?

Is it too late to change that? :)

Regards,

Tvrtko

[e-mail] July 20, 2016, 10:12 a.m. UTC | #7

On 7/20/2016 3:17 PM, Tvrtko Ursulin wrote:
>
> On 20/07/16 10:32, Goel, Akash wrote:
>>
>>
>> On 7/20/2016 2:38 PM, Tvrtko Ursulin wrote:
>>>
>>> On 20/07/16 05:42, Goel, Akash wrote:
>>>> On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 10/07/16 14:41, [e-mail] wrote:
>>>>>> From: Sagar Arun Kamble <[e-mail]>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Signed-off-by: Sagar Arun Kamble <[e-mail]>
>>>>>> Signed-off-by: Akash Goel <[e-mail]>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/i915_debugfs.c        | 32 ++++++++++++++++-
>>>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 57
>>>>>> ++++++++++++++++++++++++++++++
>>>>>>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>>>>>>   3 files changed, 89 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> index 5e35565..3c9c7f7 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>>> @@ -2644,6 +2644,35 @@ static int i915_guc_log_dump(struct seq_file
>>>>>> *m, void *data)
>>>>>>       return 0;
>>>>>>   }
>>>>>>
>>>>>> +static int
>>>>>> +i915_guc_log_control_set(void *data, u64 val)
>>>>>> +{
>>>>>> +    struct drm_device *dev = data;
>>>>>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>
>>>>> to_i915 should be used.
>>>> Sorry for missing this, need to use this at other places also.
>>>>
>>>>>
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = mutex_lock_interruptible(&dev->struct_mutex);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
>>>>>
>>>>> Wouldn't guc.log.obj be enough?
>>>>
>>>> Actually failure in allocation of log buffer, at boot time, is not
>>>> considered fatal and submission through GuC is still done.
>>>> So i915.enable_guc_submission could be 1 with guc.log.obj as NULL.
>>>
>>> If guc.log.obj is NULL it will return -EINVAL without trying to create
>>> it here. If you intended for this function to try and create the log
>>> object if not already present, via i915_guc_log_control, in that case
>>> the condition above should only be if (!i915.enable_guc_submisison), no?
>>>
>> If guc.log.obj is found to be NULL, we consider logging can't be enabled
>> at run time. Allocation of log buffer is supposed to done
>> at boot time only, otherwise GuC would have to be reset & firmware to be
>> reloaded to pass the log buffer address at run time, which is probably
>> not desirable. That's why in the first patch decoupled the allocation of
>> log buffer from log_level value.
>
> Okay so why then the check above shouldn't just be;
>
>     if (!dev_priv->guc.log.obj)
>
> as I originally suggested?

Right, so sorry got confused, I misread & interpreted that you are 
suggesting to have !i915.enable_guc_submission check instead.

(!dev_priv->guc.log.obj) check should suffice.

>
>>
>>>>>
>>>>>> +        ret = -EINVAL;
>>>>>> +        goto end;
>>>>>> +    }
>>>>>> +
>>>>>> +    intel_runtime_pm_get(dev_priv);
>>>>>> +    ret = i915_guc_log_control(dev, val);
>>>>>> +    intel_runtime_pm_put(dev_priv);
>>>>>> +
>>>>>> +end:
>>>>>> +    mutex_unlock(&dev->struct_mutex);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>>>>> +            NULL, i915_guc_log_control_set,
>>>>>> +            "0x%08llx\n");
>>>>>
>>>>> Does the readback still work with no get method?
>>>>
>>>> readback will give a 'Permission denied' error
>>>
>>> Is that what we want? I think it would be nice to allow read-back unless
>>> there is a specific reason why it shouldn't be allowed.
>>>
>>
>> Ok can implement a dummy read back function but what should be
>> shown/returned on read.
>>
>> Should I show/return the guc_log_level value (which is also available
>> from /sys/module/i915/parameters/) ?
>
> I would return the same value that was written in. Is the problem that
> it is not stored anywhere? Maybe reconstruct it from
> i915.guc_log_level ?
>

The verbosity value will be same as guc_log_level. But whether logging 
on GuC side is currently enabled or disabled can't be inferred (it could 
have been disabled at run time).
So will have to store the exact value written by User.

> Although it is not ideal that we got two formats for the same thing.
> Thinking about that, why not use the same format in debugfs as for the
> module param?
>
> And I forgot, i915.guc_log_level == 0 is logging enabled with minimum
> verbosity?
>
i915.guc_log_level == 0 just indicates the minimum verbosity. But 
logging could still be disabled on GuC side.

For example, Driver boots with 'i915.guc_log_level = 0' so logging is 
enabled, later User disables the logging by echoing 0x0 on the 
guc_log_control debugfs file.

Best regards
Akash

> Is it too late to change that? :)
>
> Regards,
>
> Tvrtko
>

Tvrtko Ursulin July 20, 2016, 10:40 a.m. UTC | #8

On 20/07/16 11:12, Goel, Akash wrote:
> On 7/20/2016 3:17 PM, Tvrtko Ursulin wrote:
>>>>>>> +        ret = -EINVAL;
>>>>>>> +        goto end;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    intel_runtime_pm_get(dev_priv);
>>>>>>> +    ret = i915_guc_log_control(dev, val);
>>>>>>> +    intel_runtime_pm_put(dev_priv);
>>>>>>> +
>>>>>>> +end:
>>>>>>> +    mutex_unlock(&dev->struct_mutex);
>>>>>>> +    return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>>>>>> +            NULL, i915_guc_log_control_set,
>>>>>>> +            "0x%08llx\n");
>>>>>>
>>>>>> Does the readback still work with no get method?
>>>>>
>>>>> readback will give a 'Permission denied' error
>>>>
>>>> Is that what we want? I think it would be nice to allow read-back
>>>> unless
>>>> there is a specific reason why it shouldn't be allowed.
>>>>
>>>
>>> Ok can implement a dummy read back function but what should be
>>> shown/returned on read.
>>>
>>> Should I show/return the guc_log_level value (which is also available
>>> from /sys/module/i915/parameters/) ?
>>
>> I would return the same value that was written in. Is the problem that
>> it is not stored anywhere? Maybe reconstruct it from
>> i915.guc_log_level ?
>>
>
> The verbosity value will be same as guc_log_level. But whether logging
> on GuC side is currently enabled or disabled can't be inferred (it could
> have been disabled at run time).
> So will have to store the exact value written by User.

That's what I meant. Code currently seem to decompose the value written 
via debugfs and store it in i915.guc_log_level:

0x00 = -1
0x10 = -1
...
0x01 = 0
0x11 = 1
0x21 = 2
0x31 = 3
...

So for readback you could translate back from i915.guc_log_level to the 
debugfs format.

Although I have suggested below even more...

>> Although it is not ideal that we got two formats for the same thing.
>> Thinking about that, why not use the same format in debugfs as for the
>> module param?

... that why do we have to have two formats? Isn't that a bit confusing?

Why couldn't we use the same integer values from i915.guc_log_level for 
debugfs control ?

>>
>> And I forgot, i915.guc_log_level == 0 is logging enabled with minimum
>> verbosity?
>>
> i915.guc_log_level == 0 just indicates the minimum verbosity. But
> logging could still be disabled on GuC side.

Yes, I can't remember any precedent where zero means enabled so it is 
just weird. But it is too late to change it now. :(

> For example, Driver boots with 'i915.guc_log_level = 0' so logging is
> enabled, later User disables the logging by echoing 0x0 on the
> guc_log_control debugfs file.

That's fine

Regards,

Tvrtko

[e-mail] July 20, 2016, 11:29 a.m. UTC | #9

On 7/20/2016 4:10 PM, Tvrtko Ursulin wrote:
>
> On 20/07/16 11:12, Goel, Akash wrote:
>> On 7/20/2016 3:17 PM, Tvrtko Ursulin wrote:
>>>>>>>> +        ret = -EINVAL;
>>>>>>>> +        goto end;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    intel_runtime_pm_get(dev_priv);
>>>>>>>> +    ret = i915_guc_log_control(dev, val);
>>>>>>>> +    intel_runtime_pm_put(dev_priv);
>>>>>>>> +
>>>>>>>> +end:
>>>>>>>> +    mutex_unlock(&dev->struct_mutex);
>>>>>>>> +    return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>>>>>>> +            NULL, i915_guc_log_control_set,
>>>>>>>> +            "0x%08llx\n");
>>>>>>>
>>>>>>> Does the readback still work with no get method?
>>>>>>
>>>>>> readback will give a 'Permission denied' error
>>>>>
>>>>> Is that what we want? I think it would be nice to allow read-back
>>>>> unless
>>>>> there is a specific reason why it shouldn't be allowed.
>>>>>
>>>>
>>>> Ok can implement a dummy read back function but what should be
>>>> shown/returned on read.
>>>>
>>>> Should I show/return the guc_log_level value (which is also available
>>>> from /sys/module/i915/parameters/) ?
>>>
>>> I would return the same value that was written in. Is the problem that
>>> it is not stored anywhere? Maybe reconstruct it from
>>> i915.guc_log_level ?
>>>
>>
>> The verbosity value will be same as guc_log_level. But whether logging
>> on GuC side is currently enabled or disabled can't be inferred (it could
>> have been disabled at run time).
>> So will have to store the exact value written by User.
>
> That's what I meant. Code currently seem to decompose the value written
> via debugfs and store it in i915.guc_log_level:
>
> 0x00 = -1
> 0x10 = -1
> ...
> 0x01 = 0
> 0x11 = 1
> 0x21 = 2
> 0x31 = 3
> ...
>
> So for readback you could translate back from i915.guc_log_level to the
> debugfs format.
>
Sorry for all the mess.

Should I add a new field 'debugfs_ctrl_val' in guc structure, to store 
the value previously written to debugfs file, considering guc_log_level 
only gives an indication of the verbosity level ?

Actually in future there may be other additions also to the value 
written to guc_log_control debugfs, have right now exposed only logging 
& verbosity level controls to User, as they are deemed most useful right 
now.
But there are some other controls also which can be passed to GuC 
firmware through UK_LOG_ENABLE_LOGGING host2guc action.

Best regards
Akash

> Although I have suggested below even more...
>
>>> Although it is not ideal that we got two formats for the same thing.
>>> Thinking about that, why not use the same format in debugfs as for the
>>> module param?
>
> ... that why do we have to have two formats? Isn't that a bit confusing?
>
> Why couldn't we use the same integer values from i915.guc_log_level for
> debugfs control ?
>

>>>
>>> And I forgot, i915.guc_log_level == 0 is logging enabled with minimum
>>> verbosity?
>>>
>> i915.guc_log_level == 0 just indicates the minimum verbosity. But
>> logging could still be disabled on GuC side.
>
> Yes, I can't remember any precedent where zero means enabled so it is
> just weird. But it is too late to change it now. :(
>
>> For example, Driver boots with 'i915.guc_log_level = 0' so logging is
>> enabled, later User disables the logging by echoing 0x0 on the
>> guc_log_control debugfs file.
>
> That's fine
>
> Regards,
>
> Tvrtko

Tvrtko Ursulin July 20, 2016, 11:50 a.m. UTC | #10

On 20/07/16 12:29, Goel, Akash wrote:
> On 7/20/2016 4:10 PM, Tvrtko Ursulin wrote:
>> On 20/07/16 11:12, Goel, Akash wrote:
>>> On 7/20/2016 3:17 PM, Tvrtko Ursulin wrote:
>>>>>>>>> +        ret = -EINVAL;
>>>>>>>>> +        goto end;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    intel_runtime_pm_get(dev_priv);
>>>>>>>>> +    ret = i915_guc_log_control(dev, val);
>>>>>>>>> +    intel_runtime_pm_put(dev_priv);
>>>>>>>>> +
>>>>>>>>> +end:
>>>>>>>>> +    mutex_unlock(&dev->struct_mutex);
>>>>>>>>> +    return ret;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>>>>>>>> +            NULL, i915_guc_log_control_set,
>>>>>>>>> +            "0x%08llx\n");
>>>>>>>>
>>>>>>>> Does the readback still work with no get method?
>>>>>>>
>>>>>>> readback will give a 'Permission denied' error
>>>>>>
>>>>>> Is that what we want? I think it would be nice to allow read-back
>>>>>> unless
>>>>>> there is a specific reason why it shouldn't be allowed.
>>>>>>
>>>>>
>>>>> Ok can implement a dummy read back function but what should be
>>>>> shown/returned on read.
>>>>>
>>>>> Should I show/return the guc_log_level value (which is also available
>>>>> from /sys/module/i915/parameters/) ?
>>>>
>>>> I would return the same value that was written in. Is the problem that
>>>> it is not stored anywhere? Maybe reconstruct it from
>>>> i915.guc_log_level ?
>>>>
>>>
>>> The verbosity value will be same as guc_log_level. But whether logging
>>> on GuC side is currently enabled or disabled can't be inferred (it could
>>> have been disabled at run time).
>>> So will have to store the exact value written by User.
>>
>> That's what I meant. Code currently seem to decompose the value written
>> via debugfs and store it in i915.guc_log_level:
>>
>> 0x00 = -1
>> 0x10 = -1
>> ...
>> 0x01 = 0
>> 0x11 = 1
>> 0x21 = 2
>> 0x31 = 3
>> ...
>>
>> So for readback you could translate back from i915.guc_log_level to the
>> debugfs format.
>>
> Sorry for all the mess.
>
> Should I add a new field 'debugfs_ctrl_val' in guc structure, to store
> the value previously written to debugfs file, considering guc_log_level
> only gives an indication of the verbosity level ?
>
> Actually in future there may be other additions also to the value
> written to guc_log_control debugfs, have right now exposed only logging
> & verbosity level controls to User, as they are deemed most useful right
> now.
> But there are some other controls also which can be passed to GuC
> firmware through UK_LOG_ENABLE_LOGGING host2guc action.

I see.

Would it work, for time being at least, to set i915.guc_log_level to -1 
when logging is disabled via debugfs?

It think that also has the advantage of making the current guc logging 
state consistent when observed from the outside. Otherwise the debugfs 
value and module parameter may disagree on it, as you said before. Which 
is not that great I think.

Apart from making the reported stated consistent, that way you could, at 
least for the time being, get away without storing a copy of 
guc_log_control but reconstruct it from the module parameter on read-back.

Regards,

Tvrtko



You could avoid storing a copy of guc_log_control like that.

[e-mail] July 20, 2016, 12:16 p.m. UTC | #11

On 7/20/2016 5:20 PM, Tvrtko Ursulin wrote:
>
> On 20/07/16 12:29, Goel, Akash wrote:
>> On 7/20/2016 4:10 PM, Tvrtko Ursulin wrote:
>>> On 20/07/16 11:12, Goel, Akash wrote:
>>>> On 7/20/2016 3:17 PM, Tvrtko Ursulin wrote:

>>>>>>>>>> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
>>>>>>>>>> +            NULL, i915_guc_log_control_set,
>>>>>>>>>> +            "0x%08llx\n");
>>>>>>>>>
>>>>>>>>> Does the readback still work with no get method?
>>>>>>>>
>>>>>>>> readback will give a 'Permission denied' error
>>>>>>>
>>>>>>> Is that what we want? I think it would be nice to allow read-back
>>>>>>> unless
>>>>>>> there is a specific reason why it shouldn't be allowed.
>>>>>>>
>>>>>>
>>>>>> Ok can implement a dummy read back function but what should be
>>>>>> shown/returned on read.
>>>>>>
>>>>>> Should I show/return the guc_log_level value (which is also available
>>>>>> from /sys/module/i915/parameters/) ?
>>>>>
>>>>> I would return the same value that was written in. Is the problem that
>>>>> it is not stored anywhere? Maybe reconstruct it from
>>>>> i915.guc_log_level ?
>>>>>
>>>>
>>>> The verbosity value will be same as guc_log_level. But whether logging
>>>> on GuC side is currently enabled or disabled can't be inferred (it
>>>> could
>>>> have been disabled at run time).
>>>> So will have to store the exact value written by User.
>>>
>>> That's what I meant. Code currently seem to decompose the value written
>>> via debugfs and store it in i915.guc_log_level:
>>>
>>> 0x00 = -1
>>> 0x10 = -1
>>> ...
>>> 0x01 = 0
>>> 0x11 = 1
>>> 0x21 = 2
>>> 0x31 = 3
>>> ...
>>>
>>> So for readback you could translate back from i915.guc_log_level to the
>>> debugfs format.
>>>
>> Sorry for all the mess.
>>
>> Should I add a new field 'debugfs_ctrl_val' in guc structure, to store
>> the value previously written to debugfs file, considering guc_log_level
>> only gives an indication of the verbosity level ?
>>
>> Actually in future there may be other additions also to the value
>> written to guc_log_control debugfs, have right now exposed only logging
>> & verbosity level controls to User, as they are deemed most useful right
>> now.
>> But there are some other controls also which can be passed to GuC
>> firmware through UK_LOG_ENABLE_LOGGING host2guc action.
>
> I see.
>
> Would it work, for time being at least, to set i915.guc_log_level to -1
> when logging is disabled via debugfs?
>
Actually had thought about this, but didn't pursue since on doing so 
will have to adjust some of the guc_log_level related asserts/ conditions.
Will do it now as currently this looks to be the best alternative.
Thanks much for the inputs.

Best regards
Akash

> It think that also has the advantage of making the current guc logging
> state consistent when observed from the outside. Otherwise the debugfs
> value and module parameter may disagree on it, as you said before. Which
> is not that great I think.
>
> Apart from making the reported stated consistent, that way you could, at
> least for the time being, get away without storing a copy of
> guc_log_control but reconstruct it from the module parameter on read-back.
>
> Regards,
>
> Tvrtko
>
>
>
> You could avoid storing a copy of guc_log_control like that.
>
>
>

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5e35565..3c9c7f7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2644,6 +2644,35 @@  static int i915_guc_log_dump(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int
+i915_guc_log_control_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	if (!i915.enable_guc_submission || !dev_priv->guc.log.obj) {
+		ret = -EINVAL;
+		goto end;
+	}
+
+	intel_runtime_pm_get(dev_priv);
+	ret = i915_guc_log_control(dev, val);
+	intel_runtime_pm_put(dev_priv);
+
+end:
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
+			NULL, i915_guc_log_control_set,
+			"0x%08llx\n");
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -5464,7 +5493,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 8cc31c6..2e3b723 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -193,6 +193,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
  *
@@ -1455,3 +1465,50 @@  void i915_guc_register(struct drm_device *dev)
 	guc_log_late_setup(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
+
+int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	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 -EINVAL;
+
+	ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
+	if (ret < 0) {
+		DRM_DEBUG_DRIVER("host2guc action failed\n");
+		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);
+		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 send the flush interrupt so that Host can
+		 * collect the left over logs also.
+		 */
+		flush_work(&dev_priv->guc.events_work);
+		host2guc_force_logbuffer_flush(&dev_priv->guc);
+	}
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index ed773b5..d56bde6 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -178,5 +178,6 @@  void i915_guc_capture_logs(struct drm_device *dev);
 void i915_guc_capture_logs_on_reset(struct drm_device *dev);
 void i915_guc_register(struct drm_device *dev);
 void i915_guc_unregister(struct drm_device *dev);
+int i915_guc_log_control(struct drm_device *dev, uint64_t control_val);
 
 #endif