diff mbox

[v2,5/6] drm/i915: Reduce pointer indirection during cmd parser lookup

Message ID 1448016961-25331-6-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson Nov. 20, 2015, 10:56 a.m. UTC
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 51 ++++++++--------------------------
 drivers/gpu/drm/i915/i915_drv.h        |  4 ++-
 2 files changed, 14 insertions(+), 41 deletions(-)

Comments

Ville Syrjala Nov. 20, 2015, 3:27 p.m. UTC | #1
On Fri, Nov 20, 2015 at 10:56:00AM +0000, Chris Wilson wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 51 ++++++++--------------------------
>  drivers/gpu/drm/i915/i915_drv.h        |  4 ++-
>  2 files changed, 14 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index cfd07bfe6e75..ea9df2bb87de 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -113,7 +113,7 @@
>  
>  /*            Command                          Mask   Fixed Len   Action
>  	      ---------------------------------------------------------- */
> -static const struct drm_i915_cmd_descriptor common_cmds[] = {
> +static struct drm_i915_cmd_descriptor common_cmds[] = {

I'm a little sad to see the const gone. All this gets moved out of
rodata.

>  	CMD(  MI_NOOP,                          SMI,    F,  1,      S  ),
>  	CMD(  MI_USER_INTERRUPT,                SMI,    F,  1,      R  ),
>  	CMD(  MI_WAIT_FOR_EVENT,                SMI,    F,  1,      M  ),
> @@ -146,7 +146,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
>  	CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
>  };
>  
> -static const struct drm_i915_cmd_descriptor render_cmds[] = {
> +static struct drm_i915_cmd_descriptor render_cmds[] = {
>  	CMD(  MI_FLUSH,                         SMI,    F,  1,      S  ),
>  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
>  	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
> @@ -207,7 +207,7 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
>  	      }},						       ),
>  };
>  
> -static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> +static struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
>  	CMD(  MI_SET_PREDICATE,                 SMI,    F,  1,      S  ),
>  	CMD(  MI_RS_CONTROL,                    SMI,    F,  1,      S  ),
>  	CMD(  MI_URB_ATOMIC_ALLOC,              SMI,    F,  1,      S  ),
> @@ -229,7 +229,7 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
>  	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_PS,  S3D,   !F,  0x1FF,  S  ),
>  };
>  
> -static const struct drm_i915_cmd_descriptor video_cmds[] = {
> +static struct drm_i915_cmd_descriptor video_cmds[] = {
>  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
>  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
>  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
> @@ -273,7 +273,7 @@ static const struct drm_i915_cmd_descriptor video_cmds[] = {
>  	CMD(  MFX_WAIT,                         SMFX,   F,  1,      S  ),
>  };
>  
> -static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> +static struct drm_i915_cmd_descriptor vecs_cmds[] = {
>  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
>  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
>  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
> @@ -311,7 +311,7 @@ static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
>  	      }},						       ),
>  };
>  
> -static const struct drm_i915_cmd_descriptor blt_cmds[] = {
> +static struct drm_i915_cmd_descriptor blt_cmds[] = {
>  	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
>  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3FF,  B,
>  	      .bits = {{
> @@ -344,7 +344,7 @@ static const struct drm_i915_cmd_descriptor blt_cmds[] = {
>  	CMD(  SRC_COPY_BLT,                     S2D,   !F,  0x3F,   S  ),
>  };
>  
> -static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
> +static struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
>  	CMD(  MI_LOAD_SCAN_LINES_INCL,          SMI,   !F,  0x3F,   M  ),
>  	CMD(  MI_LOAD_SCAN_LINES_EXCL,          SMI,   !F,  0x3F,   R  ),
>  };
> @@ -618,11 +618,6 @@ static bool validate_regs_sorted(struct intel_engine_cs *ring)
>  			     ring->master_reg_count);
>  }
>  
> -struct cmd_node {
> -	const struct drm_i915_cmd_descriptor *desc;
> -	struct hlist_node node;
> -};
> -
>  /*
>   * Different command ranges have different numbers of bits for the opcode. For
>   * example, MI commands use bits 31:23 while 3D commands use bits 31:16. The
> @@ -651,16 +646,8 @@ static int init_hash_table(struct intel_engine_cs *ring,
>  		const struct drm_i915_cmd_table *table = &cmd_tables[i];
>  
>  		for (j = 0; j < table->count; j++) {
> -			const struct drm_i915_cmd_descriptor *desc =
> -				&table->table[j];
> -			struct cmd_node *desc_node =
> -				kmalloc(sizeof(*desc_node), GFP_KERNEL);
> -
> -			if (!desc_node)
> -				return -ENOMEM;

init_hash_table() can no longer fail -> void?

> -
> -			desc_node->desc = desc;
> -			hash_add(ring->cmd_hash, &desc_node->node,
> +			struct drm_i915_cmd_descriptor *desc = &table->table[j];
> +			hash_add(ring->cmd_hash, &desc->node[ring->id],
>  				 desc->cmd.value & CMD_HASH_MASK);
>  		}
>  	}
> @@ -668,18 +655,6 @@ static int init_hash_table(struct intel_engine_cs *ring,
>  	return 0;
>  }
>  
> -static void fini_hash_table(struct intel_engine_cs *ring)
> -{
> -	struct hlist_node *tmp;
> -	struct cmd_node *desc_node;
> -	int i;
> -
> -	hash_for_each_safe(ring->cmd_hash, i, tmp, desc_node, node) {
> -		hash_del(&desc_node->node);
> -		kfree(desc_node);
> -	}
> -}
> -
>  /**
>   * i915_cmd_parser_init_ring() - set cmd parser related fields for a ringbuffer
>   * @ring: the ringbuffer to initialize
> @@ -770,7 +745,6 @@ int i915_cmd_parser_init_ring(struct intel_engine_cs *ring)
>  	ret = init_hash_table(ring, cmd_tables, cmd_table_count);
>  	if (ret) {
>  		DRM_ERROR("CMD: cmd_parser_init failed!\n");
> -		fini_hash_table(ring);
>  		return ret;
>  	}
>  
> @@ -790,8 +764,6 @@ void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring)
>  {
>  	if (!ring->needs_cmd_parser)
>  		return;
> -
> -	fini_hash_table(ring);
>  }

i915_cmd_parser_fini_ring() is a nop now. Kill it?

>  
>  /*
> @@ -801,11 +773,10 @@ static const struct drm_i915_cmd_descriptor*
>  find_cmd_in_table(struct intel_engine_cs *ring,
>  		  u32 cmd_header)
>  {
> -	struct cmd_node *desc_node;
> +	const struct drm_i915_cmd_descriptor *desc;
>  
> -	hash_for_each_possible(ring->cmd_hash, desc_node, node,
> +	hash_for_each_possible(ring->cmd_hash, desc, node[ring->id],
>  			       cmd_header & CMD_HASH_MASK) {
> -		const struct drm_i915_cmd_descriptor *desc = desc_node->desc;
>  		if (((cmd_header ^ desc->cmd.value) & desc->cmd.mask) == 0)
>  			return desc;

At least we still return this as const, so the caller can't accidentally
clobber it, even if we lose the rodata protection.

Apart from the int vs. void thing and the nop
i915_cmd_parser_fini_ring() the patch looks fine to me.

>  	}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 28d5bfceae3b..5960f76f1438 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2396,6 +2396,8 @@ struct drm_i915_cmd_descriptor {
>  		u32 condition_offset;
>  		u32 condition_mask;
>  	} bits[MAX_CMD_DESC_BITMASKS];
> +
> +	struct hlist_node node[I915_NUM_RINGS];
>  };
>  
>  /*
> @@ -2405,7 +2407,7 @@ struct drm_i915_cmd_descriptor {
>   * descriptors, which must be sorted with command opcodes in ascending order.
>   */
>  struct drm_i915_cmd_table {
> -	const struct drm_i915_cmd_descriptor *table;
> +	struct drm_i915_cmd_descriptor *table;
>  	int count;
>  };
>  
> -- 
> 2.6.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Nov. 20, 2015, 3:34 p.m. UTC | #2
On Fri, Nov 20, 2015 at 05:27:43PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 20, 2015 at 10:56:00AM +0000, Chris Wilson wrote:
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_cmd_parser.c | 51 ++++++++--------------------------
> >  drivers/gpu/drm/i915/i915_drv.h        |  4 ++-
> >  2 files changed, 14 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index cfd07bfe6e75..ea9df2bb87de 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -113,7 +113,7 @@
> >  
> >  /*            Command                          Mask   Fixed Len   Action
> >  	      ---------------------------------------------------------- */
> > -static const struct drm_i915_cmd_descriptor common_cmds[] = {
> > +static struct drm_i915_cmd_descriptor common_cmds[] = {
> 
> I'm a little sad to see the const gone. All this gets moved out of
> rodata.

I'm open to having this refused. Pretty much both of 4 & 5 can be
ignored after reducing the number of hash collisions in 6. Though not
having to duplicate the memory for the hash tables is nice.
-Chris
Ville Syrjala Nov. 20, 2015, 3:47 p.m. UTC | #3
On Fri, Nov 20, 2015 at 03:34:22PM +0000, Chris Wilson wrote:
> On Fri, Nov 20, 2015 at 05:27:43PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 20, 2015 at 10:56:00AM +0000, Chris Wilson wrote:
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_cmd_parser.c | 51 ++++++++--------------------------
> > >  drivers/gpu/drm/i915/i915_drv.h        |  4 ++-
> > >  2 files changed, 14 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > index cfd07bfe6e75..ea9df2bb87de 100644
> > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > @@ -113,7 +113,7 @@
> > >  
> > >  /*            Command                          Mask   Fixed Len   Action
> > >  	      ---------------------------------------------------------- */
> > > -static const struct drm_i915_cmd_descriptor common_cmds[] = {
> > > +static struct drm_i915_cmd_descriptor common_cmds[] = {
> > 
> > I'm a little sad to see the const gone. All this gets moved out of
> > rodata.
> 
> I'm open to having this refused. Pretty much both of 4 & 5 can be
> ignored after reducing the number of hash collisions in 6. Though not
> having to duplicate the memory for the hash tables is nice.

I don't have a strong opinion either way. On one hand rodata is nice,
on the other hand simplicity from avoiding the kmalloc()s is nice.
Jani Nikula Nov. 23, 2015, 8:09 a.m. UTC | #4
On Fri, 20 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Nov 20, 2015 at 03:34:22PM +0000, Chris Wilson wrote:
>> On Fri, Nov 20, 2015 at 05:27:43PM +0200, Ville Syrjälä wrote:
>> > On Fri, Nov 20, 2015 at 10:56:00AM +0000, Chris Wilson wrote:
>> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > > ---
>> > >  drivers/gpu/drm/i915/i915_cmd_parser.c | 51 ++++++++--------------------------
>> > >  drivers/gpu/drm/i915/i915_drv.h        |  4 ++-
>> > >  2 files changed, 14 insertions(+), 41 deletions(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> > > index cfd07bfe6e75..ea9df2bb87de 100644
>> > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> > > @@ -113,7 +113,7 @@
>> > >  
>> > >  /*            Command                          Mask   Fixed Len   Action
>> > >  	      ---------------------------------------------------------- */
>> > > -static const struct drm_i915_cmd_descriptor common_cmds[] = {
>> > > +static struct drm_i915_cmd_descriptor common_cmds[] = {
>> > 
>> > I'm a little sad to see the const gone. All this gets moved out of
>> > rodata.
>> 
>> I'm open to having this refused. Pretty much both of 4 & 5 can be
>> ignored after reducing the number of hash collisions in 6. Though not
>> having to duplicate the memory for the hash tables is nice.
>
> I don't have a strong opinion either way. On one hand rodata is nice,
> on the other hand simplicity from avoiding the kmalloc()s is nice.

One could allocate (by kmalloc or statically) an array of struct
cmd_node instead of each of them individually, however IIUC that was not
the main optimization here but rather the reduction of the pointer
chasing.

BR,
Jani.
Ville Syrjala Dec. 1, 2015, 5:39 p.m. UTC | #5
On Fri, Nov 20, 2015 at 05:27:43PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 20, 2015 at 10:56:00AM +0000, Chris Wilson wrote:
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_cmd_parser.c | 51 ++++++++--------------------------
> >  drivers/gpu/drm/i915/i915_drv.h        |  4 ++-
> >  2 files changed, 14 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index cfd07bfe6e75..ea9df2bb87de 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -113,7 +113,7 @@
> >  
> >  /*            Command                          Mask   Fixed Len   Action
> >  	      ---------------------------------------------------------- */
> > -static const struct drm_i915_cmd_descriptor common_cmds[] = {
> > +static struct drm_i915_cmd_descriptor common_cmds[] = {
> 
> I'm a little sad to see the const gone. All this gets moved out of
> rodata.
> 
> >  	CMD(  MI_NOOP,                          SMI,    F,  1,      S  ),
> >  	CMD(  MI_USER_INTERRUPT,                SMI,    F,  1,      R  ),
> >  	CMD(  MI_WAIT_FOR_EVENT,                SMI,    F,  1,      M  ),
> > @@ -146,7 +146,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
> >  	CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
> >  };
> >  
> > -static const struct drm_i915_cmd_descriptor render_cmds[] = {
> > +static struct drm_i915_cmd_descriptor render_cmds[] = {
> >  	CMD(  MI_FLUSH,                         SMI,    F,  1,      S  ),
> >  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
> >  	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
> > @@ -207,7 +207,7 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
> >  	      }},						       ),
> >  };
> >  
> > -static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> > +static struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> >  	CMD(  MI_SET_PREDICATE,                 SMI,    F,  1,      S  ),
> >  	CMD(  MI_RS_CONTROL,                    SMI,    F,  1,      S  ),
> >  	CMD(  MI_URB_ATOMIC_ALLOC,              SMI,    F,  1,      S  ),
> > @@ -229,7 +229,7 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> >  	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_PS,  S3D,   !F,  0x1FF,  S  ),
> >  };
> >  
> > -static const struct drm_i915_cmd_descriptor video_cmds[] = {
> > +static struct drm_i915_cmd_descriptor video_cmds[] = {
> >  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
> >  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
> >  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
> > @@ -273,7 +273,7 @@ static const struct drm_i915_cmd_descriptor video_cmds[] = {
> >  	CMD(  MFX_WAIT,                         SMFX,   F,  1,      S  ),
> >  };
> >  
> > -static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> > +static struct drm_i915_cmd_descriptor vecs_cmds[] = {
> >  	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
> >  	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
> >  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
> > @@ -311,7 +311,7 @@ static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> >  	      }},						       ),
> >  };
> >  
> > -static const struct drm_i915_cmd_descriptor blt_cmds[] = {
> > +static struct drm_i915_cmd_descriptor blt_cmds[] = {
> >  	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
> >  	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3FF,  B,
> >  	      .bits = {{
> > @@ -344,7 +344,7 @@ static const struct drm_i915_cmd_descriptor blt_cmds[] = {
> >  	CMD(  SRC_COPY_BLT,                     S2D,   !F,  0x3F,   S  ),
> >  };
> >  
> > -static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
> > +static struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
> >  	CMD(  MI_LOAD_SCAN_LINES_INCL,          SMI,   !F,  0x3F,   M  ),
> >  	CMD(  MI_LOAD_SCAN_LINES_EXCL,          SMI,   !F,  0x3F,   R  ),
> >  };
> > @@ -618,11 +618,6 @@ static bool validate_regs_sorted(struct intel_engine_cs *ring)
> >  			     ring->master_reg_count);
> >  }
> >  
> > -struct cmd_node {
> > -	const struct drm_i915_cmd_descriptor *desc;
> > -	struct hlist_node node;
> > -};
> > -
> >  /*
> >   * Different command ranges have different numbers of bits for the opcode. For
> >   * example, MI commands use bits 31:23 while 3D commands use bits 31:16. The
> > @@ -651,16 +646,8 @@ static int init_hash_table(struct intel_engine_cs *ring,
> >  		const struct drm_i915_cmd_table *table = &cmd_tables[i];
> >  
> >  		for (j = 0; j < table->count; j++) {
> > -			const struct drm_i915_cmd_descriptor *desc =
> > -				&table->table[j];
> > -			struct cmd_node *desc_node =
> > -				kmalloc(sizeof(*desc_node), GFP_KERNEL);
> > -
> > -			if (!desc_node)
> > -				return -ENOMEM;
> 
> init_hash_table() can no longer fail -> void?
> 
> > -
> > -			desc_node->desc = desc;
> > -			hash_add(ring->cmd_hash, &desc_node->node,
> > +			struct drm_i915_cmd_descriptor *desc = &table->table[j];
> > +			hash_add(ring->cmd_hash, &desc->node[ring->id],
> >  				 desc->cmd.value & CMD_HASH_MASK);
> >  		}
> >  	}
> > @@ -668,18 +655,6 @@ static int init_hash_table(struct intel_engine_cs *ring,
> >  	return 0;
> >  }
> >  
> > -static void fini_hash_table(struct intel_engine_cs *ring)
> > -{
> > -	struct hlist_node *tmp;
> > -	struct cmd_node *desc_node;
> > -	int i;
> > -
> > -	hash_for_each_safe(ring->cmd_hash, i, tmp, desc_node, node) {
> > -		hash_del(&desc_node->node);
> > -		kfree(desc_node);
> > -	}
> > -}
> > -
> >  /**
> >   * i915_cmd_parser_init_ring() - set cmd parser related fields for a ringbuffer
> >   * @ring: the ringbuffer to initialize
> > @@ -770,7 +745,6 @@ int i915_cmd_parser_init_ring(struct intel_engine_cs *ring)
> >  	ret = init_hash_table(ring, cmd_tables, cmd_table_count);
> >  	if (ret) {
> >  		DRM_ERROR("CMD: cmd_parser_init failed!\n");
> > -		fini_hash_table(ring);
> >  		return ret;
> >  	}
> >  
> > @@ -790,8 +764,6 @@ void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring)
> >  {
> >  	if (!ring->needs_cmd_parser)
> >  		return;
> > -
> > -	fini_hash_table(ring);
> >  }
> 
> i915_cmd_parser_fini_ring() is a nop now. Kill it?
> 
> >  
> >  /*
> > @@ -801,11 +773,10 @@ static const struct drm_i915_cmd_descriptor*
> >  find_cmd_in_table(struct intel_engine_cs *ring,
> >  		  u32 cmd_header)
> >  {
> > -	struct cmd_node *desc_node;
> > +	const struct drm_i915_cmd_descriptor *desc;
> >  
> > -	hash_for_each_possible(ring->cmd_hash, desc_node, node,
> > +	hash_for_each_possible(ring->cmd_hash, desc, node[ring->id],
> >  			       cmd_header & CMD_HASH_MASK) {
> > -		const struct drm_i915_cmd_descriptor *desc = desc_node->desc;
> >  		if (((cmd_header ^ desc->cmd.value) & desc->cmd.mask) == 0)
> >  			return desc;
> 
> At least we still return this as const, so the caller can't accidentally
> clobber it, even if we lose the rodata protection.
> 
> Apart from the int vs. void thing and the nop
> i915_cmd_parser_fini_ring() the patch looks fine to me.

This too can get
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

since the now unnecessary error handling doesn't actually do anything.
Feel free to keep the r-b even if you decide to throw out the error
handling parts.

> 
> >  	}
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 28d5bfceae3b..5960f76f1438 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2396,6 +2396,8 @@ struct drm_i915_cmd_descriptor {
> >  		u32 condition_offset;
> >  		u32 condition_mask;
> >  	} bits[MAX_CMD_DESC_BITMASKS];
> > +
> > +	struct hlist_node node[I915_NUM_RINGS];
> >  };
> >  
> >  /*
> > @@ -2405,7 +2407,7 @@ struct drm_i915_cmd_descriptor {
> >   * descriptors, which must be sorted with command opcodes in ascending order.
> >   */
> >  struct drm_i915_cmd_table {
> > -	const struct drm_i915_cmd_descriptor *table;
> > +	struct drm_i915_cmd_descriptor *table;
> >  	int count;
> >  };
> >  
> > -- 
> > 2.6.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index cfd07bfe6e75..ea9df2bb87de 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -113,7 +113,7 @@ 
 
 /*            Command                          Mask   Fixed Len   Action
 	      ---------------------------------------------------------- */
-static const struct drm_i915_cmd_descriptor common_cmds[] = {
+static struct drm_i915_cmd_descriptor common_cmds[] = {
 	CMD(  MI_NOOP,                          SMI,    F,  1,      S  ),
 	CMD(  MI_USER_INTERRUPT,                SMI,    F,  1,      R  ),
 	CMD(  MI_WAIT_FOR_EVENT,                SMI,    F,  1,      M  ),
@@ -146,7 +146,7 @@  static const struct drm_i915_cmd_descriptor common_cmds[] = {
 	CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
 };
 
-static const struct drm_i915_cmd_descriptor render_cmds[] = {
+static struct drm_i915_cmd_descriptor render_cmds[] = {
 	CMD(  MI_FLUSH,                         SMI,    F,  1,      S  ),
 	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
 	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
@@ -207,7 +207,7 @@  static const struct drm_i915_cmd_descriptor render_cmds[] = {
 	      }},						       ),
 };
 
-static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
+static struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
 	CMD(  MI_SET_PREDICATE,                 SMI,    F,  1,      S  ),
 	CMD(  MI_RS_CONTROL,                    SMI,    F,  1,      S  ),
 	CMD(  MI_URB_ATOMIC_ALLOC,              SMI,    F,  1,      S  ),
@@ -229,7 +229,7 @@  static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
 	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_PS,  S3D,   !F,  0x1FF,  S  ),
 };
 
-static const struct drm_i915_cmd_descriptor video_cmds[] = {
+static struct drm_i915_cmd_descriptor video_cmds[] = {
 	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
 	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
 	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
@@ -273,7 +273,7 @@  static const struct drm_i915_cmd_descriptor video_cmds[] = {
 	CMD(  MFX_WAIT,                         SMFX,   F,  1,      S  ),
 };
 
-static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
+static struct drm_i915_cmd_descriptor vecs_cmds[] = {
 	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      R  ),
 	CMD(  MI_SET_APPID,                     SMI,    F,  1,      S  ),
 	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   B,
@@ -311,7 +311,7 @@  static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
 	      }},						       ),
 };
 
-static const struct drm_i915_cmd_descriptor blt_cmds[] = {
+static struct drm_i915_cmd_descriptor blt_cmds[] = {
 	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   R  ),
 	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3FF,  B,
 	      .bits = {{
@@ -344,7 +344,7 @@  static const struct drm_i915_cmd_descriptor blt_cmds[] = {
 	CMD(  SRC_COPY_BLT,                     S2D,   !F,  0x3F,   S  ),
 };
 
-static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
+static struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
 	CMD(  MI_LOAD_SCAN_LINES_INCL,          SMI,   !F,  0x3F,   M  ),
 	CMD(  MI_LOAD_SCAN_LINES_EXCL,          SMI,   !F,  0x3F,   R  ),
 };
@@ -618,11 +618,6 @@  static bool validate_regs_sorted(struct intel_engine_cs *ring)
 			     ring->master_reg_count);
 }
 
-struct cmd_node {
-	const struct drm_i915_cmd_descriptor *desc;
-	struct hlist_node node;
-};
-
 /*
  * Different command ranges have different numbers of bits for the opcode. For
  * example, MI commands use bits 31:23 while 3D commands use bits 31:16. The
@@ -651,16 +646,8 @@  static int init_hash_table(struct intel_engine_cs *ring,
 		const struct drm_i915_cmd_table *table = &cmd_tables[i];
 
 		for (j = 0; j < table->count; j++) {
-			const struct drm_i915_cmd_descriptor *desc =
-				&table->table[j];
-			struct cmd_node *desc_node =
-				kmalloc(sizeof(*desc_node), GFP_KERNEL);
-
-			if (!desc_node)
-				return -ENOMEM;
-
-			desc_node->desc = desc;
-			hash_add(ring->cmd_hash, &desc_node->node,
+			struct drm_i915_cmd_descriptor *desc = &table->table[j];
+			hash_add(ring->cmd_hash, &desc->node[ring->id],
 				 desc->cmd.value & CMD_HASH_MASK);
 		}
 	}
@@ -668,18 +655,6 @@  static int init_hash_table(struct intel_engine_cs *ring,
 	return 0;
 }
 
-static void fini_hash_table(struct intel_engine_cs *ring)
-{
-	struct hlist_node *tmp;
-	struct cmd_node *desc_node;
-	int i;
-
-	hash_for_each_safe(ring->cmd_hash, i, tmp, desc_node, node) {
-		hash_del(&desc_node->node);
-		kfree(desc_node);
-	}
-}
-
 /**
  * i915_cmd_parser_init_ring() - set cmd parser related fields for a ringbuffer
  * @ring: the ringbuffer to initialize
@@ -770,7 +745,6 @@  int i915_cmd_parser_init_ring(struct intel_engine_cs *ring)
 	ret = init_hash_table(ring, cmd_tables, cmd_table_count);
 	if (ret) {
 		DRM_ERROR("CMD: cmd_parser_init failed!\n");
-		fini_hash_table(ring);
 		return ret;
 	}
 
@@ -790,8 +764,6 @@  void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring)
 {
 	if (!ring->needs_cmd_parser)
 		return;
-
-	fini_hash_table(ring);
 }
 
 /*
@@ -801,11 +773,10 @@  static const struct drm_i915_cmd_descriptor*
 find_cmd_in_table(struct intel_engine_cs *ring,
 		  u32 cmd_header)
 {
-	struct cmd_node *desc_node;
+	const struct drm_i915_cmd_descriptor *desc;
 
-	hash_for_each_possible(ring->cmd_hash, desc_node, node,
+	hash_for_each_possible(ring->cmd_hash, desc, node[ring->id],
 			       cmd_header & CMD_HASH_MASK) {
-		const struct drm_i915_cmd_descriptor *desc = desc_node->desc;
 		if (((cmd_header ^ desc->cmd.value) & desc->cmd.mask) == 0)
 			return desc;
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 28d5bfceae3b..5960f76f1438 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2396,6 +2396,8 @@  struct drm_i915_cmd_descriptor {
 		u32 condition_offset;
 		u32 condition_mask;
 	} bits[MAX_CMD_DESC_BITMASKS];
+
+	struct hlist_node node[I915_NUM_RINGS];
 };
 
 /*
@@ -2405,7 +2407,7 @@  struct drm_i915_cmd_descriptor {
  * descriptors, which must be sorted with command opcodes in ascending order.
  */
 struct drm_i915_cmd_table {
-	const struct drm_i915_cmd_descriptor *table;
+	struct drm_i915_cmd_descriptor *table;
 	int count;
 };