Re: [PATCHv9 18/25] v4l: add buffer exporting via dmabuf

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon October 8 2012 12:41:28 Tomasz Stanislawski wrote:
> 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@xxxxxxxxxxx>
> >>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> [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.

You are overengineering. If we ever want to export something that is *not*
a buffer created with REQBUFS/CREATE_BUFS, then you want to do that with a
new ioctl. If we want for some reason to export a userptr, then that should
either be from a queued/prepared buffer, or we need a separate API to create
a dmabuf from a userptr. After all, that would be completely generic and not
V4L2 specific.

Anyway, introducing such a union later won't work either because then instead
of writing expbuf.type you'd have to write expbuf.by_tip.type: API change.

BTW, I think it makes sense as well in the case of a userptr to only export
the buffer if it is under control of the kernel (e.g. after QBUF or PREPARE_BUF).
When exporting the buffer the driver has all the information it needs to do so
safely.

> 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.

'usually', yes. :-)

> 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).

Performance is not an issue here. It's about conforming to existing conventions
and making it possible to use INFO_FL_CLEAR() in v4l2-ioctl.c to have the struct
zeroed automatically after all the IN arguments, thus making it easier on drivers.

Regards,

	Hans
--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux