subject:"\[PATCHv8 01\/26\] v4l\: Add DMABUF as a memory type"

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

2012-08-22 Thread Hans Verkuil

On Wed August 22 2012 14:09:18 Tomasz Stanislawski wrote:
> Hi Hans,
> Thank your for the review.
> Please refer to the comments below.
> 
> On 08/22/2012 12:27 PM, Hans Verkuil wrote:
> > On Tue August 14 2012 17:34:31 Tomasz Stanislawski wrote:
> >> From: Sumit Semwal 
> >>
> >> Adds DMABUF memory type to v4l framework. Also adds the related file
> >> descriptor in v4l2_plane and v4l2_buffer.
> >>
> >> Signed-off-by: Tomasz Stanislawski 
> >>[original work in the PoC for buffer sharing]
> >> Signed-off-by: Sumit Semwal 
> >> Signed-off-by: Sumit Semwal 
> >> Acked-by: Laurent Pinchart 
> >> ---
> >>  drivers/media/video/v4l2-compat-ioctl32.c |   18 ++
> >>  drivers/media/video/v4l2-ioctl.c  |1 +
> >>  include/linux/videodev2.h |7 +++
> >>  3 files changed, 26 insertions(+)
> >>
> >> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c 
> >> b/drivers/media/video/v4l2-compat-ioctl32.c
> >> index 9ebd5c5..a2e0549 100644
> >> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> >> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> >> @@ -304,6 +304,7 @@ struct v4l2_plane32 {
> >>union {
> >>__u32   mem_offset;
> >>compat_long_t   userptr;
> >> +  __u32   fd;
> > 
> > Shouldn't this be int?
> > 
> 
> Notice that this field should be consistent with fd field used in
> 'struct v4l2_exportbuffer'. Therefore I prefer to use fixed-size types.
> One could use __s32 here but notice that file descriptors are defined
> as small, nonnegative integers according to POSIX spec. The type __u32
> suits well for this purpose. The negative values returned by open
> syscall are used only to indicate failures.
> 
> On the other hand, using __s32 may help to avoid compiler warning while
> building userspace apps due to 'signed-vs-unsigned comparisons'.
> 
> However, I do not have any strong opinion about 'int vs __u32' issue :).
> Do you think that using __s32 for both QUERYBUF and EXPBUF is a good
> compromise?

The type of a fd is highly variable in the kernel. Just try this for fun:

grep [^a-z]fd[^a-z] -rsI include/linux/

'int' or 'unsigned int' are by far the most common types.

But in structs I did see a few __s32 types, so I think __s32 is a decent
type to use. Just make sure it is __s32 everywhere. Right now __u32 and
int are both used.

Regards,

Hans

> 
> >>} m;
> >>__u32   data_offset;
> >>__u32   reserved[11];
> >> @@ -325,6 +326,7 @@ struct v4l2_buffer32 {
> >>__u32   offset;
> >>compat_long_t   userptr;
> >>compat_caddr_t  planes;
> >> +  __u32   fd;
> > 
> > Ditto.
> > 
> >>} m;
> >>__u32   length;
> >>__u32   reserved2;
> 
> > Regards,
> > 
> > Hans
> > 
> 
> Regards,
> 
>   Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

2012-08-22 Thread Tomasz Stanislawski

Hi Hans,
Thank your for the review.
Please refer to the comments below.

On 08/22/2012 12:27 PM, Hans Verkuil wrote:
> On Tue August 14 2012 17:34:31 Tomasz Stanislawski wrote:
>> From: Sumit Semwal 
>>
>> Adds DMABUF memory type to v4l framework. Also adds the related file
>> descriptor in v4l2_plane and v4l2_buffer.
>>
>> Signed-off-by: Tomasz Stanislawski 
>>[original work in the PoC for buffer sharing]
>> Signed-off-by: Sumit Semwal 
>> Signed-off-by: Sumit Semwal 
>> Acked-by: Laurent Pinchart 
>> ---
>>  drivers/media/video/v4l2-compat-ioctl32.c |   18 ++
>>  drivers/media/video/v4l2-ioctl.c  |1 +
>>  include/linux/videodev2.h |7 +++
>>  3 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c 
>> b/drivers/media/video/v4l2-compat-ioctl32.c
>> index 9ebd5c5..a2e0549 100644
>> --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> @@ -304,6 +304,7 @@ struct v4l2_plane32 {
>>  union {
>>  __u32   mem_offset;
>>  compat_long_t   userptr;
>> +__u32   fd;
> 
> Shouldn't this be int?
> 

Notice that this field should be consistent with fd field used in
'struct v4l2_exportbuffer'. Therefore I prefer to use fixed-size types.
One could use __s32 here but notice that file descriptors are defined
as small, nonnegative integers according to POSIX spec. The type __u32
suits well for this purpose. The negative values returned by open
syscall are used only to indicate failures.

On the other hand, using __s32 may help to avoid compiler warning while
building userspace apps due to 'signed-vs-unsigned comparisons'.

However, I do not have any strong opinion about 'int vs __u32' issue :).
Do you think that using __s32 for both QUERYBUF and EXPBUF is a good
compromise?

>>  } m;
>>  __u32   data_offset;
>>  __u32   reserved[11];
>> @@ -325,6 +326,7 @@ struct v4l2_buffer32 {
>>  __u32   offset;
>>  compat_long_t   userptr;
>>  compat_caddr_t  planes;
>> +__u32   fd;
> 
> Ditto.
> 
>>  } m;
>>  __u32   length;
>>  __u32   reserved2;

> Regards,
> 
>   Hans
> 

Regards,

Tomasz

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

2012-08-22 Thread Hans Verkuil

On Tue August 14 2012 17:34:31 Tomasz Stanislawski wrote:
> From: Sumit Semwal 
> 
> Adds DMABUF memory type to v4l framework. Also adds the related file
> descriptor in v4l2_plane and v4l2_buffer.
> 
> Signed-off-by: Tomasz Stanislawski 
>[original work in the PoC for buffer sharing]
> Signed-off-by: Sumit Semwal 
> Signed-off-by: Sumit Semwal 
> Acked-by: Laurent Pinchart 
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |   18 ++
>  drivers/media/video/v4l2-ioctl.c  |1 +
>  include/linux/videodev2.h |7 +++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c 
> b/drivers/media/video/v4l2-compat-ioctl32.c
> index 9ebd5c5..a2e0549 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -304,6 +304,7 @@ struct v4l2_plane32 {
>   union {
>   __u32   mem_offset;
>   compat_long_t   userptr;
> + __u32   fd;

Shouldn't this be int?

>   } m;
>   __u32   data_offset;
>   __u32   reserved[11];
> @@ -325,6 +326,7 @@ struct v4l2_buffer32 {
>   __u32   offset;
>   compat_long_t   userptr;
>   compat_caddr_t  planes;
> + __u32   fd;

Ditto.

>   } m;
>   __u32   length;
>   __u32   reserved2;
> @@ -348,6 +350,9 @@ static int get_v4l2_plane32(struct v4l2_plane *up, struct 
> v4l2_plane32 *up32,
>   up_pln = compat_ptr(p);
>   if (put_user((unsigned long)up_pln, >m.userptr))
>   return -EFAULT;
> + } else if (memory == V4L2_MEMORY_DMABUF) {
> + if (copy_in_user(>m.fd, >m.fd, sizeof(int)))
> + return -EFAULT;
>   } else {
>   if (copy_in_user(>m.mem_offset, >m.mem_offset,
>   sizeof(__u32)))
> @@ -371,6 +376,11 @@ static int put_v4l2_plane32(struct v4l2_plane *up, 
> struct v4l2_plane32 *up32,
>   if (copy_in_user(>m.mem_offset, >m.mem_offset,
>   sizeof(__u32)))
>   return -EFAULT;
> + /* For DMABUF, driver might've set up the fd, so copy it back. */
> + if (memory == V4L2_MEMORY_DMABUF)
> + if (copy_in_user(>m.fd, >m.fd,
> + sizeof(int)))
> + return -EFAULT;
>  
>   return 0;
>  }
> @@ -453,6 +463,10 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, 
> struct v4l2_buffer32 __user
>   if (get_user(kp->m.offset, >m.offset))
>   return -EFAULT;
>   break;
> + case V4L2_MEMORY_DMABUF:
> + if (get_user(kp->m.fd, >m.fd))
> + return -EFAULT;
> + break;
>   }
>   }
>  
> @@ -517,6 +531,10 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, 
> struct v4l2_buffer32 __user
>   if (put_user(kp->m.offset, >m.offset))
>   return -EFAULT;
>   break;
> + case V4L2_MEMORY_DMABUF:
> + if (put_user(kp->m.fd, >m.fd))
> + return -EFAULT;
> + break;
>   }
>   }
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c 
> b/drivers/media/video/v4l2-ioctl.c
> index 6bc47fc..dffd3c9 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -155,6 +155,7 @@ static const char *v4l2_memory_names[] = {
>   [V4L2_MEMORY_MMAP]= "mmap",
>   [V4L2_MEMORY_USERPTR] = "userptr",
>   [V4L2_MEMORY_OVERLAY] = "overlay",
> + [V4L2_MEMORY_DMABUF] = "dmabuf",
>  };
>  
>  #define prt_names(a, arr) a) >= 0) && ((a) < ARRAY_SIZE(arr))) ? \
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 7a147c8..7f918dc 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -186,6 +186,7 @@ enum v4l2_memory {
>   V4L2_MEMORY_MMAP = 1,
>   V4L2_MEMORY_USERPTR  = 2,
>   V4L2_MEMORY_OVERLAY  = 3,
> + V4L2_MEMORY_DMABUF   = 4,
>  };
>  
>  /* see also http://vektor.theorem.ca/graphics/ycbcr/ */
> @@ -596,6 +597,8 @@ struct v4l2_requestbuffers {
>   *   should be passed to mmap() called on the video node)
>   * @userptr: when memory is V4L2_MEMORY_USERPTR, a userspace pointer
>   *   pointing to this plane
> + * @fd:  when memory is V4L2_MEMORY_DMABUF, a userspace 
> file
> + *   descriptor associated with this plane
>   * @data_offset: offset in the plane to the start of data; usually 0,
>   *   unless there is a header in front of the data
>   *
> @@ -610,6

Re: [PATCHv8 01/26] v4l: Add DMABUF as a memory type

2012-08-22 Thread Tomasz Stanislawski

Hi Hans,
Thank your for the review.
Please refer to the comments below.

On 08/22/2012 12:27 PM, Hans Verkuil wrote:
 On Tue August 14 2012 17:34:31 Tomasz Stanislawski wrote:
 From: Sumit Semwal sumit.sem...@ti.com

 Adds DMABUF memory type to v4l framework. Also adds the related file
 descriptor in v4l2_plane and v4l2_buffer.

 Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
[original work in the PoC for buffer sharing]
 Signed-off-by: Sumit Semwal sumit.sem...@ti.com
 Signed-off-by: Sumit Semwal sumit.sem...@linaro.org
 Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
 ---
  drivers/media/video/v4l2-compat-ioctl32.c |   18 ++
  drivers/media/video/v4l2-ioctl.c  |1 +
  include/linux/videodev2.h |7 +++
  3 files changed, 26 insertions(+)

 diff --git a/drivers/media/video/v4l2-compat-ioctl32.c 
 b/drivers/media/video/v4l2-compat-ioctl32.c
 index 9ebd5c5..a2e0549 100644
 --- a/drivers/media/video/v4l2-compat-ioctl32.c
 +++ b/drivers/media/video/v4l2-compat-ioctl32.c
 @@ -304,6 +304,7 @@ struct v4l2_plane32 {
  union {
  __u32   mem_offset;
  compat_long_t   userptr;
 +__u32   fd;
 
 Shouldn't this be int?
 

Notice that this field should be consistent with fd field used in
'struct v4l2_exportbuffer'. Therefore I prefer to use fixed-size types.
One could use __s32 here but notice that file descriptors are defined
as small, nonnegative integers according to POSIX spec. The type __u32
suits well for this purpose. The negative values returned by open
syscall are used only to indicate failures.

On the other hand, using __s32 may help to avoid compiler warning while
building userspace apps due to 'signed-vs-unsigned comparisons'.

However, I do not have any strong opinion about 'int vs __u32' issue :).
Do you think that using __s32 for both QUERYBUF and EXPBUF is a good
compromise?

  } m;
  __u32   data_offset;
  __u32   reserved[11];
 @@ -325,6 +326,7 @@ struct v4l2_buffer32 {
  __u32   offset;
  compat_long_t   userptr;
  compat_caddr_t  planes;
 +__u32   fd;
 
 Ditto.
 
  } m;
  __u32   length;
  __u32   reserved2;

 Regards,
 
   Hans
 

Regards,

Tomasz
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

2012-08-14 Thread Tomasz Stanislawski

From: Sumit Semwal 

Adds DMABUF memory type to v4l framework. Also adds the related file
descriptor in v4l2_plane and v4l2_buffer.

Signed-off-by: Tomasz Stanislawski 
   [original work in the PoC for buffer sharing]
Signed-off-by: Sumit Semwal 
Signed-off-by: Sumit Semwal 
Acked-by: Laurent Pinchart 
---
 drivers/media/video/v4l2-compat-ioctl32.c |   18 ++
 drivers/media/video/v4l2-ioctl.c  |1 +
 include/linux/videodev2.h |7 +++
 3 files changed, 26 insertions(+)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c 
b/drivers/media/video/v4l2-compat-ioctl32.c
index 9ebd5c5..a2e0549 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -304,6 +304,7 @@ struct v4l2_plane32 {
union {
__u32   mem_offset;
compat_long_t   userptr;
+   __u32   fd;
} m;
__u32   data_offset;
__u32   reserved[11];
@@ -325,6 +326,7 @@ struct v4l2_buffer32 {
__u32   offset;
compat_long_t   userptr;
compat_caddr_t  planes;
+   __u32   fd;
} m;
__u32   length;
__u32   reserved2;
@@ -348,6 +350,9 @@ static int get_v4l2_plane32(struct v4l2_plane *up, struct 
v4l2_plane32 *up32,
up_pln = compat_ptr(p);
if (put_user((unsigned long)up_pln, >m.userptr))
return -EFAULT;
+   } else if (memory == V4L2_MEMORY_DMABUF) {
+   if (copy_in_user(>m.fd, >m.fd, sizeof(int)))
+   return -EFAULT;
} else {
if (copy_in_user(>m.mem_offset, >m.mem_offset,
sizeof(__u32)))
@@ -371,6 +376,11 @@ static int put_v4l2_plane32(struct v4l2_plane *up, struct 
v4l2_plane32 *up32,
if (copy_in_user(>m.mem_offset, >m.mem_offset,
sizeof(__u32)))
return -EFAULT;
+   /* For DMABUF, driver might've set up the fd, so copy it back. */
+   if (memory == V4L2_MEMORY_DMABUF)
+   if (copy_in_user(>m.fd, >m.fd,
+   sizeof(int)))
+   return -EFAULT;

return 0;
 }
@@ -453,6 +463,10 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, 
struct v4l2_buffer32 __user
if (get_user(kp->m.offset, >m.offset))
return -EFAULT;
break;
+   case V4L2_MEMORY_DMABUF:
+   if (get_user(kp->m.fd, >m.fd))
+   return -EFAULT;
+   break;
}
}

@@ -517,6 +531,10 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, 
struct v4l2_buffer32 __user
if (put_user(kp->m.offset, >m.offset))
return -EFAULT;
break;
+   case V4L2_MEMORY_DMABUF:
+   if (put_user(kp->m.fd, >m.fd))
+   return -EFAULT;
+   break;
}
}

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 6bc47fc..dffd3c9 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -155,6 +155,7 @@ static const char *v4l2_memory_names[] = {
[V4L2_MEMORY_MMAP]= "mmap",
[V4L2_MEMORY_USERPTR] = "userptr",
[V4L2_MEMORY_OVERLAY] = "overlay",
+   [V4L2_MEMORY_DMABUF] = "dmabuf",
 };

 #define prt_names(a, arr) a) >= 0) && ((a) < ARRAY_SIZE(arr))) ? \
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 7a147c8..7f918dc 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -186,6 +186,7 @@ enum v4l2_memory {
V4L2_MEMORY_MMAP = 1,
V4L2_MEMORY_USERPTR  = 2,
V4L2_MEMORY_OVERLAY  = 3,
+   V4L2_MEMORY_DMABUF   = 4,
 };

 /* see also http://vektor.theorem.ca/graphics/ycbcr/ */
@@ -596,6 +597,8 @@ struct v4l2_requestbuffers {
  * should be passed to mmap() called on the video node)
  * @userptr:   when memory is V4L2_MEMORY_USERPTR, a userspace pointer
  * pointing to this plane
+ * @fd:when memory is V4L2_MEMORY_DMABUF, a userspace 
file
+ * descriptor associated with this plane
  * @data_offset:   offset in the plane to the start of data; usually 0,
  * unless there is a header in front of the data
  *
@@ -610,6 +613,7 @@ struct v4l2_plane {
union {
__u32   mem_offset;
unsigned long   userptr;
+   int fd;
} m;
__u32

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

Re: [PATCHv8 01/26] v4l: Add DMABUF as a memory type

[PATCHv8 01/26] v4l: Add DMABUF as a memory type

5 matches

Site Navigation

Mail list logo

Footer information