microsoft / DirectXTK12

The DirectX Tool Kit (aka DirectXTK12) is a collection of helper classes for writing DirectX 12 code in C++
https://walbourn.github.io/directx-tool-kit-for-directx-12/
MIT License
1.47k stars 393 forks source link

BGR GenerateMips wrong on AMD video cards #49

Closed LigaLiao closed 5 years ago

LigaLiao commented 5 years ago

if generate mips for a BGR format texture (by GenerateMips_TexturePathBGR), mips will be messy except for MipSlice0.

but generate mips for a RGB format texture (direct by GenerateMips_UnorderedAccessPath), mips is good.

I don't see any mistakes in the code, I think it might have something to do with hardware. The graphics card I use is AMD R9 290, drivers use the latest.

walbourn commented 5 years ago

I've verified this code on AMD parts before, so it is probably a driver bug. You should report it to AMD.

You can also check out LoadTest in the test suite GitHub.

LigaLiao commented 5 years ago

qq 20190202115152

This also happens in “LoadTest”

walbourn commented 5 years ago

Feel free to point to the LoadTest as a repro in your driver bug report to AMD. I just confirmed this code works on my AMD FirePro W7000 (driver 20.19.0.32852), NVIDIA GeForce GTX 1070 (driver 24.21.13.9793), and Xbox One.

walbourn commented 5 years ago

The code also works fine on NVIDIA GeForce GTX 870M (driver 23.21.13.8873), NVIDIA GeForce GTX 960 (23.21.13.9077), Intel HD Graphics 4600 (20.19.15.4835), and the WARP12 software device (Windows 10 Version 1809).

This is an AMD driver bug.

walbourn commented 5 years ago

Actually, it looks like the code assumes D3D12_FEATURE_DATA_D3D12_OPTIONS.TypedUAVLoadAdditionalFormat is set. Can you see if it's set for your hardware?

LigaLiao commented 5 years ago

Sure, but please wait , I won't be home until three days later.

LigaLiao commented 5 years ago

QQ截图20190420120746 This bug also happens at RX Vega. I have given report to AMD before, but didn't get a useful response.

walbourn commented 5 years ago

Just to see if it helped, I wrote an alternative version to see if the problem was that the RGB+UAV layout was different than the BGR (no UAV) layout so I create an RGB alias then copy again to an RGB+UAV:

    void GenerateMips_TexturePathBGR(
        _In_ ID3D12Resource* resource)
    {
        const auto resourceDesc = resource->GetDesc();
        assert(FormatIsBGR(resourceDesc.Format));

        // Create a resource with the same description as RGB
        auto copyDesc = resourceDesc;
        copyDesc.Format = DXGI_FORMAT_R8G8B8A8_UNORM;

        D3D12_HEAP_DESC heapDesc = {};
        auto allocInfo = mDevice->GetResourceAllocationInfo(0, 1, &resourceDesc);
        heapDesc.SizeInBytes = allocInfo.SizeInBytes;
        heapDesc.Flags = D3D12_HEAP_FLAG_ALLOW_ONLY_NON_RT_DS_TEXTURES;
        heapDesc.Properties.Type = D3D12_HEAP_TYPE_DEFAULT;

        ComPtr<ID3D12Heap> heap;
        ThrowIfFailed(mDevice->CreateHeap(&heapDesc, IID_GRAPHICS_PPV_ARGS(heap.GetAddressOf())));

        SetDebugObjectName(heap.Get(), L"ResourceUploadBatch");

        ComPtr<ID3D12Resource> resourceCopy;
        ThrowIfFailed(mDevice->CreatePlacedResource(
            heap.Get(),
            0,
            &copyDesc,
            D3D12_RESOURCE_STATE_COPY_DEST,
            nullptr,
            IID_GRAPHICS_PPV_ARGS(resourceCopy.GetAddressOf())));

        SetDebugObjectName(resourceCopy.Get(), L"GenerateMips Alias Resource Copy");

        // Create a BGRA alias
        auto aliasDesc = resourceDesc;
        aliasDesc.Format = (resourceDesc.Format == DXGI_FORMAT_B8G8R8X8_UNORM || resourceDesc.Format == DXGI_FORMAT_B8G8R8X8_UNORM_SRGB) ? DXGI_FORMAT_B8G8R8X8_UNORM : DXGI_FORMAT_B8G8R8A8_UNORM;

        ComPtr<ID3D12Resource> aliasCopy;
        ThrowIfFailed(mDevice->CreatePlacedResource(
            heap.Get(),
            0,
            &aliasDesc,
            D3D12_RESOURCE_STATE_COPY_DEST,
            nullptr,
            IID_GRAPHICS_PPV_ARGS(aliasCopy.GetAddressOf())));

        // Copy the resource data BGR to RGB
        D3D12_RESOURCE_BARRIER aliasBarrier[3] = {};
        aliasBarrier[0].Type = D3D12_RESOURCE_BARRIER_TYPE_ALIASING;
        aliasBarrier[0].Aliasing.pResourceAfter = aliasCopy.Get();

        aliasBarrier[1].Type = aliasBarrier[2].Type = D3D12_RESOURCE_BARRIER_TYPE_TRANSITION;
        aliasBarrier[1].Transition.Subresource = aliasBarrier[2].Transition.Subresource = D3D12_RESOURCE_BARRIER_ALL_SUBRESOURCES;
        aliasBarrier[1].Transition.pResource = resource;
        aliasBarrier[1].Transition.StateBefore = D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE;
        aliasBarrier[1].Transition.StateAfter = D3D12_RESOURCE_STATE_COPY_SOURCE;

        mList->ResourceBarrier(2, aliasBarrier);
        mList->CopyResource(aliasCopy.Get(), resource);

        // Create UAV-capable resource
        auto uavDesc = resourceDesc;
        uavDesc.Format = DXGI_FORMAT_R8G8B8A8_UNORM;
        uavDesc.Flags |= D3D12_RESOURCE_FLAG_ALLOW_UNORDERED_ACCESS;

        CD3DX12_HEAP_PROPERTIES heapProperties(D3D12_HEAP_TYPE_DEFAULT);

        ComPtr<ID3D12Resource> uavCopy;
        ThrowIfFailed(mDevice->CreateCommittedResource(
            &heapProperties,
            D3D12_HEAP_FLAG_NONE,
            &uavDesc,
            D3D12_RESOURCE_STATE_COPY_DEST,
            nullptr,
            IID_GRAPHICS_PPV_ARGS(uavCopy.GetAddressOf())));

        SetDebugObjectName(uavCopy.Get(), L"GenerateMips UAV Resource Copy");

        // Copy the resource data to UAV
        aliasBarrier[0].Aliasing.pResourceBefore = aliasCopy.Get();
        aliasBarrier[0].Aliasing.pResourceAfter = resourceCopy.Get();

        aliasBarrier[1].Transition.pResource = resourceCopy.Get();
        aliasBarrier[1].Transition.StateBefore = D3D12_RESOURCE_STATE_COPY_DEST;
        aliasBarrier[1].Transition.StateAfter = D3D12_RESOURCE_STATE_COPY_SOURCE;

        mList->ResourceBarrier(2, aliasBarrier);
        mList->CopyResource(uavCopy.Get(), resourceCopy.Get());

        // Generate the mips
        Transition(uavCopy.Get(), D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE);
        GenerateMips_UnorderedAccessPath(uavCopy.Get());

        // Copy the resource data from UAV to alias
        D3D12_RESOURCE_BARRIER barrier[2] = {};
        barrier[0].Type = barrier[1].Type = D3D12_RESOURCE_BARRIER_TYPE_TRANSITION;
        barrier[0].Transition.Subresource = barrier[1].Transition.Subresource = D3D12_RESOURCE_BARRIER_ALL_SUBRESOURCES;
        barrier[0].Transition.pResource = uavCopy.Get();
        barrier[0].Transition.StateBefore = D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE;
        barrier[0].Transition.StateAfter = D3D12_RESOURCE_STATE_COPY_SOURCE;

        barrier[1].Transition.pResource = resourceCopy.Get();
        barrier[1].Transition.StateBefore = D3D12_RESOURCE_STATE_COPY_SOURCE;
        barrier[1].Transition.StateAfter = D3D12_RESOURCE_STATE_COPY_DEST;

        mList->ResourceBarrier(2, barrier);
        mList->CopyResource(resourceCopy.Get(), uavCopy.Get());

        // Direct copy back
        aliasBarrier[0].Aliasing.pResourceBefore = resourceCopy.Get();
        aliasBarrier[0].Aliasing.pResourceAfter = aliasCopy.Get();

        aliasBarrier[1].Transition.pResource = aliasCopy.Get();
        aliasBarrier[1].Transition.StateBefore = D3D12_RESOURCE_STATE_COPY_DEST;
        aliasBarrier[1].Transition.StateAfter = D3D12_RESOURCE_STATE_COPY_SOURCE;

        aliasBarrier[2].Transition.pResource = resource;
        aliasBarrier[2].Transition.StateBefore = D3D12_RESOURCE_STATE_COPY_SOURCE;
        aliasBarrier[2].Transition.StateAfter = D3D12_RESOURCE_STATE_COPY_DEST;

        mList->ResourceBarrier(3, aliasBarrier);

        mList->CopyResource(resource, aliasCopy.Get());
        Transition(resource, D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE);

        // Track these object lifetimes on the GPU
        mTrackedObjects.push_back(heap);
        mTrackedObjects.push_back(resourceCopy);
        mTrackedObjects.push_back(aliasCopy);
        mTrackedObjects.push_back(resource);
    }

Unfortunately, this produced the same result.

walbourn commented 5 years ago

I did some looking at the behavior with the DirectX 12 PIX for Windows and the problem happens very early on within the first CopyResource to the alias... Either the driver isn't handling alias barriers quite right or there's some subtle difference in how it treats BGR formats vs. RGB formats (which is silly since for AMD hardware there no difference at all in those formats).

I'll look more tomorrow...

walbourn commented 5 years ago

It looks like the AMD driver is using a different memory layout for BGR than RGB, so the alias fails in the BGR <-> RGB copy.

The expected working behavior when I'm doing that first aliased copy is:

nvalias_before nvalias_after

The AMD failing case instead looks like:

amdalias_before amdalias_after

This is before I do anything with DirectCompute.

LigaLiao commented 5 years ago

So ,who should fix this, Directx team? or AMD driver team?

walbourn commented 5 years ago

I've asked the AMD driver team to take a look. They can help me determine if the library needs changes or just their driver...

walbourn commented 5 years ago

Basically you can't assume BGR 8-bit and RGB 8-bit formats use the same memory layout in general. Therefore, I updated the code so that if the device does not support D3D12_FEATURE_DATA_D3D12_OPTIONS.StandardSwizzle64KBSupported, then I can't autogen mips for a BGR format.

See this commit. In the April 2019 release of DirectX Tool Kit, if you request autogen mips with the DDS or WIC loader and the format is not supported for autogen on the device, it will automatically disable autogen mips.

I can still support BGR on Xbox One even though it doesn't report support for StandardSwizzle64KBSupported because I know the device uses the same memory layout for those formats.