Re: [PATCH 06/19] drm/i915: Handle log buffer flush interrupt event from GuC

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

On 17/08/16 12:35, Tvrtko Ursulin wrote:
> On 17/08/16 12:24, Goel, Akash wrote:

[snip]

>> Won't the 2nd memcpy (from the copy on stack to the relay buffer) be
>> really fast ?
>> The copy on stack (16 bytes) will most likely be in the CPU cache and
>> the same area is used for all 3 buffer types.
> 
> Yes I realized later you use it more in later patches.
> 
> I don't think it is slow but was just wondering if it could be made 
> tidier by getting rid of one copy.
> 
> Would have to apply the series to see how the final loop looks like, but 
> would something like the below be possible:
> 
>      if (!log_buffer_snapshot_state) {
>          .. read_ptr update from wc ..
>          continue;
>      }
> 
>      memcpy from wc to final buffer
> 
>      .. the rest of processing you do reading from the copy ..
> 
> ?

Not even compile tested and possibly incorrectly refactored,
but what do you think of:

static bool
guc_log_check_overflow(struct intel_guc *guc,
		       enum guc_log_buffer_type type,
		       struct guc_log_buffer *snapshot)
{
	unsigned int full_cnt, prev_full_cnt;
	bool overflow = false;

	guc->log.flush_count[type]++;

	full_cnt = snapshot.buffer_full_cnt;
	prev_full_cnt = guc->log.prev_overflow_count[type];

	if (full_cnt != prev_full_cnt) {
		overflow = true;

		guc->log.prev_overflow_count[type] = full_cnt;
		guc->log.total_overflow_count[type] += full_cnt - prev_full_cnt;

		if (full_cnt < prev_full_cnt) {
			/* buffer_full_cnt is a 4 bit counter */
			guc->log.total_overflow_count[type] += 16;
		}
	}

	return overflow;
}

static void guc_read_update_log_buffer(struct intel_guc *guc)
{
	struct guc_log_buffer *log_buffer, *snapshot;
	void *src_data, *dst_data;
	enum guc_log_buffer_type type;

	if (WARN_ON(!guc->log.buf_addr))
		return;

	/* Get the pointer to shared GuC log buffer */
	log_buffer = src_data = guc->log.buf_addr;

	/* Get the pointer to local buffer to store the logs */
	dst_data = snapshot = guc_get_write_buffer(guc);

	/* Actual logs are present from the 2nd page */
	src_data += PAGE_SIZE;
	dst_data += PAGE_SIZE;

	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER;
	     type++, log_buffer++) {
		unsigned int buffer_size, bytes_to_copy;
		unsigned int read_offset, write_offset;
		bool new_overflow;

		/* Clear the 'flush to file' flag */
		log_buffer->flush_to_file = 0;

		if (unlikely(!snapshot)) {
			/* Update the read pointer in the shared log buffer */
			log_buffer->read_ptr = log_buffer.sampled_write_ptr;
			continue;
		}

		memcpy(snapshot, &log_buffer, sizeof(*snapshot));

		buffer_size = guc_get_log_buffer_size(type);
		read_offset = snapshot.read_ptr;
		write_offset = snapshot.sampled_write_ptr;

		/* The write pointer could have been updated by the GuC
		 * firmware, after sending the flush interrupt to Host,
		 * for consistency set the write pointer value to same
		 * value of sampled_write_ptr in the snapshot buffer.
		 */
		snapshot->write_ptr = write_offset;

		new_overflow = guc_log_check_overflow(guc, type, snapshot);
		if (unlikely(new_overflow)) {
			DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
			/* 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 = buffer_size - read_offset;

			i915_memcpy_from_wc(dst_data, src_data, write_offset);
		} else {
			bytes_to_copy = write_offset - read_offset;
		}
		i915_memcpy_from_wc(dst_data + read_offset,
				    src_data + read_offset, bytes_to_copy);

		src_data += buffer_size;
		dst_data += buffer_size;

		/* Update the read pointer in the shared log buffer */
		log_buffer->read_ptr = snapshot.sampled_write_ptr;

		snapshot++;
	}

	if (snapshot) {
		guc_move_to_next_buf(guc);
	} else {
		/* Used rate limited to avoid deluge of messages, logs might be
		 * getting consumed by User at a slow rate.
		 */
		DRM_ERROR_RATELIMITED("no sub-buffer to capture log buffer\n");
		guc->log.capture_miss_count++;
	}
}

Removes the double memcpy and shortens some variable names in order
to make the whole thing more readable.

Regards,

Tvrtko

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