[Intel-gfx] [RFC] drm/i915/guc: Enable guc logging on guc log relay write
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Thu Sep 12 22:13:49 UTC 2019
On 9/11/19 2:28 PM, Fosha, Robert M wrote:
>
> 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?
intel_* prefix is usually only used for non-static 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.
> */
>
LGTM.
Daniele
>>
>>> }
>>> 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