Re: [PATCH v2 2/4] drm/i915: Fix WaProgramMgsrForL3BankSpecificMmioReads

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: "Summers, Stuart" <stuart.summers@intel.com>,
	"Intel-gfx@lists.freedesktop.org"
	<Intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v2 2/4] drm/i915: Fix WaProgramMgsrForL3BankSpecificMmioReads
Date: Fri, 12 Jul 2019 06:32:07 +0100	[thread overview]
Message-ID: <a762d763-af3c-8ee1-c417-01a47e51ad5c@linux.intel.com> (raw)
In-Reply-To: <be6ada43cd2da587cbc4b7d535cd8a5aa8c7813e.camel@intel.com>

On 12/07/2019 00:51, Summers, Stuart wrote:
> On Thu, 2019-07-11 at 16:59 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> This is generally much more readable than the previous implementation,
> thanks! Some minor comments below...
> 
>>
>> Two issues in this code:
>>
>> 1.
>> fls() usage is incorrect causing off by one in subslice mask lookup,
>> which in other words means subslice mask of all zeroes is always used
>> (subslice mask of a slice which is not present, or even out of bounds
>> array access), rendering the checks in wa_init_mcr either futile or
>> random.
>>
>> 2.
>> Condition in WARN_ON is not correct. It is doing a bitwise and
>> operation
>> between a positive (present subslices) and negative mask (disabled L3
>> banks).
>>
>> This means that with corrected fls() usage the assert would always
>> incorrectly fail.
>>
>> We can fix this by inverting the fuse bits in the check.
>>
>> v2:
>>   * Simplify check for logic and redability.
>>   * Improve commentary explaining what is really happening ie. what
>> the
>>     assert is really trying to check and why.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: fe864b76c2ab ("drm/i915: Implement
>> WaProgramMgsrForL3BankSpecificMmioReads")
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 80 ++++++++++---------
>> --
>>   1 file changed, 40 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 9e069286d3ce..80f1159e5cda 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -761,7 +761,27 @@ static void
>>   wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
>>   {
>>   	const struct sseu_dev_info *sseu = &RUNTIME_INFO(i915)->sseu;
>> -	u32 mcr_slice_subslice_mask;
>> +	u32 mcr_mask, mcr;
>> +
>> +	/*
>> +	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl
>> +	 * Before any MMIO read into slice/subslice specific registers,
>> MCR
>> +	 * packet control register needs to be programmed to point to
>> any
>> +	 * enabled s/ss pair. Otherwise, incorrect values will be
>> returned.
>> +	 * This means each subsequent MMIO read will be forwarded to an
>> +	 * specific s/ss combination, but this is OK since these
>> registers
>> +	 * are consistent across s/ss in almost all cases. In the rare
>> +	 * occasions, such as INSTDONE, where this value is dependent
>> +	 * on s/ss combo, the read should be done with
>> read_subslice_reg.
>> +	 */
>> +	mcr = intel_calculate_mcr_s_ss_select(i915);
>> +
>> +	if (INTEL_GEN(i915) >= 11)
>> +		mcr_mask = GEN11_MCR_SLICE_MASK |
>> GEN11_MCR_SUBSLICE_MASK;
>> +	else
>> +		mcr_mask = GEN8_MCR_SLICE_MASK |
>> GEN8_MCR_SUBSLICE_MASK;
>> +
>> +	wa_write_masked_or(wal, GEN8_MCR_SELECTOR, mcr_mask, mcr);
> 
> Was there a specific reason to move this up to the top? Or this is

Yes, so below can check the actually selected MCR instead of deriving it 
from slice/subslice mask as stored in the driver.

> purely to move this functionality all together rather than spread out
> through the function? Looking at the documentation, we do want to
> specifically apply WaProgramMgsrForL3BankSpecificMmioReads before any
> other workarounds. So maybe just move this whole block to the bottom of
> the function instead?

I think it works better if MCR selection is first. Going forward, and 
for robustness, this probably needs to be improved to do both 
workarounds in a single block. Along the lines of ffs(ss_ena & l3_ena). 
And WARN_ON if no common bits. Oh well.. now I got no excuses not to do 
it...

>>   
>>   	/*
>>   	 * WaProgramMgsrForL3BankSpecificMmioReads: cnl,icl
>> @@ -776,49 +796,29 @@ wa_init_mcr(struct drm_i915_private *i915,
>> struct i915_wa_list *wal)
>>   	 * something more complex that requires checking the range of
>> every
>>   	 * MMIO read).
>>   	 */
>> -	if (INTEL_GEN(i915) >= 10 &&
>> -	    is_power_of_2(sseu->slice_mask)) {
>> +	if (INTEL_GEN(i915) >= 10 && is_power_of_2(sseu->slice_mask)) {
>>   		/*
>> -		 * read FUSE3 for enabled L3 Bank IDs, if L3 Bank
>> matches
>> -		 * enabled subslice, no need to redirect MCR packet
>> +		 * GEN8_MCR_SELECTOR contains dual-purpose bits which
>> select
>> +		 * both to which subslice, or to which L3 bank, the
>> respective
>> +		 * mmio reads will go.
>> +		 * Since we have selected one enabled subslice in
>> +		 * WaProgramMgsrForCorrectSliceSpecificMmioReads, we
>> now
>> +		 * need to check if the L3 bank of the equal "instance"
>> is also
>> +		 * enabled.
>> +		 * If that is not the case we could try to find a
>> number which
>> +		 * works for both, or going even further, implement a
>> dynamic
>> +		 * scheme where we switch at before every affected mmio
> 
> s/at //?
> 
>> read.
>> +		 * Fortunately neither seems to be needed at the moment
>> for
>> +		 * current parts and current driver behaviour.
>>   		 */
>> -		u32 slice = fls(sseu->slice_mask);
>> -		u32 fuse3 =
>> -			intel_uncore_read(&i915->uncore,
>> GEN10_MIRROR_FUSE3);
>> -		u8 ss_mask = sseu->subslice_mask[slice];
>> -
>> -		u8 enabled_mask = (ss_mask | ss_mask >>
>> -				   GEN10_L3BANK_PAIR_COUNT) &
>> GEN10_L3BANK_MASK;
>> -		u8 disabled_mask = fuse3 & GEN10_L3BANK_MASK;
>> +		unsigned int mcr_ss = BIT((mcr >> 24) & 0x7);
> 
> Can you add macros for these magic numbers?
> 
>> +		unsigned int l3_fuse =
>> +			intel_uncore_read(&i915->uncore,
>> GEN10_MIRROR_FUSE3) &
>> +			GEN10_L3BANK_MASK;
>> +		unsigned int l3_en = ~(l3_fuse << 4 | l3_fuse);
> 
> Macro here for the 4?

I knew someone would ask for it. You give fixes, people want perfection. 
;) Will do.

Regards,

Tvrtko

> Thanks,
> Stuart
> 
>>   
>> -		/*
>> -		 * Production silicon should have matched L3Bank and
>> -		 * subslice enabled
>> -		 */
>> -		WARN_ON((enabled_mask & disabled_mask) !=
>> enabled_mask);
>> +		WARN_ON(!(mcr_ss & l3_en));
>>   	}
>> -
>> -	if (INTEL_GEN(i915) >= 11)
>> -		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
>> -					  GEN11_MCR_SUBSLICE_MASK;
>> -	else
>> -		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
>> -					  GEN8_MCR_SUBSLICE_MASK;
>> -	/*
>> -	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl
>> -	 * Before any MMIO read into slice/subslice specific registers,
>> MCR
>> -	 * packet control register needs to be programmed to point to
>> any
>> -	 * enabled s/ss pair. Otherwise, incorrect values will be
>> returned.
>> -	 * This means each subsequent MMIO read will be forwarded to an
>> -	 * specific s/ss combination, but this is OK since these
>> registers
>> -	 * are consistent across s/ss in almost all cases. In the rare
>> -	 * occasions, such as INSTDONE, where this value is dependent
>> -	 * on s/ss combo, the read should be done with
>> read_subslice_reg.
>> -	 */
>> -	wa_write_masked_or(wal,
>> -			   GEN8_MCR_SELECTOR,
>> -			   mcr_slice_subslice_mask,
>> -			   intel_calculate_mcr_s_ss_select(i915));
>>   }
>>   
>>   static void
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx