[Mesa-dev] [PATCH] i965: Replace structs with bit-shifting for Gen7 SURFACE_STATE entries.

[Eric Anholt]

This mostly looks really good, and I expect it to be a nice small
performance win.  Just a few comments.

Kenneth Graunke <kenneth at whitecape.org> writes:
>  static void dump_gen7_surface_state(struct brw_context *brw, uint32_t offset)
>  {
>     const char *name = "SURF";
> -   struct gen7_surface_state *surf = brw->intel.batch.bo->virtual + offset;
> +   uint32_t *surf = brw->intel.batch.bo->virtual + offset;
>  
>     batch_out(brw, name, offset, 0, "%s %s\n",
> -	     get_965_surfacetype(surf->ss0.surface_type),
> -	     get_965_surface_format(surf->ss0.surface_format));
> +             get_965_surfacetype(GET_FIELD(surf[0], BRW_SURFACE_TYPE)),
> +             get_965_surface_format(GET_FIELD(surf[0], BRW_SURFACE_FORMAT)));
>     batch_out(brw, name, offset, 1, "offset\n");
>     batch_out(brw, name, offset, 2, "%dx%d size, %d mips\n",
> -	     surf->ss2.width + 1, surf->ss2.height + 1, surf->ss5.mip_count);
> +             GET_FIELD(surf[2], BRW_SURFACE_WIDTH) + 1,
> +             GET_FIELD(surf[2], BRW_SURFACE_HEIGHT) + 1,
> +             surf[5] & INTEL_MASK(3, 0));

GEN7_SURFACE_{HEIGHT,WIDTH} here?

>     if (surface->msaa_layout == INTEL_MSAA_LAYOUT_CMS) {
> -      gen7_set_surface_mcs_info(brw, surf, wm_surf_offset,
> -                                surface->mt->mcs_mt, is_render_target);
> +      surf[6] = gen7_surface_mcs_info(brw, wm_surf_offset, surface->mt->mcs_mt,
> +                                      is_render_target);

> -void
> -gen7_set_surface_mcs_info(struct brw_context *brw,
> -                          struct gen7_surface_state *surf,
> -                          uint32_t surf_offset,
> -                          const struct intel_mipmap_tree *mcs_mt,
> -                          bool is_render_target)
> +uint32_t
> +gen7_surface_mcs_info(struct brw_context *brw,
> +                      uint32_t surf_offset,
> +                      const struct intel_mipmap_tree *mcs_mt,
> +                      bool is_render_target)
>  {
>     /* From the Ivy Bridge PRM, Vol4 Part1 p76, "MCS Base Address":
>      *
> @@ -125,26 +121,33 @@ gen7_set_surface_mcs_info(struct brw_context *brw,
>      * the necessary address translation.
>      */
>     assert ((mcs_mt->region->bo->offset & 0xfff) == 0);
> -   surf->ss6.mcs_enabled.mcs_enable = 1;
> -   surf->ss6.mcs_enabled.mcs_surface_pitch = pitch_tiles - 1;
> -   surf->ss6.mcs_enabled.mcs_base_address = mcs_mt->region->bo->offset >> 12;
> +
> +   uint32_t ss6 =
> +      GEN7_SURFACE_MCS_ENABLE |
> +      SET_FIELD(pitch_tiles - 1, GEN7_SURFACE_MCS_PITCH) |
> +      mcs_mt->region->bo->offset;
> +
>     drm_intel_bo_emit_reloc(brw->intel.batch.bo,
> -                           surf_offset +
> -                           offsetof(struct gen7_surface_state, ss6),
> +                           surf_offset + 6 * 4,
>                             mcs_mt->region->bo,
> -                           surf->ss6.raw_data & 0xfff,
> +                           ss6 & 0xfff,
>                             is_render_target ? I915_GEM_DOMAIN_RENDER
>                             : I915_GEM_DOMAIN_SAMPLER,
>                             is_render_target ? I915_GEM_DOMAIN_RENDER : 0);
> +   return ss6;
>  }

I don't like this change, where gen7_surface_mcs_info emits a reloc
(which means that surf[6] must have exactly the value returned by
gen7_surface_mcs_info), but has the caller actually store it to surf[6].
I think this function should continue to set ss6, and the caller doesn't
get to mess with it.

>  void
> -gen7_check_surface_setup(struct gen7_surface_state *surf,
> -                         bool is_render_target)
> +gen7_check_surface_setup(uint32_t *surf, bool is_render_target)
>  {
> -   bool is_multisampled =
> -      surf->ss4.num_multisamples != GEN7_SURFACE_MULTISAMPLECOUNT_1;
> +   unsigned num_multisamples = surf[4] & INTEL_MASK(5, 3);
> +   unsigned multisampled_surface_storage_format = surf[4] & (1 << 6);
> +   unsigned surface_array_spacing = surf[0] & (1 << 10);
> +   bool is_multisampled = num_multisamples != GEN7_SURFACE_MULTISAMPLECOUNT_1;
> +
> +   (void) surface_array_spacing;
> +
>     /* From the Graphics BSpec: vol5c Shared Functions [SNB+] > State >
>      * SURFACE_STATE > SURFACE_STATE for most messages [DevIVB]: Surface Array
>      * Spacing:
> @@ -153,9 +156,9 @@ gen7_check_surface_setup(struct gen7_surface_state *surf,
>      *   Multisamples is not MULTISAMPLECOUNT_1, this field must be set to
>      *   ARYSPC_LOD0.
>      */
> -   if (surf->ss4.multisampled_surface_storage_format == GEN7_SURFACE_MSFMT_MSS
> +   if (multisampled_surface_storage_format == GEN7_SURFACE_MSFMT_MSS
>         && is_multisampled)
> -      assert(surf->ss0.surface_array_spacing == GEN7_SURFACE_ARYSPC_LOD0);
> +      assert(surface_array_spacing == GEN7_SURFACE_ARYSPC_LOD0);

Perhaps have INTEL_MASK style defines for ARYSPC so you can GET_FIELD
instead of having the temporary value that then gets (void) treatment
above to avoid warnings?  It means less magic number usage in this
function. :)

(Side note: Having been doing a bunch of release builds recently for
performance work, the number of warnings we have due to things used only
in assert()s is really irritating and I wish there was some good general
solution for it)

> @@ -507,22 +471,17 @@ gen7_update_renderbuffer_surface(struct brw_context *brw,

> +   if (irb->mt->align_h == 4)
> +      surf[0] |= GEN7_SURFACE_VALIGN_4;
> +   if (irb->mt->align_w == 8)
> +      surf[0] |= GEN7_SURFACE_HALIGN_8;

Side note: I'm tempted to go call the align_h/w fields either
align_height/align_width or valign/halign, because I had to read "if
(align_h == 4) surf[0] |= GEN7_SURFACE_VALIGN_4" a few times before it
made sense again.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130103/7dc10ae2/attachment.pgp>