"Zanoni, Paulo R" <paulo.r.zanoni@xxxxxxxxx> writes:
> 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.
>
Your answer with the example program cleared some confusion I had.
I have to test run that example on skl.
But lets imagine the situation userpsace does access unclaimed
range, and driver access pattern after the event is outside of
that detection range. We never sample the bit. We will notice the
unclaimed access only until drivers does something inside the
range. Is this acceptable?
If our goal is to catch problems only inside i915.ko, then
the range check is not harmful.
>>
>> Also the commit message in patch 2 is not valid wrt the code.
>
> Why?
>
"For the normal use case (with i915.mmio_debug=0), this commit will
avoid the extra __raw_i915_read32() call for every register outside..."
This was for v1 of the patch? It seems that whatever mmio_debug
has is secondary. The read is avoided always if the reg is outside
the range.
Thanks,
-Mika
>>
>> 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
