<div dir="ltr"><div>I'll second Topi's comment. Other than that, not much jumpped out at me. I did make a comment on 7 which also applies (somewhat) to 8 and I made another comment on 5. However, none of this is blockers so this series is </div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> </div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Mar 11, 2015 at 11:59 AM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wednesday, March 11, 2015 10:44:26 AM Pohjolainen, Topi wrote: > On Mon, Mar 09, 2015 at 01:58:51AM -0700, Kenneth Graunke wrote: > > The NIR backend hardcodes brw_wm_prog_key at the moment, which won't > > work when we support scalar VS. We could use get_tex(), but it's a > > static method. I was going to promote it to fs_visitor, but then > > realized that both parameters (stage and key) are already members. > > > > It then occured to me that we could just set up a pointer in the > > constructor, and skip having a function altogether. > > > > This patch also converts all existing users to use key_tex. > > > > Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > > With the few nits: > > Reviewed-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> > > > --- > > src/mesa/drivers/dri/i965/brw_fs.h | 2 + > > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 3 +- > > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 55 ++++++++++++---------------- > > 3 files changed, 27 insertions(+), 33 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h > > index ee6ba98..7f4916a 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs.h > > +++ b/src/mesa/drivers/dri/i965/brw_fs.h > > @@ -418,6 +418,8 @@ public: > > void visit_atomic_counter_intrinsic(ir_call *ir); > > > > const void *const key; > > + struct brw_sampler_prog_key_data *key_tex; > > This could be constant pointer as well, we only use it for reading in the > visitor. (Also prevents the need to cast the constant 'key' pointer to > non-constant in init()). </div></div>Good catch! I've changed it to: const struct brw_sampler_prog_key_Data *key_tex; and changed the casts in init() to be "const struct brw_wm_prog_key" and so on. I couldn't add the second 'const' (which prevents assignments to key_tex) since I can't really set this from a constructor initializer list - I need the switch statement in init(). But, this is probably the most important one. Also fixed the word wrapping. Thanks! _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div>