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

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

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@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.stanislaws@samsung.com>
>>    [original work in the PoC for buffer sharing]
>> Signed-off-by: Sumit Semwal <sumit.semwal@ti.com>
>> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
>> Acked-by: Laurent Pinchart <laurent.pinchart@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