-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Expose GPU device name and driver version #12579
Expose GPU device name and driver version #12579
Conversation
it's over now :D |
We should probably expose this in spite of the fact that it will be horribly abused because there are legitimate debug logging and telemetry use cases here. It sounds like @lmurray has some input here as well so I'll wait to hear back. |
* | ||
* Currently available properties: | ||
* | ||
* - `SDL_PROP_GPU_DEVICE_PHYSICAL_NAME_STRING`: The physical name of the GPU device, as reported by the system driver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the PHYSICAL
part redundant? What other device name could it be referring to? Maybe ADAPTER
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I borrowed it from Vulkan properties name: physicalDeviceProperties.properties.deviceName
.
I am just afraid to make it very general as SDL_PROP_GPU_DEVICE_NAME_STRING
, but if you are fine - I can change that.
Adapter seems to me to be mostly D3D-specific thing, but maybe I am mistaken.
I am 100% in support of exposing this information as I have use for it in my own professional projects, mainly for a toggleable debug overlay but also for making telemetry easier. I was planning on creating my own PR for this sometime soon but I have been very tight on time recently. This PR is definitely following the correct approach but I feel like there are several minor improvements that can be made to make it more useful for apps, decrease maintenance burden on SDL maintainers, and limit misuse of the API. Instead of using this PR's author as a puppet I think it would be more efficient if I just open my own PR derived from this one with all the changes that I think are appropriate. Approximately how much longer is the 3.4 feature window open for? I'll prioritize working on this soon but it could still be a week or two before I can fit it into my schedule. |
We have at least another month for the 3.4 window. I'll go ahead and leave this open for now, and when you create your PR, we can reevaluate which one makes sense to merge. |
}; | ||
|
||
#define ASSIGN_DRIVER_FUNC(func, name) \ | ||
result->func = name##_##func; | ||
#define ASSIGN_DRIVER(name) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These whitespace changes here serve no purpose and make the diff less readable
Superseded by #12706 |
Closes #12489
This feature has been requested a few times, both on Discord and Github now.
Main purpose I presume is to include device name or driver version in logs, crash reports, custom telemetry or just simply showing during debugging on screen. Although SDL logs it via
SDL_LogInfo
in each driver, not always an end-user may use SDL logging and not always it's needed just at a point of device creation.New API is:
OP at issue #12489 suggests separate functions such as
SDL_GetGPUDeviceName
to provide that information, but I suspect some additional info may be added later (like available video memory, Vulkan compliance level etc...), therefore in this PR I tried to expose these via properties on aGPUDevice
that are filled by backend implementations respectively.Possible issues: