[Intel-gfx] [PATCH 1/3] drm/i915: BUG_ON() when cmd/reg tables are not sorted

[Kenneth Graunke]

On 03/27/2014 11:43 AM, bradley.d.volkin at intel.com wrote:
> From: Brad Volkin <bradley.d.volkin at intel.com>
> 
> As suggested during review, this makes it much more obvious
> when the tables are not sorted.
> 
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Signed-off-by: Brad Volkin <bradley.d.volkin at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 788bd96..8a93db3 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -493,12 +493,13 @@ static u32 gen7_blt_get_cmd_length_mask(u32 cmd_header)
>  	return 0;
>  }
>  
> -static void validate_cmds_sorted(struct intel_ring_buffer *ring)
> +static bool validate_cmds_sorted(struct intel_ring_buffer *ring)
>  {
>  	int i;
> +	bool ret = true;
>  
>  	if (!ring->cmd_tables || ring->cmd_table_count == 0)
> -		return;
> +		return true;
>  
>  	for (i = 0; i < ring->cmd_table_count; i++) {
>  		const struct drm_i915_cmd_table *table = &ring->cmd_tables[i];
> @@ -510,35 +511,45 @@ static void validate_cmds_sorted(struct intel_ring_buffer *ring)
>  				&table->table[i];
>  			u32 curr = desc->cmd.value & desc->cmd.mask;
>  
> -			if (curr < previous)
> +			if (curr < previous) {
>  				DRM_ERROR("CMD: table not sorted ring=%d table=%d entry=%d cmd=0x%08X prev=0x%08X\n",
>  					  ring->id, i, j, curr, previous);
> +				ret = false;
> +			}
>  
>  			previous = curr;
>  		}
>  	}
> +
> +	return ret;
>  }
>  
> -static void check_sorted(int ring_id, const u32 *reg_table, int reg_count)
> +static bool check_sorted(int ring_id, const u32 *reg_table, int reg_count)
>  {
>  	int i;
>  	u32 previous = 0;
> +	bool ret = true;
>  
>  	for (i = 0; i < reg_count; i++) {
>  		u32 curr = reg_table[i];
>  
> -		if (curr < previous)
> +		if (curr < previous) {
>  			DRM_ERROR("CMD: table not sorted ring=%d entry=%d reg=0x%08X prev=0x%08X\n",
>  				  ring_id, i, curr, previous);
> +			ret = false;
> +		}
>  
>  		previous = curr;
>  	}
> +
> +	return ret;
>  }
>  
> -static void validate_regs_sorted(struct intel_ring_buffer *ring)
> +static bool validate_regs_sorted(struct intel_ring_buffer *ring)
>  {
> -	check_sorted(ring->id, ring->reg_table, ring->reg_count);
> -	check_sorted(ring->id, ring->master_reg_table, ring->master_reg_count);
> +	return check_sorted(ring->id, ring->reg_table, ring->reg_count) &&
> +		check_sorted(ring->id, ring->master_reg_table,
> +			     ring->master_reg_count);
>  }
>  
>  /**
> @@ -617,8 +628,8 @@ void i915_cmd_parser_init_ring(struct intel_ring_buffer *ring)
>  		BUG();
>  	}
>  
> -	validate_cmds_sorted(ring);
> -	validate_regs_sorted(ring);
> +	BUG_ON(!validate_cmds_sorted(ring));
> +	BUG_ON(!validate_regs_sorted(ring));
>  }
>  
>  static const struct drm_i915_cmd_descriptor*

Does any code actually rely on the tables being sorted?

I didn't see any early breaks or returns once the parser has gone past a
command...it seems to keep looking through the entire list.

As an aside:

It seems like find_cmd could be implemented a lot more efficiently.
Currently, most 3DSTATE commands are not in the tables - they only
appear to contain commands that need special handling.  This means that
find_cmd will walk through every command in every table before falling
back to the default info struct.

As an example, my window manager (KWin) submitted a render ring batch
with 700 commands in it.  Most of those commands need to walk through
every command in every table, which is about 48 entries.  That's 33,600
iterations through tables, which seems...kind of excessive.

For example, 3D commands all have the form 0x78xx or 0x79xx.  We could
use the xx as an array index into a lookup table, and get O(1) access
instead of O(n).  Or, at least something similar.

Thanks again for your work on this, Brad.

--Ken

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140327/2c9dc840/attachment.sig>