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

Daniel Vetter
2014-02-07T14:45:52Z

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