[Intel-gfx] [PATCH 6/9] drm/i915/cmdparser: Compare against the previous command descriptor

[Chris Wilson]

On Mon, Aug 15, 2016 at 12:00:32PM +0100, Matthew Auld wrote:
> On 12 August 2016 at 16:07, Chris Wilson <chris at 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 at 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

-- 
Chris Wilson, Intel Open Source Technology Centre