<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 15, 2016 at 2:30 PM, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Thu 14 Jul 2016, Chad Versace wrote:<br>
</span><span>> On Wed 13 Jul 2016, Jason Ekstrand wrote:<br>
> > Reviewed-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>><br>
> > ---<br>
> >  src/mesa/drivers/dri/i965/brw_state.h            |  8 +++<br>
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 79 ++++++++++++++++++++++++<br>
> >  2 files changed, 87 insertions(+)<br>
> ><br>
> > diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h<br>
> > index a16e876..91ebce9 100644<br>
> > --- a/src/mesa/drivers/dri/i965/brw_state.h<br>
> > +++ b/src/mesa/drivers/dri/i965/brw_state.h<br>
> > @@ -274,6 +274,14 @@ GLuint translate_tex_format(struct brw_context *brw,<br>
> >  int brw_get_texture_swizzle(const struct gl_context *ctx,<br>
> >                              const struct gl_texture_object *t);<br>
> ><br>
><br>
> Move the forward declaration below to the top of the header. As written,<br>
> it looks like the function's return type.<br></span></blockquote><div><br></div><div>I'll do one better.  Now that we have an isl_device in brw_context, we isl.h is included basically globally so there's no need for the forward declaration. :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> > +struct isl_view;<br>
> > +void brw_emit_surface_state(struct brw_context *brw,<br>
> > +                            struct intel_mipmap_tree *mt,<br>
> > +                            const struct isl_view *view,<br>
> > +                            uint32_t mocs, bool for_gather,<br>
> > +                            uint32_t *surf_offset, int surf_index,<br>
> > +                            unsigned read_domains, unsigned write_domains);<br>
<br>
</span>Ping! Unresolved issue^^^<br>
<br>
Fix that, and this patch is<br>
Reviewed-by: Chad Versace <<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>><br></blockquote><div><br></div><div>Thanks!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
But keep reading...<br>
<span><br>
<br>
<br>
> > +   if (aux_surf) {<br>
> > +      /* On gen7 and prior, the bottom 12 bits of the MCS base address are<br>
> > +       * used to store other information.  This should be ok, however, because<br>
> > +       * surface buffer addresses are always 4K page alinged.<br>
> > +       */<br>
><br>
> Could you please insert the original comment from gen7_wm_surface_state?<br>
> Because...<br>
><br>
> > +      assert((aux_offset & 0xfff) == 0);<br>
> > +      drm_intel_bo_emit_reloc(brw-><a href="http://batch.bo" rel="noreferrer" target="_blank">batch.bo</a>,<br>
> > +                              *surf_offset + 4 * ss_info.aux_reloc_dw,<br>
> > +                              mt->mcs_mt->bo, dw[ss_info.aux_reloc_dw] & 0xfff,<br>
>                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<br>
><br>
> ...that ^^^ bit of code REALLY confused me until I found the original<br>
> comment. What's happening is obvious in retrospect, yet terribly obscure<br>
> without a fuller comment.<br>
<br>
</span>I prefer you insert the original comment, or at least some more<br>
explanation. My r-b stands with or without a comment update.<br>
</blockquote></div><br></div><div class="gmail_extra">I copied the original comment.  The only change I made was to prefix it with "on gen7 and prior..."<br><br></div><div class="gmail_extra">--Jason<br></div></div>