Hi Hugues,
On Tue, Mar 05, 2019 at 10:24:15AM +0000, Hugues FRUCHET wrote:
> On 3/4/19 4:44 PM, Laurent Pinchart wrote:
> > On Mon, Mar 04, 2019 at 03:36:32PM +0000, Kieran Bingham wrote:
> >> On 04/03/2019 12:35, Hugues Fruchet wrote:
> >>> Add support of read() call from userspace by implementing
> >>> uvc_v4l2_read() with vb2_read() helper.
> >>
> >> Just thinking out loud,
> >>
> >> This opens up UVC devices to read raw full frame images through this
> >> interface as well.
> >>
> >> Due to the UVC protocol, there is /already/ a full memcpy to get these
> >> images out of the URB packets, so using a read() interface would be
> >> another full frame copy.
> >>
> >> I can perhaps see the usecase for reading compressed data through this
> >> interface - but full frames don't seem appropriate. (not impossible of
> >> course, just is it reasonable?)
> >>
> >> If this is to be enabled, should it be enabled for compressed formats
> >> only? or would that complicate matters?
> >
> > I've repeatedly refused read() support in uvcvideo for this reason, and
> > also because read() doesn't carry framing information very well. It's
> > just not a good API for capturing video frames from a webcam, and so far
> > I haven't heard a compeling reason why it should be enabled. I thus
> > haven't changed my mind :-)
>
> For sure read() is not optimal, but is very common and pretty simple to
> use from userspace as explained in the cover letter with some examples.
>
> Moreover, read() is enabled by many (all ?) camera interfaces:
>
> drivers/media/platform/pxa_camera.c: .read = vb2_fop_read,
> drivers/media/platform/atmel/atmel-isi.c: .read = vb2_fop_read,
> drivers/media/platform/rcar-vin/rcar-v4l2.c: .read = vb2_fop_read,
> drivers/media/platform/stm32/stm32-dcmi.c: .read = vb2_fop_read,
> drivers/media/platform/vivid/vivid-core.c: .read = vb2_fop_read,
> drivers/media/platform/vimc/vimc-capture.c: .read = vb2_fop_read,
> drivers/media/platform/marvell-ccic/mcam-core.c:.read = vb2_fop_read,
> drivers/media/platform/qcom/camss/camss-video.c:.read = vb2_fop_read,
Cargo-cult doesn't mean we have to repear the same mistakes everywhere
:-)
> on my setup, this leads to have /dev/video0 (mapped on DCMI camera
> interface) supporting JPEG streaming through read() while /dev/video1
> (mapped on Hercule USB camera) fails with "invalid argument" error...
Let's not encourage applications to use inefficient interfaces, when
using streaming I/O wouldn't be that more difficult. I'd argue that
supporting read() in the above drivers is part of the problem, not of
the solution.
> >>> Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxx>
> >>> ---
> >>> drivers/media/usb/uvc/uvc_queue.c | 15 ++++++++++++++-
> >>> drivers/media/usb/uvc/uvc_v4l2.c | 11 ++++++++---
> >>> drivers/media/usb/uvc/uvcvideo.h | 2 ++
> >>> 3 files changed, 24 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> >>> index 682698e..0c8a0a8 100644
> >>> --- a/drivers/media/usb/uvc/uvc_queue.c
> >>> +++ b/drivers/media/usb/uvc/uvc_queue.c
> >>> @@ -227,7 +227,7 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> >>> int ret;
> >>>
> >>> queue->queue.type = type;
> >>> - queue->queue.io_modes = VB2_MMAP | VB2_USERPTR;
> >>> + queue->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ;
> >>> queue->queue.drv_priv = queue;
> >>> queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
> >>> queue->queue.mem_ops = &vb2_vmalloc_memops;
> >>> @@ -361,6 +361,19 @@ int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type)
> >>> return ret;
> >>> }
> >>>
> >>> +ssize_t uvc_queue_read(struct uvc_video_queue *queue, struct file *file,
> >>> + char __user *buf, size_t count, loff_t *ppos)
> >>> +{
> >>> + ssize_t ret;
> >>> +
> >>> + mutex_lock(&queue->mutex);
> >>> + ret = vb2_read(&queue->queue, buf, count, ppos,
> >>> + file->f_flags & O_NONBLOCK);
> >>> + mutex_unlock(&queue->mutex);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> int uvc_queue_mmap(struct uvc_video_queue *queue, struct vm_area_struct *vma)
> >>> {
> >>> return vb2_mmap(&queue->queue, vma);
> >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> index 84be596..3866832 100644
> >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> @@ -594,7 +594,8 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
> >>> strscpy(cap->driver, "uvcvideo", sizeof(cap->driver));
> >>> strscpy(cap->card, vdev->name, sizeof(cap->card));
> >>> usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
> >>> - cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
> >>> + cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING |
> >>> + V4L2_CAP_READWRITE
> >>> | chain->caps;
> >>>
> >>> return 0;
> >>> @@ -1434,8 +1435,12 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> >>> static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> >>> size_t count, loff_t *ppos)
> >>> {
> >>> - uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_read: not implemented.\n");
> >>> - return -EINVAL;
> >>> + struct uvc_fh *handle = file->private_data;
> >>> + struct uvc_streaming *stream = handle->stream;
> >>> +
> >>> + uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_read\n");
> >>> +
> >>> + return uvc_queue_read(&stream->queue, file, data, count, ppos);
> >>> }
> >>>
> >>> static int uvc_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
> >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >>> index c7c1baa..5d0515c 100644
> >>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>> @@ -766,6 +766,8 @@ struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
> >>> struct uvc_buffer *buf);
> >>> struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
> >>> void uvc_queue_buffer_release(struct uvc_buffer *buf);
> >>> +ssize_t uvc_queue_read(struct uvc_video_queue *queue, struct file *file,
> >>> + char __user *buf, size_t count, loff_t *ppos);
> >>> int uvc_queue_mmap(struct uvc_video_queue *queue,
> >>> struct vm_area_struct *vma);
> >>> __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
--
Regards,
Laurent Pinchart
