| Message ID | 20190822180205.21071-1-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/i915: Engine relative MMIO | expand |
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
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.
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(-)