RE: [PATCH v4 1/2] v4l: Add memory-to-memory device helper framework for videobuf.

2010-04-21 Thread Hiremath, Vaibhav


 -Original Message-
 From: Pawel Osciak [mailto:p.osc...@samsung.com]
 Sent: Monday, April 19, 2010 6:00 PM
 To: linux-media@vger.kernel.org
 Cc: p.osc...@samsung.com; m.szyprow...@samsung.com;
 kyungmin.p...@samsung.com; Hiremath, Vaibhav
 Subject: [PATCH v4 1/2] v4l: Add memory-to-memory device helper framework
 for videobuf.

 A mem-to-mem device is a device that uses memory buffers passed by
 userspace applications for both their source and destination data. This
 is different from existing drivers, which utilize memory buffers for either
 input or output, but not both.

 In terms of V4L2 such a device would be both of OUTPUT and CAPTURE type.

 Examples of such devices would be: image 'resizers', 'rotators',
 'colorspace converters', etc.

 This patch adds a separate Kconfig sub-menu for mem-to-mem devices as well.
[Hiremath, Vaibhav] Some minor comments (which just came across now) -


 Signed-off-by: Pawel Osciak p.osc...@samsung.com
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 Reviewed-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/media/video/Kconfig|   14 +
  drivers/media/video/Makefile   |2 +
  drivers/media/video/v4l2-mem2mem.c |  632
 
  include/media/v4l2-mem2mem.h   |  201 
  4 files changed, 849 insertions(+), 0 deletions(-)
  create mode 100644 drivers/media/video/v4l2-mem2mem.c
  create mode 100644 include/media/v4l2-mem2mem.h

 diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
 index f8fc865..5fd041e 100644
 --- a/drivers/media/video/Kconfig
 +++ b/drivers/media/video/Kconfig
 @@ -45,6 +45,10 @@ config VIDEO_TUNER
   tristate
   depends on MEDIA_TUNER

 +config V4L2_MEM2MEM_DEV
 + tristate
 + depends on VIDEOBUF_GEN
 +
  #
  # Multimedia Video device configuration
  #
 @@ -1107,3 +,13 @@ config USB_S2255

  endif # V4L_USB_DRIVERS
  endif # VIDEO_CAPTURE_DRIVERS
 +
 +menuconfig V4L_MEM2MEM_DRIVERS
 + bool Memory-to-memory multimedia devices
 + depends on VIDEO_V4L2
 + default n
 + ---help---
 +   Say Y here to enable selecting drivers for V4L devices that
 +   use system memory for both source and destination buffers, as
 opposed
 +   to capture and output drivers, which use memory buffers for just
 +   one of those.
 diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
 index b88b617..e974680 100644
 --- a/drivers/media/video/Makefile
 +++ b/drivers/media/video/Makefile
 @@ -117,6 +117,8 @@ obj-$(CONFIG_VIDEOBUF_VMALLOC) += videobuf-vmalloc.o
  obj-$(CONFIG_VIDEOBUF_DVB) += videobuf-dvb.o
  obj-$(CONFIG_VIDEO_BTCX)  += btcx-risc.o

 +obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
 +
  obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o

  obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
 diff --git a/drivers/media/video/v4l2-mem2mem.c b/drivers/media/video/v4l2-
 mem2mem.c
 new file mode 100644
 index 000..eee9514
 --- /dev/null
 +++ b/drivers/media/video/v4l2-mem2mem.c
 @@ -0,0 +1,632 @@
 +/*
 + * Memory-to-memory device framework for Video for Linux 2 and videobuf.
 + *
 + * Helper functions for devices that use videobuf buffers for both their
 + * source and destination.
 + *
 + * Copyright (c) 2009-2010 Samsung Electronics Co., Ltd.
 + * Pawel Osciak, p.osc...@samsung.com
 + * Marek Szyprowski, m.szyprow...@samsung.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by the
 + * Free Software Foundation; either version 2 of the License, or (at your
 + * option) any later version.
 + */
 +#include linux/module.h
 +#include linux/sched.h
 +#include linux/slab.h
[Hiremath, Vaibhav] Add one line here.

 +#include media/videobuf-core.h
 +#include media/v4l2-mem2mem.h
 +
 +MODULE_DESCRIPTION(Mem to mem device framework for videobuf);
 +MODULE_AUTHOR(Pawel Osciak, p.osc...@samsung.com);
 +MODULE_LICENSE(GPL);
 +
 +static bool debug;
 +module_param(debug, bool, 0644);
 +
 +#define dprintk(fmt, arg...) \
 + do {\
 + if (debug)  \
 + printk(KERN_DEBUG %s:  fmt, __func__, ## arg);\
 + } while (0)
 +
 +
 +/* Instance is already queued on the job_queue */
 +#define TRANS_QUEUED (1  0)
 +/* Instance is currently running in hardware */
 +#define TRANS_RUNNING(1  1)
 +
 +
 +/* Offset base for buffers on the destination queue - used to distinguish
 + * between source and destination buffers when mmapping - they receive the
 same
 + * offsets but for different queues */
 +#define DST_QUEUE_OFF_BASE   (1  30)
 +
 +
 +/**
 + * struct v4l2_m2m_dev - per-device context
 + * @curr_ctx:currently running instance
 + * @job_queue:   instances queued to run
 + * @job_spinlock:protects 

RE: [PATCH v4 1/2] v4l: Add memory-to-memory device helper framework for videobuf.

2010-04-21 Thread Pawel Osciak
Hi,

thanks for the review. My responses below.


 Hiremath, Vaibhav hvaib...@ti.com wrote:

 -Original Message-
 From: Pawel Osciak [mailto:p.osc...@samsung.com]
 Sent: Monday, April 19, 2010 6:00 PM
 To: linux-media@vger.kernel.org
 Cc: p.osc...@samsung.com; m.szyprow...@samsung.com;
 kyungmin.p...@samsung.com; Hiremath, Vaibhav
 Subject: [PATCH v4 1/2] v4l: Add memory-to-memory device helper framework
 for videobuf.


[Hiremath, Vaibhav] Add one line here.


Ok...

[snip]

 +/**
 + * v4l2_m2m_querybuf() - multi-queue-aware QUERYBUF multiplexer
 + *
 + * See v4l2_m2m_mmap() documentation for details.
 + */
 +int v4l2_m2m_querybuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 +   struct v4l2_buffer *buf)
 +{
 + struct videobuf_queue *vq;
 + int ret;
 +
 + vq = v4l2_m2m_get_vq(m2m_ctx, buf-type);
 + ret = videobuf_querybuf(vq, buf);
 +
 + if (buf-memory == V4L2_MEMORY_MMAP
 +  vq-type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 + buf-m.offset += DST_QUEUE_OFF_BASE;
 + }
[Hiremath, Vaibhav] Don't you think we should check for ret value also here? 
Should it be something -

if (!ret  buf-memory == V4L2_MEMORY_MMAP
 vq-type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
buf-m.offset += DST_QUEUE_OFF_BASE;
}


I think it should stay like this. The offset should never be different
depending on whether an error is being reported or not. The unmodified offset
could confuse userspace applications or even conflict with the other buffer
type (although in case of errors userspace should not be using those offsets
anyway).

[snip]

 +/**
 + * v4l2_m2m_dqbuf() - dequeue a source or destination buffer, depending on
 + * the type
 + */
 +int v4l2_m2m_dqbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 +struct v4l2_buffer *buf)
 +{
 + struct videobuf_queue *vq;
 +
 + vq = v4l2_m2m_get_vq(m2m_ctx, buf-type);


[Hiremath, Vaibhav] Does it make sense to check the return value here?


Well, videobuf does not check it either. I mean, it would be important if
there was a possibility that userspace passes malicious data. But a NULL
here would mean a driver error.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland RD Center


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html