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