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 <[email protected]> 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.
_______________________________________________
mesa-dev mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
