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

[Hans de Goede <hdegoede@redhat.com>]

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