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.

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

Set featureLevel: "compatibility" when compatibility=1 is enabled #4081

Merged
merged 11 commits into from
Dec 12, 2024

Conversation

beaufortfrancois
Copy link
Collaborator

As suggested in https://g-issues.chromium.org/issues/366151404#comment9, this PR adds a featureLevel CTS option.

Note that I'm not 100% sure I cover all changes required for the CTS.


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)
  • Test have be tested with compatibility mode validation enabled and behave as expected. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@kainino0x
Copy link
Collaborator

It's not immediately apparent that we will need to be able to run the CTS under customizable featureLevel values. It would be much simpler and avoid a lot of migration work if the CTS just keeps ?compatibility=1 as the option. We can add ?featureLevel=compatibility later if we find it's needed.

The thing that we need to do is have CTS request adapters using featureLevel: "compatibility" so that chromium can stop obeying compatibilityMode: true (but of CTS should keep compatibilityMode: true for now as well during migration).

@beaufortfrancois
Copy link
Collaborator Author

@kainino0x This makes this PR much simpler ;)

@kainino0x kainino0x changed the title Add featureLevel CTS option Set featureLevel="compatibility" when compatibility=1 is enabled Dec 12, 2024
@kainino0x kainino0x changed the title Set featureLevel="compatibility" when compatibility=1 is enabled Set featureLevel: "compatibility" when compatibility=1 is enabled Dec 12, 2024
@kainino0x kainino0x enabled auto-merge (squash) December 12, 2024 04:10
@kainino0x kainino0x merged commit 5b26e0e into main Dec 12, 2024
1 check passed
@kainino0x kainino0x deleted the featureLevel2 branch December 12, 2024 04:12
@greggman
Copy link
Contributor

I think this should be changed so it doesn't request 'core'.

Instead of

    featureLevel: globalTestConfig.compatibility ? 'compatibility' : 'core',

It should be

    globalTestConfig.compatibility && { featureLevel: 'compatibility' },

wdyt?

@beaufortfrancois
Copy link
Collaborator Author

'core' is the default value for featureLevel so I think it's fine. See https://gpuweb.github.io/gpuweb/#dom-gpurequestadapteroptions-featurelevel

@greggman
Copy link
Contributor

The problem is it makes the CTS never test that you get core if you don't request any feature level

@beaufortfrancois
Copy link
Collaborator Author

I believe this will be tested once featureLevel GPUAdapter attribute is added to the spec in gpuweb/gpuweb#5012?

@beaufortfrancois
Copy link
Collaborator Author

Note that I've added #4077 already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants