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

Lucas De Marchi
2022-04-06T05:53:47Z

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@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@intel.com>

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