From c1dd88852a37de9f8d1d3ed75593614e41d51b46 Mon Sep 17 00:00:00 2001 From: "bsalomon@google.com" Date: Fri, 2 Mar 2012 20:36:18 +0000 Subject: [PATCH] Better workaround for osx / nv bug Review URL: http://codereview.appspot.com/5727044/ git-svn-id: http://skia.googlecode.com/svn/trunk@3308 2bbb7eff-a529-9590-31e7-b0007b416f81 --- include/gpu/gl/GrGLConfig.h | 18 ++++++--------- src/gpu/gl/GrGLIndexBuffer.cpp | 50 +++++++++++++++++------------------------ src/gpu/gl/GrGLVertexBuffer.cpp | 50 +++++++++++++++++++++++++---------------- 3 files changed, 59 insertions(+), 59 deletions(-) diff --git a/include/gpu/gl/GrGLConfig.h b/include/gpu/gl/GrGLConfig.h index 2cc4f31..806f055 100644 --- a/include/gpu/gl/GrGLConfig.h +++ b/include/gpu/gl/GrGLConfig.h @@ -152,12 +152,11 @@ /** * There is a strange bug that occurs on Macs with NVIDIA GPUs. We don't * fully understand it. When (element) array buffers are continually - * respecified using glBufferData at the same (or perhaps just a small) size - * performance can fall off of a cliff. The driver winds up performing many - * DMA mapping / unmappings and chews up ~50% of the core. However, it has been - * observed that respecifiying the buffer with a larger size and then writing - * the small amount of actual data using glBufferSubData prevents the bad - * behavior. + * respecified using glBufferData performance can fall off of a cliff. The + * driver winds up performing many DMA mapping / unmappings and chews up ~50% of + * the core. However, it has been observed that occaisonally respecifiying the + * buffer using glBufferData and then writing data using glBufferSubData + * prevents the bad behavior. * * There is a lot of uncertainty around this issue. In Chrome backgrounding * the tab somehow initiates this behavior and we don't know what the connection @@ -169,11 +168,8 @@ * The issue is tracked at: * http://code.google.com/p/chromium/issues/detail?id=114865 * - * When GR_GL_USE_BUFFER_DATA_NULL_HINT is not set we encounter the bug - * if we keep uploading small vetex / index sizes. So, every 1K uploads we - * act as though GR_GL_USE_BUFFER_DATA_NULL_HINT was set: we specify the buffer - * at full size with a NULL data ptr and then do a partial update with - * glBufferSubData. + * When the workaround is enabled we will use the glBufferData / glBufferSubData + * trick every 128 array buffer uploads. * * Hopefully we will understand this better and have a cleaner fix or get a * OS/driver level fix. diff --git a/src/gpu/gl/GrGLIndexBuffer.cpp b/src/gpu/gl/GrGLIndexBuffer.cpp index f424e78..60bd9a9 100644 --- a/src/gpu/gl/GrGLIndexBuffer.cpp +++ b/src/gpu/gl/GrGLIndexBuffer.cpp @@ -105,37 +105,29 @@ bool GrGLIndexBuffer::updateData(const void* src, size_t srcSizeInBytes) { this->bind(); GrGLenum usage = dynamic() ? GR_GL_DYNAMIC_DRAW : GR_GL_STATIC_DRAW; - bool doNullHint = GR_GL_USE_BUFFER_DATA_NULL_HINT; -#if GR_GL_MAC_BUFFER_OBJECT_PERFOMANCE_WORKAROUND - GrAssert(!doNullHint); - static int N = 0; - doNullHint = (0 == N % 1024); - ++N; -#endif - - if (doNullHint) { - if (this->sizeInBytes() == srcSizeInBytes) { - GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, - srcSizeInBytes, src, usage)); - } else { - // Before we call glBufferSubData we give the driver a hint using - // glBufferData with NULL. This makes the old buffer contents - // inaccessible to future draws. The GPU may still be processing - // draws that reference the old contents. With this hint it can - // assign a different allocation for the new contents to avoid - // flushing the gpu past draws consuming the old contents. - GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, - this->sizeInBytes(), NULL, usage)); - GL_CALL(BufferSubData(GR_GL_ELEMENT_ARRAY_BUFFER, - 0, srcSizeInBytes, src)); - } +#if GR_GL_USE_BUFFER_DATA_NULL_HINT + if (this->sizeInBytes() == srcSizeInBytes) { + GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, + srcSizeInBytes, src, usage)); } else { - // Note that we're cheating on the size here. Currently no methods - // allow a partial update that preserves contents of non-updated - // portions of the buffer (lock() does a glBufferData(..size, NULL..)) - GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, - srcSizeInBytes, src, usage)); + // Before we call glBufferSubData we give the driver a hint using + // glBufferData with NULL. This makes the old buffer contents + // inaccessible to future draws. The GPU may still be processing + // draws that reference the old contents. With this hint it can + // assign a different allocation for the new contents to avoid + // flushing the gpu past draws consuming the old contents. + GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, + this->sizeInBytes(), NULL, usage)); + GL_CALL(BufferSubData(GR_GL_ELEMENT_ARRAY_BUFFER, + 0, srcSizeInBytes, src)); } +#else + // Note that we're cheating on the size here. Currently no methods + // allow a partial update that preserves contents of non-updated + // portions of the buffer (lock() does a glBufferData(..size, NULL..)) + GL_CALL(BufferData(GR_GL_ELEMENT_ARRAY_BUFFER, + srcSizeInBytes, src, usage)); +#endif return true; } diff --git a/src/gpu/gl/GrGLVertexBuffer.cpp b/src/gpu/gl/GrGLVertexBuffer.cpp index 4928868..126a95f 100644 --- a/src/gpu/gl/GrGLVertexBuffer.cpp +++ b/src/gpu/gl/GrGLVertexBuffer.cpp @@ -103,32 +103,44 @@ bool GrGLVertexBuffer::updateData(const void* src, size_t srcSizeInBytes) { GrGLenum usage = dynamic() ? GR_GL_DYNAMIC_DRAW : GR_GL_STATIC_DRAW; bool doNullHint = GR_GL_USE_BUFFER_DATA_NULL_HINT; +#if GR_GL_USE_BUFFER_DATA_NULL_HINT + if (this->sizeInBytes() == srcSizeInBytes) { + GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage)); + } else { + // Before we call glBufferSubData we give the driver a hint using + // glBufferData with NULL. This makes the old buffer contents + // inaccessible to future draws. The GPU may still be processing + // draws that reference the old contents. With this hint it can + // assign a different allocation for the new contents to avoid + // flushing the gpu past draws consuming the old contents. + GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, + this->sizeInBytes(), NULL, usage)); + GL_CALL(BufferSubData(GR_GL_ARRAY_BUFFER, 0, srcSizeInBytes, src)); + } +#else + // Note that we're cheating on the size here. Currently no methods + // allow a partial update that preserves contents of non-updated + // portions of the buffer (lock() does a glBufferData(..size, NULL..)) + bool doSubData = false; #if GR_GL_MAC_BUFFER_OBJECT_PERFOMANCE_WORKAROUND - GrAssert(!doNullHint); static int N = 0; - doNullHint = (0 == N % 1024); + // 128 was chosen experimentally. At 256 a slight hitchiness was noticed + // when dragging a Chromium window around with a canvas tab backgrounded. + doSubData = 0 == (N % 128); ++N; #endif - if (doNullHint) { - if (this->sizeInBytes() == srcSizeInBytes) { - GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage)); - } else { - // Before we call glBufferSubData we give the driver a hint using - // glBufferData with NULL. This makes the old buffer contents - // inaccessible to future draws. The GPU may still be processing - // draws that reference the old contents. With this hint it can - // assign a different allocation for the new contents to avoid - // flushing the gpu past draws consuming the old contents. - GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, - this->sizeInBytes(), NULL, usage)); - GL_CALL(BufferSubData(GR_GL_ARRAY_BUFFER, 0, srcSizeInBytes, src)); - } + if (doSubData) { + // The workaround is to do a glBufferData followed by glBufferSubData. + // Chromium's command buffer may turn a glBufferSubData where the size + // exactly matches the buffer size into a glBufferData. So we tack 1 + // extra byte onto the glBufferData. + GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes + 1, + NULL, usage)); + GL_CALL(BufferSubData(GR_GL_ARRAY_BUFFER, 0, srcSizeInBytes, src)); } else { - // Note that we're cheating on the size here. Currently no methods - // allow a partial update that preserves contents of non-updated - // portions of the buffer (lock() does a glBufferData(..size, NULL..)) GL_CALL(BufferData(GR_GL_ARRAY_BUFFER, srcSizeInBytes, src, usage)); } +#endif return true; } -- 2.7.4