Closed mzihlmann closed 1 year ago
mzihlmann
commented
1 year ago not sure, shouldn't all calls that meddle with the queue be locked?
buffer_written, called from vidioc_qbuf (enqueue), uses spin_lock_bh, as i understand this disables interrupts and acquires the mutex lock, s.t. it is safe to use in an interrupt context.
vidioc_dqbuf (dequeue) directly modifies the list without bothering for mutex lock.
is that intended behaviour?
mzihlmann
commented
1 year ago hmmm digging into history it looks like the lock was never intended to lock the list, instead it was intended to lock the read stats if multiple consumers are present. hence its only applied to the reading. there is no protection of the buffer list in the design, but linux's list is not threadsafe. that sounds like a possible root-cause for the behaviour above?
i'm leaning a bit out of a window here. after i applied a mutex lock to the dequeueing as well, so far i have not yet seen kernel panic :crossed_fingers: . but to positively test it i would need to fix all possible concurrency issues.... :grimacing:
mzihlmann
commented
1 year ago just fyi:
this is the change i applied here
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
+ spin_lock_bh(&dev->lock);
b = list_entry(dev->outbufs_list.prev, struct v4l2l_buffer,
list_head);
list_move_tail(&b->list_head, &dev->outbufs_list);
+ spin_unlock_bh(&dev->lock);
dprintkrw("output DQBUF index: %d\n", b->buffer.index);
unset_flags(b);
*buf = b->buffer;
buf->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;but i think we should maybe spend a second mutex lock to clearly distinguish locking the stats and locking the list.
mzihlmann
commented
1 year ago apparently this fixes the kernel panic by simply freezing the output :facepalm: ill try further to get it working properly
mzihlmann
commented
1 year ago nope, turns out this fix should be fine, output is down to a single frame not because of this change, but because of some change in v0.12.7...HEAD. i did not further investigate yet which change causes this other issue, instead i rebased the fix proposed above onto v0.12.7 instead. This now works fine for me.
umlaeute
commented
1 year ago thanks for investigating.
i agree that re-using the stats mutex just because it's already there, is not a good idea though...
umlaeute
commented
1 year ago fixed with d9277249064a04cee6e3784573cf36162e09bf3e & 6b9ac6ca106eed2ee01fb2b66960c1ea10a10f40
Environment
v4l2loopbackversion:0.12.7-321-gfb410fcLinux ci5 5.10.0-21-amd64 #1 SMP Debian 5.10.162-1 (2023-01-21) x86_64 GNU/LinuxDebian GNU/Linux 11 (bullseye)note: we tried several different versions, debian package, 0.12.7 release and the above commit.
Step 3: Describe the problem:
I have not yet narrowed down to an easily reproducible setup, but i can explain what we try to do. We have a camera based android app that we try to end-to-end test. For that purpose we run an android emulator and use
v4l2loopbackandgstreamerto make the emulator recognize/dev/video0as camera input. Then we use pytest and selenium to feed an image into the loopback device and check whether the app correctly reacts on the application level.These end-to-end tests run in our CI environment. We use gitlab-runner on top of kubernetes and funnel the devices through to the containers using generic-device-plugin. This way each container sees a device
/dev/video0, but they are mapped onto different virtual devices in the underlying host system. The driver is installed on the host system directly.Now the problem is that from time-to-time (over weeks) we got a kernel panic from the driver resulting in the host server to reboot. I have now a setup where i can reproduce this in roughly 20mins intervals but I'm not sure how to proceed to get more useful information out.
Observed Results:
kernel panic on the underlying system, causing reboot of the kubernetes host.
Expected Results:
no kernel panic
Relevant Code:
ill try to narrow it down further, currently its a quite large project with a lot of moving parts (its an e2e test after all :wink:). in case i can create a minimal setup ill post it here.