[Mesa-dev] [PATCH 1/3] swrast: do depth/stencil clearing with Map/UnmapRenderbuffer()

[Eric Anholt]

On Sat, 10 Dec 2011 12:00:52 -0700, Brian Paul <brianp at vmware.com> wrote:
> Another step toward getting rid of the renderbuffer PutRow/etc functions.

> +   if (buffers & BUFFER_DS) {
> +      struct gl_renderbuffer *rb =
> +         ctx->DrawBuffer->Attachment[BUFFER_DEPTH].Renderbuffer;
> +
> +      if ((buffers & BUFFER_DS) == BUFFER_DS && rb &&
> +          _mesa_is_format_packed_depth_stencil(rb->Format)) {
> +         /* clear depth and stencil together */
> +         _swrast_clear_depth_stencil_buffer(ctx);

I think this would be wrong for the case of

Attachment[BUFFER_DEPTH] = depthstencil texture
Attachment[BUFFER_STENCIL = separate stencil RB

Granted, that's a pretty crazy setup, but I think this just needs an rb
== Attachment[BUFFER_STENCIL].Renderbuffer check.

> +   mapMode = GL_MAP_WRITE_BIT;
> +   if (_mesa_get_format_bits(rb->Format, GL_STENCIL_BITS) > 0) {
> +      mapMode |= GL_MAP_READ_BIT;
> +   }

You only set the READ bit if there are stencil bits...

> +   case MESA_FORMAT_S8_Z24:
> +   case MESA_FORMAT_X8_Z24:
> +   case MESA_FORMAT_Z24_S8:
> +   case MESA_FORMAT_Z24_X8:

>           for (i = 0; i < height; i++) {
> -            rb->PutMonoRow(ctx, rb, width, x, y + i, &clearVal16, NULL);
> +            GLuint *row = (GLuint *) map;
> +            for (j = 0; j < width; j++) {
> +               row[j] = (row[j] & mask) | clearVal;
> +            }
> +            map += rowStride;
>           }
> +

but then the S8_Z24 and X8_Z24 paths both do the same set of reads.

> +   case MESA_FORMAT_Z32_FLOAT_X24S8:
> +      /* XXX untested */
> +      {
> +         GLfloat clearVal = (GLfloat) ctx->Depth.Clear;
>           for (i = 0; i < height; i++) {
> -            rb->PutMonoRow(ctx, rb, width, x, y + i, &clearValue, NULL);
> +            GLfloat *row = (GLfloat *) map;
> +            for (j = 0; j < width; j++) {
> +               row[j * 2] = clearVal;
> +            }
> +            map += rowStride;
>           }

It looks good, at least.  Once I land my outstanding driver bits it
should be easy to test.

> +/**
> + * Clear both depth and stencil values in a combined depth+stencil buffer.
> + */
> +void
> +_swrast_clear_depth_stencil_buffer(struct gl_context *ctx)
> +{

> +   case MESA_FORMAT_Z32_FLOAT_X24S8:
> +      /* XXX untested */
> +      {
> +         GLfloat zClear = (GLfloat) ctx->Depth.Clear;
> +         GLuint sClear = ctx->Stencil.Clear;
> +         for (i = 0; i < height; i++) {
> +            GLfloat *zRow = (GLfloat *) map;
> +            GLuint *sRow = (GLuint *) map;
> +            for (j = 0; j < width; j++) {
> +               zRow[j * 2 + 0] = zClear;
> +            }
> +            for (j = 0; j < width; j++) {
> +               sRow[j * 2 + 1] = sClear;
> +            }

Missing stencil mask treatment.

> diff --git a/src/mesa/swrast/s_stencil.c b/src/mesa/swrast/s_stencil.c
> index 101ee50..5c5ebd4 100644
> --- a/src/mesa/swrast/s_stencil.c
> +++ b/src/mesa/swrast/s_stencil.c
> @@ -1124,121 +1124,108 @@ _swrast_write_stencil_span(struct gl_context *ctx, GLint n, GLint x, GLint y,
>  
>  
>  /**
> - * Clear the stencil buffer.
> + * Clear the stencil buffer.  If the buffer is a combined
> + * depth+stencil buffer, only the stencil bits will be touched.
>   */
>  void
> -_swrast_clear_stencil_buffer( struct gl_context *ctx, struct gl_renderbuffer *rb )
> +_swrast_clear_stencil_buffer(struct gl_context *ctx)
>  {
> +   struct gl_renderbuffer *rb =
> +      ctx->DrawBuffer->Attachment[BUFFER_STENCIL].Renderbuffer;
>     const GLubyte stencilBits = ctx->DrawBuffer->Visual.stencilBits;
> -   const GLuint mask = ctx->Stencil.WriteMask[0];
> -   const GLuint invMask = ~mask;
> -   const GLuint clearVal = (ctx->Stencil.Clear & mask);
> +   const GLuint writeMask = ctx->Stencil.WriteMask[0];
> +   const GLuint clearVal = (ctx->Stencil.Clear & writeMask);
>     const GLuint stencilMax = (1 << stencilBits) - 1;
>     GLint x, y, width, height;
> +   GLubyte *map;
> +   GLint rowStride, i, j;
> +   GLbitfield mapMode;

> +   mapMode = GL_MAP_WRITE_BIT;
> +   if ((writeMask & stencilMax) != stencilMax) {
> +      /* need to mask stencil values */
> +      mapMode |= GL_MAP_READ_BIT;
> +   }
> +   else if (_mesa_get_format_bits(rb->Format, GL_DEPTH_BITS) > 0) {
> +      /* combined depth+stencil, need to mask Z values */
> +      mapMode |= GL_MAP_READ_BIT;
> +   }

> +   switch (rb->Format) {
> +   case MESA_FORMAT_S8:
> +      {
> +         GLubyte clear = clearVal & 0xff;
> +         GLubyte mask = (~writeMask) & 0xff;
> +         if (mask != 0) {

mask != 0xff, right?  0 was already tested at the top, and there's that
else case below.

> -                  _mesa_memset16((short unsigned int*) stencil, clearVal, width);

note, _mesa_memset16 is dead code now.
-------------- 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/20111212/5d292320/attachment.pgp>