Em Sex, 2015-09-04 às 11:40 +0300, Mika Kuoppala escreveu:
> Daniel Vetter <daniel@xxxxxxxx> writes:
>
> > On Thu, Sep 03, 2015 at 04:51:45PM -0300, Paulo Zanoni wrote:
> > > From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > >
> > > Delay the expensive read on the FPGA_DBG register from once per
> > > mmio to
> > > once per forcewake section when we are doing the general
> > > wellbeing
> > > check rather than the targetted error detection. This almost
> > > reduces
> > > the overhead of the debug facility (for example when submitting
> > > execlists)
> > > to zero whilst keeping the debug checks around.
> > >
> > > v2: Enable one-shot mmio debugging from the interrupt check as
> > > well as a
> > > safeguard to catch invalid display writes from outside the
> > > powerwell.
> > > v3 (from Paulo): rebase since gen9 addition and
> > > intel_uncore_check_errors
> > > removal
> > >
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >
> > I'm unclear how this interacts (or how it sould interact) with
> > patch 2:
> > Forcwake is mostly for GT registers, but patch 2 also tries to
> > optimize
> > forcwake for GT registers. Do we really need both?
>
> Assuming the hardware detects access to unpowered domains and
> to unregistered ranges by setting this bit, I would say that patch 2
> is not needed. One could argue that patch 2 is somewhat harmful as
> current register access pattern affects the detection.
Can you please elaborate? I'm more in favor of keeping patch 2 instead
of this. But I'm operating under the assumption that we only flip
FPGA_DBG bit 31 to 1 if we do an access inside the range. See the
discussion on patch 2. So doing the checks outside the range is useless
since it won't catch problems inside i915.ko.
>
> Also the commit message in patch 2 is not valid wrt the code.
Why?
>
> With skl, the debug bit seems to decay with time, instead of being
> sticky. So in there we could argue that in patch 4/4, the reading
> should be done before (and after) the forcewake scope.
I tested my patch on SKL and it still does detect the same problems it
was detecting before. Maybe we could write a little debugfs file to do
the accesses in the mentioned pattern and check the forcewake theory.
>
> Paulo, have you tried if this bit detects access to unpowered
> domain with hsw/bdw?
FPGA_DBG bit 31 becomes 1 when we access an existing register on an
unpowered power well on HSW/BDW.
>
> -Mika
>
> > -Daniel
> >
> > > ---
> > > drivers/gpu/drm/i915/intel_uncore.c | 52 +++++++++++++++++++++--
> > > --------------
> > > 1 file changed, 29 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > > b/drivers/gpu/drm/i915/intel_uncore.c
> > > index 8844c314..1fe63fc 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -148,6 +148,31 @@ fw_domains_put(struct drm_i915_private
> > > *dev_priv, enum forcewake_domains fw_doma
> > > }
> > >
> > > static void
> > > +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> > > +{
> > > + static bool mmio_debug_once = true;
> > > +
> > > + if (i915.mmio_debug || !mmio_debug_once)
> > > + return;
> > > +
> > > + if (__raw_i915_read32(dev_priv, FPGA_DBG) &
> > > FPGA_DBG_RM_NOCLAIM) {
> > > + DRM_ERROR("Unclaimed register detected, "
> > > + "enabling oneshot unclaimed register
> > > reporting. "
> > > + "Please use i915.mmio_debug=N for more
> > > information.\n");
> > > + __raw_i915_write32(dev_priv, FPGA_DBG,
> > > FPGA_DBG_RM_NOCLAIM);
> > > + i915.mmio_debug = mmio_debug_once--;
> > > + }
> > > +}
> > > +
> > > +static void
> > > +fw_domains_put_debug(struct drm_i915_private *dev_priv,
> > > + enum forcewake_domains fw_domains)
> > > +{
> > > + hsw_unclaimed_reg_detect(dev_priv);
> > > + fw_domains_put(dev_priv, fw_domains);
> > > +}
> > > +
> > > +static void
> > > fw_domains_posting_read(struct drm_i915_private *dev_priv)
> > > {
> > > struct intel_uncore_forcewake_domain *d;
> > > @@ -627,26 +652,6 @@ hsw_unclaimed_reg_debug(struct
> > > drm_i915_private *dev_priv, u32 reg, bool read,
> > > }
> > > }
> > >
> > > -static void
> > > -hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv, u32
> > > reg)
> > > -{
> > > - static bool mmio_debug_once = true;
> > > -
> > > - if (!UNCLAIMED_CHECK_RANGE(reg))
> > > - return;
> > > -
> > > - if (i915.mmio_debug || !mmio_debug_once)
> > > - return;
> > > -
> > > - if (__raw_i915_read32(dev_priv, FPGA_DBG) &
> > > FPGA_DBG_RM_NOCLAIM) {
> > > - DRM_ERROR("Unclaimed register detected, "
> > > - "enabling oneshot unclaimed register
> > > reporting. "
> > > - "Please use i915.mmio_debug=N for more
> > > information.\n");
> > > - __raw_i915_write32(dev_priv, FPGA_DBG,
> > > FPGA_DBG_RM_NOCLAIM);
> > > - i915.mmio_debug = mmio_debug_once--;
> > > - }
> > > -}
> > > -
> > > #define GEN2_READ_HEADER(x) \
> > > u##x val = 0; \
> > > assert_device_not_suspended(dev_priv);
> > > @@ -900,7 +905,6 @@ hsw_write##x(struct drm_i915_private
> > > *dev_priv, off_t reg, u##x val, bool trace)
> > > gen6_gt_check_fifodbg(dev_priv); \
> > > } \
> > > hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > > - hsw_unclaimed_reg_detect(dev_priv, reg); \
> > > GEN6_WRITE_FOOTER; \
> > > }
> > >
> > > @@ -942,7 +946,6 @@ gen8_write##x(struct drm_i915_private
> > > *dev_priv, off_t reg, u##x val, bool trace
> > > __force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> > > __raw_i915_write##x(dev_priv, reg, val); \
> > > hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > > - hsw_unclaimed_reg_detect(dev_priv, reg); \
> > > GEN6_WRITE_FOOTER; \
> > > }
> > >
> > > @@ -1008,7 +1011,6 @@ gen9_write##x(struct drm_i915_private
> > > *dev_priv, off_t reg, u##x val, \
> > > __force_wake_get(dev_priv, fw_engine); \
> > > __raw_i915_write##x(dev_priv, reg, val); \
> > > hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > > - hsw_unclaimed_reg_detect(dev_priv, reg); \
> > > GEN6_WRITE_FOOTER; \
> > > }
> > >
> > > @@ -1194,6 +1196,10 @@ static void
> > > intel_uncore_fw_domains_init(struct drm_device *dev)
> > > FORCEWAKE, FORCEWAKE_ACK);
> > > }
> > >
> > > + if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
> > > + dev_priv->uncore.funcs.force_wake_put ==
> > > fw_domains_put)
> > > + dev_priv->uncore.funcs.force_wake_put =
> > > fw_domains_put_debug;
> > > +
> > > /* All future platforms are expected to require complex
> > > power gating */
> > > WARN_ON(dev_priv->uncore.fw_domains == 0);
> > > }
> > > --
> > > 2.5.0
> > >
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
