[Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic

[Volkin, Bradley D]

On Fri, Feb 07, 2014 at 06:45:48AM -0800, Daniel Vetter wrote:
> On Fri, Feb 07, 2014 at 03:58:46PM +0200, Jani Nikula wrote:
> > On Wed, 29 Jan 2014, bradley.d.volkin at intel.com wrote:
> > > +static int valid_reg(const u32 *table, int count, u32 addr)
> > > +{
> > > +	if (table && count != 0) {
> > > +		int i;
> > > +
> > > +		for (i = 0; i < count; i++) {
> > > +			if (table[i] == addr)
> > > +				return 1;
> > > +		}
> > > +	}
> > 
> > You go to great lengths to validate the register tables are sorted, but
> > in the end you don't take advantage of this fact by bailing out early if
> > the lookup goes past the addr.
> > 
> > Is this optimization the main reason for having the tables sorted, or
> > are there other reasons too (I couldn't find any)?
> > 
> > I'm beginning to wonder if this is a premature optimization that adds
> > extra code. For master restricted registers you will always scan the
> > regular reg table completely first. Perhaps a better option would be to
> > have all registers in the same table, with a separate master flag,
> > ordered by how frequently they are expected to be used. We do want to
> > optimize for the happy day scenario. But maybe it's too early to tell.
> > 
> > I'm inclined to ripping out the sort requirement and check, if the sole
> > purpose is optimization, for simplicity's sake.
> 
> tbh I don't mind the sorting requirement, and iirc Brad has patches
> already for binary search. Once we start to rely on the sorting we can
> easily add a little functions which checks for that at ring
> initialization, so I also don't see any concerns wrt code fragility.

Sorry for the delayed response. The background here is that I originally
just had the tables sorted with a comment to say as much. The idea was that
if the linear search became an issue, switching algorithms would be easier.
Chris suggested just moving to bsearch and checking that the tables are sorted
as part of the v1 series review. I implemented bsearch and found that the perf
change was the same to slightly worse. So I added the sorting check and kept
the linear search until we have better data.

Thanks,
Brad

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch