<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
bbrezillon: pinchartl, hverkuil: I'm working on the new v4l2_ext_format struct and trying to make u32 compat easier
<br> and I have a question regarding the v4l2_window struct
<br> more precisely the v4l2_format->clips field
<br> looks like clips is a linked list
<br> but v4l2_format already contains the number of entries in the ->clips array, and apparently the list is reconstructed in get_v4l2_window32() by linking each element with the next entry in the array
<br> so I'm wondering if we really need to pass an array of clips, looks like an array of rects would do the trick
hverkuil: <u>bbrezillon</u>: In fact, it already *is* an array. The next pointer in struct v4l2_clip isn't used at all. So this can be replaced by an array of v4l2_rect in the new version of the struct.
<br> Why v4l2_clip has a next pointer at all is lost in the mists of time.
bbrezillon: :)
<br> good
<br> also, I'm replacing pointers in uapi structs by __u64 which can then be converted back to pointers with u64_to_user_ptr()
hverkuil: yes, definitely do that.
bbrezillon: this way, no compat32 issues
<br> but that means exposing different structs to drivers
hverkuil: brb
bbrezillon: right now they're directly passed v4l2_format (the one defined in uapi hdr)
<br> we can't do that if we encode userspace pointers with __u64 fields
<br> unless we add an union, but then it's unclear which value has to be fille by userspace
<br> *filled
hverkuil: Not sure what you mean. I would assume that a new v4l2_ext_format + new EXT_FMT ioctls would work the same as in my v4l2_ext_buffer patch: drivers would either support the new ioctls or only the old ones, and the v4l2-ioctl.c code detects that and converts from v4l2_ext_format to v4l2_format or vice versa.
<br> That way we can convert drivers one by one.
bbrezillon: this is my new v4l2_window struct:
<br> struct v4l2_ext_window {
<br> struct v4l2_rect w;
<br> __u32 field; /* enum v4l2_field */
<br> __u32 chromakey;
<br> __u64 bitmap_ptr;
<br> __u64 clips_ptr;
<br> __u32 clipcount;
<br> __u8 global_alpha;
<br> __u8 padding[3];
<br> };
<br> see the clips_ptr and bitmap_ptr
<br> they are to be converted to void * __user pointers and then copied into memory allocated in kernel space
<br> but that means drivers can no longer dereference the clips array directly (format->win.clips[x])
<br> at least no if they manipulate v4l2_ext_format directly
<br> there needs to be a kernel side struct to represent v4l2_ext_{window,format}
<br> having something like that would work
<br> struct v4l2_ext_window {
<br> struct v4l2_rect w;
<br> __u32 field; /* enum v4l2_field */
<br> __u32 chromakey;
<br> union {
<br> __u64 user_bitmap_ptr;
<br> void *bitmap;
<br> }
<br> union {
<br> __u64 user_clips_ptr;
<br> struct v4l2_rect *clips;
<br> };
<br> __u32 clipcount;
<br> __u8 global_alpha;
<br> __u8 padding[3];
<br> };
<br> but I find it disturbing to expose a plain pointer to userspace when it should actually manipulate the __u64 field
<br> maybe it's just a matter of documenting it properly
hverkuil: There are very few drivers using v4l2_window, and even fewer using clips or a bitmap.
<br> Hmm.
bbrezillon: what do you suggest? rip v4l2_window out of v4l2_ext_format and reject these types?
hverkuil: Leave out the clips and bitmap support for now, just add a TODO comment about it.
<br> I would have to think how best to approach this. And we probably need a discussion whether the new API should support overlays at all, and if it should, should we keep the clips and bitmap support?
bbrezillon: that means breaking the ABI if we decide to support them at some point?
hverkuil: We need to decide that before we introduce the new API, but we are in the early design stages and we can ignore it for now.
<br> If we decide to keep this, then we probably want to introduce a kernel-internal v4l2_window struct and pass that to the struct v4l2_ioctl_ops overlay ops.
bbrezillon: ok, so I keep the first version of v4l2_ext_window and make sure that bitmap_ptr and clipcount are always 0
hverkuil: yes
<br> Another area where we can improve things is in struct v4l2_ioctl_ops. I never liked the fmt ops. Instead of passing v4l2_ext_format *f I would prefer that the core just passes f->type and the v4l2_ext_pix_format/ext_window/etc. struct.
<br> It would simplify driver code.
bbrezillon: <u>hverkuil</u>: one thing at a time :)
hverkuil: Another idea would be to put the 'old' format ops and the 'new' format ops in a union in struct v4l2_ioctl_ops (with a bool to determine what the driver supports). Otherwise we would waste a lot of memory. Same for the streaming ops.
<br> (just add it to the 'ideas' list!)
***: sic_ has quit IRC (Read error: Connection reset by peer)
<br> ageis has quit IRC (Quit: exit(1); echo 'https://cointel.pro' > /dev/null; x-www-browser 'https://twitter.com/ageis')
bbrezillon: grrr! there's an existing V4L2_CAP_EXT_PIX_FORMAT flag
hverkuil: <u>bbrezillon</u>: I think we can use V4L2_CAP_EXT_STREAMING for both the new streaming ioctls and the new format ioctls. I'm sure they go together anyway.
bbrezillon: shouldn't be a problem, but that's confusing
hverkuil: oh well...
bbrezillon: yes, it's set in v4l_querycap()
pinchartl: <u>bbrezillon</u>: regarding v4l2_window
<br> just drop it :-)
<br> it's for overlay only
<br> that's an old API
<br> let's not carry it forward
hverkuil: <u>pinchartl</u>: I'm not sure we can. But this is a separate discussion.
pinchartl: maybe we can't for the few existing drivers that use it
<br> but there's no need to carry it forward in the new API
<br> when was the last time a driver with overlay was merged ? :-)
<br> especially given that the overlay API involves passing a physical address from userspace to kernelspace
hverkuil: Users of V4L2_CAP_VIDEO_OVERLAY: saa7146, bttv, saa7134, fsl-viu
<br> Users of V4L2_CAP_VIDEO_OUTPUT_OVERLAY: ivtv, omap_vout
pinchartl: that's what I was looking at :-)
hverkuil: bttv, saa7134 and ivtv are the most likely to be still in use somewhere.
pinchartl: no issue with that, we can keep the existing API there
hverkuil: I think that's probably the right approach. None of these drivers use _MPLANE. I think that once this new API is merged we want to convert at least all _MPLANE using drivers to the new API.
bbrezillon: hverkuil, pinchartl: so I just forbid OVERLAY formats and drop the v4l2_ext_window from the union
<br> is that correct?
hverkuil: y
pinchartl: <u>bbrezillon</u>: yes
***: gaulishcoin_ has left
<br> sailus has quit IRC (Ping timeout: 252 seconds)
hverkuil: Hmm, I hadn't seen that before, but we shrunk the media subsystem in 5.1!
<br> 536 files changed, 15295 insertions(+), 15371 deletions(-)
<br> By just 76 lines, but still...
sailus: Wow.
<br> Continue like this, and we'll be down to zero lines in... around the year 6000.
<br> That estimate assumes a few things stay stable over the years.
***: benjiG has left
<br> prabhakarlad has left
bbrezillon: hverkuil, pinchartl: hm, it's actually more invasive than I thought
<br> I changed v4l2_ext_buffer to embed an array of v4l2_ext_planes
<br> and now I have to add a ->fill_user_ext_buffer() to vb2_buf_ops
***: hverkuil has quit IRC (Quit: ZNC 1.7.2+deb1 - https://znc.in)
<br> Kwiboo has quit IRC (Quit: .)