From d310159beee21fe1314a97bde83b0102a9e36de3 Mon Sep 17 00:00:00 2001 From: ruslantproductengine Date: Thu, 16 Nov 2017 19:14:28 +0200 Subject: MAINT-7977 [Alex Ivy] Feature Table crashes In case of buff->getVertexStrider(v) return false it mean that glMapBufferRange() return NULL The next three lines can be the reason of this crash. --- indra/newview/llglsandbox.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) (limited to 'indra/newview/llglsandbox.cpp') diff --git a/indra/newview/llglsandbox.cpp b/indra/newview/llglsandbox.cpp index c80dec0e75..44e7ae15ba 100644 --- a/indra/newview/llglsandbox.cpp +++ b/indra/newview/llglsandbox.cpp @@ -963,11 +963,16 @@ F32 gpu_benchmark() LLStrider v; LLStrider tc; - buff->getVertexStrider(v); - - v[0].set(-1,1,0); - v[1].set(-1,-3,0); - v[2].set(3,1,0); + if (buff->getVertexStrider(v)) + { + v[0].set(-1, 1, 0); + v[1].set(-1, -3, 0); + v[2].set(3, 1, 0); + } + else + { + LL_WARNS() << "GL LLVertexBuffer::getVertexStrider() return false " << (NULL == buff->getMappedData() ? "buff->getMappedData() is NULL" : "buff->getMappedData() not NULL") << LL_ENDL; + } buff->flush(); -- cgit v1.2.3 From dfc0785857249120c7f10b9cbcfce4c3b801ced2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Nov 2017 16:54:27 -0500 Subject: MAINT-7977: If getVertexStrider() returns false, abandon benchmark. Ruslan tracked the observed crash to assignments (to create a dummy triangle) through an LLStrider obtained from getVertexStrider(). When getVertexStrider() returns false, produce a warning and just skip the rest of the benchmark test. The one bit of explicit cleanup apparently required by that early exit is a call to LLImageGL::deleteTextures() to match the preceding generateTextures() call. Wrap both in a new TextureHolder class whose destructor takes care of cleanup. The only other references to the corresponding U32 array are a couple calls to LLTexUnit::bindManual(); add a bind() method to support that. Also fix apparent bug in the LL_DARWIN special case for "improbably high and likely incorrect": the code assigned -1.f (the "couldn't compute" value) to gbps, overlooking the fact that gbps is unconditionally recomputed below. In the "likely incorrect" stanza, simply return -1.f instead. --- indra/newview/llglsandbox.cpp | 65 +++++++++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 15 deletions(-) (limited to 'indra/newview/llglsandbox.cpp') diff --git a/indra/newview/llglsandbox.cpp b/indra/newview/llglsandbox.cpp index 44e7ae15ba..af0bbf9a88 100644 --- a/indra/newview/llglsandbox.cpp +++ b/indra/newview/llglsandbox.cpp @@ -64,6 +64,8 @@ #include "llspatialpartition.h" #include "llviewershadermgr.h" +#include + // Height of the yellow selection highlight posts for land const F32 PARCEL_POST_HEIGHT = 0.666f; @@ -926,9 +928,40 @@ F32 gpu_benchmark() LLGLSLShader::initProfile(); } + // This struct is used to ensure that each generateTextures() call is + // matched by a corresponding deleteTextures() call. It also handles the + // bindManual() calls using those textures. + struct TextureHolder + { + TextureHolder(U32 unit, U32 size): + texUnit(gGL.getTexUnit(unit)), + source(size) // preallocate vector + { + // takes (count, pointer) + // &vector[0] gets pointer to contiguous array + LLImageGL::generateTextures(source.size(), &source[0]); + } + + ~TextureHolder() + { + // ensure that we delete these textures regardless of how we exit + LLImageGL::deleteTextures(source.size(), &source[0]); + } + + void bind(U32 index) + { + texUnit->bindManual(LLTexUnit::TT_TEXTURE, source[index]); + } + + // capture which LLTexUnit we're going to use + LLTexUnit* texUnit; + + // use std::vector for implicit resource management + std::vector source; + }; + LLRenderTarget dest[count]; - U32 source[count]; - LLImageGL::generateTextures(count, source); + TextureHolder texHolder(0, count); std::vector results; //build a random texture @@ -950,7 +983,7 @@ F32 gpu_benchmark() dest[i].clear(); dest[i].flush(); - gGL.getTexUnit(0)->bindManual(LLTexUnit::TT_TEXTURE, source[i]); + texHolder.bind(i); LLImageGL::setManualImage(GL_TEXTURE_2D, 0, GL_RGBA, res,res,GL_RGBA, GL_UNSIGNED_BYTE, pixels); } @@ -963,17 +996,21 @@ F32 gpu_benchmark() LLStrider v; LLStrider tc; - if (buff->getVertexStrider(v)) + if (! buff->getVertexStrider(v)) { - v[0].set(-1, 1, 0); - v[1].set(-1, -3, 0); - v[2].set(3, 1, 0); - } - else - { - LL_WARNS() << "GL LLVertexBuffer::getVertexStrider() return false " << (NULL == buff->getMappedData() ? "buff->getMappedData() is NULL" : "buff->getMappedData() not NULL") << LL_ENDL; + LL_WARNS() << "GL LLVertexBuffer::getVertexStrider() returned false, " + << "buff->getMappedData() is" + << (buff->getMappedData()? " not" : "") + << " NULL" << LL_ENDL; + // abandon the benchmark test + return -1.f; } + // generate dummy triangle + v[0].set(-1, 1, 0); + v[1].set(-1, -3, 0); + v[2].set(3, 1, 0); + buff->flush(); gBenchmarkProgram.bind(); @@ -991,7 +1028,7 @@ F32 gpu_benchmark() for (U32 i = 0; i < count; ++i) { dest[i].bindTarget(); - gGL.getTexUnit(0)->bindManual(LLTexUnit::TT_TEXTURE, source[i]); + texHolder.bind(i); buff->drawArrays(LLRender::TRIANGLES, 0, 3); dest[i].flush(); } @@ -1038,8 +1075,6 @@ F32 gpu_benchmark() LLGLSLShader::finishProfile(false); } - LLImageGL::deleteTextures(count, source); - std::sort(results.begin(), results.end()); F32 gbps = results[results.size()/2]; @@ -1051,7 +1086,7 @@ F32 gpu_benchmark() { LL_WARNS() << "Memory bandwidth is improbably high and likely incorrect; discarding result." << LL_ENDL; //OSX is probably lying, discard result - gbps = -1.f; + return -1.f; } #endif -- cgit v1.2.3 From f2ffd637167295f9c5aa7b7643a621287068cc29 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Nov 2017 18:34:40 -0500 Subject: MAINT-7977: Release the LLRenderTargets when done. A classic-C array doesn't destroy its individual elements, but a std::vector does. Use a std::vector for dest, so each LLRenderTarget will be destroyed. ~LLRenderTarget() calls its release() method. --- indra/newview/llglsandbox.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/newview/llglsandbox.cpp') diff --git a/indra/newview/llglsandbox.cpp b/indra/newview/llglsandbox.cpp index af0bbf9a88..31d60c3afe 100644 --- a/indra/newview/llglsandbox.cpp +++ b/indra/newview/llglsandbox.cpp @@ -960,7 +960,7 @@ F32 gpu_benchmark() std::vector source; }; - LLRenderTarget dest[count]; + std::vector dest(count); TextureHolder texHolder(0, count); std::vector results; -- cgit v1.2.3 From 701f89625d5c709b21223164c152dbffd8fe9128 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 17 Nov 2017 15:57:41 -0500 Subject: MAINT-7977: Additional cleanup per code reviews. Introduce helper classes to manage paired initProfile() / finishProfile() calls and gBenchmarkProgram.bind() / unbind() calls. Make TextureHolder a class instead of a struct. Per Henri Beauchamp, since gpu_benchmark() takes a very early exit if (!gGLManager.mHasTimerQuery), subsequent tests of mHasTimerQuery are redundant. Remove. One of those tests controls the busted_finish bool, which can never become true. Remove that and all tests on it. --- indra/newview/llglsandbox.cpp | 82 +++++++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 41 deletions(-) (limited to 'indra/newview/llglsandbox.cpp') diff --git a/indra/newview/llglsandbox.cpp b/indra/newview/llglsandbox.cpp index 31d60c3afe..37e7c6fa64 100644 --- a/indra/newview/llglsandbox.cpp +++ b/indra/newview/llglsandbox.cpp @@ -923,16 +923,28 @@ F32 gpu_benchmark() //number of samples to take const S32 samples = 64; - if (gGLManager.mHasTimerQuery) + // This struct is used to ensure that once we call initProfile(), it will + // definitely be matched by a corresponding call to finishProfile(). It's + // a struct rather than a class simply because every member is public. + struct ShaderProfileHelper { - LLGLSLShader::initProfile(); - } + ShaderProfileHelper() + { + LLGLSLShader::initProfile(); + } + ~ShaderProfileHelper() + { + LLGLSLShader::finishProfile(false); + } + }; + ShaderProfileHelper initProfile; - // This struct is used to ensure that each generateTextures() call is - // matched by a corresponding deleteTextures() call. It also handles the - // bindManual() calls using those textures. - struct TextureHolder + // This helper class is used to ensure that each generateTextures() call + // is matched by a corresponding deleteTextures() call. It also handles + // the bindManual() calls using those textures. + class TextureHolder { + public: TextureHolder(U32 unit, U32 size): texUnit(gGL.getTexUnit(unit)), source(size) // preallocate vector @@ -953,6 +965,7 @@ F32 gpu_benchmark() texUnit->bindManual(LLTexUnit::TT_TEXTURE, source[index]); } + private: // capture which LLTexUnit we're going to use LLTexUnit* texUnit; @@ -1013,9 +1026,24 @@ F32 gpu_benchmark() buff->flush(); - gBenchmarkProgram.bind(); - - bool busted_finish = false; + // ensure matched pair of bind() and unbind() calls + class ShaderBinder + { + public: + ShaderBinder(LLGLSLShader& shader): + mShader(shader) + { + mShader.bind(); + } + ~ShaderBinder() + { + mShader.unbind(); + } + + private: + LLGLSLShader& mShader; + }; + ShaderBinder binder(gBenchmarkProgram); buff->setBuffer(LLVertexBuffer::MAP_VERTEX); glFinish(); @@ -1032,20 +1060,9 @@ F32 gpu_benchmark() buff->drawArrays(LLRender::TRIANGLES, 0, 3); dest[i].flush(); } - + //wait for current batch of copies to finish - if (busted_finish) - { - //read a pixel off the last target since some drivers seem to ignore glFinish - dest[count-1].bindTarget(); - U32 pixel = 0; - glReadPixels(0,0,1,1,GL_RGBA, GL_UNSIGNED_BYTE, &pixel); - dest[count-1].flush(); - } - else - { - glFinish(); - } + glFinish(); F32 time = timer.getElapsedTimeF32(); @@ -1053,28 +1070,11 @@ F32 gpu_benchmark() { //store result in gigabytes per second F32 gb = (F32) ((F64) (res*res*8*count))/(1000000000); - F32 gbps = gb/time; - - if (!gGLManager.mHasTimerQuery && !busted_finish && gbps > 128.f) - { //unrealistically high bandwidth for a card without timer queries, glFinish is probably ignored - busted_finish = true; - LL_WARNS() << "GPU Benchmark detected GL driver with broken glFinish implementation." << LL_ENDL; - } - else - { - results.push_back(gbps); - } + results.push_back(gbps); } } - gBenchmarkProgram.unbind(); - - if (gGLManager.mHasTimerQuery) - { - LLGLSLShader::finishProfile(false); - } - std::sort(results.begin(), results.end()); F32 gbps = results[results.size()/2]; -- cgit v1.2.3 From db0e11fae3e7db24d66a1f9be6b0f092bfe69063 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 17 Nov 2017 18:34:45 -0500 Subject: MAINT-7977: Per Graham, unbind textures as well as deleting. --- indra/newview/llglsandbox.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'indra/newview/llglsandbox.cpp') diff --git a/indra/newview/llglsandbox.cpp b/indra/newview/llglsandbox.cpp index 37e7c6fa64..1b6494195b 100644 --- a/indra/newview/llglsandbox.cpp +++ b/indra/newview/llglsandbox.cpp @@ -956,6 +956,8 @@ F32 gpu_benchmark() ~TextureHolder() { + // unbind + texUnit->unbind(LLTexUnit::TT_TEXTURE); // ensure that we delete these textures regardless of how we exit LLImageGL::deleteTextures(source.size(), &source[0]); } -- cgit v1.2.3