| Message ID | 1470983123-22127-16-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/08/16 07:25, akash.goel@intel.com wrote: > From: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > This patch provides debugfs interface i915_guc_output_control for > on the fly enabling/disabling of logging in GuC firmware and controlling > the verbosity level of logs. > The value written to the file, should have bit 0 set to enable logging and > bits 4-7 should contain the verbosity info. > > v2: Add a forceful flush, to collect left over logs, on disabling logging. > Useful for Validation. > > v3: Besides minor cleanup, implement read method for the debugfs file and > set the guc_log_level to -1 when logging is disabled. (Tvrtko) > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Signed-off-by: Akash Goel <akash.goel@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_guc_submission.c | 63 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_guc.h | 1 + > 3 files changed, 107 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 14e0dcf..f472fbcd3 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2674,6 +2674,47 @@ static int i915_guc_log_dump(struct seq_file *m, void *data) > return 0; > } > > +static int i915_guc_log_control_get(void *data, u64 *val) > +{ > + struct drm_device *dev = data; > + struct drm_i915_private *dev_priv = to_i915(dev); > + > + if (!dev_priv->guc.log.obj) > + return -EINVAL; > + > + *val = i915.guc_log_level; > + > + return 0; > +} > + > +static int i915_guc_log_control_set(void *data, u64 val) > +{ > + struct drm_device *dev = data; > + struct drm_i915_private *dev_priv = to_i915(dev); > + int ret; > + > + ret = mutex_lock_interruptible(&dev->struct_mutex); > + if (ret) > + return ret; > + > + if (!dev_priv->guc.log.obj) { > + ret = -EINVAL; > + goto end; > + } > + > + intel_runtime_pm_get(dev_priv); > + ret = i915_guc_log_control(dev_priv, val); > + intel_runtime_pm_put(dev_priv); > + > +end: > + mutex_unlock(&dev->struct_mutex); > + return ret; > +} > + > +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops, > + i915_guc_log_control_get, i915_guc_log_control_set, > + "%lld\n"); > + > static int i915_edp_psr_status(struct seq_file *m, void *data) > { > struct drm_info_node *node = m->private; > @@ -5477,7 +5518,8 @@ static const struct i915_debugfs_files { > {"i915_fbc_false_color", &i915_fbc_fc_fops}, > {"i915_dp_test_data", &i915_displayport_test_data_fops}, > {"i915_dp_test_type", &i915_displayport_test_type_fops}, > - {"i915_dp_test_active", &i915_displayport_test_active_fops} > + {"i915_dp_test_active", &i915_displayport_test_active_fops}, > + {"i915_guc_log_control", &i915_guc_log_control_fops} > }; > > void intel_display_crc_init(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 4a75c16..041cf68 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -195,6 +195,16 @@ static int host2guc_force_logbuffer_flush(struct intel_guc *guc) > return host2guc_action(guc, data, 2); > } > > +static int host2guc_logging_control(struct intel_guc *guc, u32 control_val) > +{ > + u32 data[2]; > + > + data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING; > + data[1] = control_val; > + > + return host2guc_action(guc, data, 2); > +} > + > /* > * Initialise, update, or clear doorbell data shared with the GuC > * > @@ -1538,3 +1548,56 @@ void i915_guc_register(struct drm_i915_private *dev_priv) > guc_log_late_setup(&dev_priv->guc); > mutex_unlock(&dev_priv->drm.struct_mutex); > } > + > +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) > +{ > + union guc_log_control log_param; > + int ret; > + > + log_param.logging_enabled = control_val & 0x1; > + log_param.verbosity = (control_val >> 4) & 0xF; Maybe "log_param.value = control_val" would also work since guc_log_control is conveniently defined as an union. Doesn't matter though. > + > + if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN || > + log_param.verbosity > GUC_LOG_VERBOSITY_MAX) > + return -EINVAL; > + > + /* This combination doesn't make sense & won't have any effect */ > + if (!log_param.logging_enabled && (i915.guc_log_level < 0)) > + return 0; I wonder if it would work and maybe look nicer to generalize as: int guc_log_level; guc_log_level = log_param.logging_enabled ? log_param.verbosity : -1; if (i915.guc_log_level == guc_log_level) return 0; > + > + ret = host2guc_logging_control(&dev_priv->guc, log_param.value); > + if (ret < 0) { > + DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret); > + return ret; > + } > + > + i915.guc_log_level = log_param.verbosity; This would then become i915.guc_log_level = guc_log_level. > + > + /* If log_level was set as -1 at boot time, then the relay channel file > + * wouldn't have been created by now and interrupts also would not have > + * been enabled. > + */ > + if (!dev_priv->guc.log.relay_chan) { > + ret = guc_log_late_setup(&dev_priv->guc); > + if (!ret) > + gen9_enable_guc_interrupts(dev_priv); > + } else if (!log_param.logging_enabled) { > + /* Once logging is disabled, GuC won't generate logs & send an > + * interrupt. But there could be some data in the log buffer > + * which is yet to be captured. So request GuC to update the log > + * buffer state and then collect the left over logs. > + */ > + i915_guc_flush_logs(dev_priv); > + > + /* GuC would have updated the log buffer by now, so capture it */ > + i915_guc_capture_logs(dev_priv); > + > + /* As logging is disabled, update the log level to reflect that */ > + i915.guc_log_level = -1; > + } else { > + /* In case interrupts were disabled, enable them now */ > + gen9_enable_guc_interrupts(dev_priv); > + } And this block would need some adjustments with my guc_log_level idea. Well not sure, see what you think. I am just attracted to the idea of operating in the same value domain as much as possible for readability and simplicity. Maybe it would not improve anything, I did not bother with typing it all to check. > + > + return ret; > +} > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index d3a5447..2f8c408 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -186,5 +186,6 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv); > void i915_guc_flush_logs(struct drm_i915_private *dev_priv); > void i915_guc_register(struct drm_i915_private *dev_priv); > void i915_guc_unregister(struct drm_i915_private *dev_priv); > +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val); > > #endif > Patch looks correct as is, so: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Although I would be happier though if my suggestion to use the same value domain as for the module parameter was used. In other words: {"i915_guc_log_level", &i915_guc_log_control_fops} ... int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) ... int guc_log_level = (int)control_val; ... log_param.logging_enabled = guc_log_level > -1; log_param.verbosity = guc_log_level > -1 ? guc_log_level : 0; ... It think it would be simpler for the user and developer to only have to think about one set of values when dealing with guc logging. But maybe extensions to guc_log_control are imminent and expected so it would not be worth it in the long run. No idea. Regards, Tvrtko
On 8/12/2016 9:27 PM, Tvrtko Ursulin wrote: > > On 12/08/16 07:25, akash.goel@intel.com wrote: >> From: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> >> This patch provides debugfs interface i915_guc_output_control for >> on the fly enabling/disabling of logging in GuC firmware and controlling >> the verbosity level of logs. >> The value written to the file, should have bit 0 set to enable logging >> and >> bits 4-7 should contain the verbosity info. >> >> v2: Add a forceful flush, to collect left over logs, on disabling >> logging. >> Useful for Validation. >> >> v3: Besides minor cleanup, implement read method for the debugfs file and >> set the guc_log_level to -1 when logging is disabled. (Tvrtko) >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Signed-off-by: Akash Goel <akash.goel@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 44 ++++++++++++++++++++- >> drivers/gpu/drm/i915/i915_guc_submission.c | 63 >> ++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_guc.h | 1 + >> 3 files changed, 107 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index 14e0dcf..f472fbcd3 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -2674,6 +2674,47 @@ static int i915_guc_log_dump(struct seq_file >> *m, void *data) >> return 0; >> } >> >> +static int i915_guc_log_control_get(void *data, u64 *val) >> +{ >> + struct drm_device *dev = data; >> + struct drm_i915_private *dev_priv = to_i915(dev); >> + >> + if (!dev_priv->guc.log.obj) >> + return -EINVAL; >> + >> + *val = i915.guc_log_level; >> + >> + return 0; >> +} >> + >> +static int i915_guc_log_control_set(void *data, u64 val) >> +{ >> + struct drm_device *dev = data; >> + struct drm_i915_private *dev_priv = to_i915(dev); >> + int ret; >> + >> + ret = mutex_lock_interruptible(&dev->struct_mutex); >> + if (ret) >> + return ret; >> + >> + if (!dev_priv->guc.log.obj) { >> + ret = -EINVAL; >> + goto end; >> + } >> + >> + intel_runtime_pm_get(dev_priv); >> + ret = i915_guc_log_control(dev_priv, val); >> + intel_runtime_pm_put(dev_priv); >> + >> +end: >> + mutex_unlock(&dev->struct_mutex); >> + return ret; >> +} >> + >> +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops, >> + i915_guc_log_control_get, i915_guc_log_control_set, >> + "%lld\n"); >> + >> static int i915_edp_psr_status(struct seq_file *m, void *data) >> { >> struct drm_info_node *node = m->private; >> @@ -5477,7 +5518,8 @@ static const struct i915_debugfs_files { >> {"i915_fbc_false_color", &i915_fbc_fc_fops}, >> {"i915_dp_test_data", &i915_displayport_test_data_fops}, >> {"i915_dp_test_type", &i915_displayport_test_type_fops}, >> - {"i915_dp_test_active", &i915_displayport_test_active_fops} >> + {"i915_dp_test_active", &i915_displayport_test_active_fops}, >> + {"i915_guc_log_control", &i915_guc_log_control_fops} >> }; >> >> void intel_display_crc_init(struct drm_device *dev) >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 4a75c16..041cf68 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -195,6 +195,16 @@ static int host2guc_force_logbuffer_flush(struct >> intel_guc *guc) >> return host2guc_action(guc, data, 2); >> } >> >> +static int host2guc_logging_control(struct intel_guc *guc, u32 >> control_val) >> +{ >> + u32 data[2]; >> + >> + data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING; >> + data[1] = control_val; >> + >> + return host2guc_action(guc, data, 2); >> +} >> + >> /* >> * Initialise, update, or clear doorbell data shared with the GuC >> * >> @@ -1538,3 +1548,56 @@ void i915_guc_register(struct drm_i915_private >> *dev_priv) >> guc_log_late_setup(&dev_priv->guc); >> mutex_unlock(&dev_priv->drm.struct_mutex); >> } >> + >> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 >> control_val) >> +{ >> + union guc_log_control log_param; >> + int ret; >> + >> + log_param.logging_enabled = control_val & 0x1; >> + log_param.verbosity = (control_val >> 4) & 0xF; > > Maybe "log_param.value = control_val" would also work since > guc_log_control is conveniently defined as an union. Doesn't matter though. > >> + >> + if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN || >> + log_param.verbosity > GUC_LOG_VERBOSITY_MAX) >> + return -EINVAL; >> + >> + /* This combination doesn't make sense & won't have any effect */ >> + if (!log_param.logging_enabled && (i915.guc_log_level < 0)) >> + return 0; > > I wonder if it would work and maybe look nicer to generalize as: > > int guc_log_level; > > guc_log_level = log_param.logging_enabled ? log_param.verbosity : -1; > if (i915.guc_log_level == guc_log_level) > return 0; Fine, will try to refactor the code as per your suggestions. Thanks for the suggestions. >> + >> + ret = host2guc_logging_control(&dev_priv->guc, log_param.value); >> + if (ret < 0) { >> + DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret); >> + return ret; >> + } >> + >> + i915.guc_log_level = log_param.verbosity; > > This would then become i915.guc_log_level = guc_log_level. > >> + >> + /* If log_level was set as -1 at boot time, then the relay >> channel file >> + * wouldn't have been created by now and interrupts also would >> not have >> + * been enabled. >> + */ >> + if (!dev_priv->guc.log.relay_chan) { >> + ret = guc_log_late_setup(&dev_priv->guc); >> + if (!ret) >> + gen9_enable_guc_interrupts(dev_priv); >> + } else if (!log_param.logging_enabled) { >> + /* Once logging is disabled, GuC won't generate logs & send an >> + * interrupt. But there could be some data in the log buffer >> + * which is yet to be captured. So request GuC to update the log >> + * buffer state and then collect the left over logs. >> + */ >> + i915_guc_flush_logs(dev_priv); >> + >> + /* GuC would have updated the log buffer by now, so capture >> it */ >> + i915_guc_capture_logs(dev_priv); >> + >> + /* As logging is disabled, update the log level to reflect >> that */ >> + i915.guc_log_level = -1; >> + } else { >> + /* In case interrupts were disabled, enable them now */ >> + gen9_enable_guc_interrupts(dev_priv); >> + } > > And this block would need some adjustments with my guc_log_level idea. > > Well not sure, see what you think. I am just attracted to the idea of > operating in the same value domain as much as possible for readability > and simplicity. Maybe it would not improve anything, I did not bother > with typing it all to check. > >> + >> + return ret; >> +} >> diff --git a/drivers/gpu/drm/i915/intel_guc.h >> b/drivers/gpu/drm/i915/intel_guc.h >> index d3a5447..2f8c408 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -186,5 +186,6 @@ void i915_guc_capture_logs(struct drm_i915_private >> *dev_priv); >> void i915_guc_flush_logs(struct drm_i915_private *dev_priv); >> void i915_guc_register(struct drm_i915_private *dev_priv); >> void i915_guc_unregister(struct drm_i915_private *dev_priv); >> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 >> control_val); >> >> #endif >> > > Patch looks correct as is, so: > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Although I would be happier though if my suggestion to use the same > value domain as for the module parameter was used. In other words: > > {"i915_guc_log_level", &i915_guc_log_control_fops} > > ... > > int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 > control_val) > ... > int guc_log_level = (int)control_val; > ... > log_param.logging_enabled = guc_log_level > -1; > log_param.verbosity = guc_log_level > -1 ? guc_log_level : 0; > ... > > It think it would be simpler for the user and developer to only have to > think about one set of values when dealing with guc logging. > Really nice suggestion, but as you mentioned below this log control interface is most likely to get extended in near future. Best regards Akash > But maybe extensions to guc_log_control are imminent and expected so it > would not be worth it in the long run. No idea. > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 14e0dcf..f472fbcd3 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2674,6 +2674,47 @@ static int i915_guc_log_dump(struct seq_file *m, void *data) return 0; } +static int i915_guc_log_control_get(void *data, u64 *val) +{ + struct drm_device *dev = data; + struct drm_i915_private *dev_priv = to_i915(dev); + + if (!dev_priv->guc.log.obj) + return -EINVAL; + + *val = i915.guc_log_level; + + return 0; +} + +static int i915_guc_log_control_set(void *data, u64 val) +{ + struct drm_device *dev = data; + struct drm_i915_private *dev_priv = to_i915(dev); + int ret; + + ret = mutex_lock_interruptible(&dev->struct_mutex); + if (ret) + return ret; + + if (!dev_priv->guc.log.obj) { + ret = -EINVAL; + goto end; + } + + intel_runtime_pm_get(dev_priv); + ret = i915_guc_log_control(dev_priv, val); + intel_runtime_pm_put(dev_priv); + +end: + mutex_unlock(&dev->struct_mutex); + return ret; +} + +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops, + i915_guc_log_control_get, i915_guc_log_control_set, + "%lld\n"); + static int i915_edp_psr_status(struct seq_file *m, void *data) { struct drm_info_node *node = m->private; @@ -5477,7 +5518,8 @@ static const struct i915_debugfs_files { {"i915_fbc_false_color", &i915_fbc_fc_fops}, {"i915_dp_test_data", &i915_displayport_test_data_fops}, {"i915_dp_test_type", &i915_displayport_test_type_fops}, - {"i915_dp_test_active", &i915_displayport_test_active_fops} + {"i915_dp_test_active", &i915_displayport_test_active_fops}, + {"i915_guc_log_control", &i915_guc_log_control_fops} }; void intel_display_crc_init(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 4a75c16..041cf68 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -195,6 +195,16 @@ static int host2guc_force_logbuffer_flush(struct intel_guc *guc) return host2guc_action(guc, data, 2); } +static int host2guc_logging_control(struct intel_guc *guc, u32 control_val) +{ + u32 data[2]; + + data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING; + data[1] = control_val; + + return host2guc_action(guc, data, 2); +} + /* * Initialise, update, or clear doorbell data shared with the GuC * @@ -1538,3 +1548,56 @@ void i915_guc_register(struct drm_i915_private *dev_priv) guc_log_late_setup(&dev_priv->guc); mutex_unlock(&dev_priv->drm.struct_mutex); } + +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) +{ + union guc_log_control log_param; + int ret; + + log_param.logging_enabled = control_val & 0x1; + log_param.verbosity = (control_val >> 4) & 0xF; + + if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN || + log_param.verbosity > GUC_LOG_VERBOSITY_MAX) + return -EINVAL; + + /* This combination doesn't make sense & won't have any effect */ + if (!log_param.logging_enabled && (i915.guc_log_level < 0)) + return 0; + + ret = host2guc_logging_control(&dev_priv->guc, log_param.value); + if (ret < 0) { + DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret); + return ret; + } + + i915.guc_log_level = log_param.verbosity; + + /* If log_level was set as -1 at boot time, then the relay channel file + * wouldn't have been created by now and interrupts also would not have + * been enabled. + */ + if (!dev_priv->guc.log.relay_chan) { + ret = guc_log_late_setup(&dev_priv->guc); + if (!ret) + gen9_enable_guc_interrupts(dev_priv); + } else if (!log_param.logging_enabled) { + /* Once logging is disabled, GuC won't generate logs & send an + * interrupt. But there could be some data in the log buffer + * which is yet to be captured. So request GuC to update the log + * buffer state and then collect the left over logs. + */ + i915_guc_flush_logs(dev_priv); + + /* GuC would have updated the log buffer by now, so capture it */ + i915_guc_capture_logs(dev_priv); + + /* As logging is disabled, update the log level to reflect that */ + i915.guc_log_level = -1; + } else { + /* In case interrupts were disabled, enable them now */ + gen9_enable_guc_interrupts(dev_priv); + } + + return ret; +} diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index d3a5447..2f8c408 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -186,5 +186,6 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv); void i915_guc_flush_logs(struct drm_i915_private *dev_priv); void i915_guc_register(struct drm_i915_private *dev_priv); void i915_guc_unregister(struct drm_i915_private *dev_priv); +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val); #endif