[Mesa-dev] [PATCH 4/6] intel: Fix framebuffer blitting to GL_COLOR_ATTACHMENTi when i!=0
Anuj Phogat
anuj.phogat at gmail.com
Thu Dec 20 13:40:15 PST 2012
On Tue, Dec 18, 2012 at 11:26 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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.
Yes, I'll change that.
>> + /* 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;
There are no per color draw buffer bits in mask.
GL_COLOR_BUFFER_BIT in mask represents all the color draw
buffers. It'll be incorrect to do mask &= ~GL_COLOR_BUFFER_BIT;
if we are able to successfully blit 1 (or few) out of n draw buffers. This will
completely skip blitting to remaining draw buffers in blorp or meta
path because GL_COLOR_BUFFER_BIT is already gone from mask.
Please elaborate if I misunderstood your suggestion.
However, we can avoid the overhead of blitting to few of the draw buffers
in intel_blit_framebuffer_copy_tex_sub_image() and then fall back to blorp
or meta path to do the blitting all over again to all the drawbuffers. This can
be done by checking in advance if all the draw renderbuffers are textures or
not. We'll completely skip intel_blit_framebuffer_copy_tex_sub_image()
path even if one of the draw renderbuffers is not a texture.
More information about the mesa-dev
mailing list