On 8/15/2016 9:06 PM, Tvrtko Ursulin wrote:
On 15/08/16 15:49, akash.goel@xxxxxxxxx wrote:
From: Akash Goel <akash.goel@xxxxxxxxx>
GuC firmware sends an interrupt to flush the log buffer when it becomes
half full, so Driver doesn't really need to sample the complete buffer
and can just copy only the newly written data by GuC into the local
buffer, i.e. as per the read & write pointer values.
Moreover the flush interrupt would generally come for one type of log
buffer, when it becomes half full, so at that time the other 2 types of
log buffer would comparatively have much lesser unread data in them.
In case of overflow reported by GuC, Driver do need to copy the entire
buffer as the whole buffer would contain the unread data.
v2: Rebase.
v3: Fix the blooper of doing the copy twice. (Tvrtko)
Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 40
+++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index c7b4a57..b8d6313 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1003,6 +1003,8 @@ static void guc_read_update_log_buffer(struct
intel_guc *guc)
void *src_data_ptr, *dst_data_ptr;
unsigned int buffer_size, expected_size;
enum guc_log_buffer_type type;
+ unsigned int read_offset, write_offset, bytes_to_copy;
+ bool new_overflow;
if (WARN_ON(!guc->log.buf_addr))
return;
@@ -1025,11 +1027,14 @@ static void guc_read_update_log_buffer(struct
intel_guc *guc)
memcpy(&log_buffer_state_local, log_buffer_state,
sizeof(struct guc_log_buffer_state));
buffer_size = log_buffer_state_local.size;
+ read_offset = log_buffer_state_local.read_ptr;
+ write_offset = log_buffer_state_local.sampled_write_ptr;
/* Bookkeeping stuff */
guc->log.flush_count[type] +=
log_buffer_state_local.flush_to_file;
if (log_buffer_state_local.buffer_full_cnt !=
guc->log.prev_overflow_count[type]) {
+ new_overflow = 1;
guc->log.total_overflow_count[type] +=
(log_buffer_state_local.buffer_full_cnt -
guc->log.prev_overflow_count[type]);
@@ -1043,7 +1048,8 @@ static void guc_read_update_log_buffer(struct
intel_guc *guc)
guc->log.prev_overflow_count[type] =
log_buffer_state_local.buffer_full_cnt;
DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
- }
+ } else
+ new_overflow = 0;
Nitpick: normally the rule is if one branch has curlies all of them have
to. Checkpatch I think warns about that, or maybe only in strict mode.
Did ran checkpatch with strict option.
Probably overlooked the warning. Will check again
if (log_buffer_snapshot_state) {
/* First copy the state structure in snapshot buffer */
@@ -1055,8 +1061,7 @@ static void guc_read_update_log_buffer(struct
intel_guc *guc)
* for consistency set the write pointer value to same
* value of sampled_write_ptr in the snapshot buffer.
*/
- log_buffer_snapshot_state->write_ptr =
- log_buffer_snapshot_state->sampled_write_ptr;
+ log_buffer_snapshot_state->write_ptr = write_offset;
log_buffer_snapshot_state++;
@@ -1079,7 +1084,31 @@ static void guc_read_update_log_buffer(struct
intel_guc *guc)
buffer_size = expected_size;
}
- memcpy(dst_data_ptr, src_data_ptr, buffer_size);
+ if (unlikely(new_overflow)) {
+ /* copy the whole buffer in case of overflow */
+ read_offset = 0;
+ write_offset = buffer_size;
+ } else if (unlikely((read_offset > buffer_size) ||
+ (write_offset > buffer_size))) {
Could also check for read_offset == write_offset for even more safety?
That is already handled implicitly, in this case we don't do any copy.
As per the below code bytes_to_copy will come as 0.
if (read_offset <= write_offset) {
bytes_to_copy = write_offset - read_offset;
Best regards
Akash
+ DRM_ERROR("invalid log buffer state\n");
+ /* copy whole buffer as offsets are unreliable */
+ read_offset = 0;
+ write_offset = buffer_size;
+ }
+
+ /* Just copy the newly written data */
+ if (read_offset <= write_offset) {
+ bytes_to_copy = write_offset - read_offset;
+ memcpy(dst_data_ptr + read_offset,
+ src_data_ptr + read_offset, bytes_to_copy);
+ } else {
+ bytes_to_copy = buffer_size - read_offset;
+ memcpy(dst_data_ptr + read_offset,
+ src_data_ptr + read_offset, bytes_to_copy);
+
+ bytes_to_copy = write_offset;
+ memcpy(dst_data_ptr, src_data_ptr, bytes_to_copy);
+ }
src_data_ptr += buffer_size;
dst_data_ptr += buffer_size;
@@ -1088,8 +1117,7 @@ static void guc_read_update_log_buffer(struct
intel_guc *guc)
/* FIXME: invalidate/flush for log buffer needed */
/* Update the read pointer in the shared log buffer */
- log_buffer_state->read_ptr =
- log_buffer_state_local.sampled_write_ptr;
+ log_buffer_state->read_ptr = write_offset;
/* Clear the 'flush to file' flag */
log_buffer_state->flush_to_file = 0;
Looks OK to me.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
