Re: [PATCH 11/18] drm/i915: Optimization to reduce the sampling time of GuC log buffer

Tvrtko Ursulin
2016-08-15T15:36:42Z

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