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

[Jason Ekstrand]

On Dec 22, 2014 2:49 PM, "Ian Romanick" <idr at freedesktop.org> wrote:
>
> On 12/19/2014 06:20 PM, Jason Ekstrand wrote:
> > I wrote basically this same patch when I was trying to speed up
> > basically the same test.  Unfortunately, I wasn't able to get much of an
> > improvement out of it.  Other than the one nit below,
> > Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com
> > <mailto:jason.ekstrand at intel.com>>
> >
> > On Fri, Dec 19, 2014 at 2:20 PM, Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >
> >     From: Ian Romanick <ian.d.romanick at intel.com
> >     <mailto: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
> >     <mailto: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];
> >
> >
> > Maybe a #define?
>
> Hmm... What I really wanted was to somehow use ARRAY_SIZE from the other
> arrays.  Since the define would only be used in one place, I don't think
> it's that helpful.

If you really care, you could do a bunch of MAX2 of ARRAY_SIZE and it
should evaluate as a constant expression.  I don't care that much.

>
> >         /* If (INTEL_DEBUG & DEBUG_BATCH) */
> >         struct {
> >     diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> >     b/src/mesa/drivers/dri/i965/brw_state_upload.c
> >     index a579781..9a5d49a 100644
> >     --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> >     +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> >     @@ -357,6 +357,11 @@ void brw_init_state( struct brw_context *brw )
> >         const struct brw_tracked_state **atoms;
> >         int num_atoms;
> >
> >     +   STATIC_ASSERT(ARRAY_SIZE(gen4_atoms) <= ARRAY_SIZE(brw->atoms));
> >     +   STATIC_ASSERT(ARRAY_SIZE(gen6_atoms) <= ARRAY_SIZE(brw->atoms));
> >     +   STATIC_ASSERT(ARRAY_SIZE(gen7_atoms) <= ARRAY_SIZE(brw->atoms));
> >     +   STATIC_ASSERT(ARRAY_SIZE(gen8_atoms) <= ARRAY_SIZE(brw->atoms));
> >     +
> >         brw_init_caches(brw);
> >
> >         if (brw->gen >= 8) {
> >     @@ -373,9 +378,17 @@ void brw_init_state( struct brw_context *brw )
> >            num_atoms = ARRAY_SIZE(gen4_atoms);
> >         }
> >
> >     -   brw->atoms = atoms;
> >         brw->num_atoms = num_atoms;
> >
> >     +   /* This is to work around brw_context::atoms being declared
> >     const.  We want
> >     +    * it to be const, but it needs to be initialized somehow!
> >     +    */
> >     +   struct brw_tracked_state *context_atoms =
> >     +      (struct brw_tracked_state *) &brw->atoms[0];
> >     +
> >     +   for (int i = 0; i < num_atoms; i++)
> >     +      context_atoms[i] = *atoms[i];
> >     +
> >         while (num_atoms--) {
> >            assert((*atoms)->dirty.mesa | (*atoms)->dirty.brw);
> >            assert((*atoms)->emit);
> >     @@ -607,7 +620,7 @@ void brw_upload_state(struct brw_context *brw)
> >            prev = *state;
> >
> >            for (i = 0; i < brw->num_atoms; i++) {
> >     -        const struct brw_tracked_state *atom = brw->atoms[i];
> >     +        const struct brw_tracked_state *atom = &brw->atoms[i];
> >              struct brw_state_flags generated;
> >
> >              if (check_state(state, &atom->dirty)) {
> >     @@ -627,7 +640,7 @@ void brw_upload_state(struct brw_context *brw)
> >         }
> >         else {
> >            for (i = 0; i < brw->num_atoms; i++) {
> >     -        const struct brw_tracked_state *atom = brw->atoms[i];
> >     +        const struct brw_tracked_state *atom = &brw->atoms[i];
> >
> >              if (check_state(state, &atom->dirty)) {
> >                 atom->emit(brw);
> >     --
> >     1.8.1.4
> >
> >     _______________________________________________
> >     mesa-dev mailing list
> >     mesa-dev at lists.freedesktop.org <mailto:
mesa-dev at lists.freedesktop.org>
> >     http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141222/9ebcc504/attachment.html>