This mostly looks really good, and I expect it to be a nice small performance win. Just a few comments.
Kenneth Graunke <kenn...@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.
pgpDZVprzY5tA.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev