[Intel-gfx] [PATCH] drm/i915/uncore: Warn on previous unclaimed accesses

[Lucas De Marchi]

On Tue, Apr 05, 2022 at 10:53:43PM -0700, Lucas De Marchi wrote:
>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 at intel.com>
>>>---
>>> 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 at intel.com>
>
>thanks
>Lucas De Marchi


Applied to drm-intel-next.

Lucas De Marchi