| Message ID | 20181128083747.18530-1-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| Headers | show |
| Series | vb2 fixes (mostly request API related) | expand |
On Wed, Nov 28, 2018 at 09:37:42AM +0100, hverkuil-cisco@xs4all.nl wrote: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > While improving the v4l2-compliance tests I came across several vb2 > problems. > > After modifying v4l2-compliance I was now able to use the vivid error > injection feature to test what happens if VIDIOC_STREAMON fails and > a following STREAMON succeeds. > > This generated patches 1/5 and 4/5+5/5. > > Patch 1/5 fixes an issue that was never noticed before since it didn't > result in kernel oopses or warnings, but after the second STREAMON it > won't call start_streaming anymore until you call REQBUFS(0) or close > the device node. > > Patches 4 and 5 are request specific fixes for the same corner case: > the code would unbind (in vb2) or complete (in vivid) a request if > start_streaming fails, but it should just leave things as-is. The > buffer is just put back in the queue, ready for the next attempt at > STREAMON. > > Patch 2/5 was also discovered by v4l2-compliance: the request fd in > v4l2_buffer should be ignored by VIDIOC_PREPARE_BUF, but it wasn't. > > Patch 3/5 fixes a nasty corner case: a buffer with associated request > is queued, and then the request fd is closed by v4l2-compliance. > > When the driver calls vb2_buffer_done, the buffer request object is > unbound, the object is put, and indirectly the associated request > is put as well, and since that was the last references to the request > the whole request is released, which requires the ability to call > mutex_lock. But vb2_buffer_done is atomic (it can be called > from interrupt context), so this shouldn't happen. > > vb2 now takes an extra refcount to the request on qbuf and releases > it on dqbuf and at two other places where an internal dqbuf is done. > > Note that 'skip request checks for VIDIOC_PREPARE_BUF' is a duplicate > of https://patchwork.linuxtv.org/patch/53171/, which is now marked as > superseded. > > I've marked all these patches for 4.20, but I think it is also possible > to apply them for 4.21 since the request API is only used by virtual > drivers and a staging driver. For patches 2--5: Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
From: Hans Verkuil <hverkuil-cisco@xs4all.nl> While improving the v4l2-compliance tests I came across several vb2 problems. After modifying v4l2-compliance I was now able to use the vivid error injection feature to test what happens if VIDIOC_STREAMON fails and a following STREAMON succeeds. This generated patches 1/5 and 4/5+5/5. Patch 1/5 fixes an issue that was never noticed before since it didn't result in kernel oopses or warnings, but after the second STREAMON it won't call start_streaming anymore until you call REQBUFS(0) or close the device node. Patches 4 and 5 are request specific fixes for the same corner case: the code would unbind (in vb2) or complete (in vivid) a request if start_streaming fails, but it should just leave things as-is. The buffer is just put back in the queue, ready for the next attempt at STREAMON. Patch 2/5 was also discovered by v4l2-compliance: the request fd in v4l2_buffer should be ignored by VIDIOC_PREPARE_BUF, but it wasn't. Patch 3/5 fixes a nasty corner case: a buffer with associated request is queued, and then the request fd is closed by v4l2-compliance. When the driver calls vb2_buffer_done, the buffer request object is unbound, the object is put, and indirectly the associated request is put as well, and since that was the last references to the request the whole request is released, which requires the ability to call mutex_lock. But vb2_buffer_done is atomic (it can be called from interrupt context), so this shouldn't happen. vb2 now takes an extra refcount to the request on qbuf and releases it on dqbuf and at two other places where an internal dqbuf is done. Note that 'skip request checks for VIDIOC_PREPARE_BUF' is a duplicate of https://patchwork.linuxtv.org/patch/53171/, which is now marked as superseded. I've marked all these patches for 4.20, but I think it is also possible to apply them for 4.21 since the request API is only used by virtual drivers and a staging driver. Regards, Hans Hans Verkuil (5): vb2: don't call __vb2_queue_cancel if vb2_start_streaming failed vb2: skip request checks for VIDIOC_PREPARE_BUF vb2: keep a reference to the request until dqbuf vb2: don't unbind/put the object when going to state QUEUED vivid: drop v4l2_ctrl_request_complete() from start_streaming .../media/common/videobuf2/videobuf2-core.c | 44 +++++++++++++++---- .../media/common/videobuf2/videobuf2-v4l2.c | 11 +++-- drivers/media/platform/vivid/vivid-sdr-cap.c | 2 - drivers/media/platform/vivid/vivid-vbi-cap.c | 2 - drivers/media/platform/vivid/vivid-vbi-out.c | 2 - drivers/media/platform/vivid/vivid-vid-cap.c | 2 - drivers/media/platform/vivid/vivid-vid-out.c | 2 - include/media/videobuf2-core.h | 2 + 8 files changed, 44 insertions(+), 23 deletions(-)