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

Goel, Akash
2016-08-15T16:13:22Z

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
>