Skip to content

add EGL_EXT_device_query_pci#109

Draft
1ace wants to merge 1 commit into
KhronosGroup:mainfrom
1ace:device_query_vendor
Draft

add EGL_EXT_device_query_pci#109
1ace wants to merge 1 commit into
KhronosGroup:mainfrom
1ace:device_query_vendor

Conversation

@1ace

@1ace 1ace commented Jul 2, 2020

Copy link
Copy Markdown
Contributor

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
Comment thread extensions/EXT/EGL_EXT_device_query_vendor.txt Outdated
Comment thread extensions/EXT/EGL_EXT_device_query_vendor.txt Outdated
Comment thread extensions/EXT/EGL_EXT_device_query_vendor.txt Outdated
@kbrenneman

Copy link
Copy Markdown
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

1ace commented Jul 3, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

@1ace

1ace commented Jul 5, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

1ace commented Jul 6, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
Contributor

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

@1ace

1ace commented Aug 1, 2020

Copy link
Copy Markdown
Contributor Author

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

@stonesthrow

stonesthrow commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

@oddhack: Ready to merge

@1ace

1ace commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

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
Comment thread registry.tcl
}
# Next free extension number: 140
extension EGL_EXT_device_query_pci {
number 140

Copy link
Copy Markdown
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
Copy Markdown
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.

@jlHertel

Copy link
Copy Markdown

Hey folks,

I have a new use case for this extension and I'm wondering what people might think about it:
In adriconf we make use of libdrm in order to enumerate all devices and get their PCI IDs, but ideally I would like to see us getting this directly from EGL. Enumerating devices is already possible under EGL, but getting their PCI Ids isn't.

Sorry for reviving this old thread. If there is a better place to ask about this I kindly ask you to point me there.

@stonesthrow

Copy link
Copy Markdown
Contributor

This extension is work in progress, it was approved and then put on hold. It can be re activated at any time and modified as needed by authors. But then needs review.

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.

6 participants