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

In/out extensibility of WGPULimits and WGPUInstanceFeatures #260

Closed
1 task done
kainino0x opened this issue Jan 5, 2024 · 5 comments
Closed
1 task done

In/out extensibility of WGPULimits and WGPUInstanceFeatures #260

kainino0x opened this issue Jan 5, 2024 · 5 comments
Labels
extensibility Adding features without breaking API changes has resolution Issue is resolved, just needs to be done

Comments

@kainino0x
Copy link
Collaborator

kainino0x commented Jan 5, 2024

It should be possible to ask wgpuGetInstanceFeatures for more info by adding structs to the chain. As written out here:
#199 (comment)
this is not possible because nextInChain is const *.

We can fix this by mirroring what we have already for WGPURequiredLimits/WGPUSupportedLimits:

typedef struct WGPUInstanceFeatures {
    WGPUBool timedWaitAnyEnable;
    size_t timedWaitAnyMaxCount;
} WGPUInstanceFeatures WGPU_STRUCTURE_ATTRIBUTE;

typedef struct WGPUSupportedInstanceFeatures {
    WGPUChainedStructOut * nextInChain;
    WGPUInstanceFeatures features; // out
} WGPUSupportedInstanceFeatures WGPU_STRUCTURE_ATTRIBUTE;

typedef struct WGPUInstanceDescriptor {
    WGPUChainedStruct const * nextInChain;
    WGPUInstanceFeatures features; // in
} WGPUInstanceDescriptor WGPU_STRUCTURE_ATTRIBUTE;

Unfortunately for both this and WGPULimits, we need to duplicate stuff for every extension, like:

typedef struct WGPUDawnExperimentalSubgroupLimits {
    uint32_t minSubgroupSize;
    uint32_t maxSubgroupSize;
} WGPUDawnExperimentalSubgroupLimits WGPU_STRUCTURE_ATTRIBUTE;

// Can be chained in WGPUSupportedLimits
typedef struct WGPUDawnExperimentalSubgroupSupportedLimits {
    WGPUChainedStructOut chain;
    WGPUDawnExperimentalSubgroupLimits limits; // out
} WGPUDawnExperimentalSubgroupSupportedLimits WGPU_STRUCTURE_ATTRIBUTE;

// Can be chained in WGPURequiredLimits
typedef struct WGPUDawnExperimentalSubgroupRequiredLimits {
    WGPUChainedStruct chain;
    WGPUDawnExperimentalSubgroupLimits limits; // in
} WGPUDawnExperimentalSubgroupRequiredLimits WGPU_STRUCTURE_ATTRIBUTE;

(Dawn is actually currently missing an equivalent of the latter. I suspect this is because we're abusing "limits" for what are really adapter properties?)

Note the JS API has a similar "problem" except it doesn't have to deal with extension structs.


A totally different option could be to use an array of structs instead of a chain of structs, kind of a key-value-pair system vaguely like what JS uses. We could use this for both. Example for limits:

struct WGPUExtendedLimitsBase {
    WGPUSType sType;
};

struct WGPUDawnExperimentalSubgroupLimits {
    WGPUExtendedLimitsBase type;
    uint32_t minSubgroupSize;
    uint32_t maxSubgroupSize;
};

// Pass in an array of pointers to the limit structs you want populated
WGPUBool wgpuAdapterGetLimits(WGPUAdapter adapter, WGPULimits * limits,
    size_t extendedLimitStructCount, WGPUExtendedLimitsBase * const * extendedLimitStructs);

struct WGPUDeviceDescriptor {
    // ...
    WGPU_NULLABLE WGPURequiredLimits const * requiredLimits;
    // Pass in an array of pointers to the limit structs containing limits you require.
    size_t requiredExtendedLimitStructCount;
    WGPU_NULLABLE WGPULimitStructBase const * const * requiredExtendedLimitsStructs;
    // ...
};

Or we could go all the way to what JS has with a real key-value interface, which is much simpler to write out - though probably not easier to use, and unfortunately a much more breaking change.

struct WGPULimit {
    WGPULimitName name;
    // Due to JS we are probably unlikely to ever have limits that can't be doubles (though possible with BigInt).
    // Note we no longer need WGPU_LIMIT_*_UNDEFINED because you can just not list the limit.
    double value;
};

WGPUBool wgpuAdapterGetLimits(WGPUAdapter adapter, size_t limitCount, WGPULimit * limits);

struct WGPUDeviceDescriptor {
    // ...
    size_t limitCount;
    WGPULimit const * limits;
    // ...
};

Related to #216.
cc @lokokung

@kainino0x kainino0x added the extensibility Adding features without breaking API changes label Jan 5, 2024
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Jan 11, 2024
@kainino0x
Copy link
Collaborator Author

kainino0x commented Jan 25, 2024

Jan 25 meeting:

  • RM: Array of structs idea seems less ergonomic
  • KN: Could just use "out" chain for both in and out? (But rename WGPUChainedStructOut to be clearer)
  • CF: That seems nicest
  • RM: Some generator changes - would be nice for it to know the difference between out and in-out
  • Probably do that, have to make sure it works

@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done and removed !discuss Needs discussion (at meeting or online) labels Jan 25, 2024
@jspanchu
Copy link

jspanchu commented Jun 6, 2024

Looks like the WGPUInstanceDescriptor in the existing webgpu.h header from this repo doesn't have the extra WGPUInstanceFeatures member. For this reason, when I try to dynamically load libwebgpu_dawn.so and use webgpu.h from this repository, wgpuCreateInstance complains.

Please let me know if a separate issue is required, I'd be happy to create one.

Sample:

// clang -g -O0 -fno-omit-frame-pointer -fsanitize=memory main.c
#include <dlfcn.h>
#include <stdlib.h>

#define WGPU_SKIP_DECLARATIONS
#include "webgpu.h"

int main(int argc, char **argv) {
  void *lib =
      dlopen("./build/build/dawn/src/dawn/native/libwebgpu_dawn.so", RTLD_LAZY);
  WGPUProcCreateInstance wgpuCreateInstance =
      (WGPUProcCreateInstance)dlsym(lib, "wgpuCreateInstance");
  WGPUProcInstanceRelease wgpuInstanceRelease =
      (WGPUProcInstanceRelease)dlsym(lib, "wgpuInstanceRelease");
  WGPUInstance instance;
  WGPUInstanceDescriptor desc = {};
  desc.nextInChain = NULL;
  instance = wgpuCreateInstance(&desc);

  wgpuInstanceRelease(instance);
  return EXIT_SUCCESS;
}

Error log:

Error: Requested timedWaitAnyMaxCount is not supported
    at Initialize (/home/local/KHQ/jaswant.panchumarti/dev/webgpu-for-vtk/build/dawn/src/dawn/native/EventManager.cpp:328)

MemorySanitizer:DEADLYSIGNAL
==288867==ERROR: MemorySanitizer: SEGV on unknown address 0x000000000018 (pc 0x7fb5b1bf85ab bp 0x000000000008 sp 0x7ffd3acbea88 T288867)
==288867==The signal is caused by a WRITE memory access.
==288867==Hint: address points to the zero page.
    #0 0x7fb5b1bf85ab in dawn::RefCount::Decrement() (/home/local/KHQ/jaswant.panchumarti/dev/webgpu-for-vtk/build/build/dawn/src/dawn/native/libdawn_native.so+0x3b15ab) (BuildId: e44e3168fb67d0868f9f085719d992121dffce0c)
    #1 0x7fb5b1a95c79 in dawn::native::NativeInstanceRelease(WGPUInstanceImpl*) (/home/local/KHQ/jaswant.panchumarti/dev/webgpu-for-vtk/build/build/dawn/src/dawn/native/libdawn_native.so+0x24ec79) (BuildId: e44e3168fb67d0868f9f085719d992121dffce0c)
    #2 0x5598efc354aa in main /home/local/KHQ/jaswant.panchumarti/dev/webgpu-for-vtk/main2.c:19:3
    #3 0x7fb5b38dfd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #4 0x7fb5b38dfe3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #5 0x5598efbaf2a4 in _start (/home/local/KHQ/jaswant.panchumarti/dev/webgpu-for-vtk/a.out+0x1e2a4) (BuildId: 158826293b6a9eca2cb14269b0a790af48c09c8e)

MemorySanitizer can not provide additional info.
SUMMARY: MemorySanitizer: SEGV (/home/local/KHQ/jaswant.panchumarti/dev/webgpu-for-vtk/build/build/dawn/src/dawn/native/libdawn_native.so+0x3b15ab) (BuildId: e44e3168fb67d0868f9f085719d992121dffce0c) in dawn::RefCount::Decrement()
==288867==ABORTING

@kainino0x
Copy link
Collaborator Author

This repo isn't currently in-sync with any implementation. Since you're using Dawn, for now you need to stick with the webgpu.h provided by Dawn.

We're still working through all the decided-but-not-implemented changes, separately in Dawn/wgpu-native/webgpu-headers and they won't be in sync for a while longer.

@jspanchu
Copy link

jspanchu commented Jun 6, 2024

I see, will wait until they are all in sync.

kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Sep 21, 2024
Renames WGPUWGSLFeatureName to WGPUWGSLLanguageFeatureName (the JS API
uses "wgslLanguageFeatures" to emphasize that they are language features
and not hardware features).

TODO: need to resolve the conflict between:
- Using the same chain for both supported and requested
  webgpu-native#260 (comment)
- Making it part of GetInstanceFeatures
  webgpu-native#265 (comment)

Fixes: 265
CC: 252
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Sep 21, 2024
Renames WGPUWGSLFeatureName to WGPUWGSLLanguageFeatureName (the JS API
uses "wgslLanguageFeatures" to emphasize that they are language features
and not hardware features).

TODO: need to resolve the conflict between:
- Using the same chain for both supported and requested
  webgpu-native#260 (comment)
- Making it part of GetInstanceFeatures
  webgpu-native#265 (comment)

Fixes: 265
CC: 252
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 18, 2024
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 18, 2024
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 18, 2024
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 18, 2024
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 18, 2024
@kainino0x
Copy link
Collaborator Author

This is done, except for #265 and #395 which are separately tracked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensibility Adding features without breaking API changes has resolution Issue is resolved, just needs to be done
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants