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 <sumit.semwal@xxxxxx>
> >>
> >> 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.stanislaws@xxxxxxxxxxx>
> >> [original work in the PoC for buffer sharing]
> >> Signed-off-by: Sumit Semwal <sumit.semwal@xxxxxx>
> >> Signed-off-by: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> >> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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
