On 7 December 2011 00:50, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div>On 12/05/2011 09:40 AM, Paul Berry wrote:<br> > This patch series introduces a geometry shader program for i965 Gen6<br> > (Sandy Bridge) that does nothing--it simply passes vertices through to<br> > later stages of the graphics pipeline. This is a preliminary step<br> > towards implementing transform feedback, since on Gen6, transform<br> > feedback is accomplished by having the geometry shader write vertices<br> > to memory.<br> ><br> > Patch 01/10 is a core mesa patch--it introduces a new bit to the<br> > ctx->NewState bitfield to track when changes to the transform feedback<br> > setup have occurred. This allows the i965 back-end to avoid<br> > unnecessary recomputations when the transform feedback setup is<br> > unchanged.<br> ><br> > Patches 02/10-09/10 refactor the i965 driver in preparation for adding<br> > the new geometry shader program. In particular, they clean up the old<br> > (Gen4-Gen5) geometry shader code so that it can be expanded, and fix<br> > some code generation bugs that had previously been benign. Note:<br> > patches 06/10 and 07/10 are from Kenneth Graunke's "gs2" branch, which<br> > I used as the starting point for my development. They ensure that URB<br> > space is allocated for the GS unit so that the geometry shader program<br> > can run. Down the road, we may want to tweak the URB space allocation<br> > to improve performance, but that seems like it should wait until<br> > transform feedback functionality works.<br> ><br> > Patch 10/10 adds the pass-through program itself. At the moment it is<br> > not used (since transform feedback support isn't enabled by default<br> > yet), however it can be tested by setting the "INTEL_FORCE_GS"<br> > environment variable.<br> ><br> > [PATCH 01/10] mesa: Track changes to transform feedback state.<br> <br> </div></div>I agree with Eric's assessment about FLUSH_VERTICES. With that change,<br> Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>><br> <div><br> > [PATCH 02/10] i965 gs: Reduce information in key to avoid unnecessary recompiles.<br> <br> </div>No comment here; sounds like you and Eric have it figured out.<br> <div><br> > [PATCH 03/10] i965 gs: Remove unnecessary mapping of key->primitive.<br> > [PATCH 04/10] i965: Don't convert if/else to conditional adds on Gen6.<br> > [PATCH 05/10] i965: Fix convert_IF_ELSE_to_ADD for gen7.<br> <br> </div>These three are<br> Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>><br></blockquote><div><br>With regard to patch 04/10, I assume your review includes the change that extends this patch to Gen>=6 rather than Gen==6, right?<br> <br>Patch 05/10 is moot once patch 04/10 is extended to Gen>=6 so I'm just going to drop it.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><br> > [PATCH 06/10] i965: Set the maximum number of GS URB entries on Sandybridge.<br> > [PATCH 07/10] i965: Initial stab at GS URB space allocation.<br> <br> </div>No comment here.<br> <div><br> > [PATCH 08/10] i965 gs: Clean up dodgy register re-use, at the cost of a few MOVs.<br> <br> </div>Had comments/ideas; would love to see feedback from others. Still, it<br> looks correct, so you get a Reviewed-by.<br> <div><br> > [PATCH 09/10] i965: Clean up misleading defines for DWORD 2 of URB_WRITE header.<br> <br> </div>Made a small suggestion but either way you get a Reviewed-by.<br> <div><br> > [PATCH 10/10] i965 gen6: Implement pass-through GS for transform feedback.<br> <br> </div>Aside from the scary lack of GEN6_GS_RENDERING_ENABLE, you get a R-b.<br> <br> Thanks, Paul. This looks like a great series.<br> </blockquote></div><br>Thanks, Ken. With your review I believe everything is covered except for the compromise Eric and I arrived at for patch 07/10. Since I'd like to get some review on that, I'm going to go ahead and post a v2 of the series just so people can see it in context.<br>