On Fri, Jul 15, 2016 at 2:30 PM, Chad Versace <chad.vers...@intel.com> wrote:
> On Thu 14 Jul 2016, Chad Versace wrote: > > On Wed 13 Jul 2016, Jason Ekstrand wrote: > > > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com> > > > --- > > > src/mesa/drivers/dri/i965/brw_state.h | 8 +++ > > > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 79 > ++++++++++++++++++++++++ > > > 2 files changed, 87 insertions(+) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_state.h > b/src/mesa/drivers/dri/i965/brw_state.h > > > index a16e876..91ebce9 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_state.h > > > +++ b/src/mesa/drivers/dri/i965/brw_state.h > > > @@ -274,6 +274,14 @@ GLuint translate_tex_format(struct brw_context > *brw, > > > int brw_get_texture_swizzle(const struct gl_context *ctx, > > > const struct gl_texture_object *t); > > > > > > > Move the forward declaration below to the top of the header. As written, > > it looks like the function's return type. > 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. :) > > > +struct isl_view; > > > +void brw_emit_surface_state(struct brw_context *brw, > > > + struct intel_mipmap_tree *mt, > > > + const struct isl_view *view, > > > + uint32_t mocs, bool for_gather, > > > + uint32_t *surf_offset, int surf_index, > > > + unsigned read_domains, unsigned > write_domains); > > Ping! Unresolved issue^^^ > > Fix that, and this patch is > Reviewed-by: Chad Versace <chad.vers...@intel.com> > Thanks! > > But keep reading... > > > > > > + if (aux_surf) { > > > + /* On gen7 and prior, the bottom 12 bits of the MCS base > address are > > > + * used to store other information. This should be ok, > however, because > > > + * surface buffer addresses are always 4K page alinged. > > > + */ > > > > Could you please insert the original comment from gen7_wm_surface_state? > > Because... > > > > > + assert((aux_offset & 0xfff) == 0); > > > + drm_intel_bo_emit_reloc(brw->batch.bo, > > > + *surf_offset + 4 * ss_info.aux_reloc_dw, > > > + mt->mcs_mt->bo, > dw[ss_info.aux_reloc_dw] & 0xfff, > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > ...that ^^^ bit of code REALLY confused me until I found the original > > comment. What's happening is obvious in retrospect, yet terribly obscure > > without a fuller comment. > > I prefer you insert the original comment, or at least some more > explanation. My r-b stands with or without a comment update. > I copied the original comment. The only change I made was to prefix it with "on gen7 and prior..." --Jason
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev