[Intel-gfx] [PATCH v2] drm/i915/display: Record the plane update times for debugging

[Chris Wilson]

Quoting Ville Syrjälä (2020-12-02 19:24:33)
> On Fri, Nov 27, 2020 at 04:18:41PM +0000, Chris Wilson wrote:
> > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_VBLANK_EVADE)
> > +static void crtc_updates_info(struct seq_file *m,
> > +                           struct intel_crtc *crtc,
> > +                           const char *hdr)
> > +{
> > +     char buf[ARRAY_SIZE(crtc->debug.vbl.times) + 1] = {};
> > +     int h, row, max;
> > +     u64 count;
> > +
> > +     max = 0;
> > +     count = 0;
> > +     for (h = 0; h < ARRAY_SIZE(crtc->debug.vbl.times); h++) {
> > +             if (crtc->debug.vbl.times[h] > max)
> > +                     max = crtc->debug.vbl.times[h];
> > +             count += crtc->debug.vbl.times[h];
> > +     }
> > +     seq_printf(m, "%sUpdates: %llu\n", hdr, count);
> > +     if (!count)
> > +             return;
> > +
> > +     memset(buf, '-', sizeof(buf) - 1);
> > +     seq_printf(m, "%s  |%s|\n", hdr, buf);
> > +
> > +     for (row = ilog2(max) - 1; row; row--) {
> 
> row >= 0?

I skipped the last row, as the ilog2() would also catch all the empty
bins. The alternative is s/>=/>/, but my gut feeling was because of the
rounding down from ilog2, >= would be better.

> > +             memset(buf, ' ', sizeof(buf) - 1);
> > +             for (h = 0; h < ARRAY_SIZE(crtc->debug.vbl.times); h++) {
> > +                     if (ilog2(crtc->debug.vbl.times[h]) >= row)
> > +                             buf[h] = '*';
> > +             }
> > +             seq_printf(m, "%s  |%s|\n", hdr, buf);
> > +     }
> 
> I have a feeling that putting the graph on its side would make it more
> readable since then we could easily label more (all even?) of the bins.
> Right now I'm having a hard time seeing what's what exactly.

Ok, labelling the axis (axes if you are lucky) is a definite advantage.

> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index ce82d654d0f2..30c82bc5ca98 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1186,6 +1186,15 @@ struct intel_crtc {
> >               ktime_t start_vbl_time;
> >               int min_vbl, max_vbl;
> >               int scanline_start;
> > +#ifdef CONFIG_DRM_I915_DEBUG_VBLANK_EVADE
> > +             struct {
> > +                     u64 min;
> > +                     u64 max;
> > +                     u64 sum;
> > +                     unsigned long over;
> 
> Was there a particular reason for the long? The bins are
> ints so can't really see why this couldn't be an in too.

A touch of pessimism.
-Chris