Hi Hans,
On 10/08/2012 11:54 AM, Hans Verkuil wrote:
On Mon October 8 2012 11:40:37 Tomasz Stanislawski wrote:
Hi Hans,
On 10/07/2012 04:17 PM, Hans Verkuil wrote:
On Sun October 7 2012 15:38:30 Laurent Pinchart wrote:
Hi Hans,
On Friday 05 October 2012 10:55:40 Hans Verkuil wrote:
On Tue October 2 2012 16:27:29 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.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
[snip]
+struct v4l2_exportbuffer {
- __s32 fd;
- __u32 flags;
- __u32 type; /* enum v4l2_buf_type */
- __u32 index;
- __u32 plane;
As suggested in my comments in the previous patch, I think it is a more natural order to have the type/index/plane fields first in this struct.
Actually, I think that flags should also come before fd:
struct v4l2_exportbuffer { __u32 type; /* enum v4l2_buf_type */ __u32 index; __u32 plane; __u32 flags; __s32 fd; __u32 reserved[11]; };
It would indeed feel more natural, but putting them right before the reserved fields allows creating an anonymous union around type, index and plane and extending it with reserved fields if needed. That's (at least to my understanding) the rationale behind the current structure layout.
The anonymous union argument makes no sense to me, to be honest.
I agree that the anonymous unions are not good solutions because they are not supported in many C dialects. However I have nothing against using named unions.
Named or unnamed, I don't see how a union will help. What do you want to do with a union?
Currently, there exist three sane layouts of the structure, that use only one reserved field:
A) struct v4l2_exportbuffer { __s32 fd; __u32 flags; __u32 type; /* enum v4l2_buf_type */ __u32 index; __u32 plane; __u32 reserved[11]; }
B) struct v4l2_exportbuffer { __u32 type; /* enum v4l2_buf_type */ __u32 index; __u32 plane; __u32 flags; __s32 fd; __u32 reserved[11]; }
C) struct v4l2_exportbuffer { __u32 type; /* enum v4l2_buf_type */ __u32 index; __u32 plane; __u32 reserved[11]; __u32 flags; __s32 fd; }
Only the layout B follows 'input/output/reserved' rule.
The layouts A and C allows to extend (type/index/plane) tuple without mixing it with (flags,fd).
For layouts A and C it is possible to use unions to provide new means of describing a buffer to be exported.
struct v4l2_exportbuffer { __s32 fd; __u32 flags; union { struct by_tip { /* type, index, plane */ __u32 type; /* enum v4l2_buf_type */ __u32 index; __u32 plane; } by_tip; struct by_userptr { u64 userptr; u64 length; } by_userptr; __u32 reserved[6]; } b; __u32 union_type; /* BY_TIP or BY_USERPTR */ __u32 reserved[4]; };
No such an extension can be applied for layout B.
The similar scheme can be used for layout C. Moreover it support extensions and variants for (flags/fd) tuple. It might be useful if one day we would like to export a buffer as something different from DMABUF file descriptors.
Anyway, we have to choose between the elegance of the layout and the extensibility.
I think that layout A is a good trade-off. We could swap fd and flags to get little closer to "the rule".
It's standard practice within V4L2 to have the IN fields first, then the OUT fields, followed by reserved fields for future expansion.
IMO, the "input/output/reserved rule" is only a recommendation. The are places in V4L2 where this rule is violated with structure v4l2_buffer being the most notable example.
Notice that if at least one of the reserved fields becomes an input file then "the rule" will be violated anyway.
Sure, but there is no legacy yet, so why not keep to the recommendation?
Should we ever need a, say, sub-plane index (whatever that might be), then we can use one of the reserved fields.
Maybe not subplane :). But maybe some data_offset for exporting only a part of the buffer will be needed some day. Moreover, the integration of DMABUF with the DMA synchronization framework may involve passing additional parameters from the userspace.
Notice that flags and fd fields are not logically connected with (type/index/plane) tuple. Therefore both field sets should be separated by some reserved fields to ensure that any of them can be extended if needed.
This was the rationale for the structure layout in v9.
It's a bad idea to add multiple 'reserved' arrays, that makes userspace harder since it has to zero all of them instead of just one. Actually, the same applies to kernel space, which has to zero them as well.
Userspace usually cleans the whole structure using memset call. Notice that memset is a build-in functions therefore fields are not zeroed if they are initialized just below memset.
The number of reserved fields has no impact on initialization code. There has also negligible impact on performance (if any at all).
Regards, Tomasz Stanislawski
I still don't know why you want to use a non-standard field order.
Regards,
Hans