Hello, On Monday 10 August 2015 15:38:48 Hans Verkuil wrote: > On 08/10/2015 02:55 PM, Mauro Carvalho Chehab wrote: > > Em Mon, 10 Aug 2015 14:07:21 +0200 Hans Verkuil escreveu: > >> Hi Junghak, > >> > >> I'm reviewing the header changes since I think there are several > >> improvements that can be done that will make things more logical and > >> will simplify the code. > >> > >> My comments below are a mix of suggestions for improvement and > >> brainstorming. > >> > >> Feel free to ask for clarification if something is not clear. > >> > >> On 07/31/2015 10:44 AM, Junghak Sung wrote: > >>> Divide videobuf2-core into core part and v4l2-specific part > >>> > >>> - core part: videobuf2 core related with buffer management & memory > >>> allocation - v4l2-specific part: v4l2-specific stuff > >>> > >>> Signed-off-by: Junghak Sung <jh1009.sung@xxxxxxxxxxx> > >>> Signed-off-by: Geunyoung Kim <nenggun.kim@xxxxxxxxxxx> > >>> Acked-by: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx> > >>> Acked-by: Inki Dae <inki.dae@xxxxxxxxxxx> > >>> --- > >> > >> <snip> > > > > ... > > > >> I noticed that __qbuf_mmap/userptr/dmabuf are all in -v4l2.c. That's a > >> bad sign: those are some of the most complex vb2 functions and they > >> really belong in the core since you'll need it for DVB as well. As > >> suggested above, by moving the index, length and offset/userptr/fd data > >> to the core structs these functions can all be moved back into core.c as > >> far as I can see. > > > > Well, that will depend on how the DVB implementation will actually be. > > > > Currently, VB2 has lot of V4L2-dependent code on it, with lots of V4L2 > > structs from videodev2.h that are there. > > > > Well, if we want the core to be re-used, it should not include videodev2.h > > anymore. Also, it should not assume that all non-V4L2 cores would use > > exactly the same logic for the userspace API. > > Agreed. > > > In the DVB case, it makes no sense to have anything similar to OVERLAY > > there. > > VB2 doesn't support overlay at all, so that's no problem. > > > I also can't see any usage for USERPTR at DVB neither, as either > > MMAP or DMABUF should fulfill all userspace needs I'm aware of. > > While USERPTR isn't needed for DVB, the actual handling of such buffers > is completely independent of the API. I think it is from an architecture > point-of-view a really bad idea if anyone other than the vb2 core would > call the memops. So yes, the core would have code that is not needed by > DVB, but it still is something that belongs to the core in my view. Anything > else is very ugly. I agree, USERPTR handling belongs to the core as that's not dependent on V4L2. However, I'd make USERPTR usage not recommended for new drivers/subsystems (that's a polite way to say "don't even think about using it" :-)) as DMABUF should support the USERPTR use cases nowadays, and USERPTR wobbly as DMA to userspace memory isn't officially supported on all architectures. > > Also, the meta data for the DVB upcoming ioctls for MMAP/DMABUF aren't > > yet proposed. They can be very different than the ones inside the V4L2 > > ioctls. > > Well, it's pretty much constrained by mmap() and the dma-buf API. I.e. an > offset for for mmap and a fd for dmabuf. You don't have a choice here. > > > So, I guess it is better for now to keep those API-dependent stuff at > > VB2-v4l2 and, once the DVB code (and the corresponding API bits) are > > written, revisit it and then move the common code to the VB2 core. > > I strongly disagree with that. Having API-dependent code calling memops > defeats the purpose. There is nothing wrong with having a vb2 core that > supports e.g. USERPTR and dma-buf as long as that core is API-independent. > > But there is a lot wrong if the API-dependent code is bypassing the vb2 core > code to call low-level memops. > > >> It is good to remember that today the v4l2_buffer struct is used in the > >> vb2 core because vb2 is only used with v4l2, so why duplicate v4l2_buffer > >> fields in the vb2 core structs? > > > > We should not have any v4l2_* struct inside VB2 core, as the DVB core > > should not be dependent on the V4L2 structs. So, everything that it is > > V4L2-specific should be inside the VB2-v4l2. The reverse is also true: > > we should not pollute the VB2 core with DVB-specific data structures. > > I never said anything else. I was talking about what's used today and why > it is the way it is now. > > > So, all VB2-specific struct should be at VB2-dvb. > > > >> But if we want to reuse it for other subsystems, then > >> the vb2 core structs should contain all the core buffer information. This > >> avoids the need for a lot of the ops that you added and makes it > >> possible to keep the __qbuf_mmap/userptr/dmabuf in the core code as > >> well. > >> > >> Adding these fields to the vb2 core structs is something that can be done > >> first, before splitting up core.c into core.c and v4l2.c. > > > > I'm afraid that we'll lose the big picture if we try to put the > > API-dependent parts at the core before adding a non-V4L2 usage on VB2. > > > > We can always simplify the code latter, but IMHO we should focus first > > on adding the new functionality (support for DVB). Afterwards, we'll have > > a better view on what API-dependent code could be shared. > > This is core code. I must be able to follow all the changes and right now > I can't. So simplifications like this should be done first before the split. > It will makes things much easier to follow. > > The whole goal of a vb2 core is that it: > > 1) does not use any v4l2/dvb/whatever subsystem specific structs or > includes. 2) deals with *all* the buffer memory handling. This is the most > complex part, and that's what really belongs here. So memops being called > from API-dependent code is wrong and I'll NACK that. > 3) does all the ringbuffer handling etc. For what it's worth, I have the same view for the vb2 core goals. > The split will be much more obvious and logical if things like index, > length, offset/userptr/fd are added to the vb2 core structs. We could have > done that when we created vb2 since it's obviously essential core buffer > information, but we had the v4l2_buffer struct anyway so there was no real > need to do so. But now there is. -- 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
