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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 06, 2018 at 04:51:27PM +0530, Sagar Arun Kamble wrote:
> 
> 
> 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.

I'm simply not a fan of random debugfs counters.
Is someone really using this?
We do have individual flush counts, do we really need to count the interrupts?

> >   and tidy the debugfs formatting.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > ---
> >   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;

Oops.

-Michał

> >   		}
> >   		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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux