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

Kernel/HID: Add support for virtio input devices (+ use them for RISC-V in run.py) #24254

Merged
merged 2 commits into from May 23, 2024

Conversation

spholz
Copy link
Contributor

@spholz spholz commented May 7, 2024

This PR makes keyboard and (absolute) mouse input work on RISC-V!

The QEMU virtio keyboard, mouse and tablet (absolute mouse) work on both x86-64 and riscv64.
I only tested the keyboard driver with my german keyboard (+german layout), but that should hopefully not be an issue. I would still appreciate it if someone could test this with a different layout.

We could also consider changing run.py to use the virtio input devices on x86-64 in the future, since those might be a better option than using PS/2 when running in QEMU.

The new virtio driver is not in the Bus/VirtIO directory, as #24152 will move all other virtio drivers out of there.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 7, 2024
@spholz spholz force-pushed the virtio-input branch 4 times, most recently from 16ba956 to 94a852f Compare May 8, 2024 09:16
Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

Couple comments, but overall I don't see any actual problems with the code.

Kernel/Bus/VirtIO/Transport/PCIe/Detect.cpp Show resolved Hide resolved
Kernel/Devices/HID/VirtIO/EvDevDefinitions.h Show resolved Hide resolved
Kernel/Devices/HID/VirtIO/Input.cpp Outdated Show resolved Hide resolved
Kernel/Arch/riscv64/PCI/Initializer.cpp Outdated Show resolved Hide resolved
@supercomputer7
Copy link
Member

We could also consider changing run.py to use the virtio input devices on x86-64 in the future, since those might be a better option than using PS/2 when running in QEMU.

If the VirtIO driver is stable enough, I'd suggest that we should move to that and ditch PS/2 for bare metal usage.

HIDManagement::the().attach_standalone_hid_device(*m_mouse_device);
} else {
dbgln("Virtio::Input: Unknown product ID");
VERIFY_NOT_REACHED();
Copy link
Contributor Author

@spholz spholz May 12, 2024

Choose a reason for hiding this comment

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

There sadly is no nice way to ignore devices with unknown devids, since the caller of initialize_virtio_resources uses MUST. We also would have to leave the Input device in a half-initialized state, which doesn't seem ideal.
So I would rather wait for #24210 to be merged, since I don't think we should be using VERIFY_NOT_REACHED here.

Copy link
Contributor Author

@spholz spholz May 15, 2024

Choose a reason for hiding this comment

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

I removed the devid matching code now.
Only the mouse will reliably work until #24210 is merged.

The keyboard actually somewhat already works with the way it is added in run.py because it is listed first, and QEMU apparently assigns PCI addresses based on the order of the command line arguments.
As a result, the keyboard is located at "/dev/input/keyboard/0", which is where WindowServer currently expects it to be.

@spholz spholz force-pushed the virtio-input branch 4 times, most recently from 125fc1d to f3e4e7f Compare May 17, 2024 22:09
@ADKaster
Copy link
Member

conflicts

@ADKaster ADKaster added the ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased label May 23, 2024
@spholz
Copy link
Contributor Author

spholz commented May 23, 2024

rebased

@ADKaster ADKaster removed the ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased label May 23, 2024
@ADKaster ADKaster merged commit dc92bc2 into SerenityOS:master May 23, 2024
12 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label May 23, 2024
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.

None yet

4 participants