<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 15, 2017 at 10:14 AM, 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">On Thu, Jun 15, 2017 at 09:41:42AM -0700, Jason Ekstrand wrote: > On Thu, Jun 15, 2017 at 9:36 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> > wrote: > > > This patch makes the assumption that, if stencil_irb != NULL then > > depth_irb != NULL. I did a bunch of digging and I am now convinced that > > this is a fine assumption. Specifically, in intel_renderbuffer_format() in > > intel_fbo.c, we map all STENCIL_INDEX internal formats to Z24_UNORM_S8_UINT > > on gen4-5. > > It shouldn't. <div><div class="m_-7605881083323770664h5"> > > Except that patch 1 only makes a difference if (stencil_irb && !depth_irb) > can be true. Now I'm very confused... > > > > On Mon, May 22, 2017 at 12:12 PM, Topi Pohjolainen < > > <a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: > > > >> In case of gen < 6 stencil (if present) is always combined with > >> depth. Both stencil and depth attachments point to the same > >> physical surface. > >> Alignment workaround starts by considering depth and updates > >> stencil accordingly. Current logic continues with stencil and > >> in vain considers the case where depth would refer to different > >> surface than stencil. > >> > >> Signed-off-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>> > >> --- > >> src/mesa/drivers/dri/i965/brw_misc_state.c | 63 > >> ++++++------------------------ > >> 1 file changed, 12 insertions(+), 51 deletions(-) > >> > >> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c > >> b/src/mesa/drivers/dri/i965/brw_misc_state.c > >> index 6ea1fb0..9fe3655 100644 > >> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c > >> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c > >> @@ -295,28 +295,14 @@ brw_workaround_depthstencil_alignment(struct > >> brw_context *brw, > >> } > >> > >> if (stencil_irb) { > >> - stencil_mt = get_stencil_miptree(stencil_irb); > >> - intel_miptree_get_image_offset(stencil_mt, > >> - stencil_irb->mt_level, > >> - stencil_irb->mt_layer, > >> - &stencil_draw_x, > >> &stencil_draw_y); > >> - int stencil_tile_x = stencil_draw_x & tile_mask_x; > >> - int stencil_tile_y = stencil_draw_y & tile_mask_y; > >> - > >> - /* If stencil doesn't match depth, then we'll need to rebase > >> stencil > >> - * as well. (if we hadn't decided to rebase stencil before, the > >> - * post-stencil depth test will also rebase depth to try to > >> match it > >> - * up). > >> - */ > >> - if (tile_x != stencil_tile_x || > >> - tile_y != stencil_tile_y) { > >> - rebase_stencil = true; > >> - } > >> + assert(stencil_irb->mt == depth_irb->mt); > >> + assert(stencil_irb->mt_level == depth_irb->mt_level); > >> + assert(stencil_irb->mt_layer == depth_irb->mt_layer); </div></div>This is inside a branch "if (depth_irb)" and therefore I'm simply asserting that in case stencil also exists, it must point to the same miptree. </blockquote><div> </div><div>Sorry, I missed that. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > >> } > >> } > >> > >> /* If we have (just) stencil, check it for ignored low bits as well */ > >> - if (stencil_irb) { > >> + if (!depth_irb && stencil_irb) { Here we handle the case where there is stencil but no depth - a valid case and hit by a few piglits. <div class="m_-7605881083323770664HOEnZb"><div class="m_-7605881083323770664h5"></div></div></blockquote><div> </div><div>Ok, that makes more sense. </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div>Carrying on... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_-7605881083323770664HOEnZb"><div class="m_-7605881083323770664h5"> > >> intel_miptree_get_image_offset(stencil_mt, > >> stencil_irb->mt_level, > >> stencil_irb->mt_layer, > >> @@ -334,6 +320,14 @@ brw_workaround_depthstencil_alignment(struct > >> brw_context *brw, > >> } > >> > >> if (rebase_stencil) { > >> + /* If stencil needs rebase, there isn't a depth attachment and the > >> + * combined depth-stencil is used for stencil only. Otherwise in > >> case > >> + * depth attachment is present both stencil and depth point to the > >> same > >> + * miptree. Rebase of depth is considered first updating stencil > >> + * attachment accordingly - hence stencil is rebased only if there > >> is no > >> + * depth attachment. > >> + */ > >> + assert(!depth_irb); > >> perf_debug("HW workaround: blitting stencil level %d to a > >> temporary " > >> "to fix alignment (stencil tile offset %d,%d)\n", > >> stencil_irb->mt_level, stencil_tile_x, stencil_tile_y); > >> @@ -347,39 +341,6 @@ brw_workaround_depthstencil_alignment(struct > >> brw_context *brw, > >> &stencil_draw_x, &stencil_draw_y); > >> stencil_tile_x = stencil_draw_x & tile_mask_x; > >> stencil_tile_y = stencil_draw_y & tile_mask_y; > >> - > >> - if (depth_irb && depth_irb->mt == stencil_irb->mt) { > >> - intel_miptree_reference(&depth_irb->mt, stencil_irb->mt); > >> - intel_renderbuffer_set_draw_offset(depth_irb); > >> - } else if (depth_irb && !rebase_depth) { > >> - if (tile_x != stencil_tile_x || > >> - tile_y != stencil_tile_y) { > >> - perf_debug("HW workaround: blitting depth level %d to a > >> temporary " > >> - "to match stencil level %d alignment (depth tile > >> offset " > >> - "%d,%d, stencil offset %d,%d)\n", > >> - depth_irb->mt_level, > >> - stencil_irb->mt_level, > >> - tile_x, tile_y, > >> - stencil_tile_x, stencil_tile_y); > >> - > >> - intel_renderbuffer_move_to_temp(brw, depth_irb, > >> invalidate_depth); > >> - > >> - tile_x = depth_irb->draw_x & tile_mask_x; > >> - tile_y = depth_irb->draw_y & tile_mask_y; > >> - > >> - if (stencil_irb && stencil_irb->mt == depth_mt) { > >> - intel_miptree_reference(&stencil_irb->mt, depth_irb->mt); > >> - intel_renderbuffer_set_draw_offset(stencil_irb); > >> - } > >> - > >> - WARN_ONCE(stencil_tile_x != tile_x || > >> - stencil_tile_y != tile_y, > >> - "Rebased stencil tile offset (%d,%d) doesn't match > >> depth " > >> - "tile offset (%d,%d).\n", > >> - stencil_tile_x, stencil_tile_y, > >> - tile_x, tile_y); > >> - } > >> - } > >> } > >> > >> if (!depth_irb) { > >> -- > >> 2.9.3 > >> > >> _______________________________________________ > >> mesa-dev mailing list > >> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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>