<div dir="ltr">On 14 March 2013 12:38, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
<br>
> On 12 March 2013 16:33, Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> wrote:<br>
><br>
>> Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
>> >  void<br>
>> > -brw_workaround_depthstencil_alignment(struct brw_context *brw)<br>
>> > +brw_workaround_depthstencil_alignment(struct brw_context *brw,<br>
>> > +                                      GLbitfield clear_mask)<br>
>> >  {<br>
>> >     struct intel_context *intel = &brw->intel;<br>
>> >     struct gl_context *ctx = &intel->ctx;<br>
>> > @@ -341,10 +343,24 @@ brw_workaround_depthstencil_alignment(struct<br>
>> brw_context *brw)<br>
>> >     struct intel_mipmap_tree *stencil_mt =<br>
>> get_stencil_miptree(stencil_irb);<br>
>> >     uint32_t tile_x = 0, tile_y = 0, stencil_tile_x = 0, stencil_tile_y<br>
>> = 0;<br>
>> >     uint32_t stencil_draw_x = 0, stencil_draw_y = 0;<br>
>> > +   bool invalidate_depth = clear_mask & GL_DEPTH_BUFFER_BIT;<br>
>> > +   bool invalidate_stencil = clear_mask & GL_STENCIL_BUFFER_BIT;<br>
>> ><br>
>> >     if (depth_irb)<br>
>> >        depth_mt = depth_irb->mt;<br>
>> ><br>
>> > +   if (depth_irb && invalidate_depth<br>
>> > +       && _mesa_is_depthstencil_format(<br>
>> > +              _mesa_get_format_base_format(depth_mt->format))<br>
>> > +       && !depth_mt->stencil_mt) {<br>
>><br>
>> The only _mesa_is_depthstencil_format() returned by<br>
>> _mesa_get_format_base_format() is GL_DEPTH_STENCIL, so calling that<br>
>> seems kinda overkill.<br>
>><br>
><br>
> Good point.  I'll fix that before pushing.  I'll also make a follow-up<br>
> patch to fix the function I borrowed this test from<br>
> (intel_miptree_create_layout).<br>
><br>
><br>
>><br>
>> If depth_mt->stencil_mt, then depth_mt->format's base format will not be<br>
>> GL_DEPTH_STENCIL.  I'm concerned that you're going to lose the<br>
>> depth_mt->stencil_mt contents of a gl-level packed depth/stencil texture<br>
>> that's backed by separate stencil.<br>
<br>
</div></div>For a separate stencil packed depth/stencil texture, mt->format will be,<br>
for example, Z24X8, and mt->stencil_mt->format will be S8 -- see the<br>
intel_mipmap_tree.h comment for the format field.<br></blockquote><div><br></div><div>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).<br>
<br></div><div>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.<br>
<br></div><div>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.<br>
</div></div></div></div>