On Tue, Apr 05, 2022 at 09:02:42PM -0700, Matt Roper wrote:
On Mon, Apr 04, 2022 at 05:11:49PM -0700, Lucas De Marchi wrote:
Since gen6 we use FPGA_DBG register to detect unclaimed MMIO registers.
This register is in the display engine IP and can only ever detect
unclaimed accesses to registers in this area. However sometimes there
are reports of this triggering for registers in other areas, which
should not be possible.
Right now we always warn after the read/write of registers going through
unclaimed_reg_debug(). However places using __raw_uncore_* may be
triggering the unclaimed access and those being later accounted to a
different register. Let's warn both before and after the read/write
with a slightly different message, so it's clear if the register
reported in the warning is actually the culprit.
You should probably probably give an explicit mention of commit
dda960335e020 ("drm/i915: Just clear the mmiodebug before a register
access") in the commit message since we're reversing direction here.
I will add a note about that commit, but this is not going a reverse
direction: The reason for the change is exactly the same as expressed in
that commit: earlier failures being accounted for unrelated registers.
In that commit it dropped the warning before the read, but it failed to
account for cases like we are having: a read or write to a non-display
register will still read FPGA_DBG and be marked as failure. Even if it's
unrelated to the register that actually caused the failure. So I think
this is just a slightly different implementation: instead of dropping the
warning, warn with a more appropriate message to be clear what happened.
Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_uncore.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8b9caaaacc21..df59ec88459e 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1502,11 +1502,10 @@ ilk_dummy_write(struct intel_uncore *uncore)
static void
__unclaimed_reg_debug(struct intel_uncore *uncore,
const i915_reg_t reg,
- const bool read,
- const bool before)
+ const bool read)
{
if (drm_WARN(&uncore->i915->drm,
- check_for_unclaimed_mmio(uncore) && !before,
+ check_for_unclaimed_mmio(uncore),
"Unclaimed %s register 0x%x\n",
Might be simpler to just keep it all in one function and do something
like a
before ? "MMIO operation *before* a " : ""
in the message? Up to you. Either way,
those bool arguments ar too easy to mess up in the caller IMO. I'd
rather keep different functions, particularly since it's just a few
lines.
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
thanks
Lucas De Marchi
read ? "read from" : "write to",
i915_mmio_reg_offset(reg)))
@@ -1514,6 +1513,20 @@ __unclaimed_reg_debug(struct intel_uncore *uncore,
uncore->i915->params.mmio_debug--;
}
+static void
+__unclaimed_previous_reg_debug(struct intel_uncore *uncore,
+ const i915_reg_t reg,
+ const bool read)
+{
+ if (drm_WARN(&uncore->i915->drm,
+ check_for_unclaimed_mmio(uncore),
+ "Unclaimed access detected before %s register 0x%x\n",
+ read ? "read from" : "write to",
+ i915_mmio_reg_offset(reg)))
+ /* Only report the first N failures */
+ uncore->i915->params.mmio_debug--;
+}
+
static inline void
unclaimed_reg_debug(struct intel_uncore *uncore,
const i915_reg_t reg,
@@ -1526,13 +1539,13 @@ unclaimed_reg_debug(struct intel_uncore *uncore,
/* interrupts are disabled and re-enabled around uncore->lock usage */
lockdep_assert_held(&uncore->lock);
- if (before)
+ if (before) {
spin_lock(&uncore->debug->lock);
-
- __unclaimed_reg_debug(uncore, reg, read, before);
-
- if (!before)
+ __unclaimed_previous_reg_debug(uncore, reg, read);
+ } else {
+ __unclaimed_reg_debug(uncore, reg, read);
spin_unlock(&uncore->debug->lock);
+ }
}
#define __vgpu_read(x) \
--
2.35.1
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
