Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement wgpuAdapterGetLimits, add test that limit requests are applied #21799

Merged
merged 9 commits into from May 1, 2024
Merged

Implement wgpuAdapterGetLimits, add test that limit requests are applied #21799

merged 9 commits into from May 1, 2024

Conversation

pkomon-tgm
Copy link
Contributor

@pkomon-tgm pkomon-tgm commented Apr 22, 2024 •
edited by kainino0x

  • Implement the wgpuAdapterGetLimits entry point.
  • Add a test which verifies that limit requests are correctly passed through to requestDevice(), by requesting all of the adapter's max limits, then ensuring that the device has the same limits. This verifies that wgpuAdapterGetLimits and wgpuDeviceGetLimits work, and that limit requests are applied (though it can only verify limits for which the current system supports a value better than the default).

Fixes #21798

@kripken kripken requested a review from kainino0x April 23, 2024 17:14
kainino0x
kainino0x previously approved these changes Apr 24, 2024
View reviewed changes
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

kainino0x
kainino0x reviewed Apr 24, 2024
View reviewed changes
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebGPU isn't available on the test VMs so you'll need to exit cleanly if WebGPU isn't available, similar to the neighboring test:

emscripten/test/webgpu_basic_rendering.cpp

Line 28 in e57881a

if (status == WGPURequestAdapterStatus_Unavailable) {

@pkomon-tgm
Copy link
Contributor Author

Ah, right, I missed that webGPU might not be available - fixed it :)

@kainino0x kainino0x dismissed their stale review April 24, 2024 22:42

pending more changes

kainino0x
kainino0x reviewed Apr 24, 2024
View reviewed changes
test/webgpu_required_limits.c Outdated Show resolved Hide resolved
test/webgpu_required_limits.c Outdated Show resolved Hide resolved
test/webgpu_required_limits.c Outdated Show resolved Hide resolved
Add impl for wgpuAdapterGetLimits, add missing field maxBufferSize to…
77e66b9 
… required limits

- move code filling WGPULimits structs into func
- use from wgpuAdapterGetLimits and wgpuDeviceGetLimits
- rewrite test case for required limit to check if supported limits
  match requested limits
- add missing field maxBufferSize when requesting device
@pkomon-tgm
Copy link
Contributor Author

Figured this should fix all comments, so I replied here rather than indiviually.

Implemented binding for wgpuAdapterGetLimits, I moved the code for filling up the WGPULimits structs into a function in the WebGPU object, hope this is the correct place to add it. Also changed the test case to test whether the requested device supports all requested limits rather than just maxColorAttachmentBytesPerSample. This way I found that another field was not set when requesting a device, which was maxBufferSize. So I also added that.

@kainino0x kainino0x mentioned this pull request May 1, 2024
Merged
kainino0x
kainino0x approved these changes May 1, 2024
View reviewed changes
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you! And nice catch on maxBufferSize.

test/webgpu_required_limits.c Outdated Show resolved Hide resolved
test/webgpu_required_limits.c Outdated Show resolved Hide resolved
src/library_webgpu.js Outdated Show resolved Hide resolved
@kainino0x kainino0x enabled auto-merge (squash) May 1, 2024 00:43
@kainino0x kainino0x disabled auto-merge May 1, 2024 21:17
@kainino0x
Copy link
Collaborator

@sbc100 the remaining failure is happening on main too, could you merge this after that's fixed, or force merge this?

@sbc100
Copy link
Collaborator

sbc100 commented May 1, 2024 •
edited

Happy to force merge this. Could you prepare to good commit message in the PR description. The current on I get if I merge now would be:

* Add test case for field maxColorAttachmentBytesPerSample when requesting webGPU device

* Add missing field maxColorAttachmentBytesPerSample when requesting webGPU device

* Fix failing test for webGPU required limits when webGPU unavailable

* Use only c style interface in webGPU required limits test

* Fix C style for webgpu required limits test

* Add impl for wgpuAdapterGetLimits, add missing field maxBufferSize to required limits

- move code filling WGPULimits structs into func
- use from wgpuAdapterGetLimits and wgpuDeviceGetLimits
- rewrite test case for required limit to check if supported limits
  match requested limits
- add missing field maxBufferSize when requesting device

* Update test/webgpu_required_limits.c

* Apply suggestions from code review

I'm not sure which parts are relevent.

@kainino0x kainino0x changed the title Add missing field maxColorAttachmentBytesPerSample to requiredFields when requesting webGPU device Implement wgpuAdapterGetLimits, add test that limit requests are applied May 1, 2024
@kainino0x
Copy link
Collaborator

kainino0x commented May 1, 2024 •
edited

@sbc100 Yes, good idea. I've rewritten the PR title and summary, please use those.

Implement wgpuAdapterGetLimits, add test that limit requests are applied

- Implement the wgpuAdapterGetLimits entry point.
- Add a test which verifies that limit requests are correctly passed through to
  requestDevice(), by requesting all of the adapter's max limits, then ensuring
  that the device has the same limits. This verifies that wgpuAdapterGetLimits
  and wgpuDeviceGetLimits work, and that limit requests are applied (though it
  can only verify limits for which the current system supports a value better
  than the default).

Fixes #21798

@sbc100 sbc100 merged commit 849342f into emscripten-core:main May 1, 2024
27 of 29 checks passed
@pkomon-tgm pkomon-tgm mentioned this pull request May 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbc100 sbc100 sbc100 left review comments

@kainino0x kainino0x kainino0x approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing field maxColorAttachmentBytesPerSample in requiredLimits for wgpuAdapterRequestDevice
3 participants
@pkomon-tgm @kainino0x @sbc100