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

interfaces: support Tegra display drivers #7173

Merged
merged 7 commits into from
Aug 26, 2019

Conversation

alfonsosanchezbeato
Copy link
Member

Support Tegra display drivers in x11/opengl interfaces

/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,
Copy link
Collaborator

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?

Copy link
Member Author

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/*

@@ -121,6 +125,7 @@ var openglConnectedPlugUDev = []string{
`KERNEL=="renderD[0-9]*"`,
`KERNEL=="nvhost-*"`,
`KERNEL=="nvmap"`,
`KERNEL=="tegra_dc_*"`,
Copy link
Collaborator

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?

Copy link
Member Author

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.

@alfonsosanchezbeato alfonsosanchezbeato changed the title Support Tegra display drivers in x11/opengl interfaces interfaces/builtin/{x11,opengl}: support Tegra display drivers Jul 26, 2019
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

LGTM

@zyga zyga changed the title interfaces/builtin/{x11,opengl}: support Tegra display drivers interfaces: support Tegra display drivers Jul 26, 2019
Copy link

@jdstrand jdstrand left a 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/x11.go Show resolved Hide resolved
@@ -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,

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?

Copy link
Member Author

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).

Copy link
Member Author

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.

/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,

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,

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do

Copy link
Member Author

@alfonsosanchezbeato alfonsosanchezbeato left a 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.
@chipaca
Copy link
Contributor

chipaca commented Aug 13, 2019

@jdstrand ping

Copy link

@jdstrand jdstrand left a 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.

interfaces/builtin/x11.go Show resolved Hide resolved
@alfonsosanchezbeato
Copy link
Member Author

Comment added now

@mvo5 mvo5 merged commit 5fbd51c into snapcore:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants