RE: [Intel-gfx] [PATCH] drm/i915: avoid concurrent writes to aux_inv

From: "Yang, Fei" <fei.yang@intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Subject: RE: [Intel-gfx] [PATCH] drm/i915: avoid concurrent writes to aux_inv
Date: Fri, 18 Mar 2022 18:12:11 +0000	[thread overview]
Message-ID: <BYAPR11MB25678F24A1093AB70032745A9A139@BYAPR11MB2567.namprd11.prod.outlook.com> (raw)
In-Reply-To: <8d283c4b-6f16-a235-7c57-ad8a67252ca9@linux.intel.com>

>>   static u32 *gen12_emit_aux_table_inv(const i915_reg_t inv_reg, u32 *cs)
>>   {
>>   	*cs++ = MI_LOAD_REGISTER_IMM(1);
>> @@ -296,7 +272,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode)
>>   		if (!HAS_FLAT_CCS(rq->engine->i915)) {
>>   			aux_inv = rq->engine->mask & ~BIT(BCS0);
>>   			if (aux_inv)
>> -				cmd += 2 * hweight32(aux_inv) + 2;
>> +				cmd += 4;
>>   		}
>>   	}
>>   
>> @@ -329,14 +305,16 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode)
>>   	*cs++ = 0; /* value */
>>   
>>   	if (aux_inv) { /* hsdes: 1809175790 */
>> -		struct intel_engine_cs *engine;
>> -		unsigned int tmp;
>> -
>> -		*cs++ = MI_LOAD_REGISTER_IMM(hweight32(aux_inv));
>> -		for_each_engine_masked(engine, rq->engine->gt, aux_inv, tmp) {
>> -			*cs++ = i915_mmio_reg_offset(aux_inv_reg(engine));
>> -			*cs++ = AUX_INV;
>> +		*cs++ = MI_LOAD_REGISTER_IMM(1) | MI_LRI_MMIO_REMAP_EN;
> 
> Cool, I didn't know this exists. First Bspec link I found did not mention these register, but second did.
> That one however (29545) has a worrying "removed by" tag which seems to point to a story suggesting the
> remapping table might be gone on machines with flat ccs?! Could you double check please?

The variable aux_inv is set only if (!HAS_FLAT_CCS(rq->engine->i915)).

>> +		if (rq->engine->class == VIDEO_DECODE_CLASS) {
>> +			*cs++ = i915_mmio_reg_offset(GEN12_VD0_AUX_NV);
>> +		} else if (rq->engine->class == VIDEO_ENHANCEMENT_CLASS) {
>> +			*cs++ = i915_mmio_reg_offset(GEN12_VE0_AUX_NV);
>> +		} else {
>> +			GEM_BUG_ON("unknown aux_inv reg\n");
>> +			*cs++ = i915_mmio_reg_offset(INVALID_MMIO_REG);
>
> I'd consider not emitting the LRI if we don't know what to put in unless there is some hidden point to do it?

That's true. I was following the original logic flow here. I think it would be better to check for engine class before setting the variable aux_inv.

>
>>   		}
>> +		*cs++ = AUX_INV;
>>   		*cs++ = MI_NOOP;
>>   	}
>>   
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h 
>> b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
>> index d112ffd56418..2d150eec5c65 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
>> @@ -144,6 +144,7 @@
>>   #define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
>>   /* Gen11+. addr = base + (ctx_restore ? offset & GENMASK(12,2) : offset) */
>>   #define   MI_LRI_LRM_CS_MMIO		REG_BIT(19)
>> +#define   MI_LRI_MMIO_REMAP_EN		(1 << 17)
>>   #define   MI_LRI_FORCE_POSTED		(1<<12)
>
> Passing observation - three bits, three flavours of expressing them, sigh...
Haha, REG_BIT(17) it is. The other one causes a CHECK:SPACING, but don't want to touch that in this patch.

>>   #define MI_LOAD_REGISTER_IMM_MAX_REGS (126)
>>   #define MI_STORE_REGISTER_MEM        MI_INSTR(0x24, 1)