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@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Signed-off-by: Robert M. Fosha <robert.m.fosha@xxxxxxxxx>
---
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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
