Bug 211882 – [WebGPU] Validation for GPUDevice.createTexture()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WebGPU] Validation for GPUDevice.createTexture()

[Myles C. Maxfield]

Created attachment 399335 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/63215999>

[Myles C. Maxfield]

Remaining: Test usage flags, determine which of the gazillion tests are relevant / interesting (maybe all of them?), and create an -expected.txt file.

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #3)
> Remaining: Test usage flags, determine which of the gazillion tests are
> relevant / interesting (maybe all of them?), and create an -expected.txt
> file.

And make sure all the validation in this patch is present in the spec.

[Myles C. Maxfield]

Created attachment 399336 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 399336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399336&action=review

> LayoutTests/webgpu/texture-creation.html:31
> +                    assert_equals(error, null, "Texture failed to be created");

This could use error.message.

[Myles C. Maxfield]

Created attachment 399338 [details]
WIP

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #4)
> (In reply to Myles C. Maxfield from comment #3)
> > Remaining: Test usage flags, determine which of the gazillion tests are
> > relevant / interesting (maybe all of them?), and create an -expected.txt
> > file.
> 
> And make sure all the validation in this patch is present in the spec.

And test maximum texture sizes

[Myles C. Maxfield]

...And make sure the rest of the tests still pass

[Myles C. Maxfield]

Created attachment 399433 [details]
Patch

[Myles C. Maxfield]

Created attachment 399456 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 399456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399456&action=review

> LayoutTests/webgpu/texture-creation.html:89
> +                let texture = device.createTexture({size: {width: test.width, height: test.height, depth: test.depth}, mipLevelCount: test.mipLevelCount, sampleCount: test.sampleCount, dimension: test.dimension, format: test.format, usage: test.usage});

We can just pass in the test object instead of copying all the values out of it.

Also, we should test default values.

[Myles C. Maxfield]

Comment on attachment 399456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399456&action=review

> LayoutTests/webgpu/texture-creation.html:89
> +                let texture = device.createTexture({size: {width: test.width, height: test.height, depth: test.depth}, mipLevelCount: test.mipLevelCount, sampleCount: test.sampleCount, dimension: test.dimension, format: test.format, usage: test.usage});

We can just pass in the test object instead of copying all the values out of it.

Also, we should test default values.

[Myles C. Maxfield]

The test fails on my machine even without my patch applied.

[Myles C. Maxfield]

I can reproduce the failure in a standalone Swift Playground:

import Foundation
import Metal

let device = MTLCreateSystemDefaultDevice()!
let textureDescriptor = MTLTextureDescriptor()
textureDescriptor.textureType = .type2DArray
textureDescriptor.pixelFormat = .rgba8Unorm
textureDescriptor.width = 512
textureDescriptor.height = 512
textureDescriptor.depth = 1
textureDescriptor.mipmapLevelCount = 9
textureDescriptor.sampleCount = 1
textureDescriptor.arrayLength = 2
textureDescriptor.storageMode = .managed
textureDescriptor.usage = .shaderRead
let texture = device.makeTexture(descriptor: textureDescriptor)!

let buffer = device.makeBuffer(length: 1048576, options: .storageModeShared)!
let ptr = buffer.contents().bindMemory(to: UInt8.self, capacity: 4)
ptr[0] = 0
ptr[1] = 255
ptr[2] = 0
ptr[3] = 255

let queue = device.makeCommandQueue()!
let commandBuffer = queue.makeCommandBuffer()!
let encoder = commandBuffer.makeBlitCommandEncoder()!
encoder.copy(from: buffer, sourceOffset: 0, sourceBytesPerRow: 2048, sourceBytesPerImage: 0, sourceSize: MTLSize(width: 512, height: 512, depth: 1), to: texture, destinationSlice: 0, destinationLevel: 0, destinationOrigin: MTLOrigin(x: 0, y: 0, z: 0))
encoder.synchronize(resource: texture)
encoder.endEncoding()
commandBuffer.addCompletedHandler {(commandBuffer) in
    var bytes = Array(repeating: UInt8(), count: 4)
    texture.getBytes(&bytes, bytesPerRow: 4, bytesPerImage: 4, from: MTLRegion(origin: MTLOrigin(x: 0, y: 0, z: 0), size: MTLSize(width: 1, height: 1, depth: 1)), mipmapLevel: 0, slice: 1)
    bytes[0]
    bytes[1]
    bytes[2]
    bytes[3]
}
commandBuffer.commit()

RunLoop.main.run()

[Myles C. Maxfield]

(The result is [0, 0, 255, 59], which is unexpected.)

[Myles C. Maxfield]

GFX9_MtlDevice
AMD Radeon Pro Vega 56

[Myles C. Maxfield]

Here's a better version:

import Foundation
import Metal

let device = MTLCopyAllDevices()[0]
device.name
let textureDescriptor = MTLTextureDescriptor()
textureDescriptor.textureType = .type2DArray
textureDescriptor.pixelFormat = .rgba8Unorm
textureDescriptor.width = 512
textureDescriptor.height = 512
textureDescriptor.depth = 1
textureDescriptor.mipmapLevelCount = 9
textureDescriptor.sampleCount = 1
textureDescriptor.arrayLength = 2
textureDescriptor.storageMode = .managed
textureDescriptor.usage = .shaderRead
let texture = device.makeTexture(descriptor: textureDescriptor)!

let buffer = device.makeBuffer(length: 1048576, options: .storageModeShared)!
let ptr = buffer.contents().bindMemory(to: UInt8.self, capacity: 1048576)
ptr[0] = 0
ptr[1] = 255
ptr[2] = 0
ptr[3] = 255

let queue = device.makeCommandQueue()!
let commandBuffer = queue.makeCommandBuffer()!
let encoder = commandBuffer.makeBlitCommandEncoder()!
encoder.copy(from: buffer, sourceOffset: 0, sourceBytesPerRow: 2048, sourceBytesPerImage: 1048576, sourceSize: MTLSize(width: 1, height: 1, depth: 1), to: texture, destinationSlice: 0, destinationLevel: 0, destinationOrigin: MTLOrigin(x: 0, y: 0, z: 0))
encoder.synchronize(resource: texture)
encoder.endEncoding()
commandBuffer.addCompletedHandler {(commandBuffer) in
    if commandBuffer.error != nil {
        print("\(commandBuffer.error!)")
    }
    var bytes = Array(repeating: UInt8(), count: 1048576)
    texture.getBytes(&bytes, bytesPerRow: 4, bytesPerImage: 4, from: MTLRegion(origin: MTLOrigin(x: 0, y: 0, z: 0), size: MTLSize(width: 512, height: 512, depth: 1)), mipmapLevel: 0, slice: 0)
    bytes[0]
    bytes[1]
    bytes[2]
    bytes[3]
}
commandBuffer.commit()

RunLoop.main.run()

[Myles C. Maxfield]

Oh, if you switch

bytesPerRow: 4, bytesPerImage: 4

with

bytesPerRow: 2048, bytesPerImage: 1048576

Then it looks like it starts working.

[Myles C. Maxfield]

Changing the test to pass in a non-zero imageHeight doesn't solve the problem.

[Myles C. Maxfield]

Created attachment 399628 [details]
???

[Myles C. Maxfield]

Created attachment 399629 [details]
Patch

[Myles C. Maxfield]

    const textureDataBufferDescriptor = {
        size: imageData.data.length,
        usage: GPUBufferUsage.COPY_SRC | GPUBufferUsage.MAP_READ
    };


Seems to fix it for some reason?

[Myles C. Maxfield]

Looks like GPUBuffer::copyStagingBufferToGPU() doesn't call -[MTLBuffer didModifyRange:]

[Myles C. Maxfield]

Created attachment 399631 [details]
Patch

[Myles C. Maxfield]

Looks like the combination of no mapping flags along with createBufferMapped is busted somehow.

[Myles C. Maxfield]

Created attachment 399632 [details]
Patch

[Myles C. Maxfield]

Created attachment 399633 [details]
Patch

[Dean Jackson]

Comment on attachment 399633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399633&action=review

Remember to not commit the change to the scheme.

> Source/WebCore/WebCore.xcodeproj/xcshareddata/xcschemes/WebCore.xcscheme:47
> +      <PathRunnable
> +         runnableDebuggingMode = "0"
> +         BundleIdentifier = "org.webkit.MiniBrowser"
> +         FilePath = "/Users/mmaxfield/Build/Products/Debug/MiniBrowser.app">
> +      </PathRunnable>

oops

> Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:118
> +        if (descriptor.size.width > 8192 || descriptor.size.height > 8192) {

Please static const this max value.

> Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:122
> +        if (descriptor.size.depth > 2048) {

And this.

> Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:128
> +        if (descriptor.size.width > 2048 || descriptor.size.height > 2048 || descriptor.size.depth > 2048) {

And this.

Also, should we provide the maximum values in the error?

[Simon Fraser (smfr)]

Comment on attachment 399633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399633&action=review

> Source/WebCore/ChangeLog:10
> +        Add lots of validation for texture creation. The logic was gathered from
> +        trial and error.

"by trial and error"

> Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:83
> +    for (; !(maxDimension & (1 << i)) && i >= 0; --i) {
> +    }

Weird to have this empty loop; maybe a while() is clearer? Is this just finding where the high bits are?

>> Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:128
>> +        if (descriptor.size.width > 2048 || descriptor.size.height > 2048 || descriptor.size.depth > 2048) {
> 
> And this.
> 
> Also, should we provide the maximum values in the error?

Yes.

> Source/WebCore/platform/graphics/gpu/GPUDevice.cpp:141
> +        m_errorScopes->generatePrefixedError("Too many miplevels.");

Show the values in the error.

> Source/WebCore/platform/graphics/gpu/GPUObjectBase.h:43
> +    GPUErrorScopes& errorScopes() const { return m_errorScopes; }

Return const GPUErrorScopes&?

> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:44
>          CopySource = 1 << 0,
>          CopyDestination = 1 << 1,
>          Sampled = 1 << 2,
>          Storage = 1 << 3,
>          OutputAttachment = 1 << 4,
> +        MaximumValue = 1 << 5,

Cool kids are lining these up these days.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUTextureMetal.mm:163
> +        errorScopes.generatePrefixedError("Device does not support this sample count.");

Error should show the value of descriptor.sampleCount.

[Myles C. Maxfield]

Committed r261843: <https://trac.webkit.org/changeset/261843>