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

[Brian Paul]

On Mon, Dec 12, 2011 at 10:28 AM, Eric Anholt <eric at anholt.net> wrote:
> 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.

Will do.


>> +   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.

Will fix.


>> 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.

I think I have that right.  mask is the complement of the stencil
WriteMask value here.  And if mask==0 it means that we're replacing
the dest values completely.  That's what the else clauses do.


>> -                  _mesa_memset16((short unsigned int*) stencil, clearVal, width);
>
> note, _mesa_memset16 is dead code now.

OK, I'll rm that later.

-Brian