On Jul 5, 2016 11:13 PM, "Pohjolainen, Topi" <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> wrote: > > On Tue, Jul 05, 2016 at 07:12:49AM -0700, Jason Ekstrand wrote: > > On Jul 5, 2016 6:32 AM, "Pohjolainen, Topi" > > <[1]<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> wrote: > > > > > > On Wed, Jun 29, 2016 at 05:14:51PM -0700, Jason Ekstrand wrote: > > > > Reviewed-by: Topi Pohjolainen <[2]<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> > > > > > > > > v3: Stomp more surface state fields > > > > --- > > > > src/mesa/drivers/dri/i965/brw_blorp.c | 163 > > ++++++++++++++++++++++++++++++++++ > > > > src/mesa/drivers/dri/i965/brw_blorp.h | 6 ++ > > > > 2 files changed, 169 insertions(+) > > > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c > > b/src/mesa/drivers/dri/i965/brw_blorp.c > > > > index 9590968..275f10c 100644 > > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.c > > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c > > > > @@ -240,6 +240,169 @@ brw_blorp_compile_nir_shader(struct > > brw_context *brw, struct nir_shader *nir, > > > > return program; > > > > } > > > > > > > > +static enum isl_msaa_layout > > > > +get_isl_msaa_layout(enum intel_msaa_layout layout) > > > > +{ > > > > + switch (layout) { > > > > + case INTEL_MSAA_LAYOUT_NONE: > > > > + return ISL_MSAA_LAYOUT_NONE; > > > > + case INTEL_MSAA_LAYOUT_IMS: > > > > + return ISL_MSAA_LAYOUT_INTERLEAVED; > > > > + case INTEL_MSAA_LAYOUT_UMS: > > > > + case INTEL_MSAA_LAYOUT_CMS: > > > > + return ISL_MSAA_LAYOUT_ARRAY; > > > > + default: > > > > + unreachable("Invalid MSAA layout"); > > > > + } > > > > +} > > > > + > > > > +struct surface_state_info { > > > > + unsigned num_dwords; > > > > + unsigned ss_align; /* Required alignment of > > RENDER_SURFACE_STATE in bytes */ > > > > + unsigned reloc_dw; > > > > + unsigned aux_reloc_dw; > > > > + unsigned tex_mocs; > > > > + unsigned rb_mocs; > > > > +}; > > > > + > > > > +static const struct surface_state_info surface_state_infos[] = { > > > > + [6] = {6, 32, 1, 0}, > > > > + [7] = {8, 32, 1, 6, GEN7_MOCS_L3, GEN7_MOCS_L3}, > > > > + [8] = {13, 64, 8, 10, BDW_MOCS_WB, BDW_MOCS_PTE}, > > > > + [9] = {16, 64, 8, 10, SKL_MOCS_WB, SKL_MOCS_PTE}, > > > > +}; > > > > + > > > > +uint32_t > > > > +brw_blorp_emit_surface_state(struct brw_context *brw, > > > > + const struct brw_blorp_surface_info > > *surface, > > > > + uint32_t read_domains, uint32_t > > write_domain, > > > > + bool is_render_target) > > > > +{ > > > > + /* TODO: This should go in the context */ > > > > + struct isl_device isl_dev; > > > > + isl_device_init(&isl_dev, brw->intelScreen->devinfo, > > brw->has_swizzling); > > > > + > > > > + const struct surface_state_info ss_info = > > surface_state_infos[brw->gen]; > > > > + > > > > + struct isl_surf surf; > > > > + intel_miptree_get_isl_surf(brw, surface->mt, &surf); > > > > + > > > > + /* Stomp surface dimensions and tiling (if needed) with info > > from blorp */ > > > > + surf.dim = ISL_SURF_DIM_2D; > > > > + surf.dim_layout = ISL_DIM_LAYOUT_GEN4_2D; > > > > + surf.msaa_layout = get_isl_msaa_layout(surface->msaa_layout); > > > > + surf.logical_level0_px.width = surface->width; > > > > + surf.logical_level0_px.height = surface->height; > > > > + surf.logical_level0_px.depth = 1; > > > > + surf.logical_level0_px.array_len = 1; > > > > > > You added these two lines which make sense. You didn't need them > > before, did > > > anything change or just assertions firing? > > > > The newly added X/YOffset assertions require this. > > > > > > + surf.levels = 1; > > > > + surf.samples = MAX2(surface->num_samples, 1); > > > > + > > > > + /* Alignment doesn't matter since we have 1 miplevel and 1 > > array slice so > > > > + * just pick something that works for everybody. > > > > + */ > > > > + surf.image_alignment_el = isl_extent3d(4, 4, 1); > > > > > > Same here, looks sane but makes me womnder what made you add this? > > > > There are two cases where we can end up with an alignment which is > > invalid for 3D surfaces: gen6 stencil and gen9 1D. In > > Intel_mipmap_tree, gen9 1D surfaces have an invalid halign value and we > > just special-case gen9 1D to use 64 everywhere. When I started using > > ISL for offset calculations in blorp, I needed image_alignment_el to > > have the correct value of (64, 1, 1) so I had to add that to > > miptree_get_isl_surf which meant we now had to undo it here. > > But this patch would still work without this, or am I misunderstanding? We > still call brw_blorp_compute_tile_offsets() down below. In fact, I don't > really understand why setting image_alignment_el in miptree_get_isl_surf() > would help isl-based offset calculation either. That is going to replace > the blorp specific call below, and we are overriding the calculation > of miptree_get_isl_surf() here. As part of the blorp transition, we need isl_surf_get_image_offset_el to work. That requires that we have assignments. Another way to think about this is just that this gets the resulting isl_surf closer to what isl itself would produce. > I suppose there is just too many things happening the same time for me to keep > track. Some preparing for future patches and some needed locally... Yeah... This mess is tricky... > > > > > > + > > > > + if (brw->gen == 6 && surface->num_samples > 1) { > > > > + /* Since gen6 uses INTEL_MSAA_LAYOUT_IMS, width and height > > are measured > > > > + * in samples. But SURFACE_STATE wants them in pixels, so > > we need to > > > > + * divide them each by 2. > > > > + */ > > > > + surf.logical_level0_px.width /= 2; > > > > + surf.logical_level0_px.height /= 2; > > > > + } > > > > + > > > > + if (brw->gen == 6 && surf.image_alignment_el.height > 4) { > > > > + /* This can happen on stencil buffers on Sandy Bridge due to > > the > > > > + * single-LOD work-around. It's fairly harmless as long as > > we don't > > > > + * pass a bogus value into isl_surf_fill_state(). > > > > + */ > > > > + surf.image_alignment_el = isl_extent3d(4, 2, 1); > > > > > > Earlier you set this unconditionally to isl_extent3d(4, 4, 1), isn't > > that > > > valid? > > > > Yes, I believe this can go now. > > > > > > + } > > > > + > > > > + if (surface->map_stencil_as_y_tiled) { > > > > + /* We need to fake W-tiling with Y-tiling */ > > > > + surf.tiling = ISL_TILING_Y0; > > > > + surf.row_pitch *= 2; > > > > + } > > > > + > > > > + union isl_color_value clear_color = { .u32 = { 0, 0, 0, 0 } }; > > > > + > > > > + struct isl_surf *aux_surf = NULL, aux_surf_s; > > > > + uint64_t aux_offset = 0; > > > > + enum isl_aux_usage aux_usage = ISL_AUX_USAGE_NONE; > > > > + if (surface->mt->mcs_mt) { > > > > + /* We should probably to similar stomping to above but most > > of the aux > > > > + * surf gets ignored when we fill out the surface state > > anyway so > > > > + * there's no point. > > > > + */ > > > > + intel_miptree_get_aux_isl_surf(brw, surface->mt, > > &aux_surf_s, &aux_usage); > > > > + aux_surf = &aux_surf_s; > > > > + assert(surface->mt->mcs_mt->offset == 0); > > > > + aux_offset = surface->mt->mcs_mt->bo->offset64; > > > > + > > > > + /* We only really need a clear color if we also have an > > auxiliary > > > > + * surface. Without one, it does nothing. > > > > + */ > > > > + clear_color = intel_miptree_get_isl_clear_color(brw, > > surface->mt); > > > > + } > > > > + > > > > + struct isl_view view = { > > > > + .format = surface->brw_surfaceformat, > > > > + .base_level = 0, > > > > + .levels = 1, > > > > + .base_array_layer = 0, > > > > + .array_len = 1, > > > > + .channel_select = { > > > > + ISL_CHANNEL_SELECT_RED, > > > > + ISL_CHANNEL_SELECT_GREEN, > > > > + ISL_CHANNEL_SELECT_BLUE, > > > > + ISL_CHANNEL_SELECT_ALPHA, > > > > + }, > > > > + .usage = is_render_target ? ISL_SURF_USAGE_RENDER_TARGET_BIT > > : > > > > + ISL_SURF_USAGE_TEXTURE_BIT, > > > > + }; > > > > + > > > > + uint32_t offset, tile_x, tile_y; > > > > + offset = brw_blorp_compute_tile_offsets(surface, &tile_x, > > &tile_y); > > > > + > > > > + uint32_t surf_offset; > > > > + uint32_t *dw = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, > > > > + ss_info.num_dwords * 4, > > ss_info.ss_align, > > > > + &surf_offset); > > > > + > > > > + const uint32_t mocs = is_render_target ? ss_info.rb_mocs : > > ss_info.tex_mocs; > > > > + > > > > + isl_surf_fill_state(&isl_dev, dw, .surf = &surf, .view = &view, > > > > + .address = surface->mt->bo->offset64 + > > offset, > > > > + .aux_surf = aux_surf, .aux_usage = > > aux_usage, > > > > + .aux_address = aux_offset, > > > > + .mocs = mocs, .clear_color = clear_color, > > > > + .x_offset_sa = tile_x, .y_offset_sa = > > tile_y); > > > > + > > > > + /* Emit relocation to surface contents */ > > > > + drm_intel_bo_emit_reloc(brw->[3]<a href="http://batch.bo">batch.bo</a>, > > > > + surf_offset + ss_info.reloc_dw * 4, > > > > + surface->mt->bo, > > > > + dw[ss_info.reloc_dw] - > > surface->mt->bo->offset64, > > > > + read_domains, write_domain); > > > > + > > > > + 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. > > > > + */ > > > > + assert((aux_offset & 0xfff) == 0); > > > > + drm_intel_bo_emit_reloc(brw->[4]<a href="http://batch.bo">batch.bo</a>, > > > > + surf_offset + ss_info.aux_reloc_dw * > > 4, > > > > + surface->mt->mcs_mt->bo, > > > > + dw[ss_info.aux_reloc_dw] & 0xfff, > > > > + read_domains, write_domain); > > > > + } > > > > + > > > > + return surf_offset; > > > > +} > > > > + > > > > /** > > > > * Perform a HiZ or depth resolve operation. > > > > * > > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h > > b/src/mesa/drivers/dri/i965/brw_blorp.h > > > > index 7ec5875..bacdecb 100644 > > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.h > > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.h > > > > @@ -372,6 +372,12 @@ brw_blorp_compile_nir_shader(struct > > brw_context *brw, struct nir_shader *nir, > > > > struct brw_blorp_prog_data > > *prog_data, > > > > unsigned *program_size); > > > > > > > > +uint32_t > > > > +brw_blorp_emit_surface_state(struct brw_context *brw, > > > > + const struct brw_blorp_surface_info > > *surface, > > > > + uint32_t read_domains, uint32_t > > write_domain, > > > > + bool is_render_target); > > > > + > > > > void > > > > gen6_blorp_init(struct brw_context *brw); > > > > > > > > -- > > > > 2.5.0.400.gff86faf > > > > > > > > _______________________________________________ > > > > mesa-dev mailing list > > > > [5]<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > > [6]<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > References > > > > 1. mailto:<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> > > 2. mailto:<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> > > 3. <a href="http://batch.bo/">http://batch.bo/</a> > > 4. <a href="http://batch.bo/">http://batch.bo/</a> > > 5. mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > 6. <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>