...round 2 because building locally missed some :)
Most are safe, similar to crrev.com/c/4014832
Two require an exception, similar to crrev.com/c/4018392
BUG=b:157245930, crbug:908640
TEST=CQ with needless_return suppression disabled
Change-Id: I0f54ee1e4688afe9c9f596bd32a98070aac20612
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4019273
Commit-Queue: Clarissa Garvey <clarissagarvey@chromium.org>
Reviewed-by: Dennis Kempin <denniskempin@google.com>
MappableHandle was implemented on the backend handle, but doing so is
sub-optimal in the case of VAAPI which needs to re-create the VA Image
with each call to a method of MappableHandle, which is a costly
operation.
Fix this by implementing MappableHandle on the Image directly. That way
callers can request the mappable handler once to create the image, and
call all the methods on the same instance.
This also allows us to make MappableHandle::image_size return the size
instead of a Result.
BUG=b:214478588
TEST=cargo build --features `video-decoder,video-encoder,ffmpeg,vaapi`
TEST=cargo test --features vaapi -p cros-codecs
Change-Id: Iaa048c1f488021d49376f612ebde560e84a11dc4
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4006228
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.corp-partner.google.com>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
The concept of mapped resolution is only used in the context of copying
the frame to a CPU-mapped buffer in order to compute the size of said
buffer according to the underlying format.
This requires the caller to check the buffer format and perform the
computation themselves. It would be more efficient to just have a
function performing that computation in a single place and returning the
final buffer size.
Thus, replace MappedHandle's mapped_resolution() method with
image_size(), which returns the size of the buffer that should be passed
to read().
BUG=b:214478588
TEST=cargo build --features `video-decoder,video-encoder,ffmpeg,vaapi`
TEST=cargo test --features vaapi -p cros-codecs
Change-Id: Icff17aa5e2e08676edb450a53316e44897604958
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4005492
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.corp-partner.google.com>
Improve the --serial option's errors for file operations by including
the path that could not be opened or created in the message.
Also replace the FileError uses for socket errors with their own
specific error variants.
BUG=None
TEST=crosvm run --serial type=file,path==/does/not/exist ...
Change-Id: Ia5eb71629b824e67f828e6516bf7771b2dba4903
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4010942
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Auto-Submit: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
This CL adds allow directives for needless return on functions that have
void returns and use the pattern of "error and then return early", and
happen to have a currently-needless return as the last statement. This
is to prevent future bugs if the functions are extended.
Each case is documented with a comment explaining why the exception is
allowed.
BUG=chromium:908640, b:157245930
TEST=cargo clippy with needless_return suppression removed
Change-Id: I44f978b0cd9aa0d574acc73cd2f0a34f1b181e84
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4018392
Auto-Submit: Clarissa Garvey <clarissagarvey@chromium.org>
Commit-Queue: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Before this CL, there were several places with suppressed clippy
warnings about needless_return statements following the same pattern of
spawning a worker thread in a match statement. After this CL, those
places have had the work refactored out of the match statement such that
there is no needless_return triggered by the code, but it does the same
thing.
BUG=chromium:908640, b:157245930
TEST=cargo clippy with needless_return suppression removed
Change-Id: Idfa4a16597d973ef840a031990c01640c4a8252d
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4014833
Commit-Queue: Clarissa Garvey <clarissagarvey@chromium.org>
Reviewed-by: Dennis Kempin <denniskempin@google.com>
This reduces the runtime of devices unit tests from ~5s to 1.7s.
BUG=None
TEST=None
Change-Id: I9801bba025db71ea3c69d26f8afcfce07db7d894
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4004680
Reviewed-by: Vikram Auradkar <auradkar@google.com>
This CL contains a few fixes to needless_return which need approval from
someone familiar with the code. In two cases, the return was removed
from a function that returns a value, so it is considered safe because
if code is added later into the function, it will be clear that this is
removing an implicit return from the function. In the last case
(iommu.rs), the function has several error! macros that do not trigger
an early return, so it seems that it is okay to remove the last early
return.
BUG=b:157245930, chromium:908640
TEST=cargo clippy (no logical code changes)
Change-Id: I00685c9033499a92713eb591eadb5f90c29e761d
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4014835
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Clarissa Garvey <clarissagarvey@chromium.org>
Reviewed-by: David Stevens <stevensd@chromium.org>
Currently, we ignore clippy::needless_return, with many violations in
the code base. This CL fixes trivial, safe-to-fix violations of
needless_return. These were found by removing the needless_return
suppression in .cargo/config.toml and then fixing needless returns.
These are not all needless returns in the code base; it is only the
trivial fixes. This CL is a part of a chain of CLs to fix the
needless_returns in the code base.
BUG=chromium:908640, b:157245930
TEST=cargo clippy + cargo build
Change-Id: I6f8f3a0bed25d774fba7ae4b6e4c021af439ec22
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4014832
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Commit-Queue: Clarissa Garvey <clarissagarvey@chromium.org>
The value of VIRTIO_GPU_RESP_OK_RESOURCE_PLANE_INFO (which is not
upstream) conflicts with upstream VIRTIO_GPU_RESP_OK_EDID. We assign
both VIRTIO_GPU_RESP_OK_EDID and VIRTIO_GPU_RESP_OK_RESOURCE_UUID to the
same value.
BUG=b:2050923
TEST=none (just comments)
Change-Id: I8fe2c3818e451cc456ae6c15b9b827c22cfbd6e9
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3993318
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Auto-Submit: Chad Versace <chadversary@chromium.org>
Commit-Queue: Dennis Kempin <denniskempin@google.com>
There is no CLI change in this CL. All existing tests are either
moved to gpu_config.rs or merged with other tests.
BUG=b:254284360
TEST=presubmit
Change-Id: Id9be8247d540337230d20db6f313a983743664b1
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3971033
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Commit-Queue: Pujun Lun <lunpujun@google.com>
The patch makes usb feature no-op on windows as USB pass-through is
not supported on windows.
BUG=b:241251677
BUG=b:213149155
TEST=tools/presubmit
Change-Id: Id82d4c732a86e782695d2af698cc08e8e3fd2d35
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4006819
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Vikram Auradkar <auradkar@google.com>
The `chrono` crate attempts to read the local timezone from the TZ
environment variable, and if it's missing, it will try to read
`/etc/localtime`, which uses system calls that are not allowed in the
common device sandbox policy.
Preserve the TZ environment variable from the parent process across the
minijail fork so the /etc/localtime fallback is avoided and no
additional system calls are required in the seccomp policy.
BUG=b:257987535
TEST=Boot ARCVM
Change-Id: Ieaf5e59d0c839ad4c017cc283b81b9eb1430ff67
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4009580
Reviewed-by: Allen Webb <allenwebb@google.com>
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
The non-vhost counterpart already provides this feature flag,
specifying it in the vhost-user implementation will allow
vhost-user net devices to negotiate MTUs too.
TEST=tools/presubmit
Change-Id: If83929671218c45d3e37f782a66b438f9fef5147
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4003960
Commit-Queue: Kevin Hamacher <hamacher@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
The `blocking` boolean was converted into a `BlockingMode` anyway, and
using `BlockingMode` directly is also more explicit about what the
parameters is about.
BUG=b:214478588
TEST=cargo build --features `video-decoder,video-encoder,ffmpeg,vaapi`
TEST=cargo test --features vaapi -p cros-codecs
Change-Id: If72067d8218d53ff8d092cef384909d282862e0a
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3995885
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
When decoding H.264, ffmpeg tries to set the frame format before
receiving the first DRC event, which is currently rejected even though
this is valid from the point of view of the client. We need to silently
accept the temporary format and all queued output buffers until the
initial DRC event is fired, even though we won't emit any frame.
BUG=None
TEST=`ffmpeg -codec:v h264_v4l2m2m -i Big_Buck_Bunny_720_10s_1MB.mp4 BBB-%d.png`
completes successfully in the guest and produces valid frames.
Change-Id: I9a190ab2748bd59ff57d54be681f2a1553d1c6a3
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3841484
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Makes the async vhost-user backend cross-platform. The next change will
add the plumbing to turn it on. The plan is to create GpuBackendConfig
and GpuVmmConfig in the broker and pass to the relevant processes.
This way, we can also pass GpuBackendConfig to the main process if we
want to use the original non-vhost-user worker. The config changes will
be included with the plumbing CL that follows.
- Split into a sys module.
- Introduce 'platform_workers' that tracks platform-dependent futures.
Reasoning: Windows will need to be able to launch more futures at
runtime due to our input handling, it's useful to have a vector of
workers to append to. This way the specific worker function doesn't
need to leak into the shared file. We can also put the resource
bridge workers here following the same logic.
- Introduce backend and VMM config structures to pass around.
BUG=b:243061269
TEST=downstream / presubmit
Change-Id: I53458c4dd2cf74b9e6bf5d10819533206e47a683
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3963645
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Kaiyi Li <kaiyili@google.com>
Reviewed-by: Ryan Neph <ryanneph@google.com>
Commit-Queue: Idan Raiter <idanr@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
https://crrev.com/c/3977111 factored out the integration tests for all
irqchip implementations, except for whpx.
Factor out the tests. Add `#[allow(unused)]` attribute for certain
helper functions that are not used in whpx. Make `interrupt_requested`
and `get_external_interrupt` public methods for tests to call into.
Test: Verified tests on windows. Using following command:
Test: `cargo test --tests -p devices --features="whpx,slirp" whpx -- --nocapture --test-threads 1`
Change-Id: I1863d509357193fdbc309e90cd0631fe5849a3bc
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3993814
Commit-Queue: Vaibhav Nagarnaik <vnagarnaik@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
libva Displays and Contexts are only useful if they are
reference-counted, and all callers of `create_context` immediately wraps
the result into a Rc. Make `create_context` return a `Rc` directly to
make sure we cannot end up with stray objects.
BUG=b:214478588
TEST=cargo test --features vaapi -p cros-codecs
TEST=cargo test --features "video-decoder,vaapi" -p devices
Change-Id: I04c90059df71ea8a09a1fa937d731e1f4a5c27fc
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3932441
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
The attributes array can be passed as empty to indicate we don't have
any preference for the configuration (which is what passing None
results into anyway), so remove the Option in order to simplify the
arguments.
BUG=b:214478588
TEST=cargo test --features vaapi -p cros-codecs
TEST=cargo test --features "video-decoder,vaapi" -p devices
Change-Id: I677b61f595f82cfbb519da3d78f433478eede8b9
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3932439
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
This option has just been introduced and has no user, so we can slip
this one under the rug.
BUG=b:255223604
TEST=cargo test -p devices net::tests
TEST=`cargo run -- .. --net tap-name=crosvm_tap` properly creates a TAP
network device.
Change-Id: I228e5f2539c49314399cf254ddaf795bd0265a2f
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3990100
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
Fix one `needless_borrow` and two `redundant_closures`.
BUG=None
TEST=`cargo clippy --features direct` passes without warning.
Change-Id: I69e295ddec233d57fbc5e549f2ab34c40fe4d834
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3990284
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Covers the slightly awkward ffmpeg v4l2 stateful implementation for the
capture queue setup.
ffmpeg will queue a single OUTPUT buffer and immediately follow up with
a VIDIOC_G_FMT call on the CAPTURE queue. This leads to a race
condition, because it takes some appreciable time for the real
resolution to propagate back to the guest as the virtio machinery
processes and delivers the event.
In the event that VIDIOC_G_FMT(capture) returns the default format,
ffmpeg allocates buffers of the default resolution (640x480) only to
immediately reallocate as soon as it processes the SRC_CH v4l2 event.
Otherwise (if the resolution has propagated in time), this path will not
be taken during the initialization.
This leads to the following workflow in the virtio video worker:
RESOURCE_QUEUE -> QUEUE_CLEAR -> RESOURCE_QUEUE
Failing to accept this (as we previously did), leaves us with bad state
and completely breaks the decoding process. We should replace the queue
even if this is not 100% according to spec.
On the other hand, this branch still exists to highlight the fact that
we should assert that we have emitted a buffer with the LAST flag when
support for buffer flags is implemented in a future CL. If a buffer
with the LAST flag hasn't been emitted, it's technically a mistake to be
take the branch because we still have buffers of the old resolution to
deliver.
BUG=b:214478588
TEST="The error 'Invalid state' is not randomly returned anymore while
decoding with ffmpeg in the guest"
Change-Id: I41b85818a5451d814f03c6a4ea0c328b5161437c
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3928130
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Do not flush the codec if the submit queue is not empty. Flushing the
codec backend will release the reference frames in most (all) codecs.
This cannot happen if we still have pending frames in the submit queue
as these frames would not find their references, breaking the decoding
process.
Not only that, but as the length of the submit/ready queues is
runtime-dependent, we would end up with a random number of corrupted
frames during flush.
Instead, signal that we want to flush and try making progress
accordingly, but only flush the backend once it has processed all the
pending frames in the submit queue.
BUG=b:214478588
TEST=test-25fps.h264 decodes without corruption in the last (flushed)
frames
Change-Id: I4200102344bab86c9938ae898383bb82e6542594
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3907530
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Introduce a reordering mechanism to reorder from decode order into
display order. This builds upon the new display_order() call introduced
in cros-codecs.
As frames are delivered in decode order, we have to wait before
outputting them if we notice any gaps. Thus a monotonically increasing
counter is required. This is because we rely on the monotonically
increasing property to identify the gaps.
BUG=b:214478588
TEST="The decoded data is in order (unless corruption took place)"
Change-Id: Ia910aa739088d110e0ee0f24c0b50e049625a399
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3907529
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Add a convertion for the VP8 VA profile, otherwise the profile will be
skipped when the Capabilities are built and VP8 will be unsupported.
BUG=b:214478588
TEST="Decoder reports VP8 as supported (e.g.: if using GStreamer,
gst-inspect-1.0 video4linux2 lists a VP8 decoder)"
Change-Id: I6af62b475d840f309c3bdc7cc05e7722149c9b3b
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3907527
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Do not drain the ready queue if we have not been provided buffers by
CrosVM.
The previous implementation would attempt to drain as soon as a picture
was ready to be outputted, but this does not account for the fact that
there is a window of time between ProvidePictureBuffers being emitted
and set_output_parameters() being called, as the guest needs to process
the request.
This meant that ready frames would be dequeued, but the call to
output_picture would return Err, effectively dropping the frames before
they could be outputted.
Simply return early if OutputQueueState != OutputQueueState::Decoding,
leaving the queue as is.
BUG=b:214478588
TEST="Decoding test-25fps.h264 yields exactly 250 frames."
Change-Id: I6042f4556933d4b59a54c6ca986510356a040e18
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3903094
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
The reset implementation for the VAAPI backend was wrong for a couple of
reasons.
It would create a new session, thereby invalidating the eventfd file
descriptor passed to the WaitContext. This meant that events would not
get signalled from that point on.
Also it would recreate the codec instance, but the right thing to do
here is to flush, which resets the internal state without needlessly
invalidating the allocations already made.
The above is also the same logic done by the ffmpeg decoder, which calls
avcodec_flush_buffers instead of recreating the AVCodecContext.
The previous implementation would also not clear the submit and ready
queues.
Not clearing the submit queue meant we could not return the buffers to
crosvm, which is against the contract for reset(). This is because this
queue keeps a reference to GuestResourceHandle.
Not clearing the ready queue meant that we would return stale buffers
once/if decoding resumed.
Lastly, it would reset the state of the output queue to
AwaitingOutputBuffers, but we can resume decoding with no new buffers
provided, so this is incorrect.
In particular, this change eliminates the following error reported by
the virtio video kernel driver:
virtio-video virtio-video.5: timed out waiting for queue clear
And also the related CrosVM virtio video worker error: returning async
error response: invalid operation
As the ResetCompleted event is correctly signalled now.
BUG=b:214478588
TEST="Decoding in the guest does not cause these two warnings to be
emitted: virtio-video virtio-video.5: timed out waiting for queue clear
and returning async error response: invalid operation right after that"
Change-Id: I0ed2fc59819875fb3cc4462d401811f3126ab686
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3903093
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Introduce try_make_progress() as a way to enforce the following
invariant between the submit and ready queues: the ready queue must
always be drained first.
This is to avoid deadlocks: draining the submit queue will fail if the
ready queue is full enough, since this prevents the deallocation of the
VASurfaces embedded in the handles stored in the ready queue. This
means that no progress gets done.
While we are at it, make sure to call it in places where we were only
erroneously draining the submit queue (which again, means that no
progress could be made)
BUG=b:214478588
TEST="The decoder does not randomly stalls if the guest app is too slow
to retrieve frames in the ready queue"
Change-Id: I9ea34089b7df4417272cdc775d42422b865a993c
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3901198
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
We are always done with the input buffer even if no frames are generated
by the decoder. Reflect this by always releasing the buffer after a call
to self.codec.decode()
While we are at it, note that it's the resource_id that has to be passed
to NotifyEndOfBitstreamBuffer.
BUG=b:214478588
TEST="Guest does not complain about unknown resource_ids when processing
NotifyEndOfBitstreamBuffer"
Change-Id: I61c0d2f0e7ea6a4907e853bb2f005bb421447042
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3901197
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
The current code will add an input format even though it might not be
able to be decoded to NV12. This will be fixed in a later patch, but for
now just drop these formats instead.
BUG=b:214478588
TEST=cargo test -p devices --features=video-decoder,vaapi virtio::video::decoder::backend::vaapi::tests::test_get_capabilities -- --include-ignored
Change-Id: I89e7261fdc935207f1fcb11d18e7701cdaf54ba9
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3901274
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
The max width and height from the coded side are the best approximation
for the max width and height of the raw side if the actual values are
not available in VA-API.
In particular, it's extremely unlikely to have 1x1 as acceptable values
for max_width and max_height.
BUG=b:214478588
TEST=cargo test -p devices --features=video-decoder,vaapi virtio::video::decoder::backend::vaapi::tests::test_get_capabilities -- --include-ignored
Change-Id: I6d9830b4da69f29f5e2e11c3fa688db4a7007726
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3901273
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
The raw capabilities should not depend on the value for rt_format.
Both in the Intel and Mesa implementations it can be seen that setting
VAConfigAttribRTFormat as a parameter to vaCreateConfig only serves as a
way to validate that the to-be-generated config supports it.
This means that the Intel implementation will only use it in
CheckAttribList, while Mesa will AND it with a precomputed bitmask of
supported values in vaCreateConfig.
Thus, creating one RawCaps instance per value of RT_FORMAT only serves
to create needless duplication.
Not only that, but it frequently makes the call to QUERY_CAPABILITIES
fail, as the resulting structures become too large to fit into the 1024
byte buffer.
Remove all traces of VA_RT_FORMAT_* from this code path. While we are at
it, note that the libva crate API for vaQuerySurfaceAttribute changed to
reflect that this call can return more than one attribute at once.
This is particularly important for VASurfaceAttribPixelFormat, as one
value per supported fourcc is returned. We now properly return one
instance of RawCaps per VA_FOURCC supported.
Also reflect that Surface attributes can be defined more than once
This is particularly the case with VASurfaceAttribPixelFormat, where the
driver will define one attrib per pixel format supported.
This can be seen in the Intel driver, for example, in
MediaLibvaCaps::QuerySurfaceAttributes.
The previous code assumed that a given attribute could only be defined
once, and thus used find(). It was refactored to use filter() instead.
The name of the method was changed to reflect that it can now return
more than one attribute per call. The return type was changed to Vec for
the same reason.
BUG=b:214478588
TEST="cargo test --package devices --lib --features video-decoder --features vaapi -- virtio::video::decoder::backend::vaapi::tests::test_get_capabilities --include-ignored"
Change-Id: I914376beb026e30b2a52ca8f7d01c81e6a9a2775
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3946301
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Queue incoming requests so that they can be retried if the backend runs
out of resources.
Previously the VA-API backend would just refuse to decode if all
allocated surfaces were in use. Notably this can be the case if the
surfaces are kept in use by virtue of being in the ready queue. An
example of which could be seen whenever the guest would not retrieve the
decoded images as fast as the hardware decoder would produce them.
This issue can be worked around by keeping a queue of the decode
requests in the order they were submitted by the guest. The operation
can then be retried either on the next call to decode or when a new
output buffer becomes available, whatever comes first.
BUG=b:214478588
TEST="Decoding from the guest does not fail with
StatelessBackendError::OutOfResources"
Change-Id: I53b972f6520bdc704be6eaca14c5343391122914
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3875045
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Port the VAAPI backend to the new cros-codecs crate. This crate now
contains all codec related code and is independent from the rest of the
CrosVM code.
BUG=b:214478588
TEST="cargo test --package devices --lib --features video-decoder --features vaapi -- virtio::video::decoder::backend::vaapi::tests::test_get_capabilities --include-ignored"
Change-Id: Id207c53c0c4200e03ce8793d7c37cb5fbe808829
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3875044
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
This change moves most ircchip tests into an integration test.
These tests rely on kvm, and if they do not - reuse much of the
test code from each other, so they need to move together.
Note: This removes the apic_timer test. The test has been disabled
for a while due to it's use of sleep in unit tests.
We cannot support it as an integration test either, since it combines
the sleep with a FakeClock. userspace.rs swaps the real clock for
FakeClock when compiled with cfg(test), but integration tests do not
compile with cfg(test), so we cannot use the FakeClock.
The biggest side-effect is faster execution as we can run all other 300+
tests in parallel and via user-space emulation, significantly cutting
down on the test times. It also allows those tests to run in a
podman container.
BUG=b:244620308
TEST=CQ
Change-Id: I1728a736d27e924daf228752711435885dacfa6a
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3977111
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Dennis Kempin <denniskempin@google.com>
Other device parameters with a backend type use `backend` as field name,
so do the same for video devices. This is backward-compatible as the
backend field is implicit for the command-line.
BUG=b:255223604
TEST=cargo build --features "video-decoder,video-encoder,ffmpeg"
Change-Id: I5133080714d3292295468a2c7152a06fc669da1c
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3974353
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Dennis Kempin <denniskempin@google.com>
If crosvm's connection to the host compositor is suddenly broken
(e.g. from a compositor crash), a single POLLHUP is sent on the listened
file descriptor, but it is ignored. Then the VirtioGpu backend is
repeatedly awoken to handle display events, although there are none.
Since the compositor is lost and there is currently no path for
recovery, we can at least prevent runaway CPU usage by removing the
display from VirtioGpu's WaitContext. For VMs that can continue to
function without a guest display (headless crostini), this is
non-lethal. For VMs that require a display (ARCVM), other mechanisms
already exist for shutting down the VM under such unrecoverable
conditions.
BUG=b:250923109
TEST=Run Crostini and trigger a chrome crash; inspect virtio_gpu CPU
usage
Change-Id: I8b2261a093191dfe142697c4c4adc4e9ffab751a
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3975942
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Ryan Neph <ryanneph@google.com>
The `root` and `rwroot` command-line parameters allow to specify a block
device for which the right parameters are passed to the kernel to mount
it as the root filesystem. This approach relies purely on the name of
the parameter given and won't allow us to pass a "root" flag to a
unified block device command-line option.
Address this by adding a "root" member to `DiskOption`. On top of
allowing us to specify the will to mount a particular device as root
through a flag, it also clarifies the code a bit as we deal with one
less unnamed tuple.
BUG=b:218223240
TEST=Guest Linux boots as expected with the `--rwroot` option.
Change-Id: I5d5eda1cc8b1fc2f08e798064fc0db3b17f000a3
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3970363
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Only gfxstream is being actively used on Windows for now, so we
either use it or fall back to 2D rendering by default.
This CL also puts ModeVirglRenderer under the "virglrenderer"
feature flag, so that the arg parser would reject it when specified
by mistake.
BUG=b:254284360
TEST=presubmit
Change-Id: Ifa39e4a528acf1bd45a85e7327b3edded3a4e7d6
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3971026
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Kaiyi Li <kaiyili@google.com>
Commit-Queue: Pujun Lun <lunpujun@google.com>
