-
Notifications
You must be signed in to change notification settings - Fork 579
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
interfaces: support Tegra display drivers #7173
Conversation
interfaces/builtin/opengl.go
Outdated
/sys/devices/pci[0-9a-f]*/**/revision r, | ||
/sys/devices/pci[0-9a-f]*/**/{,subsystem_}device r, | ||
/sys/devices/pci[0-9a-f]*/**/{,subsystem_}vendor r, | ||
/sys/devices/**pci[0-9a-f]*/**/config r, |
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.
What is the ** for? what was the actual path you saw here?
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.
It is in the commit message:
/sys/devices/1003000.pcie-controller/pci0000:00/0000:00:02.0/*
interfaces/builtin/opengl.go
Outdated
@@ -121,6 +125,7 @@ var openglConnectedPlugUDev = []string{ | |||
`KERNEL=="renderD[0-9]*"`, | |||
`KERNEL=="nvhost-*"`, | |||
`KERNEL=="nvmap"`, | |||
`KERNEL=="tegra_dc_*"`, |
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.
Did you see any additional devices in /dev that snap-confine's Nvidia code needs to support?
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.
No, fortunately these devices do show in /sys and have proper udev properties.
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.
LGTM
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.
Thanks for the PR! :) Couple of clarifying questions and one change inline.
interfaces/builtin/opengl.go
Outdated
@@ -83,6 +83,9 @@ unix (bind,listen) type=seqpacket addr="@cuda-uvmfd-[0-9a-f]*", | |||
/dev/nvhost-* rw, | |||
/dev/nvmap rw, | |||
|
|||
# Tegra display driver | |||
/dev/tegra_dc_* rw, |
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.
What does this match in practice?
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.
tegra_dc_ctrl
, tegra_dc_0
and tegra_dc_1
(control device plus additional devices per display you can connect).
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 have created slightly more fine-grained rules.
interfaces/builtin/opengl.go
Outdated
/sys/devices/**pci[0-9a-f]*/**/revision r, | ||
/sys/devices/**pci[0-9a-f]*/**/{,subsystem_}class r, | ||
/sys/devices/**pci[0-9a-f]*/**/{,subsystem_}device r, | ||
/sys/devices/**pci[0-9a-f]*/**/{,subsystem_}vendor r, |
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.
Can you adjust these to be somewhat finer with:
/sys/devices/{,*pcie-controller/}pci[0-9a-f]*/**/config r,
/sys/devices/{,*pcie-controller/}pci[0-9a-f]*/**/revision r,
/sys/devices/{,*pcie-controller/}pci[0-9a-f]*/**/{,subsystem_}class r,
/sys/devices/{,*pcie-controller/}pci[0-9a-f]*/**/{,subsystem_}device r,
/sys/devices/{,*pcie-controller/}pci[0-9a-f]*/**/{,subsystem_}vendor r,
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.
Sure, will do
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.
Thanks for the review! See answers below.
Tegra drivers try to read this path: /sys/devices/1003000.pcie-controller/pci0000:00/0000:00:02.0/* Consider this case by allowing parent directories before the pci0000:00 folder.
Allow access to devices created by the Tegra display driver.
Make tests pass now that Tegra display driver has been added to the opengl interface.
5f2a938
to
706cb0b
Compare
@jdstrand ping |
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.
Thanks for the changes! This looks good. I've asked for you to add a comment (see inline), but otherwise LGTM. In the interest of time, approving.
Comment added now |
Support Tegra display drivers in x11/opengl interfaces