| Message ID | 1471272599-14586-12-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/08/16 15:49, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > 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@intel.com> > --- > 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. > > 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? > + 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@intel.com> Regards, Tvrtko
On 8/15/2016 9:06 PM, Tvrtko Ursulin wrote: > > On 15/08/16 15:49, akash.goel@intel.com wrote: >> From: Akash Goel <akash.goel@intel.com> >> >> 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@intel.com> >> --- >> 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@intel.com> > > Regards, > > Tvrtko >
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; 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))) { + 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;