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
