[Mesa-dev] [PATCH 3/8] i965: Store the atoms directly in the context

[Kenneth Graunke]

On Sunday, December 21, 2014 09:19:53 PM Jason Ekstrand wrote:
> On Sun, Dec 21, 2014 at 9:11 PM, Kenneth Graunke <kenneth at whitecape.org>
> wrote:
> >
> > On Friday, December 19, 2014 02:20:54 PM Ian Romanick wrote:
> > > From: Ian Romanick <ian.d.romanick at intel.com>
> > >
> > > Instead of having an extra pointer indirection in one of the hottest
> > > loops in the driver.
> > >
> > > On Bay Trail-D using Fedora 20 compile flags (-m64 -O2 -mtune=generic
> > > for 64-bit and -m32 -march=i686 -mtune=atom for 32-bit), affects
> > > Gl32Batch7:
> > >
> > > 32-bit: Difference at 95.0% confidence 1.98515% +/- 0.20814% (n=40)
> > > 64-bit: Difference at 95.0% confidence 1.5163% +/- 0.811016% (n=60)
> > >
> > > Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_context.h      |  2 +-
> > >  src/mesa/drivers/dri/i965/brw_state_upload.c | 19 ++++++++++++++++---
> > >  2 files changed, 17 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> > b/src/mesa/drivers/dri/i965/brw_context.h
> > > index 92eb022..5f5f807 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > > @@ -1384,7 +1384,7 @@ struct brw_context
> > >     } perfmon;
> > >
> > >     int num_atoms;
> > > -   const struct brw_tracked_state **atoms;
> > > +   const struct brw_tracked_state atoms[64];
> >
> > Making this atoms[57] would save memory, and the static asserts make it
> > easy
> > to remember to increase the size if we ever add additional atoms.
> >
> > Honestly, it seems like there should be a better way to do this.
> > Currently,
> > the atoms list only depends on the generation number, and thus is identical
> > for all GL contexts.  Storing an additional 1.5 kB of redundant data in
> > each
> > GL context seems a bit wasteful.
> >
> > Would it help to make brw_upload_state cache the array and size?
> >
> > void
> > brw_upload_state(struct brw_context *brw)
> > {
> >    static const struct brw_tracked_state *atoms = brw->atoms;
> >    static const int num_atoms = brw->num_atoms;
> >
> >    ...
> >
> >    for (i = 0; i < num_atoms; i++) {
> >       ...use atoms[i]...
> >    }
> >
> >    ...
> > }
> >
> > It seems like that would achieve the same effect - one indirection.
> >
> 
> Yeah, but then we have deal with threading issues.  It might be ok, but I
> think we'd have to be a bit more careful than that.

Should be doable.  But yeah, I guess it is kind of hacky.

> > One downside to that approach is that we couldn't have separate atom
> > lists for API_CORE vs. API_COMPAT (while we could using your approach).
> > (We could skip stippling for core contexts, and skip GS/HS/DS for legacy
> > contexts, which might help a tiny bit...)
> >
> 
> Another option would be to make the atoms list a screen thing and do the
> copy at screen creation time.  That would allow it to be shared across
> contexts without the above threading issues.  If we wanted, we could store
> one core version and one compat version and have the context point to
> whichever one it needs.  The problem with this is that it's wasteful in the
> one-context case where we would have both core an compat lists.  I guess
> you can't win them all.

Ah, store the actual atoms, like Ian's doing...but in the screen.

That's not a crazy plan either.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141221/dd6d8178/attachment.sig>