Re: [PATCH] drm/i915: Include register polling in reg_rw traces

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Feb 04, 2019 at 09:33:08PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2019-02-04 21:16:44)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We generally omit register polling from the i915_reg_rw tracepoint.
> > Understandable since polling could generate a lot of noise in the
> > trace. The downside is that the trace is incomplete. As a compromise
> > let's trace the final register value observed while polling. That
> > should be generally sufficient to observe what the code should be
> > doing next.
> 
> Fair compromise; I admit the subject line had me freaking out.
>  
> > I suppose in some cases it might make sense to also trace the initial
> > register value, and maybe the number of times we polled. But that
> > would require a separate tracepoint so let's leave it for the future.
> 
> I postulate when you get down to wanting that amount of detail, you
> throw in dedicated debug.
>  
> > The other users of _NOTRACE() are i915_pmu and i2c bitbanging,
> > which I decided to leave alone.
> > 
> > Next we should do something to claw back the tracepoints for
> > planes and whatnot which were switched to _FW() a while back.
> > I guess just new macros for raw_rw+trace. The question is
> > what to call it?
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c     | 12 ++++++++++--
> >  drivers/gpu/drm/i915/intel_dp.c     |  6 ++++++
> >  drivers/gpu/drm/i915/intel_uncore.c |  3 +++
> >  3 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index a7aaa1ac4c99..7de90701f6f1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -2543,6 +2543,10 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> >  static int vlv_wait_for_pw_status(struct drm_i915_private *dev_priv,
> >                                   u32 mask, u32 val)
> >  {
> > +       i915_reg_t reg = VLV_GTLC_PW_STATUS;
> > +       u32 reg_value;
> > +       int ret;
> > +
> >         /* The HW does not like us polling for PW_STATUS frequently, so
> >          * use the sleeping loop rather than risk the busy spin within
> >          * intel_wait_for_register().
> > @@ -2550,8 +2554,12 @@ static int vlv_wait_for_pw_status(struct drm_i915_private *dev_priv,
> >          * Transitioning between RC6 states should be at most 2ms (see
> >          * valleyview_enable_rps) so use a 3ms timeout.
> >          */
> > -       return wait_for((I915_READ_NOTRACE(VLV_GTLC_PW_STATUS) & mask) == val,
> > -                       3);
> > +       ret = wait_for(((reg_value = I915_READ_NOTRACE(reg)) & mask) == val, 3);
> > +
> > +       /* just trace the final value */
> > +       trace_i915_reg_rw(false, reg, reg_value, sizeof(reg_value), true);
> 
> I feel like there's a pattern here that could benefit from a convenient
> macro.

Entirely possible. I've not yet looked at the current _FW() users
to see if they share the same pattern in any signidicant numbers.
Which brings me back to the topic of what to do with _FW()...

Maybe I should just split _FW() into _FW() and FW_NOTRACE()?
Or if we go the other way, maybe add _FW_TRACE()? Bur all the
plane regs would want _FW_TRACE() which is a bit of an eyesore
if used extensively, so a better name would be nice. Alas,
I've not yet thought of one.

I guess I should first figure out how many instances of each
type there are.

> Nevertheless,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Ta. Pushed.

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