[Mesa-dev] [PATCH 6/7] i965/cs: Implement brw_emit_gpgpu_walker

[Kenneth Graunke]

On Tuesday, April 28, 2015 12:04:50 AM Jordan Justen wrote:
> On 2015-04-27 19:02:38, Kenneth Graunke wrote:
> > On Friday, April 24, 2015 04:33:43 PM Jordan Justen wrote:
> > > +   BEGIN_BATCH(dwords);
> > > +   OUT_BATCH(GPGPU_WALKER << 16 | (dwords - 2));
> > 
> > I was going to suggest splitting this into separate Gen8+ and Gen7
> > blocks, but now that I look at the code...these two are slightly
> > different indirect handling, and the later one is just a DWord of MBZ,
> > so...it's not really that different.  I think what you have is fine :)
> 
> Hmm. Maybe time to press my luck. :)
> 
> In my other 20 patch series
> "[PATCH v2 19/20] i965/cs: Upload brw_cs_state"
> 
> We discussed this somewhat ugly code:
> > +   int dw = 0;
> > +   desc[dw++] = brw->cs.base.prog_offset;
> > +   if (brw->gen >= 8)
> > +      dw++; /* Kernel Start Pointer High */
> > +   dw++;
> > +   dw++;
> > +   desc[dw++] = stage_state->bind_bo_offset;
> 
> It turns out it eventually doesn't look quite so pointless to use the
> dw var. Later, it would look like:
> 
> http://cgit.freedesktop.org/~jljusten/mesa/tree/src/mesa/drivers/dri/i965/brw_cs.cpp?h=cs-27#n392
> 
> Basically, the structure is pretty similar, but an extra dword appears
> for the high address in gen8.
> 
> If it seems cleaner, I wouldn't mind splitting either or both of these
> to be initialized in separate paths based on the gen.
> 
> Does the link above change your opinion on the other patch?
> 
> Thanks for your time,
> 
> -Jordan

I guess that's fine.  In the OUT_BATCH paradigm, we'd do things like:

   if (brw->gen >= 8) {
       OUT_RELOC64(...)
   } else {
       OUT_RELOC(...)
   }

which effectively does the same thing.  But here you're writing indirect
state, so you have to track dwords yourself.

How about just changing 'dw++' to something like:

   desc[dw++] = 0; /* MBZ */
   desc[dw++] = 0; /* Kernel Start Pointer High */

I feel like that makes it readily apparent that you're just filling in
DWords in order, and not doing anything fancy.

Thoughts?
--Ken
-------------- 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/20150428/39015f97/attachment.sig>