[Mesa-dev] [PATCH 2/2] i965: Avoid unnecessary copy when depthstencil workaround invoked by clear.

[Paul Berry]

On 14 March 2013 12:38, Eric Anholt <eric at anholt.net> wrote:

> Paul Berry <stereotype441 at gmail.com> writes:
>
> > On 12 March 2013 16:33, Eric Anholt <eric at anholt.net> wrote:
> >
> >> Paul Berry <stereotype441 at gmail.com> writes:
> >> >  void
> >> > -brw_workaround_depthstencil_alignment(struct brw_context *brw)
> >> > +brw_workaround_depthstencil_alignment(struct brw_context *brw,
> >> > +                                      GLbitfield clear_mask)
> >> >  {
> >> >     struct intel_context *intel = &brw->intel;
> >> >     struct gl_context *ctx = &intel->ctx;
> >> > @@ -341,10 +343,24 @@ brw_workaround_depthstencil_alignment(struct
> >> brw_context *brw)
> >> >     struct intel_mipmap_tree *stencil_mt =
> >> get_stencil_miptree(stencil_irb);
> >> >     uint32_t tile_x = 0, tile_y = 0, stencil_tile_x = 0,
> stencil_tile_y
> >> = 0;
> >> >     uint32_t stencil_draw_x = 0, stencil_draw_y = 0;
> >> > +   bool invalidate_depth = clear_mask & GL_DEPTH_BUFFER_BIT;
> >> > +   bool invalidate_stencil = clear_mask & GL_STENCIL_BUFFER_BIT;
> >> >
> >> >     if (depth_irb)
> >> >        depth_mt = depth_irb->mt;
> >> >
> >> > +   if (depth_irb && invalidate_depth
> >> > +       && _mesa_is_depthstencil_format(
> >> > +              _mesa_get_format_base_format(depth_mt->format))
> >> > +       && !depth_mt->stencil_mt) {
> >>
> >> The only _mesa_is_depthstencil_format() returned by
> >> _mesa_get_format_base_format() is GL_DEPTH_STENCIL, so calling that
> >> seems kinda overkill.
> >>
> >
> > Good point.  I'll fix that before pushing.  I'll also make a follow-up
> > patch to fix the function I borrowed this test from
> > (intel_miptree_create_layout).
> >
> >
> >>
> >> If depth_mt->stencil_mt, then depth_mt->format's base format will not be
> >> GL_DEPTH_STENCIL.  I'm concerned that you're going to lose the
> >> depth_mt->stencil_mt contents of a gl-level packed depth/stencil texture
> >> that's backed by separate stencil.
>
> For a separate stencil packed depth/stencil texture, mt->format will be,
> for example, Z24X8, and mt->stencil_mt->format will be S8 -- see the
> intel_mipmap_tree.h comment for the format field.
>

Thanks for carefully reviewing this, Eric.  You're right, this check is
still doing the wrong thing on hardware that uses separate stencil
buffers.  It turns out the reason I didn't discover this error earlier was
that my patch was completely bogus--it was using GL_DEPTH_BUFFER_BIT and
GL_STENCIL_BUFFER_BIT to check clear_mask, when in point of fact, at this
point in the code the bitmask has been converted to use BUFFER_BIT_DEPTH
and BUFFER_BIT_STENCIL.  So the patch wasn't having any effect at all (and
shame on me, I should have used a breakpoint or a temporary printf to
verify that the optimization I intended was actually taking effect).

Once I had fixed both of those bugs, I discovered a third bug: it turns out
that when the invalidate flag was set, I was bypassing not only the copy,
but also the logic that associates the intel_texture_image struct to the
reallocated miptree.

V3 patch to follow.  This time I've not only run it through a full piglit
run on Gen5-7, I've also verified that the copy is actually skipped in the
circumstances I intended, and there is no image corruption.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130316/8b8fc850/attachment-0001.html>