[Mesa-dev] [v2 3/7] i965/gen4: Remove redundant check for depth when rebasing stencil
Jason Ekstrand
jason at jlekstrand.net
Thu Jun 15 17:26:25 UTC 2017
On Thu, Jun 15, 2017 at 10:14 AM, Pohjolainen, Topi <
topi.pohjolainen at gmail.com> wrote:
> On Thu, Jun 15, 2017 at 09:41:42AM -0700, Jason Ekstrand wrote:
> > On Thu, Jun 15, 2017 at 9:36 AM, Jason Ekstrand <jason at jlekstrand.net>
> > 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.
>
> >
> > 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 <
> > > topi.pohjolainen at gmail.com> 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 <topi.pohjolainen at intel.com>
> > >> ---
> > >> 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);
>
> 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.
>
Sorry, I missed that.
> > >> }
> > >> }
> > >>
> > >> /* 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.
>
Ok, that makes more sense.
Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
Carrying on...
> > >> 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
> > >> mesa-dev at lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >>
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170615/1c606d93/attachment-0001.html>
More information about the mesa-dev
mailing list