[Mesa-dev] [PATCH 3/3] i965: Port Gen4-5 VS_STATE to genxml.

[Kenneth Graunke]

On Thursday, May 11, 2017 9:54:13 AM PDT Rafael Antognolli wrote:
> On Wed, May 10, 2017 at 12:41:39PM -0700, Kenneth Graunke wrote:
[snip]
> > -   struct brw_vs_unit_state *vs;
> 
> It looks like brw_vs_unit_state is not used anywhere else, so maybe we could
> delete it too. Also, it looks weird that some fields in vs->thread0 don't
> really match the dw1 in the docs for gen5 at least, but they are not used
> anyway.

Good call, I'll delete that!

[snip]
> > -const struct brw_tracked_state brw_vs_unit = {
> > -   .dirty = {
> > -      .mesa  = 0,
> > -      .brw   = BRW_NEW_BATCH |
> 
> If I understood correctly, BRW_NEW_BATCH will get covered by BRW_NEW_CONTEXT
> when needed, right?

BRW_NEW_BATCH is necessary here - VS_STATE is indirect state stuffed
into the "data" portion of the batch via brw_state_batch().  It's
pointed to via 3DSTATE_PIPELINED_POINTERS.  We need to re-emit VS_STATE
and 3DSTATE_PIPELINED_POINTERS on every new batch, so we don't have
dangling pointers to previous batches.

BRW_NEW_BATCH is stronger than BRW_NEW_CONTEXT - if you have
BRW_NEW_BATCH you don't need BRW_NEW_CONTEXT.

[snip]
> > +#if GEN_GEN < 6
> > +      vs.GRFRegisterCount = DIV_ROUND_UP(vue_prog_data->total_grf, 16) - 1;
> > +      vs.ConstantURBEntryReadLength = stage_prog_data->curb_read_length;
> > +      vs.ConstantURBEntryReadOffset = brw->curbe.vs_start * 2;
> > +
> > +      vs.NumberofURBEntries = brw->urb.nr_vs_entries >> (GEN_GEN == 5 ? 2 : 0);
> 
> I was looking and it seems that NumberofURBEntries should be bits 11:17, while
> bit 18 should be MBZ. It shouldn't matter much here, just the checks that
> genX_pack.h do. Also it's my fault, since I submitted the xml without checking
> everything, so I can send a patch to fix that if you prefer.

Oh, good call - "Number of URB Entries" is 18:11 on Gen4, but 17:11 with
18 MBZ on G45 and Gen5.  At least in VS_STATE (I didn't check GS_STATE).

Feel free to send a patch to fix that.  Thanks!

> Anyway, this patch is:
> Reviewed-by: Rafael Antognolli <rafael.antognolli at intel.com>

Awesome, thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170511/a5948001/attachment.sig>