On 14 March 2013 12:38, Eric Anholt <e...@anholt.net> wrote:
> Paul Berry <stereotype...@gmail.com> writes:
>
> > On 12 March 2013 16:33, Eric Anholt <e...@anholt.net> wrote:
> >
> >> Paul Berry <stereotype...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
