TextureGPUManager::setWorkerThreadMinimumBudget will trigger the error check on budget set in constructor

[knox31085]

Hello,

the error-check will trigger the warning if you just fill in the default data for the budget. I think that just the error condition is wrong in this case.

Shouldn't it be

if( itor->minNumSlices > 1u && itor->minResolution > maxSplitResolution )

instead of

if( itor->minNumSlices > 1u && itor->minResolution >= maxSplitResolution )?

Thanks

ogre-next/OgreMain/src/OgreTextureGpuManager.cpp

Line 1357 in a481059

if( itor->minNumSlices > 1u && itor->minResolution >= maxSplitResolution )

[darksylinc]

Mmm... (very long mmmmm.....)

The condition evaluation is not wrong. The train of thought is the following (speaking out loud to see if I sort it out):

• maxSplitResolution indicates when a texture is considered "too big". If it's too big, then it will be considered as an abnormality. This affects how we allocate memory for streaming.
  • The problem is that a 4096x4096 RGBA8_UNORM texture is 64MB. If we'd expect this to be normal, then we expect a lot of textures like that to be streamed in and may reserve a lot of memory for a long of time, even if it goes unused.
  • It is important to note that GART/GTT size is 256 MB, so with just one 4096² texture we're already close to that limit. Drivers can handle more, but they more work and having so much memory mapped either causes slowdowns, out of memory errors, or reveals driver bugs. Resizable BAR is meant to address this, but it's too recent (e.g. AMD calls it Smart Access Memory aka SAM, and you need Ryzen 5xxxx + AMD Radeon 6xxxx XT; they came out end of last year, if you manage to buy one).
    • It shouldn't be a problem on Vulkan because we explicitly control which heap is used, and we try to avoid the GPU-local, CPU visible heap for staging area.
  • If you load many 4096² textures we should handle it fine, but quickly after the textures stop coming in, we'll deallocate resources because we don't expect this to repeat again soon (and if it repeats, then we'll reallocate).
  • A better approximation would be to use total memory in bytes instead of a single texture dimension. Unfortunately D3D11 uses the notion of StagingTextures, and while a 4096x16 texture consumes little memory (which GL, Vulkan, Metal and D3D12 would handle just fine, because all transfers are 1D instead of 2D and 3D), D3D11 needs a StagingTexture of 4096x16 or bigger, causing a lot of waste. So it's better to consider this 4096x16 texture an abnormality too.
    • To explain why: Let's say there's a 4096x16 texture; and a 4096² was already available. We will consume a fraction of that, and leave 4096x4080 for next transfers. If two 2048² appear in a row, that D3D11 StagingTexture can be reused. That means the 4096² staging texture was used for 3 transfers.
    • But if a 4096² appears after the 4096x16 (instead of two 2048²) then we need to allocate another StagingTexture of 4096². And that's how memory consumption can easily blow up if the 4096x16 isn't considered an abnormality
• minResolution = 4096; minNumSlices = 2; means if we encounter a 4096x4096 texture, we should reserve GPU memory to allocate two of them together (i.e. we'll create a 2D texture array of 4096x4096x2). That means, we expect them to be at least 2 4096x4096 textures. Which sort of contradicts the previous statement that 4096² textures are an abnormality.
  • Originally minNumSlices was 1 for 4096; but it was raised to 2 because 4096² textures are not that uncommon. Larger values like 4 would blow up memory consumption with diminishing returns (the point of packing textures together is to reduce texture state switches to avoid being CPU bound. But if you have many 4096² textures, then you're likely to be GPU-bound by bandwidth)
  • However it is common these 4096² textures to be compressed or single channel. That means actual consumption is between 16-32MB per texture. These are fine.

OK, now that I laid out the reasoning, perhaps the warning is too exaggerated. minNumSlices > 1 and minResolution >= maxSplitResolution are not contradictory.

But having minNumSlices > very_large and minResolution >= maxSplitResolution may indicate something is incorrectly setup (but not necessarily. If you're very tight on memory you may wanna treat lots of textures as spikes; and hurt streaming performance in exchange for... not crashing your app because you're already close to the memory limit)

[knox31085]

That is a real detailed description about the memory management :).

But the problem is very simple:
In the constructor the budget is set to the following parameters without a check of the warning:
`
// 64-bit have plenty of virtual addresss to spare. We can reserve much more.
mStreamingData.maxSplitResolution = 4096u;

    const uint32 maxResolution = mStreamingData.maxSplitResolution;
    //32MB / 128MB for RGBA8, that's two 4096x4096 / 2048x2048 texture.
    format = PixelFormatGpuUtils::getFamily( PFG_RGBA8_UNORM );
    mBudget.push_back( BudgetEntry( format, maxResolution, 2u ) );

...
`

If this budget was now reset with the function we get the warning since we have two slices with a resolution which equals to the maxSplitResolution...

In my opinion it should be possible to set the default values without warnings. It seems that there is an inconsistency in the warning-condition with the default parameterset.

[darksylinc]

In my opinion it should be possible to set the default values without warnings. It seems that there is an inconsistency in the warning-condition with the default parameterset.

Agreed. Thanks for the feedback. Stuff that is obvious from the user's perspective sometimes eludes the library maintainer.

I loosened the check a bit (and documented the warning) so that defaults don't trigger the warning.

Thanks!