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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx