Hi Thomasz,

On 03/22/2012 04:46 PM, Laurent Pinchart wrote:
Hi Tomasz,

Thanks for the patch.

On Tuesday 13 March 2012 11:17:02 Tomasz Stanislawski wrote:
This patch adds extension to V4L2 api. It allow to export a mmap buffer as
file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset
used by mmap and return a file descriptor on success.

I know code is more fun to write than documentation, but
Documentation/DocBook/media/v4l will be sad if this patch is merged as-is ;-)

Signed-off-by: Tomasz Stanislawski<t.stanisl...@samsung.com>
Signed-off-by: Kyungmin Park<kyungmin.p...@samsung.com>
---
  drivers/media/video/v4l2-compat-ioctl32.c |    1 +
  drivers/media/video/v4l2-ioctl.c          |   11 +++++++++++
  include/linux/videodev2.h                 |   20 ++++++++++++++++++++
  include/media/v4l2-ioctl.h                |    2 ++
  4 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c index e6f67aa..fd157cb 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -954,6 +954,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int
cmd, unsigned long arg) case VIDIOC_S_FBUF32:
        case VIDIOC_OVERLAY32:
        case VIDIOC_QBUF32:
+       case VIDIOC_EXPBUF:
        case VIDIOC_DQBUF32:
        case VIDIOC_STREAMON32:
        case VIDIOC_STREAMOFF32:
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c index 74cab51..a125016 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -207,6 +207,7 @@ static const char *v4l2_ioctls[] = {
        [_IOC_NR(VIDIOC_S_FBUF)]           = "VIDIOC_S_FBUF",
        [_IOC_NR(VIDIOC_OVERLAY)]          = "VIDIOC_OVERLAY",
        [_IOC_NR(VIDIOC_QBUF)]             = "VIDIOC_QBUF",
+       [_IOC_NR(VIDIOC_EXPBUF)]           = "VIDIOC_EXPBUF",
        [_IOC_NR(VIDIOC_DQBUF)]            = "VIDIOC_DQBUF",
        [_IOC_NR(VIDIOC_STREAMON)]         = "VIDIOC_STREAMON",
        [_IOC_NR(VIDIOC_STREAMOFF)]        = "VIDIOC_STREAMOFF",
@@ -938,6 +939,16 @@ static long __video_do_ioctl(struct file *file,
                        dbgbuf(cmd, vfd, p);
                break;
        }
+       case VIDIOC_EXPBUF:
+       {
+               struct v4l2_exportbuffer *p = arg;
+
+               if (!ops->vidioc_expbuf)
+                       break;
+
+               ret = ops->vidioc_expbuf(file, fh, p);

You can pass arg to ops->vidioc_expbuf() directly, there's no need to create a
struct v4l2_exportbuffer *p variable.

+               break;
+       }
        case VIDIOC_DQBUF:
        {
                struct v4l2_buffer *p = arg;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index bb6844e..e71c787 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -680,6 +680,25 @@ struct v4l2_buffer {
  #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE     0x0800
  #define V4L2_BUF_FLAG_NO_CACHE_CLEAN          0x1000

+/**
+ * struct v4l2_exportbuffer - export of video buffer as DMABUF file
descriptor
+ *
+ * @fd:                file descriptor associated with DMABUF (set by driver)
+ * @mem_offset:        for non-multiplanar buffers with memory ==
V4L2_MEMORY_MMAP;

I don't think we will ever support exporting anything else than
V4L2_MEMORY_MMAP buffers. What will happen for multiplanar buffers ?

+ *             offset from the start of the device memory for this plane,
+ *             (or a "cookie" that should be passed to mmap() as offset)
+ *

Shouldn't the mem_offset field always be set to the mmap cookie value ? I'm a
bit confused by the "or" part, it seems to have been copied from the
v4l2_buffer documentation directly. We should clarify that.

+ * Contains data used for exporting a video buffer as DMABUF file
+ * descriptor. Uses the same 'cookie' as mmap() syscall. All reserved
fields
+ * must be set to zero.
+ */
+struct v4l2_exportbuffer {
+       __u32           fd;
+       __u32           reserved0;

Why is there a reserved field here ?
+1 to Laurent. Any particular need for reserved0 and reserved[13] below? I think one void user pointer is sufficient even for future need.

+       __u32           mem_offset;
+       __u32           reserved[13];
+};
+
Also, what is the reason for returning the fd through this structure? To keep it aligned with other v4l2 calls? I liked(or now hate making change in the app) how it was being returned through the ioctl in your last patch.

  /*
   *    O V E R L A Y   P R E V I E W
   */
@@ -2303,6 +2322,7 @@ struct v4l2_create_buffers {
  #define VIDIOC_S_FBUF          _IOW('V', 11, struct v4l2_framebuffer)
  #define VIDIOC_OVERLAY                 _IOW('V', 14, int)
  #define VIDIOC_QBUF           _IOWR('V', 15, struct v4l2_buffer)
+#define VIDIOC_EXPBUF          _IOWR('V', 16, struct v4l2_exportbuffer)
  #define VIDIOC_DQBUF          _IOWR('V', 17, struct v4l2_buffer)
  #define VIDIOC_STREAMON                _IOW('V', 18, int)
  #define VIDIOC_STREAMOFF       _IOW('V', 19, int)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 4df031a..d8716c6f 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -120,6 +120,8 @@ struct v4l2_ioctl_ops {
        int (*vidioc_reqbufs) (struct file *file, void *fh, struct
v4l2_requestbuffers *b); int (*vidioc_querybuf)(struct file *file, void
*fh, struct v4l2_buffer *b); int (*vidioc_qbuf)    (struct file *file, void
*fh, struct v4l2_buffer *b);
+       int (*vidioc_expbuf)  (struct file *file, void *fh,
+                               struct v4l2_exportbuffer *e);
        int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer
*b);

        int (*vidioc_create_bufs)(struct file *file, void *fh, struct
v4l2_create_buffers *b);

Regards,
Subash
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to