Add experimental vhost-user-video backend #445

aesteve-rh · 2023-09-14T09:33:59Z

Summary of the PR

This PR adds an experimental virtio-video vhost-user backend, that support v4l2 decoding.

Comments

The virtio specification concerning virtio-video is still being discussed. As such, this device should be considered as experimental. It is based in the v3 patch of the specification, as it has its driver working, and even included in some
out-of-tree kernels (e.g. chromeos and AAOS). This version of the specification patch was also used for the experimental support of virtio-video in crosvm.

In that sense, I have these branches available to be able to test the device using Qemu:

The driver is available at
https://github.com/aesteve-rh/linux

And the Qemu backend device is available at
https://github.com/aesteve-rh/qemu/tree/virtio-video-v3

The plan in the long term is to update this crate to follow virtio specification once it gets agreed on and published, at which point it will lose its experimental status. In the meantime this can be used as PoC, add encoder support, or be used as a basis for verifying a later revision of the specs and help to get them merged.

Related: #364

Requirements

These items may not be covered by the patch, I'll address (and mark) them ASAP.

All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

aesteve-rh · 2023-09-14T10:02:32Z

~~I will fix clippy issues locally, please hold the reviews until that is solved so that we start in a sane status.~~ Fixed!

Regarding the code coverage, unfortunately I have decreased it :( I will try to cover as much as posible, but for testing the decoder module we may need to have a video device available.

Ablu

Just some random, all over the place comments. Nothing serious, but also no systematic review.

Looks overall, fairly straight forward. Some of the unsafe blocks look pretty big... But I will wait for the // SAFETY comments on those :)

Ablu · 2023-09-14T10:27:08Z

crates/video/src/main.rs

+}
+
+pub(crate) fn start_backend(config: VuVideoConfig) -> Result<()> {
+    loop {


We should maybe find a standard way for all the vhost-device daemons regarding looping serving requests.

I think currently looping by restarting daemons is a bit hacky since the socket will actually killed and reinitialized, leaving a window where it becomes unavailable...

At the moment, this loop is in line with what other backends do. If we decide to modify the strategy, I would be happy to prepare a separate PR with a proposal that applies to all backends to keep them consistent.

Except for scsi (I just realized), that it has no loop... do you think it would be better to remove this loop for now?

My comment was mostly reflecting on the current situation. This PR is fine with or without loop. We should just eventually figure out how to prevent each backend to re-decide on this :). Thats of course out-of-scope for this PR.

crates/video/src/stream.rs

crates/video/src/video_backends/v4l2_decoder.rs

crates/video/src/video.rs

crates/video/src/video_backends/v4l2_decoder.rs

crates/video/Cargo.toml

Ablu · 2023-09-14T10:59:06Z

I will fix clippy issues locally, please hold the reviews until that is solved so that we start in a sane status.

Regarding the code coverage, unfortunately I have decreased it :( I will try to cover as much as posible, but for testing the decoder module we may need to have a video device available.

Ah, did not read that. My fault :).

aesteve-rh · 2023-09-14T11:21:22Z

Ah, did not read that. My fault :).

No worries, I just didn't want to waste anyone's time. I'll check your comments now :)

bilelmoussaoui

random comments until I get a pretty close look at the whole thing.

crates/video/Cargo.toml

crates/video/src/main.rs

crates/video/src/video.rs

crates/video/src/video_backends/null.rs

crates/video/src/video_backends/v4l2_decoder.rs

aesteve-rh · 2023-09-27T08:32:34Z

So, unless I missed something, I tackled all review comments above. I think this PR is ready for another round.

In parallel, I want to close the coverage gap as much as possible, but I try to run pytest rust-vmm-ci/integration_tests/test_coverage.py locally and fails. Any hints on how to run the coverage script locally?

Ablu · 2023-09-27T08:37:17Z

So, unless I missed something, I tackled all review comments above. I think this PR is ready for another round.

Will try to take a look soon. But won't get around it this week. We will also need to decide how to handle devices that are not fully upstreamed yet. Probably we will want to move them to some separate folder...

In parallel, I want to close the coverage gap as much as possible, but I try to run pytest rust-vmm-ci/integration_tests/test_coverage.py locally and fails. Any hints on how to run the coverage script locally?

You can follow https://github.com/rust-vmm/rust-vmm-ci#running-the-tests-locally to run things locally.

stefano-garzarella · 2023-09-27T08:40:00Z

So, unless I missed something, I tackled all review comments above. I think this PR is ready for another round.

In parallel, I want to close the coverage gap as much as possible, but I try to run pytest rust-vmm-ci/integration_tests/test_coverage.py locally and fails. Any hints on how to run the coverage script locally?

Which failure do you have?

You can also use the rustvmm/dev container. I usually run in this way from the root of the repo:

cd vhost-device
podman run --device=/dev/kvm -it --rm --security-opt=seccomp=unconfined --volume $(pwd):/crate rustvmm/dev:v26
cd crate
pytest --profile=devel rust-vmm-ci/integration_tests/test_coverage.py

Inspired from https://github.com/rust-vmm/rust-vmm-ci#adaptive-coverage

aesteve-rh · 2023-09-27T09:12:06Z

Which failure do you have?

I was getting:

>               raise CalledProcessError(retcode, process.args,
                                         output=stdout, stderr=stderr)
E               subprocess.CalledProcessError: Command 'CARGO_TARGET_DIR=/home/aesteve/Work/automotive/vhost-device/cov_build cargo llvm-cov test --summary-only --workspace ' returned non-zero exit status 101.

/usr/lib64/python3.11/subprocess.py:571: CalledProcessError

You can also use the rustvmm/dev container. I usually run in this way from the root of the repo:

I was trying yesterday, but I think I picked every source except docker.io, which was the right one 🤦 . The steps that you've written were giving me an access permission error. Probably missing --priviledged? But finally this command did work:

podman run -t -i --rm --init --volume $(pwd):/workdir --workdir /workdir --privileged rustvmm/dev:v26 /bin/sh -e -c pytest\ rust-vmm-ci/integration_tests/test_coverage.py

So I will add some tests and see where I can get. Thanks!

stefano-garzarella · 2023-09-27T09:24:26Z

Which failure do you have?

I was getting:

>               raise CalledProcessError(retcode, process.args,
                                         output=stdout, stderr=stderr)
E               subprocess.CalledProcessError: Command 'CARGO_TARGET_DIR=/home/aesteve/Work/automotive/vhost-device/cov_build cargo llvm-cov test --summary-only --workspace ' returned non-zero exit status 101.

/usr/lib64/python3.11/subprocess.py:571: CalledProcessError

Maybe cargo install cargo-llvm-cov could help with this error.

You can also use the rustvmm/dev container. I usually run in this way from the root of the repo:

I was trying yesterday, but I think I picked every source except docker.io, which was the right one 🤦 . The steps that you've written were giving me an access permission error. Probably missing --priviledged?

Strange, I'm not using --priviledged, maybe some seccomp configuration.

But finally this command did work:
podman run -t -i --rm --init --volume $(pwd):/workdir --workdir /workdir --privileged rustvmm/dev:v26 /bin/sh -e -c pytest\ rust-vmm-ci/integration_tests/test_coverage.py
So I will add some tests and see where I can get. Thanks!

Great :-)

stefano-garzarella · 2023-09-27T09:33:50Z

So, unless I missed something, I tackled all review comments above. I think this PR is ready for another round.

Will try to take a look soon. But won't get around it this week. We will also need to decide how to handle devices that are not fully upstreamed yet. Probably we will want to move them to some separate folder...

Yep, like a nested workspace maybe called experimental or something like that. I'll open an issue to track that!

Ablu · 2023-09-27T09:35:29Z

Yep, like a nested workspace maybe called experimental or something like that. I'll open an issue to track that!

We briefly discussed this next to our Linaro Employee meeting in Amsterdam. I think @epilys plans to post a RFC to suggest a solution.

aesteve-rh · 2023-09-27T09:38:46Z

Maybe cargo install cargo-llvm-cov could help with this error.

Yes, that works, and I can omit the container altogether. Thanks! I am not so experienced in Rust so running the cargo install was not obvious to me.

stefano-garzarella · 2023-09-27T09:41:19Z

Yep, like a nested workspace maybe called experimental or something like that. I'll open an issue to track that!

We briefly discussed this next to our Linaro Employee meeting in Amsterdam. I think @epilys plans to post a RFC to suggest a solution.

Okay, I just opened an issue #459. I'll update it, so we are all aware of that.

aesteve-rh · 2023-09-27T09:44:25Z

We briefly discussed this next to our Linaro Employee meeting in Amsterdam. I think @epilys plans to post a RFC to suggest a solution.

I'll keep an eye on both the issue and the RFC, and update this PR accordingly on the decisions. I agree in that having them separated is probably a good idea.

vireshk · 2023-10-18T06:14:22Z

@stsquad any last comments before we merge this ?

aesteve-rh · 2023-10-19T13:03:30Z

Should I retrigger the pipelines now that the staging CI is (iinm) fixed?

stefano-garzarella · 2023-10-19T13:09:04Z

Should I retrigger the pipelines now that the staging CI is (iinm) fixed?

I didn't think it would be so long ;-P but we have to wait for this last PR to be merged, then we have to update the submodule here, and at that point we should be ready ;-)

aesteve-rh · 2023-10-23T07:50:34Z

  package `vhost` is specified twice in the lockfile

Looks like the merge did break the lockfile (?). Anyway, let me fix it before merging, so that we leave CIs in a sane state. Will the coverage check run now for the staging folder?

stefano-garzarella · 2023-10-23T07:53:50Z

  package `vhost` is specified twice in the lockfile
Looks like the merge did break the lockfile (?). Anyway, let me fix it before merging, so that we leave CIs in a sane state.

Yep, thanks!

Will the coverage check run now for the staging folder?

Not yet :-( sorry. We still have some issues: #493

Don't worry about that for now, we can fix it later if we have issues.

Initial skeleton for virtio-video crate. This crate is based on the v3 of the virtio-video specs patch[1]. It has a big part of the infrastructure required, although not all commands are implemented, and does not have any backend available. Includes support for async responses to the driver (through VIDEO_EVENT) to QueueResource messages. [1] - https://lists.oasis-open.org/archives/virtio-dev/202002/msg00002.html Related: rust-vmm#364 Signed-off-by: Albert Esteve <[email protected]>

Add new v4l2-decoder backend, that uses v4l2r [1] for interactions with the video device in the host. Specialized for Linux systems. [1] - https://github.com/Gnurou/v4l2r Signed-off-by: Albert Esteve <[email protected]>

Add test for all modules, with dev-dependencies (including rstest [1] for parametrized tests), and infrastructure. [1] - https://docs.rs/rstest/latest/rstest/ Signed-off-by: Albert Esteve <[email protected]>

Complete the README file for the video crate with the help information for the CLI, and a working example to run and test the device. Signed-off-by: Albert Esteve <[email protected]>

Add v4l2 wrapper functions to be able to mock (and test) v4l2r library responses without the need of having a underlying video device. Signed-off-by: Albert Esteve <[email protected]>

stefano-garzarella · 2023-10-24T09:14:14Z

@vireshk @stsquad you had already approved the PR before the rebase, can you please approve again?

aesteve-rh requested review from mathieupoirier, stsquad, vireshk and stefano-garzarella as code owners September 14, 2023 09:34

aesteve-rh force-pushed the virtio-video branch from 4e81427 to bc18810 Compare September 14, 2023 09:49

Ablu reviewed Sep 14, 2023

View reviewed changes

aesteve-rh force-pushed the virtio-video branch from bc18810 to 8885fd1 Compare September 14, 2023 11:18

aesteve-rh force-pushed the virtio-video branch from 8885fd1 to 97dc637 Compare September 14, 2023 11:25

bilelmoussaoui reviewed Sep 14, 2023

View reviewed changes

aesteve-rh force-pushed the virtio-video branch 2 times, most recently from 39169b0 to 2dc64c7 Compare September 18, 2023 11:58

aesteve-rh force-pushed the virtio-video branch 5 times, most recently from 44c12ee to 9f3f4c4 Compare September 26, 2023 07:45

stefano-garzarella mentioned this pull request Sep 27, 2023

Add a folder or nested workspace to support experimental devices #459

Closed

vireshk previously approved these changes Oct 18, 2023

View reviewed changes

stefano-garzarella previously approved these changes Oct 18, 2023

View reviewed changes

stefano-garzarella mentioned this pull request Oct 18, 2023

CI: run nightly cargo fmt wiht a custom rustfmt.toml #488

Open

3 tasks

stsquad previously approved these changes Oct 23, 2023

View reviewed changes

stsquad dismissed stale reviews from stefano-garzarella, vireshk, and themself via c7cd582 October 23, 2023 07:37

stefano-garzarella previously approved these changes Oct 23, 2023

View reviewed changes

aesteve-rh added 5 commits October 23, 2023 10:00

video: add v4l2-decoder backend

4d9825f

Add new v4l2-decoder backend, that uses v4l2r [1] for interactions with the video device in the host. Specialized for Linux systems. [1] - https://github.com/Gnurou/v4l2r Signed-off-by: Albert Esteve <[email protected]>

video: add tests

a5066ec

Add test for all modules, with dev-dependencies (including rstest [1] for parametrized tests), and infrastructure. [1] - https://docs.rs/rstest/latest/rstest/ Signed-off-by: Albert Esteve <[email protected]>

video: fill readme

2a58c80

Complete the README file for the video crate with the help information for the CLI, and a working example to run and test the device. Signed-off-by: Albert Esteve <[email protected]>

video: add v4l2 mocks

2113ad1

Add v4l2 wrapper functions to be able to mock (and test) v4l2r library responses without the need of having a underlying video device. Signed-off-by: Albert Esteve <[email protected]>

aesteve-rh dismissed stefano-garzarella’s stale review via 2113ad1 October 23, 2023 08:02

aesteve-rh force-pushed the virtio-video branch from c7cd582 to 2113ad1 Compare October 23, 2023 08:02

stefano-garzarella approved these changes Oct 23, 2023

View reviewed changes

aesteve-rh requested review from stsquad and vireshk October 24, 2023 07:31

Merge branch 'main' into virtio-video

28179a5

stefano-garzarella enabled auto-merge (rebase) October 24, 2023 09:14

vireshk approved these changes Oct 24, 2023

View reviewed changes

stefano-garzarella merged commit 3921238 into rust-vmm:main Oct 24, 2023
2 checks passed

stefano-garzarella mentioned this pull request Oct 24, 2023

sound: fix broken tests #496

Merged

aesteve-rh deleted the virtio-video branch October 24, 2023 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental vhost-user-video backend #445

Add experimental vhost-user-video backend #445

aesteve-rh commented Sep 14, 2023 •

edited

Loading

aesteve-rh commented Sep 14, 2023 •

edited

Loading

Ablu left a comment

Ablu Sep 14, 2023

aesteve-rh Sep 15, 2023

aesteve-rh Sep 18, 2023

Ablu Sep 18, 2023

Ablu commented Sep 14, 2023

aesteve-rh commented Sep 14, 2023 •

edited

Loading

bilelmoussaoui left a comment

aesteve-rh commented Sep 27, 2023

Ablu commented Sep 27, 2023

stefano-garzarella commented Sep 27, 2023

aesteve-rh commented Sep 27, 2023 •

edited

Loading

stefano-garzarella commented Sep 27, 2023

stefano-garzarella commented Sep 27, 2023

Ablu commented Sep 27, 2023

aesteve-rh commented Sep 27, 2023

stefano-garzarella commented Sep 27, 2023

aesteve-rh commented Sep 27, 2023

vireshk commented Oct 18, 2023

aesteve-rh commented Oct 19, 2023

stefano-garzarella commented Oct 19, 2023

aesteve-rh commented Oct 23, 2023

stefano-garzarella commented Oct 23, 2023

stefano-garzarella commented Oct 24, 2023

Add experimental vhost-user-video backend #445

Add experimental vhost-user-video backend #445

Conversation

aesteve-rh commented Sep 14, 2023 • edited Loading

Summary of the PR

Comments

Requirements

aesteve-rh commented Sep 14, 2023 • edited Loading

Ablu left a comment

Choose a reason for hiding this comment

Ablu Sep 14, 2023

Choose a reason for hiding this comment

aesteve-rh Sep 15, 2023

Choose a reason for hiding this comment

aesteve-rh Sep 18, 2023

Choose a reason for hiding this comment

Ablu Sep 18, 2023

Choose a reason for hiding this comment

Ablu commented Sep 14, 2023

aesteve-rh commented Sep 14, 2023 • edited Loading

bilelmoussaoui left a comment

Choose a reason for hiding this comment

aesteve-rh commented Sep 27, 2023

Ablu commented Sep 27, 2023

stefano-garzarella commented Sep 27, 2023

aesteve-rh commented Sep 27, 2023 • edited Loading

stefano-garzarella commented Sep 27, 2023

stefano-garzarella commented Sep 27, 2023

Ablu commented Sep 27, 2023

aesteve-rh commented Sep 27, 2023

stefano-garzarella commented Sep 27, 2023

aesteve-rh commented Sep 27, 2023

vireshk commented Oct 18, 2023

aesteve-rh commented Oct 19, 2023

stefano-garzarella commented Oct 19, 2023

aesteve-rh commented Oct 23, 2023

stefano-garzarella commented Oct 23, 2023

stefano-garzarella commented Oct 24, 2023

aesteve-rh commented Sep 14, 2023 •

edited

Loading

aesteve-rh commented Sep 14, 2023 •

edited

Loading

aesteve-rh commented Sep 14, 2023 •

edited

Loading

aesteve-rh commented Sep 27, 2023 •

edited

Loading