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

add EGL_EXT_device_query_pci #109

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

1ace
Copy link
Contributor

@1ace 1ace commented Jul 2, 2020

Firefox requested to have a way to query the device's vendor PCI ID and device PCI ID, so here's an extension that adds this.

Cross-references:

/cc @rmader

@1ace 1ace marked this pull request as draft July 2, 2020 23:39
@kbrenneman
Copy link
Contributor

Would it make sense to add queries for the domain/bus/slot ID's as well?

@1ace 1ace force-pushed the device_query_vendor branch from 5737aee to 6392f9b Compare July 3, 2020 01:31
@1ace
Copy link
Contributor Author

1ace commented Jul 3, 2020

Would it make sense to add queries for the domain/bus/slot ID's as well?

Why not, but is there a use-case for these?

@1ace 1ace changed the title add EGL_EXT_device_query_vendor add EGL_EXT_device_query_pci_id Jul 3, 2020
@kbrenneman
Copy link
Contributor

One use I can think of would be to distinguish between multiple identical devices.

@1ace
Copy link
Contributor Author

1ace commented Jul 5, 2020

It way beyond the problem we're trying to solve here, but it's trivial to implement and conceptually it belongs in the same extension so I'm adding these and re-renaming the spec to EGL_EXT_device_query_pci 👍

@1ace 1ace changed the title add EGL_EXT_device_query_pci_id add EGL_EXT_device_query_pci Jul 5, 2020
@1ace 1ace force-pushed the device_query_vendor branch 2 times, most recently from 457aa86 to 58e5081 Compare July 5, 2020 17:03
@1ace 1ace marked this pull request as ready for review July 6, 2020 12:29
@kbrenneman
Copy link
Contributor

As a minor nitpick, the "The query always succeeds and returns EGL_TRUE, and is set appropriately" line probably isn't necessary, since that's the default assumption for EGL functions.

Other than generic errors (invalid EGLDeviceEXT handle, out of memory, etc), EGL functions list the errors that they can produce.

In this case, it doesn't make much difference, though, since I don't think an implementation could run into any internal errors for those queries. Anything that did go wrong would have gone wrong in eglQueryDevicesEXT instead.

Anyway, change that if you want. The spec looks good to me either way.

@1ace
Copy link
Contributor Author

1ace commented Jul 6, 2020

Thanks for the feedback; you're right, I'll drop the line.

I also was wondering if I should explicitly say that EGL_QUERY_PCI_VENDOR_ID_EXT is the vendor ID, etc. or if it's all obvious enough as it is?

@kbrenneman
Copy link
Contributor

I'd say it's obvious enough what each attribute is.

@1ace 1ace force-pushed the device_query_vendor branch from 58e5081 to 38ff3a4 Compare July 11, 2020 08:54
@stonesthrow
Copy link
Contributor

Change Milestone to "Approved to Merge" when ready and assign to oddhack for Jon to merge.

@1ace
Copy link
Contributor Author

1ace commented Aug 1, 2020

@stonesthrow I don't have permission to change the milestones; could you give me permission, or do it for me? Thanks!

@stonesthrow
Copy link
Contributor

stonesthrow commented Aug 3, 2020

@oddhack: Ready to merge

@1ace
Copy link
Contributor Author

1ace commented Aug 3, 2020

Actually can this wait? @evelikov has requested a change of enum values, I'm waiting on @fooishbar to confirm.

@1ace 1ace marked this pull request as draft August 3, 2020 23:00
@stonesthrow stonesthrow removed this from the Approved to Merge milestone Aug 18, 2020
@@ -719,4 +719,9 @@ extension EGL_NV_stream_consumer_eglimage {
flags public
filename extensions/NV/EGL_NV_stream_consumer_eglimage.txt
}
# Next free extension number: 140
extension EGL_EXT_device_query_pci {
number 140
Copy link
Contributor

Choose a reason for hiding this comment

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

The extension number here and in the spec text will need to be updated when the PR is actually accepted, up through 141 are used at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@1ace @evelikov @fooishbar where does this extension stand - was your issue about enum values resolved? Would be nice to get it finished and published.

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.

5 participants