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

[Kenneth Graunke]

On 01/03/2013 10:24 AM, Eric Anholt wrote:
> 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?

Oops.  Fixed, thanks.

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

Fine by me.  Changed in v2.

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

The reason I didn't do this is because GEN7_SURFACE_ARYSPC_LOD0 is (1 << 
10), and GET_FIELD would undo the shift, just returning me 1.  So they 
wouldn't be directly comparable.

There are a number of possible solutions to this, but none of them 
seemed particularly appealing, so I just did this.

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

Yeah.  I'm definitely in favor of renaming them. 
align_height/align_width are much better.  valign/halign would also be okay.