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.
Also the commit message in patch 2 is not valid wrt the code.
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.
Paulo, have you tried if this bit detects access to unpowered
domain with 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
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
