[0/3] media: uvcvideo: Prepare deprecation of nodrop

Message

Ricardo Ribalda Dec. 17, 2024, 9:06 p.m. UTC

We intend to deprecate the nodrop parameter in the future and adopt the
default behaviour of the other media drivers: drop invalid packages.

Make the first step in the deprecation by changing the default value of
the parameter and printing an error message when the value is changed.

Signed-off-by: Ricardo Ribalda <[e-mail]>
---
Ricardo Ribalda (3):
      media: uvcvideo: Swap default value for nodrop module param
      media: uvcvideo: Allow changing noparam on the fly
      media: uvcvideo: Announce the user our deprecation intentions

 drivers/media/usb/uvc/uvc_driver.c | 23 ++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_queue.c  |  6 ++----
 drivers/media/usb/uvc/uvcvideo.h   |  4 +---
 3 files changed, 23 insertions(+), 10 deletions(-)
---
base-commit: d216d9cb4dd854ef0a2ec1701f403facb298af51
change-id: 20241217-uvc-deprecate-fbd6228fa1e2

Best regards,

Comments

Hans de Goede Dec. 18, 2024, 8:37 p.m. UTC | #1

Hi Ricardo,

Thank you for working on this.

On 17-Dec-24 10:06 PM, Ricardo Ribalda wrote:
> We intend to deprecate the nodrop parameter in the future and adopt the
> default behaviour of the other media drivers: drop invalid packages.

Actually the default behaviour of other media drivers is:

"return buffers with an error to userspace with V4L2_BUF_FLAG_ERROR set
in v4l2_buffer.flags".

It is not "drop invalid packages". The commit messages of patch 1/3
has some related unclear wording, please fix this.

Looking at this I actually have found what arguably is a bug in
the UVC driver when nodrop is set, or at least something which
we must change before making nodrop=1 the default.

Currently uvc_queue_buffer_complete() looks like this:

static void uvc_queue_buffer_complete(struct kref *ref)
{
        struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
        struct vb2_buffer *vb = &buf->buf.vb2_buf;
        struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);

        if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
                uvc_queue_buffer_requeue(queue, buf);
                return;
        }

        buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
        vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
        vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
}

Notice how the last line does not propagate buf->error to the
videobuf2 code, so when nodrop=1 is set then buffers with errors
are not only returned to userspace, they are returned to userspace
without V4L2_BUF_FLAG_ERROR getting set in v4l2_buffer.flags .

The right thing to do in this case is to set V4L2_BUF_FLAG_ERROR
IOW the last line of uvc_queue_buffer_complete() should be changed to:

        vb2_buffer_done(&buf->buf.vb2_buf, buf->error ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);

And this should probably be the first patch in a v2 series for this.

Regards,

Hans