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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxx>
Ta. Pushed.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
