Re: [PATCH 12/15] drm/i915/guc: Don't print out relay statistics when relay is disabled

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 2/27/2018 6:22 PM, Michał Winiarski wrote:
> If nobody has enabled the relay, we're not comunicating with GuC, which
> means that the stats don't have any meaning. Let's also remove interrupt
> counter
reason for this? I think this was needed for verifying log flushes.
>   and tidy the debugfs formatting.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c  | 47 ++++++++++++++++++++++++------------
>   drivers/gpu/drm/i915/intel_guc.c     |  5 +---
>   drivers/gpu/drm/i915/intel_guc_log.c | 24 +++++++++---------
>   drivers/gpu/drm/i915/intel_guc_log.h | 12 +++++----
>   4 files changed, 51 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 866d44a091b3..65f758f7c425 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2326,30 +2326,45 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>   	return 0;
>   }
>   
> +static const char *
> +stringify_guc_log_type(enum guc_log_buffer_type type)
> +{
> +	switch (type) {
> +	case GUC_ISR_LOG_BUFFER:
> +		return "ISR";
> +	case GUC_DPC_LOG_BUFFER:
> +		return "DPC";
> +	case GUC_CRASH_DUMP_LOG_BUFFER:
> +		return "CRASH";
> +	default:
> +		MISSING_CASE(type);
> +	}
> +
> +	return "";
> +}
> +
>   static void i915_guc_log_info(struct seq_file *m,
>   			      struct drm_i915_private *dev_priv)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
> +	enum guc_log_buffer_type type;
>   
> -	seq_puts(m, "GuC logging stats:\n");
> -
> -	seq_printf(m, "\tISR:   flush count %10u, overflow count %10u\n",
> -		   guc->log.flush_count[GUC_ISR_LOG_BUFFER],
> -		   guc->log.total_overflow_count[GUC_ISR_LOG_BUFFER]);
> -
> -	seq_printf(m, "\tDPC:   flush count %10u, overflow count %10u\n",
> -		   guc->log.flush_count[GUC_DPC_LOG_BUFFER],
> -		   guc->log.total_overflow_count[GUC_DPC_LOG_BUFFER]);
> +	if (!intel_guc_log_relay_enabled(guc)) {
> +		seq_puts(m, "GuC log relay disabled\n");
> +		return;
> +	}
>   
> -	seq_printf(m, "\tCRASH: flush count %10u, overflow count %10u\n",
> -		   guc->log.flush_count[GUC_CRASH_DUMP_LOG_BUFFER],
> -		   guc->log.total_overflow_count[GUC_CRASH_DUMP_LOG_BUFFER]);
> +	seq_puts(m, "GuC logging stats:\n");
>   
> -	seq_printf(m, "\tTotal flush interrupt count: %u\n",
> -		   guc->log.flush_interrupt_count);
> +	seq_printf(m, "\tRelay full count: %u\n",
> +		   guc->log.relay_full_count);
>   
> -	seq_printf(m, "\tCapture miss count: %u\n",
> -		   guc->log.capture_miss_count);
> +	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
> +		seq_printf(m, "\t%s:\tflush count %10u, overflow count %10u\n",
> +			   stringify_guc_log_type(type),
> +			   guc->log.stats[type].flush,
> +			   guc->log.stats[type].overflow);
> +	}
>   }
>   
>   static void i915_guc_client_info(struct seq_file *m,
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index e3b6ae158a12..0500b4164254 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -388,12 +388,9 @@ void intel_guc_notification_handler(struct intel_guc *guc)
>   	spin_unlock(&guc->irq_lock);
>   
>   	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> -		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED)) {
> +		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
>   		queue_work(guc->log.flush_wq,
>   			   &guc->log.flush_work);
> -
> -		guc->log.flush_interrupt_count++;
> -	}
>   }
>   
>   int intel_guc_sample_forcewake(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index f95e18be1c5f..bdf6b3178488 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -176,22 +176,22 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
>   	return relay_reserve(guc->log.relay_chan, 0);
>   }
>   
> -static bool guc_check_log_buf_overflow(struct intel_guc *guc,
> +static bool guc_check_log_buf_overflow(struct intel_guc_log *log,
>   				       enum guc_log_buffer_type type,
>   				       unsigned int full_cnt)
>   {
> -	unsigned int prev_full_cnt = guc->log.prev_overflow_count[type];
> +	unsigned int prev_full_cnt = log->stats[type].sampled_overflow;
>   	bool overflow = false;
>   
>   	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;
> +		log->stats[type].overflow = full_cnt;
> +		log->stats[type].sampled_overflow += 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;
> +			log->stats[type].overflow += 16;
Should be
log->stats[type].sampled_overflow += 16;
>   		}
>   		DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
>   	}
> @@ -224,7 +224,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   	void *src_data, *dst_data;
>   	bool new_overflow;
>   
> -	if (WARN_ON(!guc->log.buf_addr))
> +	if (WARN_ON(!intel_guc_log_relay_enabled(guc)))
>   		return;
>   
>   	/* Get the pointer to shared GuC log buffer */
> @@ -239,7 +239,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   		 * getting consumed by User at a slow rate.
>   		 */
>   		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
> -		guc->log.capture_miss_count++;
> +		guc->log.relay_full_count++;
>   
>   		return;
>   	}
> @@ -262,8 +262,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   		full_cnt = log_buf_state_local.buffer_full_cnt;
>   
>   		/* Bookkeeping stuff */
> -		guc->log.flush_count[type] += log_buf_state_local.flush_to_file;
> -		new_overflow = guc_check_log_buf_overflow(guc, type, full_cnt);
> +		guc->log.stats[type].flush += log_buf_state_local.flush_to_file;
> +		new_overflow = guc_check_log_buf_overflow(&guc->log, type, full_cnt);
>   
>   		/* Update the state of shared log buffer */
>   		log_buf_state->read_ptr = write_offset;
> @@ -323,7 +323,7 @@ static void capture_logs_work(struct work_struct *work)
>   	mutex_unlock(&guc->log.lock);
>   }
>   
> -static bool guc_log_relay_enabled(struct intel_guc *guc)
> +bool intel_guc_log_relay_enabled(struct intel_guc *guc)
>   {
>   	return guc->log.buf_addr != NULL;
>   }
> @@ -553,7 +553,7 @@ int intel_guc_log_relay_open(struct intel_guc *guc)
>   
>   	mutex_lock(&guc->log.lock);
>   
> -	if (guc_log_relay_enabled(guc)) {
> +	if (intel_guc_log_relay_enabled(guc)) {
>   		ret = -EEXIST;
>   		goto out_unlock;
>   	}
> @@ -619,7 +619,7 @@ void intel_guc_log_relay_flush(struct intel_guc *guc)
>   
>   void intel_guc_log_relay_close(struct intel_guc *guc)
>   {
> -	GEM_BUG_ON(!guc_log_relay_enabled(guc));
> +	GEM_BUG_ON(!intel_guc_log_relay_enabled(guc));
>   
>   	guc_log_flush_irq_disable(guc);
>   	flush_work(&guc->log.flush_work);
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index 11f49ef41f01..a8ccb04e2b0a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -48,11 +48,12 @@ struct intel_guc_log {
>   	struct rchan *relay_chan;
>   	struct mutex lock;
>   	/* logging related stats */
> -	u32 capture_miss_count;
> -	u32 flush_interrupt_count;
> -	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
> -	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
> -	u32 flush_count[GUC_MAX_LOG_BUFFER];
> +	u32 relay_full_count;
> +	struct {
> +		u32 sampled_overflow;
> +		u32 overflow;
> +		u32 flush;
> +	} stats[GUC_MAX_LOG_BUFFER];
>   };
>   
>   int intel_guc_log_create(struct intel_guc *guc);
> @@ -63,5 +64,6 @@ int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val);
>   int intel_guc_log_relay_open(struct intel_guc *guc);
>   void intel_guc_log_relay_close(struct intel_guc *guc);
>   void intel_guc_log_relay_flush(struct intel_guc *guc);
> +bool intel_guc_log_relay_enabled(struct intel_guc *guc);
>   
>   #endif

-- 
Thanks,
Sagar

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