[Mesa-dev] [PATCH 1/2] i965: Apply depthstencil alignment workaround when doing fast clears.

[Eric Anholt]

Paul Berry <stereotype441 at gmail.com> writes:

> On 12 March 2013 12:53, Paul Berry <stereotype441 at gmail.com> wrote:
>
>> On 12 March 2013 12:28, Eric Anholt <eric at anholt.net> wrote:
>>
>>> Paul Berry <stereotype441 at gmail.com> writes:
>>>
>>> > Fast depth clears have the same depth/stencil alignment requirements
>>> > as other drawing operations.  Therefore, we need to call
>>> > brw_workaround_depthstencil_alignment() from both the clear and
>>> > drawing paths.
>>> >
>>> > Without this fix, we get image corruption if the following conditions
>>> > hold: (a) the first ever drawing operation to a depth miplevel (or the
>>> > first drawing operation after having used the texture for sampling) is
>>> > a clear, (b) the depth miplevel has a size that is eligible for fast
>>> > depth clears, and (c) the depth miplevel has an offset within the
>>> > miptree that isn't 8x8 aligned.
>>> >
>>> > Fixes piglit "depthstencil-render-miplevels" tests with size 273.
>>> >
>>> > NOTE: This is a candidate for stable branches
>>> > ---
>>> >  src/mesa/drivers/dri/i965/brw_clear.c | 6 +++++-
>>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
>>> b/src/mesa/drivers/dri/i965/brw_clear.c
>>> > index 53d8e54..cde1a06 100644
>>> > --- a/src/mesa/drivers/dri/i965/brw_clear.c
>>> > +++ b/src/mesa/drivers/dri/i965/brw_clear.c
>>> > @@ -40,6 +40,8 @@
>>> >  #include "intel_mipmap_tree.h"
>>> >  #include "intel_regions.h"
>>> >
>>> > +#include "brw_context.h"
>>> > +
>>> >  #define FILE_DEBUG_FLAG DEBUG_BLIT
>>> >
>>> >  static const char *buffer_names[] = {
>>> > @@ -219,7 +221,8 @@ brw_fast_clear_depth(struct gl_context *ctx)
>>> >  static void
>>> >  brw_clear(struct gl_context *ctx, GLbitfield mask)
>>> >  {
>>> > -   struct intel_context *intel = intel_context(ctx);
>>> > +   struct brw_context *brw = brw_context(ctx);
>>> > +   struct intel_context *intel = &brw->intel;
>>> >
>>> >     if (!_mesa_check_conditional_render(ctx))
>>> >        return;
>>> > @@ -229,6 +232,7 @@ brw_clear(struct gl_context *ctx, GLbitfield mask)
>>> >     }
>>> >
>>> >     intel_prepare_render(intel);
>>> > +   brw_workaround_depthstencil_alignment(brw);
>>>
>>> It seems like this should be happening in brw_fast_clear(), either
>>> before before calling blorp or inside of it, instead of in the potential
>>> caller of brw_fast_clear().  Makes sense, though.
>>>
>>
>> Chad made the same comment to me in person yesterday.  The reason I put it
>> here is to accommodate patch 2/2 (which allows
>> brw_workaround_depthstencil_alignment to avoid an unnecessary copy when
>> clearing the whole miplevel).  If I move the call to
>> brw_workaround_depthstencil_alignment into brw_fast_clear_depth(), then the
>> unnecessary copy will only be avoided when doing depth clears.  If I leave
>> it here, the unnecessary copy will be avoided for all clears.
>>
>>
> Correction: when I wrote this I momentarily forgot that the workaround is
> only needed for depth and stencil buffers.  So leaving the call to
> brw_workaround_depthstencil_alignment here allows us to avoid the
> unnecessary copy for both depth and stencil clears, not just depth clears.
>  I still think it's worth it, but it's a far less convincing case.

You convinced me, though.

Reviewed-by: Eric Anholt <eric at anholt.net> for both.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130313/bb3a92a5/attachment.pgp>