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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
