<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 19, 2017 at 1:37 PM, Pohjolainen, Topi <<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Jul 19, 2017 at 01:27:16PM -0700, Jason Ekstrand wrote: > On Wed, Jul 19, 2017 at 12:51 PM, Topi Pohjolainen < > <a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote: > > > This makes intel_mipmap_tree::pitch and isl_surf::row_pitch > > semantically equivalent. > > > > Signed-off-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> > > --- > > src/mesa/drivers/dri/i965/gen7_misc_state.c | 12 +------ > > src/mesa/drivers/dri/i965/gen8_depth_state.c | 16 +-------- > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 50 > > ++++++++++----------------- > > 3 files changed, 20 insertions(+), 58 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > index 6c69fa8ba5..e189788a88 100644 > > --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > @@ -170,19 +170,9 @@ gen7_emit_depth_stencil_hiz(struct brw_context *brw, > > > > BEGIN_BATCH(3); > > OUT_BATCH(GEN7_3DSTATE_STENCIL_BUFFER << 16 | (3 - 2)); > > - /* The stencil buffer has quirky pitch requirements. From the > > - * Sandybridge PRM, Volume 2 Part 1, page 329 > > (3DSTATE_STENCIL_BUFFER > > - * dword 1 bits 16:0 - Surface Pitch): > > - * > > - * The pitch must be set to 2x the value computed based on > > width, as > > - * the stencil buffer is stored with two rows interleaved. > > - * > > - * While the Ivybridge PRM lacks this comment, the BSpec contains > > the > > - * same text, and experiments indicate that this is necessary. > > - */ > > OUT_BATCH(enabled | > > mocs << 25 | > > - (2 * stencil_mt->pitch - 1)); > > + (stencil_mt->pitch - 1)); > > OUT_RELOC(stencil_mt->bo, > > I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, > > 0); > > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c > > b/src/mesa/drivers/dri/i965/gen8_depth_state.c > > index 52c6dd0787..d05e1ba32e 100644 > > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c > > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c > > @@ -111,22 +111,8 @@ emit_depth_packets(struct brw_context *brw, > > } else { > > BEGIN_BATCH(5); > > OUT_BATCH(GEN7_3DSTATE_STENCIL_BUFFER << 16 | (5 - 2)); > > - /* The stencil buffer has quirky pitch requirements. From the > > Graphics > > - * BSpec: vol2a.11 3D Pipeline Windower > Early Depth/Stencil > > Processing > > - * > Depth/Stencil Buffer State > 3DSTATE_STENCIL_BUFFER [DevIVB+], > > - * field "Surface Pitch": > > - * > > - * The pitch must be set to 2x the value computed based on > > width, as > > - * the stencil buffer is stored with two rows interleaved. > > - * > > - * (Note that it is not 100% clear whether this intended to apply to > > - * Gen7; the BSpec flags this comment as "DevILK,DevSNB" (which > > would > > - * imply that it doesn't), however the comment appears on a > > "DevIVB+" > > - * page (which would imply that it does). Experiments with the > > hardware > > - * indicate that it does. > > - */ > > OUT_BATCH(HSW_STENCIL_ENABLED | mocs_wb << 22 | > > - (2 * stencil_mt->pitch - 1)); > > + (stencil_mt->pitch - 1)); > > OUT_RELOC64(stencil_mt->bo, > > I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0); > > OUT_BATCH(stencil_mt ? stencil_mt->qpitch >> 2 : 0); > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > index 925c67fc50..9244a35d4f 100644 > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > @@ -858,6 +858,18 @@ miptree_create(struct brw_context *brw, > > mt->surf.tiling), > > &mt->pitch, > > alloc_flags); > > + > > + /* The stencil buffer has quirky pitch requirements. From the > > + * Sandybridge PRM, Volume 2 Part 1, page 329 > > (3DSTATE_STENCIL_BUFFER > > + * dword 1 bits 16:0 - Surface Pitch): > > + * > > + * The pitch must be set to 2x the value computed based on > > width, as > > + * the stencil buffer is stored with two rows interleaved. > > + * > > + * While the Ivybridge PRM lacks this comment, the BSpec contains > > the > > + * same text, and experiments indicate that this is necessary. > > + */ > > + mt->pitch *= 2; </div></div>Here we make it to what gpu wants. > > } else { > > mt->bo = brw_bo_alloc_tiled_2d(brw->bufmgr, "miptree", > > mt->total_width, mt->total_height, > > @@ -2819,7 +2831,7 @@ intel_offset_S8(uint32_t stride, uint32_t x, > > uint32_t y, bool swizzled) > > uint32_t tile_size = 4096; > > uint32_t tile_width = 64; > > uint32_t tile_height = 64; > > - uint32_t row_size = 64 * stride; > > + uint32_t row_size = 64 * stride / 2; /* Two rows are interleaved. */ And here we take into account the "pitch-for-two-interleaved rows" for byte offset calculation. > > > > uint32_t tile_x = x / tile_width; > > uint32_t tile_y = y / tile_height; > > @@ -3197,12 +3209,8 @@ intel_miptree_map_s8(struct brw_context *brw, > > * temporary buffer back out. > > */ > > if (!(map->mode & GL_MAP_INVALIDATE_RANGE_BIT)) { > > - /* ISL uses a stencil pitch value that is expected by hardware > > whereas > > - * traditional miptree uses half of that. Below the value gets > > supplied > > - * to intel_offset_S8() which expects the legacy interpretation. > > - */ > > const unsigned pitch = mt->surf.size > 0 ? > > - mt->surf.row_pitch / 2 : mt->pitch; > > + mt->surf.row_pitch : mt->pitch; > > > > Wait, don't you want to add a "/ 2" to mt->pitch not remove it from > surf.row_pitch? It needs to get removed. See above. <div class="HOEnZb"><div class="h5"></div></div></blockquote><div> </div><div>Ok, I missed the interaction with offset_S8. This looks good </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > > > > uint8_t *untiled_s8_map = map->ptr; > > uint8_t *tiled_s8_map = intel_miptree_map_raw(brw, mt, > > GL_MAP_READ_BIT); > > unsigned int image_x, image_y; > > @@ -3239,12 +3247,8 @@ intel_miptree_unmap_s8(struct brw_context *brw, > > unsigned int slice) > > { > > if (map->mode & GL_MAP_WRITE_BIT) { > > - /* ISL uses a stencil pitch value that is expected by hardware > > whereas > > - * traditional miptree uses half of that. Below the value gets > > supplied > > - * to intel_offset_S8() which expects the legacy interpretation. > > - */ > > const unsigned pitch = mt->surf.size > 0 ? > > - mt->surf.row_pitch / 2: mt->pitch; > > + mt->surf.row_pitch : mt->pitch; > > > > Same here. > > > > unsigned int image_x, image_y; > > uint8_t *untiled_s8_map = map->ptr; > > uint8_t *tiled_s8_map = intel_miptree_map_raw(brw, mt, > > GL_MAP_WRITE_BIT); > > @@ -3352,12 +3356,8 @@ intel_miptree_map_depthstencil(struct brw_context > > *brw, > > * temporary buffer back out. > > */ > > if (!(map->mode & GL_MAP_INVALIDATE_RANGE_BIT)) { > > - /* ISL uses a stencil pitch value that is expected by hardware > > whereas > > - * traditional miptree uses half of that. Below the value gets > > supplied > > - * to intel_offset_S8() which expects the legacy interpretation. > > - */ > > const unsigned s_pitch = s_mt->surf.size > 0 ? > > - s_mt->surf.row_pitch / 2 : s_mt->pitch; > > + s_mt->surf.row_pitch : s_mt->pitch; > > > > And here. > > > > uint32_t *packed_map = map->ptr; > > uint8_t *s_map = intel_miptree_map_raw(brw, s_mt, GL_MAP_READ_BIT); > > uint32_t *z_map = intel_miptree_map_raw(brw, z_mt, GL_MAP_READ_BIT); > > @@ -3419,12 +3419,8 @@ intel_miptree_unmap_depthstencil(struct > > brw_context *brw, > > bool map_z32f_x24s8 = mt->format == MESA_FORMAT_Z_FLOAT32; > > > > if (map->mode & GL_MAP_WRITE_BIT) { > > - /* ISL uses a stencil pitch value that is expected by hardware > > whereas > > - * traditional miptree uses half of that. Below the value gets > > supplied > > - * to intel_offset_S8() which expects the legacy interpretation. > > - */ > > const unsigned s_pitch = s_mt->surf.size > 0 ? > > - s_mt->surf.row_pitch / 2 : s_mt->pitch; > > + s_mt->surf.row_pitch : s_mt->pitch; > > > > And here. > > > > uint32_t *packed_map = map->ptr; > > uint8_t *s_map = intel_miptree_map_raw(brw, s_mt, GL_MAP_WRITE_BIT); > > uint32_t *z_map = intel_miptree_map_raw(brw, z_mt, > > GL_MAP_WRITE_BIT); > > @@ -3738,17 +3734,7 @@ intel_miptree_get_isl_surf(struct brw_context *brw, > > mt->array_layout); > > surf->msaa_layout = mt->surf.msaa_layout; > > surf->tiling = intel_miptree_get_isl_tiling(mt); > > - > > - if (mt->format == MESA_FORMAT_S_UINT8) { > > - /* The ISL definition of row_pitch matches the surface state pitch > > field > > - * a bit better than intel_mipmap_tree. In particular, ISL > > incorporates > > - * the factor of 2 for W-tiling in row_pitch. > > - */ > > - surf->row_pitch = 2 * mt->pitch; > > - } else { > > - surf->row_pitch = mt->pitch; > > - } > > - > > + surf->row_pitch = mt->pitch; > > surf->format = translate_tex_format(brw, mt->format, false); > > > > if (brw->gen >= 9) { > > -- > > 2.11.0 > > > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > </div></div></blockquote></div> </div></div>