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

[Goel, Akash]

On 8/15/2016 9:06 PM, Tvrtko Ursulin wrote:
>
> On 15/08/16 15:49, akash.goel at intel.com wrote:
>> From: Akash Goel <akash.goel at 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 at 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 at intel.com>
>
> Regards,
>
> Tvrtko
>