[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7)

[John Harrison]

On 9/4/2019 12:33, Rodrigo Vivi wrote:
> On Thu, Aug 22, 2019 at 06:21:23PM -0000, Patchwork wrote:
>> == Series Details ==
>>
>> Series: drm/i915: Engine relative MMIO (rev7)
>> URL   : https://patchwork.freedesktop.org/series/57117/
>> State : warning
>>
>> == Summary ==
>>
>> $ dim checkpatch origin/drm-tip
>> 2eab059bc87e drm/i915: Engine relative MMIO
>> -:203: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
>> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
>> +#define __MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
>>                                    	                ^
>>
>> -:203: CHECK:SPACING: spaces preferred around that '-' (ctx:VxV)
>> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
>> +#define __MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
>>                                    	                    ^
>>
>> -:205: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>> #205: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
>> +#define   MI_LRI_ADD_CS_MMIO_START_GEN11	(1<<19)
>>                                           	  ^
>>
>> -:618: ERROR:SPACING: space prohibited after that open parenthesis '('
>> #618: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:225:
>> +	CMD(  __MI_LOAD_REGISTER_IMM(1),        SMI,   !F,  0xFF,   W,
>>
>> -:619: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
>> #619: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:226:
>> +	CMD(  __MI_LOAD_REGISTER_IMM(1),        SMI,   !F,  0xFF,   W,
>>   	      .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 }    ),
>>
>> total: 2 errors, 0 warnings, 8 checks, 531 lines checked
>> 901081d701fe drm/i915: Engine relative MMIO for Gen12
>> -:271: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>> #271: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
>> +#define   MI_LRI_MMIO_REMAP_GEN12		(1<<17)
>>                                    		  ^
>>
>> I looks like we need to fix many (most) of the issues here before proceed.
The above are all in keeping with the style of the surrounding code. 
Indeed, the command parser ones are because the code is laid out in a 
formatted table. Should they be changed to obey the style rules or left 
as is?

>> Also, did you check the naming with Tvrtko. If I remember correctly
>> his preference was for "MI_LOAD_REGISTER_IMM_REL for
>> usage on relevant (new) paths."
>>
>> and don't touch the old ones.
>>
>> Also I'm not sure if I like _MI_LOAD for the old and MI_LOAD for the new.
>> Tvrtko original's suggestion makes the distinction clean.

I believe Tvrtko is still on vacation. However, the point is that this 
is not old versus new code. It is also not about code that wants to use 
some fancy new feature of indirect addressing that did not exist before.

This is about making existing code paths work on new hardware. The point 
is that if someone wishes to emit an LRI instruction and they want it to 
work on both Gen7 and on Gen11 then it needs to be issued differently 
for each of those devices. The code that is emitting the LRI doesn't 
care which device it is on. It just wants to make a write happen. So my 
argument is that the obvious, default, this is the normal way to do an 
LRI version should be the one that copes with the idiosyncrasies of the 
hardware. Whereas, if the code writer specifically wishes to not support 
Gen11, or is doing something else strange then it is not an issue that 
they have to knowingly use a 'here-be-dragons' version of the LRI 
instruction.

Otherwise you end up with the 'here-be-dragons' version actually being 
the one needed to support hardware of any generation. Whereas the 
'use-me-please' version will fail on newer devices. That situation is 
just inviting people to use the wrong one and create bugs on Gen11 onward.

John.