On 17/08/16 12:24, Goel, Akash wrote:
On 8/17/2016 4:37 PM, Tvrtko Ursulin wrote:
On 17/08/16 11:14, akash.goel@xxxxxxxxx wrote:
From: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
GuC ukernel sends an interrupt to Host to flush the log buffer
and expects Host to correspondingly update the read pointer
information in the state structure, once it has consumed the
log buffer contents by copying them to a file or buffer.
Even if Host couldn't copy the contents, it can still update the
read pointer so that logging state is not disturbed on GuC side.
v2:
- Use a dedicated workqueue for handling flush interrupt. (Tvrtko)
- Reduce the overall log buffer copying time by skipping the copy of
crash buffer area for regular cases and copying only the state
structure data in first page.
v3:
- Create a vmalloc mapping of log buffer. (Chris)
- Cover the flush acknowledgment under rpm get & put.(Chris)
- Revert the change of skipping the copy of crash dump area, as
not really needed, will be covered by subsequent patch.
v4:
- Destroy the wq under the same condition in which it was created,
pass dev_piv pointer instead of dev to newly added GuC function,
add more comments & rename variable for clarity. (Tvrtko)
v5:
- Allocate & destroy the dedicated wq, for handling flush interrupt,
from the setup/teardown routines of GuC logging. (Chris)
- Validate the log buffer size value retrieved from state structure
and do some minor cleanup. (Tvrtko)
- Fix error/warnings reported by checkpatch. (Tvrtko)
- Rebase.
v6:
- Remove the interrupts_enabled check from guc_capture_logs_work, need
to process that last work item also, queued just before disabling
the
interrupt as log buffer flush interrupt handling is a bit different
case where GuC is actually expecting an ACK from host, which
should be
provided to keep the logging going.
Sync against the work will be done by caller disabling the
interrupt.
- Don't sample the log buffer size value from state structure,
directly
use the expected value to move the pointer & do the copy and that
cannot
go wrong (out of bounds) as Driver only allocated the log buffer
and the
relay buffers. Driver should refrain from interpreting the log
packet,
as much possible and let Userspace parser detect the anomaly.
(Chris)
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 186
+++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_irq.c | 28 ++++-
drivers/gpu/drm/i915/intel_guc.h | 4 +
3 files changed, 217 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index b062da6..ade51cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -172,6 +172,15 @@ static int host2guc_sample_forcewake(struct
intel_guc *guc,
return host2guc_action(guc, data, ARRAY_SIZE(data));
}
+static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
+{
+ u32 data[1];
+
+ data[0] = HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE;
+
+ return host2guc_action(guc, data, 1);
+}
+
/*
* Initialise, update, or clear doorbell data shared with the GuC
*
@@ -828,6 +837,163 @@ err:
return NULL;
}
+static void guc_move_to_next_buf(struct intel_guc *guc)
+{
+}
+
+static void *guc_get_write_buffer(struct intel_guc *guc)
+{
+ return NULL;
+}
+
+static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type
type)
+{
+ if (type == GUC_ISR_LOG_BUFFER)
+ return (GUC_LOG_ISR_PAGES + 1) * PAGE_SIZE;
+ else if (type == GUC_DPC_LOG_BUFFER)
+ return (GUC_LOG_DPC_PAGES + 1) * PAGE_SIZE;
+ else
+ return (GUC_LOG_CRASH_PAGES + 1) * PAGE_SIZE;
+}
Could do it with a switch statement to get automatic reminder of size
not being handled if some day a new log buffer type gets added. It would
probably more in the style of the rest of the driver as well.
Fine will use switch statement here.
Should I use BUG_ON for the default/unhandled case ?
case GUC_ISR_LOG_BUFFER
case GUC_DPC_LOG_BUFFER
case GUC_LOG_CRASH_PAGES
default
BUG_ON(1)
You could use MISSING_CASE, but I think compiler should complain as
well. Suppose MISSING_CASE is safer.
+
+static void guc_read_update_log_buffer(struct intel_guc *guc)
+{
+ struct guc_log_buffer_state *log_buffer_state,
*log_buffer_snapshot_state;
+ struct guc_log_buffer_state log_buffer_state_local;
+ void *src_data_ptr, *dst_data_ptr;
+ unsigned int buffer_size;
+ enum guc_log_buffer_type type;
+
+ if (WARN_ON(!guc->log.buf_addr))
+ return;
+
+ /* Get the pointer to shared GuC log buffer */
+ log_buffer_state = src_data_ptr = guc->log.buf_addr;
+
+ /* Get the pointer to local buffer to store the logs */
+ dst_data_ptr = log_buffer_snapshot_state =
guc_get_write_buffer(guc);
+
+ /* Actual logs are present from the 2nd page */
+ src_data_ptr += PAGE_SIZE;
+ dst_data_ptr += PAGE_SIZE;
+
+ for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER;
type++) {
+ /* Make a copy of the state structure in GuC log buffer (which
+ * is uncached mapped) on the stack to avoid reading from it
+ * multiple times.
+ */
+ memcpy(&log_buffer_state_local, log_buffer_state,
+ sizeof(struct guc_log_buffer_state));
+ buffer_size = guc_get_log_buffer_size(type);
+
+ if (log_buffer_snapshot_state) {
+ /* First copy the state structure in snapshot buffer */
+ memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
+ sizeof(struct guc_log_buffer_state));
I've noticed now log_buffer_state_local is used only for the
sample_write_ptr below and otherwise it causes two copies of the same
data.
Actually log_buffer_state_local gets used more in the subsequent
patches, where we do the bookkeeping and optimize the copying.
[PATCH 10/19] drm/i915: Add stats for GuC log buffer flush interrupts
[PATCH 11/19] drm/i915: Optimization to reduce the sampling time of GuC
log buffer
Since this branch is an interesting one, you could avoid one copy by
updating the log_buffer_state->read_ptr from the
log_buffer_snapshot_state in this branch, and add an else branch to
update it directly from log_buffer_state when log_buffer_snapshot_state
is not available.
Sounds like it would be worth eliminating double memcpy, what do you
think>
Won't the 2nd memcpy (from the copy on stack to the relay buffer) be
really fast ?
The copy on stack (16 bytes) will most likely be in the CPU cache and
the same area is used for all 3 buffer types.
Yes I realized later you use it more in later patches.
I don't think it is slow but was just wondering if it could be made
tidier by getting rid of one copy.
Would have to apply the series to see how the final loop looks like, but
would something like the below be possible:
if (!log_buffer_snapshot_state) {
.. read_ptr update from wc ..
continue;
}
memcpy from wc to final buffer
.. the rest of processing you do reading from the copy ..
?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
