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

[Lucas De Marchi]

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

>
>>  		     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