[Intel-gfx] [RFC] drm/i915/guc: Enable guc logging on guc log relay write
Fosha, Robert M
robert.m.fosha at intel.com
Wed Sep 11 21:28:01 UTC 2019
On 9/10/19 5:48 PM, Daniele Ceraolo Spurio wrote:
>
>
> On 9/10/19 3:46 PM, Robert M. Fosha wrote:
>> Creating and opening the GuC log relay file enables and starts
>> the relay potentially before the caller is ready to consume logs.
>> Change the behavior so that relay starts only on an explicit call
>> to the write function (with a value of '1'). Other values flush
>> the log relay as before.
>>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Signed-off-by: Robert M. Fosha <robert.m.fosha at intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 38 +++++++++++++++++-----
>> drivers/gpu/drm/i915/gt/uc/intel_guc_log.h | 2 ++
>> drivers/gpu/drm/i915/i915_debugfs.c | 27 +++++++++++++--
>> 3 files changed, 56 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>> index 36332064de9c..9a98270d05b6 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
>> @@ -361,6 +361,7 @@ void intel_guc_log_init_early(struct
>> intel_guc_log *log)
>> {
>> mutex_init(&log->relay.lock);
>> INIT_WORK(&log->relay.flush_work, capture_logs_work);
>> + log->relay_started = false;
>> }
>> static int guc_log_relay_create(struct intel_guc_log *log)
>> @@ -585,15 +586,6 @@ int intel_guc_log_relay_open(struct
>> intel_guc_log *log)
>> mutex_unlock(&log->relay.lock);
>> - guc_log_enable_flush_events(log);
>> -
>> - /*
>> - * When GuC is logging without us relaying to userspace, we're
>> ignoring
>> - * the flush notification. This means that we need to
>> unconditionally
>> - * flush on relay enabling, since GuC only notifies us once.
>> - */
>> - queue_work(system_highpri_wq, &log->relay.flush_work);
>> -
>> return 0;
>> out_relay:
>> @@ -604,12 +596,38 @@ int intel_guc_log_relay_open(struct
>> intel_guc_log *log)
>> return ret;
>> }
>> +int intel_guc_log_relay_start(struct intel_guc_log *log)
>> +{
>> + int ret = 0;
>> +
>> + if (log->relay_started) {
>> + ret = -EEXIST;
>> + } else {
>
> style: for this kind of checks, we usually just return early instead
> of using an if-else, i.e.:
>
> if (log->relay_started)
> return -EEXIST;
>
> [...] /* code */
>
> return 0;
>
>> + guc_log_enable_flush_events(log);
>> +
>> + /*
>> + * When GuC is logging without us relaying to userspace, we're
>> + * ignoring the flush notification. This means that we need to
>> + * unconditionally * flush on relay enabling, since GuC only
>
> stray "*"
>
>> + * notifies us once.
>> + */
>> + queue_work(system_highpri_wq, &log->relay.flush_work);
>> +
>> + log->relay_started = false;
>
> s/false/true/
>
>> + }
>> +
>> + return ret;
>> +}
>> +
>> void intel_guc_log_relay_flush(struct intel_guc_log *log)
>> {
>> struct intel_guc *guc = log_to_guc(log);
>> struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>> intel_wakeref_t wakeref;
>> + if (!log->relay_started)
>> + return;
>> +
>> /*
>> * Before initiating the forceful flush, wait for any
>> pending/ongoing
>> * flush to complete otherwise forceful flush may not actually
>> happen.
>> @@ -638,6 +656,8 @@ void intel_guc_log_relay_close(struct
>> intel_guc_log *log)
>> guc_log_unmap(log);
>> guc_log_relay_destroy(log);
>> mutex_unlock(&log->relay.lock);
>> +
>> + log->relay_started = false;
>
>
> For symmetry, it might be worth adding a guc_log_relay_stop:
Should it be intel_guc_log_relay_stop to be consistent with naming of
other functions?
>
> static void guc_log_relay_stop(...)
> {
> if (!log->relay_started)
> return;
>
> guc_log_disable_flush_events(log);
> intel_synchronize_irq(i915);
>
> flush_work(&log->relay.flush_work);
>
> log->relay_started = false;
> }
>
> and call it from intel_guc_log_relay_close().
>
> Also, it should be impossible to race the start/flush with the stop
> because the relay_write can't race the relay_close, but it might be
> worth a comment in case we decide to have guc_log_relay_stop()
> callable from the debugfs in the future.
How about this comment just above the relay_stop function:
/*
* Stops the relay log. Called from intel_guc_log_relay_close(), so no
* possibility of race with start/flush since relay_write cannot race
* relay_close.
*/
>
>> }
>> void intel_guc_log_handle_flush_event(struct intel_guc_log *log)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
>> index 6f764879acb1..ecf7a49416b4 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
>> @@ -44,6 +44,7 @@ struct intel_guc;
>> struct intel_guc_log {
>> u32 level;
>> + bool relay_started;
>
> this should move inside the relay structure below. Just "started" will
> be enough as a name at that point because the structure is already
> called relay.
>
>> struct i915_vma *vma;
>> struct {
>> void *buf_addr;
>> @@ -67,6 +68,7 @@ void intel_guc_log_destroy(struct intel_guc_log *log);
>> int intel_guc_log_set_level(struct intel_guc_log *log, u32 level);
>> bool intel_guc_log_relay_enabled(const struct intel_guc_log *log);
>
> intel_guc_log_relay_enabled() implied both created and started before,
> but now that you're splitting that in 2 separate states you'll need to
> account for that. From a quick look, all the callers seem to just care
> about the buffers being allocated, so a rename should be enough.
>
>> int intel_guc_log_relay_open(struct intel_guc_log *log);
>> +int intel_guc_log_relay_start(struct intel_guc_log *log);
>> void intel_guc_log_relay_flush(struct intel_guc_log *log);
>> void intel_guc_log_relay_close(struct intel_guc_log *log);
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 708855e051b5..c3683fdd0deb 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2049,9 +2049,32 @@ i915_guc_log_relay_write(struct file *filp,
>> loff_t *ppos)
>> {
>> struct intel_guc_log *log = filp->private_data;
>> + char *input_buffer;
>> + int val;
>> + int ret;
>> - intel_guc_log_relay_flush(log);
>> - return cnt;
>> + input_buffer = memdup_user_nul(ubuf, cnt);
>> + if (IS_ERR(input_buffer))
>> + return PTR_ERR(input_buffer);
>> +
>> + ret = kstrtoint(input_buffer, 10, &val);
>> + if (ret < 0)
>> + goto out;
>> +
>
> you can use kstrtoint_from_user to convert user input to int directly,
> without having to copy to a temp buf.
>
>> + /*
>> + * Enable and start the guc log relay on value of 1.
>> + * Flush log relay for any other value.
>> + */
>> + if (val == 1) {
>> + ret = intel_guc_log_relay_start(log);
>> + } else {
>> + intel_guc_log_relay_flush(log);
>> + ret = cnt;
>> + }
>> +
>> +out:
>> + kfree(input_buffer);
>> + return ret;
>
> Probably better to always return cnt in case of success, i.e:
>
> return ret ?: cnt;
>
> And remove the ret = cnt above.
>
> Daniele
>
Thanks for the feedback. Will include all the recommended fixes/changes.
>> }
>> static int i915_guc_log_relay_release(struct inode *inode, struct
>> file *file)
>>
More information about the Intel-gfx
mailing list