Hi Mauro,
On Wednesday 29 October 2014 10:40:33 Mauro Carvalho Chehab wrote:
> Em Wed, 29 Oct 2014 11:01:24 +0100 Hans Verkuil escreveu:
> > On 10/29/14 10:13, Mauro Carvalho Chehab wrote:
> > > Em Wed, 29 Oct 2014 09:59:56 +0100 Hans Verkuil escreveu:
> > >> On 10/29/14 09:29, Mauro Carvalho Chehab wrote:
> > >>> Em Wed, 29 Oct 2014 08:29:08 +0100 Hans Verkuil escreveu:
> > >>>> On 10/28/2014 07:26 PM, Mauro Carvalho Chehab wrote:
> > >>>>> Em Fri, 10 Oct 2014 10:04:58 +0200 Hans Verkuil escreveu:
> > >>>>>> (This patch is from Divneil except for the vivid changes which I
> > >>>>>> added. He had difficulties posting the patch without the mailer
> > >>>>>> mangling it, so I'm reposting it for him)
> > >>>>>>
> > >>>>>> - vb2 drivers to rely on VB2_MAX_FRAME.
> > >>>>>>
> > >>>>>> - VB2_MAX_FRAME bumps the value to 64 from current 32
> > >>>>>
> > >>>>> Hmm... what's the point of announcing a maximum of 32 buffers to
> > >>>>> userspace, but using internally 64?
> > >>>>
> > >>>> Where do we announce 32 buffers?
> > >>>
> > >>> VIDEO_MAX_FRAME is defined at videodev2.h:
> > >>>
> > >>> include/uapi/linux/videodev2.h:#define VIDEO_MAX_FRAME
> > >>> 32
> > >>>
> > >>> So, it is part of userspace API. Yeah, I know, it sucks, but apps
> > >>> may be using it to limit the max number of buffers.
> > >>
> > >> So? Userspace is free to ask for 32 buffers, and it will get 32 buffers
> > >> if memory allows. vb2 won't be returning more than 32, so I don't see
> > >> how things can break.
> > >
> > > Well, VIDEO_MAX_FRAME has nothing to do with the max VB1 support. It is
> > > the maximum number of buffers supported by V4L2. Properly-written apps
> > > will never request more than 32 buffers, because we're telling them that
> > > this is not supported.
> > >
> > > So, it makes no sense to change internally to 64, but keeping announcing
> > > that the maximum is 32. We're just wasting memory inside the Kernel with
> > > no reason.
> >
> > Hmm, so you think VIDEO_MAX_FRAME should just be updated to 64?
>
> Yes.
>
> > I am a bit afraid that that might break applications (especially if there
> > are any that use bits in a 32-bit unsigned variable).
>
> What 32-bits have to do with that? This is just the maximum number of
> buffers, and not the number of bits.
Applications might use a bitmask to track buffers.
> > Should userspace know about this at all? I think that the maximum number
> > of frames is driver dependent, and in fact one of the future vb2
> > improvements would be to stop hardcoding this and leave the maximum up to
> > the driver.
>
> It is not driver dependent. It basically depends on the streaming logic.
> Both VB and VB2 are free to set whatever size it is needed. They can
> even change the logic to use a linked list, to avoid pre-allocating
> anything.
>
> Ok, there's actually a hardware limit, with is the maximum amount of
> memory that could be used for DMA on a given hardware/architecture.
>
> The 32 limit was just a random number that was chosen.
So, can't we just mark VIDEO_MAX_FRAME as deprecated ? We can't remove it as
applications might depend on it, but it's pretty useless otherwise.
> Actually, with V4L1 API, one struct were relying on it:
>
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 264) #define
> VIDEO_MAX_FRAME 32
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 265)
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 266) struct
> video_mbuf
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 267) {
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 268) int
> size; /* Total memory to map */
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 269) int
> frames; /* Frames */
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 270) int
> offsets[VIDEO_MAX_FRAME];
> ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 271) };
>
> But, for V4L2, this is just a number that can be used by apps when
> then want to get the biggest number of buffers that the Kernel
> supports.
>
> > Basically I would like to deprecate VIDEO_MAX_FRAME.
>
> As usual, this requires to first fix all applications that use it,
> give a few years for them to stop using it. Then, it can be removed.
>
> If you want to do so, you should start with v4l-utils:
>
> $ git grep VIDEO_MAX_FRAME
> contrib/freebsd/include/linux/videodev2.h:#define VIDEO_MAX_FRAME
> 32 include/linux/videodev2.h:#define VIDEO_MAX_FRAME 32
> utils/v4l2-compliance/v4l-helpers.h: __u32
> mem_offsets[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
> utils/v4l2-compliance/v4l-helpers.h: void
> *mmappings[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
> utils/v4l2-compliance/v4l-helpers.h: unsigned long
> userptrs[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
> utils/v4l2-compliance/v4l-helpers.h: int
> fds[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
> utils/v4l2-compliance/v4l-helpers.h: for (i = 0; i < VIDEO_MAX_FRAME;
> i++) utils/v4l2-compliance/v4l2-test-buffers.cpp: fail_on_test(g_index()
> >= VIDEO_MAX_FRAME); utils/v4l2-compliance/v4l2-test-buffers.cpp:
> buf.s_index(buf.g_index() + VIDEO_MAX_FRAME);
> utils/v4l2-compliance/v4l2-test-buffers.cpp:
> buf.s_index(buf.g_index() - VIDEO_MAX_FRAME);
> utils/v4l2-compliance/v4l2-test-formats.cpp:
> fail_on_test(cap->readbuffers > VIDEO_MAX_FRAME);
> utils/v4l2-compliance/v4l2-test-formats.cpp:
> fail_on_test(out->writebuffers > VIDEO_MAX_FRAME);
> utils/v4l2-ctl/v4l2-ctl-streaming.cpp: for (i = 0; i < VIDEO_MAX_FRAME;
> i++) { utils/v4l2-ctl/v4l2-ctl-streaming.cpp: for (int i = 0; i <
> VIDEO_MAX_FRAME; i++) utils/v4l2-ctl/v4l2-ctl-streaming.cpp: for
> (int i = 0; i < VIDEO_MAX_FRAME; i++)
> utils/v4l2-ctl/v4l2-ctl-streaming.cpp: struct v4l2_plane
> planes[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
> utils/v4l2-ctl/v4l2-ctl-streaming.cpp: void
> *bufs[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
> utils/v4l2-ctl/v4l2-ctl-streaming.cpp: int
> fds[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
--
Regards,
Laurent Pinchart
--
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
