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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux