| Message ID | 20190910224603.23164-1-robert.m.fosha@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/i915/guc: Enable guc logging on guc log relay write | expand |
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@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Robert M. Fosha <robert.m.fosha@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: 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. > } > > 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 > } > > static int i915_guc_log_relay_release(struct inode *inode, struct file *file) >
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@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Signed-off-by: Robert M. Fosha <robert.m.fosha@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) >>
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@intel.com> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Signed-off-by: Robert M. Fosha <robert.m.fosha@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) >>>
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 { + 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); + + log->relay_started = false; + } + + 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; } 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; 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); 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; + + /* + * 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; } static int i915_guc_log_relay_release(struct inode *inode, struct file *file)
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@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Robert M. Fosha <robert.m.fosha@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(-)