[Mesa-dev] [PATCH 4/6] intel: Fix framebuffer blitting to GL_COLOR_ATTACHMENTi when i!=0
Kenneth Graunke
kenneth at whitecape.org
Tue Dec 18 11:26:15 PST 2012
On 12/12/2012 03:25 PM, Anuj Phogat wrote:
> This patch fixes a case when blitting to a framebuffer with
> renderbuffers/textures attached to GL_COLOR_ATTACHMENT{1, 2, ...}.
> Earlier we were incorrectly blitting to GL_COLOR_ATTACHMENT0 by default.
>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
> src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 5 +-
> src/mesa/drivers/dri/intel/intel_fbo.c | 85 ++++++++++++++++----------
> 2 files changed, 56 insertions(+), 34 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> index e8604e7..41a8734 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> @@ -263,8 +263,9 @@ try_blorp_blit(struct intel_context *intel,
> }
> for (unsigned i = 0; i < ctx->DrawBuffer->_NumColorDrawBuffers; ++i) {
> dst_irb = intel_renderbuffer(ctx->DrawBuffer->_ColorDrawBuffers[i]);
> - do_blorp_blit(intel, buffer_bit, src_irb, dst_irb, srcX0, srcY0,
> - dstX0, dstY0, dstX1, dstY1, mirror_x, mirror_y);
> + if (dst_irb)
> + do_blorp_blit(intel, buffer_bit, src_irb, dst_irb, srcX0, srcY0,
> + dstX0, dstY0, dstX1, dstY1, mirror_x, mirror_y);
> }
This change looks unrelated. Why is it in this patch?
> break;
> case GL_DEPTH_BUFFER_BIT:
> diff --git a/src/mesa/drivers/dri/intel/intel_fbo.c b/src/mesa/drivers/dri/intel/intel_fbo.c
> index 6a66521..d0e9fe2 100644
> --- a/src/mesa/drivers/dri/intel/intel_fbo.c
> +++ b/src/mesa/drivers/dri/intel/intel_fbo.c
> @@ -793,44 +793,65 @@ intel_blit_framebuffer_copy_tex_sub_image(struct gl_context *ctx,
> GLbitfield mask, GLenum filter)
> {
> if (mask & GL_COLOR_BUFFER_BIT) {
> + GLboolean result = false;
> const struct gl_framebuffer *drawFb = ctx->DrawBuffer;
> const struct gl_framebuffer *readFb = ctx->ReadBuffer;
> - const struct gl_renderbuffer_attachment *drawAtt =
> - &drawFb->Attachment[drawFb->_ColorDrawBufferIndexes[0]];
> + const struct gl_renderbuffer_attachment *drawAtt;
> struct intel_renderbuffer *srcRb =
> intel_renderbuffer(readFb->_ColorReadBuffer);
>
> - /* If the source and destination are the same size with no
> - mirroring, the rectangles are within the size of the
> - texture and there is no scissor then we can use
> - glCopyTexSubimage2D to implement the blit. This will end
> - up as a fast hardware blit on some drivers */
> - if (srcRb && drawAtt && drawAtt->Texture &&
> - srcX0 - srcX1 == dstX0 - dstX1 &&
> - srcY0 - srcY1 == dstY0 - dstY1 &&
> - srcX1 >= srcX0 &&
> - srcY1 >= srcY0 &&
> - srcX0 >= 0 && srcX1 <= readFb->Width &&
> - srcY0 >= 0 && srcY1 <= readFb->Height &&
> - dstX0 >= 0 && dstX1 <= drawFb->Width &&
> - dstY0 >= 0 && dstY1 <= drawFb->Height &&
> - !ctx->Scissor.Enabled) {
> - const struct gl_texture_object *texObj = drawAtt->Texture;
> - const GLuint dstLevel = drawAtt->TextureLevel;
> - const GLenum target = texObj->Target;
> -
> - struct gl_texture_image *texImage =
> - _mesa_select_tex_image(ctx, texObj, target, dstLevel);
> -
> - if (intel_copy_texsubimage(intel_context(ctx),
> - intel_texture_image(texImage),
> - dstX0, dstY0,
> - srcRb,
> - srcX0, srcY0,
> - srcX1 - srcX0, /* width */
> - srcY1 - srcY0))
> - mask &= ~GL_COLOR_BUFFER_BIT;
> + /* Blit to all active draw buffers */
> + for (int i = 0; i < ctx->DrawBuffer->_NumColorDrawBuffers; i++) {
> + int idx = ctx->DrawBuffer->_ColorDrawBufferIndexes[i];
> + if (idx != -1) {
> + drawAtt = &drawFb->Attachment[idx];
> + }
> + else
> + continue;
Could we please do:
if (idx == -1)
continue;
drawAtt = &drawFb->Attachment[idx];
it's just a bit easier to read.
> + /* If the source and destination are the same size with no
> + mirroring, the rectangles are within the size of the
> + texture and there is no scissor then we can use
> + glCopyTexSubimage2D to implement the blit. This will end
> + up as a fast hardware blit on some drivers */
> + if (srcRb && drawAtt && drawAtt->Texture &&
> + srcX0 - srcX1 == dstX0 - dstX1 &&
> + srcY0 - srcY1 == dstY0 - dstY1 &&
> + srcX1 >= srcX0 &&
> + srcY1 >= srcY0 &&
> + srcX0 >= 0 && srcX1 <= readFb->Width &&
> + srcY0 >= 0 && srcY1 <= readFb->Height &&
> + dstX0 >= 0 && dstX1 <= drawFb->Width &&
> + dstY0 >= 0 && dstY1 <= drawFb->Height &&
> + !ctx->Scissor.Enabled) {
> + const struct gl_texture_object *texObj = drawAtt->Texture;
> + const GLuint dstLevel = drawAtt->TextureLevel;
> + const GLenum target = texObj->Target;
> +
> + struct gl_texture_image *texImage =
> + _mesa_select_tex_image(ctx, texObj, target, dstLevel);
> +
> + result = intel_copy_texsubimage(intel_context(ctx),
> + intel_texture_image(texImage),
> + dstX0, dstY0,
> + srcRb,
> + srcX0, srcY0,
> + srcX1 - srcX0, /* width */
> + srcY1 - srcY0);
> + if (!result)
> + break;
This "result" boolean looks unnecessary. Why not leave the code as
before...:
if (intel_copy_texsubimage(intel_context(ctx),
intel_texture_image(texImage),
dstX0, dstY0,
srcRb,
srcX0, srcY0,
srcX1 - srcX0, /* width */
srcY1 - srcY0))
mask &= ~GL_COLOR_BUFFER_BIT;
> + }
> + /* This handles a case where not all the draw buffer attachments
> + * are textures.
> + */
> + else if (srcRb && drawAtt) {
> + result = false;
> + break;
> + }
> }
> +
> + if (result)
> + mask &= ~GL_COLOR_BUFFER_BIT;
...and then you don't need this hunk at all. In fact, the "break" here
means when you encounter the first target that isn't a texture, you bail
completely on the BLT path and fall back to either blorp or meta for all
other attachments. That isn't necessary: you may as well continue and
try to BLT whatever attachments you can. (After all, you've already
BLT'd some of them!) In other words, handle as much as possible via
this method.
The rest will still remain in mask, so the caller will deal with them
via other methods.
> }
>
> return mask;
More information about the mesa-dev
mailing list