Hi Vincent,
Thank you for the patch.
On Monday 03 Oct 2016 13:27:16 Vincent Abriou wrote:
> Allow uvcvideo compatible devices to allocate their output buffers using
> contiguous DMA buffers.
Why do you need this ? If it's for buffer sharing with a device that requires
dma-contig, can't you allocate the buffers on the other device and import them
on the UVC side ?
> Add the "allocators" module parameter option to let uvcvideo use the
> dma-contig instead of vmalloc.
>
> Signed-off-by: Vincent Abriou <vincent.abriou@xxxxxx>
> ---
> Documentation/media/v4l-drivers/uvcvideo.rst | 12 ++++++++++++
> drivers/media/usb/uvc/Kconfig | 2 ++
> drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> drivers/media/usb/uvc/uvc_queue.c | 23 ++++++++++++++++++++---
> drivers/media/usb/uvc/uvcvideo.h | 4 ++--
> 5 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/media/v4l-drivers/uvcvideo.rst
> b/Documentation/media/v4l-drivers/uvcvideo.rst index d68b3d5..786cff5
> 100644
> --- a/Documentation/media/v4l-drivers/uvcvideo.rst
> +++ b/Documentation/media/v4l-drivers/uvcvideo.rst
> @@ -7,6 +7,18 @@ driver-specific ioctls and implementation notes.
> Questions and remarks can be sent to the Linux UVC development mailing list
> at linux-uvc-devel@xxxxxxxxxxxxxxxx.
>
> +Configuring the driver
> +----------------------
> +
> +The driver is configurable using the following module option:
> +
> +- allocators:
> +
> + memory allocator selection, default is 0. It specifies the way buffers
> + will be allocated.
> +
> + - 0: vmalloc
> + - 1: dma-contig
>
> Extension Unit (XU) support
> ---------------------------
> diff --git a/drivers/media/usb/uvc/Kconfig b/drivers/media/usb/uvc/Kconfig
> index 6ed85efa..71e4d7e 100644
> --- a/drivers/media/usb/uvc/Kconfig
> +++ b/drivers/media/usb/uvc/Kconfig
> @@ -1,7 +1,9 @@
> config USB_VIDEO_CLASS
> tristate "USB Video Class (UVC)"
> depends on VIDEO_V4L2
> + depends on HAS_DMA
This will prevent using the uvcvideo driver on platforms that don't set
HAS_DMA, which would be a regression compared to the current situation.
> select VIDEOBUF2_VMALLOC
> + select VIDEOBUF2_DMA_CONTIG
Shouldn't you make this configurable ? I don't think we want to hardcode the
dependency on VIDEOBUF2_DMA_CONTIG, it should be possible to compile a kernel
with USB_VIDEO_CLASS and without VIDEOBUF2_DMA_CONTIG when the user isn't
interested in the dma-contig allocator.
> ---help---
> Support for the USB Video Class (UVC). Currently only video
> input devices, such as webcams, are supported.
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 302e284..1c20aa0 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1757,7 +1757,8 @@ static int uvc_register_video(struct uvc_device *dev,
> int ret;
>
> /* Initialize the video buffers queue. */
> - ret = uvc_queue_init(&stream->queue, stream->type,
!uvc_no_drop_param);
> + ret = uvc_queue_init(dev, &stream->queue, stream->type,
> + !uvc_no_drop_param);
> if (ret)
> return ret;
>
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index 77edd20..5eab146 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -22,6 +22,7 @@
> #include <linux/wait.h>
> #include <media/videobuf2-v4l2.h>
> #include <media/videobuf2-vmalloc.h>
> +#include <media/videobuf2-dma-contig.h>
Alphabetically sorted please.
> #include "uvcvideo.h"
>
> @@ -37,6 +38,12 @@
> * the driver.
> */
>
> +static unsigned int allocators;
> +module_param(allocators, uint, 0444);
> +MODULE_PARM_DESC(allocators, " memory allocator selection, default is 0.\n"
> + "\t\t 0 == vmalloc\n"
> + "\t\t 1 == dma-contig");
> +
> static inline struct uvc_streaming *
> uvc_queue_to_stream(struct uvc_video_queue *queue)
> {
> @@ -188,20 +195,30 @@ static const struct vb2_ops uvc_queue_qops = {
> .stop_streaming = uvc_stop_streaming,
> };
>
> -int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
> - int drop_corrupted)
> +int uvc_queue_init(struct uvc_device *dev, struct uvc_video_queue *queue,
You don't need the new argument, you can call uvc_queue_to_stream() to get the
struct uvc_streaming pointer for the queue, from which you can retrieve the
device pointer you're interested in.
> + enum v4l2_buf_type type, int drop_corrupted)
> {
> int ret;
> + static const struct vb2_mem_ops * const uvc_mem_ops[2] = {
> + &vb2_vmalloc_memops,
> + &vb2_dma_contig_memops,
> + };
> +
> + if (allocators == 1)
Please define macros instead of hardcoding values.
> + dma_coerce_mask_and_coherent(dev->vdev.dev, DMA_BIT_MASK(32));
This is completely artificial, why 32 bits and not 24 or 64 ?
> + else if (allocators >= ARRAY_SIZE(uvc_mem_ops))
> + allocators = 0;
>
> queue->queue.type = type;
> queue->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> queue->queue.drv_priv = queue;
> queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
> queue->queue.ops = &uvc_queue_qops;
> - queue->queue.mem_ops = &vb2_vmalloc_memops;
> + queue->queue.mem_ops = uvc_mem_ops[allocators];
> queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> | V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
> queue->queue.lock = &queue->mutex;
> + queue->queue.dev = dev->vdev.dev;
You should use the physical device here, not the device corresponding to the
device node.
> ret = vb2_queue_init(&queue->queue);
> if (ret)
> return ret;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..330ba64 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -632,8 +632,8 @@ extern struct uvc_driver uvc_driver;
> extern struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
>
> /* Video buffers queue management. */
> -extern int uvc_queue_init(struct uvc_video_queue *queue,
> - enum v4l2_buf_type type, int drop_corrupted);
> +extern int uvc_queue_init(struct uvc_device *dev, struct uvc_video_queue
> *queue,
> + enum v4l2_buf_type type, int drop_corrupted);
> extern void uvc_queue_release(struct uvc_video_queue *queue);
> extern int uvc_request_buffers(struct uvc_video_queue *queue,
> struct v4l2_requestbuffers *rb);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
