| Message ID | 1471014450-21020-7-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12 August 2016 at 16:07, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On the blitter (and in test code), we see long sequences of repeated > commands, e.g. XY_PIXEL_BLT, XY_SCANLINE_BLT, or XY_SRC_COPY. For these, > we can skip the hashtable lookup by remembering the previous command > descriptor and doing a straightforward compare of the command header. > The corollary is that we need to do one extra comparison before lookup > up new commands. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 274f2136a846..3b1100a0e0cb 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -350,6 +350,9 @@ static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = { > CMD( MI_LOAD_SCAN_LINES_EXCL, SMI, !F, 0x3F, R ), > }; > > +static const struct drm_i915_cmd_descriptor noop_desc = > + CMD(MI_NOOP, SMI, F, 1, S); > + > #undef CMD > #undef SMI > #undef S3D > @@ -898,11 +901,14 @@ find_cmd_in_table(struct intel_engine_cs *engine, > static const struct drm_i915_cmd_descriptor* > find_cmd(struct intel_engine_cs *engine, > u32 cmd_header, > + const struct drm_i915_cmd_descriptor *desc, > struct drm_i915_cmd_descriptor *default_desc) > { > - const struct drm_i915_cmd_descriptor *desc; > u32 mask; > > + if (((cmd_header ^ desc->cmd.value) & desc->cmd.mask) == 0) > + return desc; > + > desc = find_cmd_in_table(engine, cmd_header); > if (desc) > return desc; > @@ -911,10 +917,10 @@ find_cmd(struct intel_engine_cs *engine, > if (!mask) > return NULL; > > - BUG_ON(!default_desc); Why remove this, was it overkill? > - default_desc->flags = CMD_DESC_SKIP; > + default_desc->cmd.value = cmd_header; > + default_desc->cmd.mask = 0xffff0000; Where did you pluck this mask from?
On Mon, Aug 15, 2016 at 12:00:32PM +0100, Matthew Auld wrote: > On 12 August 2016 at 16:07, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On the blitter (and in test code), we see long sequences of repeated > > commands, e.g. XY_PIXEL_BLT, XY_SCANLINE_BLT, or XY_SRC_COPY. For these, > > we can skip the hashtable lookup by remembering the previous command > > descriptor and doing a straightforward compare of the command header. > > The corollary is that we need to do one extra comparison before lookup > > up new commands. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_cmd_parser.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > > index 274f2136a846..3b1100a0e0cb 100644 > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > > @@ -350,6 +350,9 @@ static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = { > > CMD( MI_LOAD_SCAN_LINES_EXCL, SMI, !F, 0x3F, R ), > > }; > > > > +static const struct drm_i915_cmd_descriptor noop_desc = > > + CMD(MI_NOOP, SMI, F, 1, S); > > + > > #undef CMD > > #undef SMI > > #undef S3D > > @@ -898,11 +901,14 @@ find_cmd_in_table(struct intel_engine_cs *engine, > > static const struct drm_i915_cmd_descriptor* > > find_cmd(struct intel_engine_cs *engine, > > u32 cmd_header, > > + const struct drm_i915_cmd_descriptor *desc, > > struct drm_i915_cmd_descriptor *default_desc) > > { > > - const struct drm_i915_cmd_descriptor *desc; > > u32 mask; > > > > + if (((cmd_header ^ desc->cmd.value) & desc->cmd.mask) == 0) > > + return desc; > > + > > desc = find_cmd_in_table(engine, cmd_header); > > if (desc) > > return desc; > > @@ -911,10 +917,10 @@ find_cmd(struct intel_engine_cs *engine, > > if (!mask) > > return NULL; > > > > - BUG_ON(!default_desc); > Why remove this, was it overkill? The NULL dereference on the next line is all we need to debug this, i.e. it gives us no more information and we know by construction it can never be NULL. > > > - default_desc->flags = CMD_DESC_SKIP; > > + default_desc->cmd.value = cmd_header; > > + default_desc->cmd.mask = 0xffff0000; > Where did you pluck this mask from? It is the most general cmd header mask. #define MIN_OPCODE_SHIFT 16 cmd.mask = ~0u << MIN_OPCODE_SHIFT ? -Chris
>> > - default_desc->flags = CMD_DESC_SKIP; >> > + default_desc->cmd.value = cmd_header; >> > + default_desc->cmd.mask = 0xffff0000; >> Where did you pluck this mask from? > > It is the most general cmd header mask. > > #define MIN_OPCODE_SHIFT 16 > cmd.mask = ~0u << MIN_OPCODE_SHIFT > ? Reviewed-by: Matthew Auld <matthew.auld@intel.com>
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 274f2136a846..3b1100a0e0cb 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -350,6 +350,9 @@ static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = { CMD( MI_LOAD_SCAN_LINES_EXCL, SMI, !F, 0x3F, R ), }; +static const struct drm_i915_cmd_descriptor noop_desc = + CMD(MI_NOOP, SMI, F, 1, S); + #undef CMD #undef SMI #undef S3D @@ -898,11 +901,14 @@ find_cmd_in_table(struct intel_engine_cs *engine, static const struct drm_i915_cmd_descriptor* find_cmd(struct intel_engine_cs *engine, u32 cmd_header, + const struct drm_i915_cmd_descriptor *desc, struct drm_i915_cmd_descriptor *default_desc) { - const struct drm_i915_cmd_descriptor *desc; u32 mask; + if (((cmd_header ^ desc->cmd.value) & desc->cmd.mask) == 0) + return desc; + desc = find_cmd_in_table(engine, cmd_header); if (desc) return desc; @@ -911,10 +917,10 @@ find_cmd(struct intel_engine_cs *engine, if (!mask) return NULL; - BUG_ON(!default_desc); - default_desc->flags = CMD_DESC_SKIP; + default_desc->cmd.value = cmd_header; + default_desc->cmd.mask = 0xffff0000; default_desc->length.mask = mask; - + default_desc->flags = CMD_DESC_SKIP; return default_desc; } @@ -1165,7 +1171,8 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, bool is_master) { u32 *cmd, *batch_end; - struct drm_i915_cmd_descriptor default_desc = { 0 }; + struct drm_i915_cmd_descriptor default_desc = noop_desc; + const struct drm_i915_cmd_descriptor *desc = &default_desc; bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */ bool needs_clflush_after = false; int ret = 0; @@ -1185,13 +1192,12 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, */ batch_end = cmd + (batch_len / sizeof(*batch_end)); while (cmd < batch_end) { - const struct drm_i915_cmd_descriptor *desc; u32 length; if (*cmd == MI_BATCH_BUFFER_END) break; - desc = find_cmd(engine, *cmd, &default_desc); + desc = find_cmd(engine, *cmd, desc, &default_desc); if (!desc) { DRM_DEBUG_DRIVER("CMD: Unrecognized command: 0x%08X\n", *cmd);
On the blitter (and in test code), we see long sequences of repeated commands, e.g. XY_PIXEL_BLT, XY_SCANLINE_BLT, or XY_SRC_COPY. For these, we can skip the hashtable lookup by remembering the previous command descriptor and doing a straightforward compare of the command header. The corollary is that we need to do one extra comparison before lookup up new commands. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_cmd_parser.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)