[Mesa-dev] [PATCH v4 19/34] i965/state: Add a helper for emitting a surface state using isl

[Chad Versace]

On Thu 14 Jul 2016, Chad Versace wrote:
> On Wed 13 Jul 2016, Jason Ekstrand wrote:
> > Reviewed-by: Topi Pohjolainen <topi.pohjolainen at 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.

> > +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.versace at intel.com>

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.