Re: [PATCH 02/13] drm/i915: Implement command buffer parsing logic

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 11 Feb 2014, "Volkin, Bradley D" <bradley.d.volkin@intel.com> wrote:
> 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@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.

Ok. For the linear search I think you could add the check if you've
iterated past the register and bail out early, and gather the data with
that.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center