Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
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
Signed-off-by: Akash Goel
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
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
Signed-off-by: Akash Goel
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
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
Signed-off-by: Akash Goel
---
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
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
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
Signed-off-by: Akash Goel
---
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
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
On 7/19/2016 4:54 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
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
Signed-off-by: Akash Goel
---
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, th
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Sagar Arun Kamble
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
Signed-off-by: Akash Goel
---
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
Re: [Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
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 infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/17] drm/i915: Debugfs support for GuC logging control
From: Sagar Arun Kamble
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
Signed-off-by: Akash Goel
---
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;
+ 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
