[0/2] drm/i915: Engine relative MMIO
mbox series

Message ID 20190822180205.21071-1-John.C.Harrison@Intel.com
Headers show
Series
  • drm/i915: Engine relative MMIO
Related show

Message

John Harrison Aug. 22, 2019, 6:02 p.m. UTC
From: John Harrison <John.C.Harrison@Intel.com>

Newer hardware has support for making LRI accesses to MMIO registers
relative to the engine that is executing the LRI instruction. This is
required for things like hardware based load balancing across engines.

John Harrison (2):
  drm/i915: Engine relative MMIO
  drm/i915: Engine relative MMIO for Gen12

 drivers/gpu/drm/i915/gem/i915_gem_context.c   |  14 +-
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   3 +-
 drivers/gpu/drm/i915/gt/intel_engine.h        |   5 +
 drivers/gpu/drm/i915/gt/intel_engine_cs.c     | 219 ++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_engine_types.h  |  10 +
 drivers/gpu/drm/i915/gt/intel_gpu_commands.h  |   9 +-
 drivers/gpu/drm/i915/gt/intel_lrc.c           |  77 +++---
 drivers/gpu/drm/i915/gt/intel_lrc_reg.h       |   4 +-
 drivers/gpu/drm/i915/gt/intel_mocs.c          |  17 +-
 drivers/gpu/drm/i915/gt/intel_ringbuffer.c    |  25 +-
 drivers/gpu/drm/i915/gt/intel_workarounds.c   |   4 +-
 .../gpu/drm/i915/gt/selftest_workarounds.c    |  15 +-
 drivers/gpu/drm/i915/gvt/mmio_context.c       |  16 +-
 drivers/gpu/drm/i915/i915_cmd_parser.c        |   6 +-
 drivers/gpu/drm/i915/i915_perf.c              |  17 +-
 15 files changed, 361 insertions(+), 80 deletions(-)

Comments

Rodrigo Vivi Sept. 4, 2019, 7:33 p.m. UTC | #1
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
> -:108: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
> #108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
> +#define MI_LOAD_REGISTER_IMM(engine, count)	\
> +					(engine)->get_lri_cmd((engine), (count))
> 
> -:108: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'engine' - possible side-effects?
> #108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
> +#define MI_LOAD_REGISTER_IMM(engine, count)	\
> +					(engine)->get_lri_cmd((engine), (count))
> 
> -: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)
>                                          	  ^
> 
> -:491: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
> #491: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:1719:
> +	*cs++ = MI_LOAD_REGISTER_IMM(rq->engine, GEN7_L3LOG_SIZE/4);
>  	                                                        ^
> 
> -:522: CHECK:CAMELCASE: Avoid CamelCase: <regLRI>
> #522: FILE: drivers/gpu/drm/i915/gt/selftest_workarounds.c:483:
> +		u32 regLRI = i915_get_lri_reg(engine,
> 
> -: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 }    ),
> 
> -:629: CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
> #629: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:1187:
> +				if (desc->cmd.value == __MI_LOAD_REGISTER_IMM(1)
> +				    && (offset + 2 > length ||
> 
> total: 2 errors, 0 warnings, 8 checks, 531 lines checked
> 901081d701fe drm/i915: Engine relative MMIO for Gen12
> -:182: CHECK:BRACES: braces {} should be used on all arms of this statement
> #182: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:407:
> +	if (i915_engine_has_relative_lri(engine)) {
> [...]
> +	} else
> [...]
> 
> -:183: CHECK:BRACES: braces {} should be used on all arms of this statement
> #183: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:408:
> +		if (INTEL_GEN(engine->i915) < 12)
> [...]
> +		else {
> [...]
> 
> -:185: CHECK:BRACES: Unbalanced braces around else statement
> #185: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:410:
> +		else {
> 
> -:191: CHECK:BRACES: Unbalanced braces around else statement
> #191: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:416:
> +	} else
> 
> -: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)
>                                   		  ^
> 
> total: 0 errors, 0 warnings, 5 checks, 252 lines checked

I looks like we need to fix many (most) of the issues here before proceed.

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.

> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
John Harrison Sept. 5, 2019, 11:42 p.m. UTC | #2
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.