[Mesa-dev] [PATCH 4/4] intel: Do the drawable x/y offset in intel_renderbuffer_map() for spans.

[Chad Versace]

On Tue,  7 Jun 2011 11:47:14 -0700, Eric Anholt <eric at anholt.net> wrote:
> We were mapping the renderbuffer once, then walking over all the
> buffers to map just the texture ones using the other texture mapping
> function that handled the x/y offset to the image in the region.  But
> then we would go and overwrite *those* mappings with the original
> mappings for depth/stencil, which was wrong.
> 
> Instead, just walk over the attachments once and map the attachments.
> Wasn't that easy?
> ---
>  src/mesa/drivers/dri/intel/intel_span.c |   87 ++++++++----------------------
>  1 files changed, 23 insertions(+), 64 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/intel/intel_span.c b/src/mesa/drivers/dri/intel/intel_span.c
> index 8978129..92a7586 100644
> --- a/src/mesa/drivers/dri/intel/intel_span.c
> +++ b/src/mesa/drivers/dri/intel/intel_span.c
> @@ -122,10 +122,16 @@ intel_renderbuffer_map(struct intel_context *intel, struct gl_renderbuffer *rb)
>     rb->Data = irb->region->buffer->virtual;
>     rb->RowStride = irb->region->pitch;
>  
> -   /* Flip orientation if it's the window system buffer */
>     if (!rb->Name) {
> +      /* Flip orientation of the window system buffer */
>        rb->Data += rb->RowStride * (irb->region->height - 1) * irb->region->cpp;
>        rb->RowStride = -rb->RowStride;
> +   } else {
> +      /* Adjust the base pointer of a texture image drawbuffer to the image
> +       * within the miptree region (all else has draw_x/y = 0).
> +       */
> +      rb->Data += irb->draw_x * irb->region->cpp;
> +      rb->Data += irb->draw_y * rb->RowStride * irb->region->cpp;
>     }
>  
>     intel_set_span_functions(intel, rb);
> @@ -148,71 +154,26 @@ intel_renderbuffer_unmap(struct intel_context *intel,
>     rb->RowStride = 0;
>  }
>  
> -/**
> - * Map or unmap all the renderbuffers which we may need during
> - * software rendering.
> - * XXX in the future, we could probably convey extra information to
> - * reduce the number of mappings needed.  I.e. if doing a glReadPixels
> - * from the depth buffer, we really only need one mapping.
> - *
> - * XXX Rewrite this function someday.
> - * We can probably just loop over all the renderbuffer attachments,
> - * map/unmap all of them, and not worry about the _ColorDrawBuffers
> - * _ColorReadBuffer, _DepthBuffer or _StencilBuffer fields.
> - */
>  static void
> -intel_map_unmap_framebuffer(struct intel_context *intel,
> -			    struct gl_framebuffer *fb,
> -			    GLboolean map)
> +intel_framebuffer_map(struct intel_context *intel, struct gl_framebuffer *fb)
>  {
> -   GLuint i;
> -
> -   /* color draw buffers */
> -   for (i = 0; i < fb->_NumColorDrawBuffers; i++) {
> -      if (map)
> -         intel_renderbuffer_map(intel, fb->_ColorDrawBuffers[i]);
> -      else
> -         intel_renderbuffer_unmap(intel, fb->_ColorDrawBuffers[i]);
> -   }
> -
> -   /* color read buffer */
> -   if (map)
> -      intel_renderbuffer_map(intel, fb->_ColorReadBuffer);
> -   else
> -      intel_renderbuffer_unmap(intel, fb->_ColorReadBuffer);
> +   int i;
>  
> -   /* check for render to textures */
>     for (i = 0; i < BUFFER_COUNT; i++) {
> -      struct gl_renderbuffer_attachment *att =
> -         fb->Attachment + i;
> -      struct gl_texture_object *tex = att->Texture;
> -      if (tex) {
> -         /* render to texture */
> -         ASSERT(att->Renderbuffer);
> -         if (map)
> -            intel_tex_map_images(intel, intel_texture_object(tex));
> -         else
> -            intel_tex_unmap_images(intel, intel_texture_object(tex));
> -      }
> +      intel_renderbuffer_map(intel, fb->Attachment[i].Renderbuffer);
>     }
>  
> -   /* depth buffer (Note wrapper!) */
> -   if (fb->_DepthBuffer) {
> -      if (map)
> -         intel_renderbuffer_map(intel, fb->_DepthBuffer->Wrapped);
> -      else
> -         intel_renderbuffer_unmap(intel, fb->_DepthBuffer->Wrapped);
> -   }
> +   intel_check_front_buffer_rendering(intel);
> +}
>  
> -   /* stencil buffer (Note wrapper!) */
> -   if (fb->_StencilBuffer) {
> -      if (map)
> -         intel_renderbuffer_map(intel, fb->_StencilBuffer->Wrapped);
> -      else
> -         intel_renderbuffer_unmap(intel, fb->_StencilBuffer->Wrapped);
> -   }
> +static void
> +intel_framebuffer_unmap(struct intel_context *intel, struct gl_framebuffer *fb)
> +{
> +   int i;
>  
> -   intel_check_front_buffer_rendering(intel);
> +   for (i = 0; i < BUFFER_COUNT; i++) {
> +      intel_renderbuffer_unmap(intel, fb->Attachment[i].Renderbuffer);
> +   }
>  }
>  
>  /**
> @@ -239,9 +200,8 @@ intelSpanRenderStart(struct gl_context * ctx)
>        }
>     }
>  
> -   intel_map_unmap_framebuffer(intel, ctx->DrawBuffer, GL_TRUE);
> -   if (ctx->ReadBuffer != ctx->DrawBuffer)
> -      intel_map_unmap_framebuffer(intel, ctx->ReadBuffer, GL_TRUE);
> +   intel_framebuffer_map(intel, ctx->DrawBuffer);
> +   intel_framebuffer_map(intel, ctx->ReadBuffer);
>  }

I think this hunk should be:

@@ -239,9 +200,8 @@ intelSpanRenderStart(struct gl_context * ctx)
       }
    }
 
-   intel_map_unmap_framebuffer(intel, ctx->DrawBuffer, GL_TRUE);
+   intel_framebuffer_map(intel, ctx->DrawBuffer);
-   if (ctx->ReadBuffer != ctx->DrawBuffer)
-      intel_map_unmap_framebuffer(intel, ctx->ReadBuffer, GL_TRUE);
+      intel_framebuffer_map(intel, ctx->ReadBuffer);
 }

There's no reason to re-map the read buffer if we've already done so.

>  
>  /**
> @@ -263,9 +223,8 @@ intelSpanRenderFinish(struct gl_context * ctx)
>        }
>     }
>  
> -   intel_map_unmap_framebuffer(intel, ctx->DrawBuffer, GL_FALSE);
> -   if (ctx->ReadBuffer != ctx->DrawBuffer)
> -      intel_map_unmap_framebuffer(intel, ctx->ReadBuffer, GL_FALSE);
> +   intel_framebuffer_unmap(intel, ctx->DrawBuffer);
> +   intel_framebuffer_unmap(intel, ctx->ReadBuffer);
>  }

Ditto for this hunk.