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

[Jason Ekstrand]

On Fri, Jul 15, 2016 at 2:30 PM, Chad Versace <chad.versace at intel.com>
wrote:

> 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.
>

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.versace at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160715/ce0d8b6a/attachment.html>