Hi Hans,
Thank you for your review.
I reply to your comments. See below.
Regards,
Junghak
On 06/12/2015 07:51 PM, Hans Verkuil wrote:
Hi Junghak,
On 06/08/2015 03:35 PM, Junghak Sung wrote:
Hello everybody,
This patch series refactories exsiting Videobuf2, so that not only V4L2
but also other frameworks can use it to manage buffer and utilize
queue.
I would separate existing Videobuf2-core framework into two parts - common
and v4l2-specific part. This work is as follows :
1. Separate existing vb2_buffer structure into common buffer and
v4l2-specific parts by removing v4l2-specific members and
embedding it into vb2_v4l2_buffer structure like this:
struct vb2_v4l2_buffer {
struct vb2_buffer vb2;
struct v4l2_buffer v4l2_buf;
struct v4l2_plane v4l2_planes[VIDEO_MAX_PLANES];
};
2. Abstract the v4l2-specific elements, and specify them when the device
drives use them. For example, vb2_v4l2_buffer structure can be abstracted
by vb2_buffer structure, and device drivers can get framework-specific
object such as vb2_v4l2_buffer by using container_of().
3. Separate VB2-core framework into real VB2-core and v4l2-specific part.
This means that it moves V4L2-specific parts of VB2-core to v4l2-specific
part because current VB2-core framework has some codes dependent of V4L2.
As a result, we will have two VB2 files - videobuf2-core.c and
videobuf2-v4l2.c.
Why do we try to make the VB2 framework to be common?
As you may know, current DVB framework uses ringbuffer mechanism to demux
MPEG-2 TS data and pass it to userspace. However, this mechanism requires
extra memory copy because DVB framework provides only read() system call for
application - read() system call copies the kernel data to user-space buffer.
So if we can use VB2 framework which supports streaming I/O and buffer
sharing mechanism, then we could enhance existing DVB framework by removing
the extra memory copy - with VB2 framework, application can access the kernel
data directly through mmap system call.
This patch series is the first step for it.
We have a plan for this work as follows:
1. Separate existing VB2 framework into three parts - VB2 common, VB2-v4l2,
and VB2-dvb. Of course, this change should not affect other v4l2-based
device drivers. This patch series includes some parts of this step.
2. Add new APIs for DVB streaming I/O. These APIs will be implemented
in VB2-dvb framework. So, we can remove unnecessary memory copy between
kernel-space and user-space by using these new APIs.
However, we leaves legacy interfaces as-is for backward compatibility.
We are working on this project with Mauro and have a discussion with him
on IRC channel weekly. Nowaday, we are discussing more detailed DVB user
scenario for streaming I/O.
The final goal of this project is to enhance current DVB framework.
The first mission is to achieve zero-copy functionality between kernel-space
and user-space with mmap system call. More missions are under consideration:
i.e., we could share the buffer not only between kernel-space and user-space
but also between devices - demux, hw video codec - by exporting a buffer
to dmabuf fd with VB2 framework.
Any suggestions and comments are welcome.
I've tried to review it, but in the current form it is too hard to do. It's not
helped by the fact that patch 2 didn't make it to the mailinglist. It might
be better to just split up such large patches and make a note that the pull
request will combine the two (or more) patches into one.
I agree with your opinion that the patches should be rework to be
more easier to review. I will try to split up the patches and resend it.
BTW, I fully agree with what you want to do, where I am having trouble is
verifying that nothing breaks and in the details of the vb2 API changes.
Can you make a new patch series that only shows the changes to the videobuf2
sources and headers, so without all the driver changes? The driver changes
are automatically checked by the compiler when you build the drivers, so I
believe that that's correct (I hope :-) ).
Also try to split it up in smaller pieces. I had the feeling that you combined
multiple but otherwise independent changes in one patch. The vb2 framework is
an important piece of code, and I want to be confident about the changes made.
Regarding the split into core and v4l2 sources/headers: I've been thinking about
this a bit more, and what would probably make reviewing easiest is if you start
off with making a simple videobuf2-v4l2.h header that just includes core.h and
slowly move the v4l2-specific parts from videobuf2-core.h to videobuf2-v4l2.h.
And do the same with videobuf2-v4l2.c: start off with an empty source and move
code from core.c to v4l2.c whenever a v4l2-specific part migrates.
That's a good point. I think I have made these patches too hastily.
As your comment, I will split the patch into two or more pieces.
The first is just renaming videobuf2-core.[ch] to videobuf2-v4l2.[ch].
Another note: I noticed that the queue_setup op was changed so the v4l2_format
argument became a void *. I do not think that that is the right approach since
we lose strong typing.
I wonder if v4l2-specific queue ops should be introduced. I'm not sure how that
would work out, but I think we can look at that for the next round of patches.
OK, I will try to find out more reasonable way to make it commonly (not
v4l2-specific)
without changing the v4l2-specivic queue ops.
Regards,
Hans
Best regards,
Junghak
Junghak Sung (3):
modify the vb2_buffer structure for common video buffer and make
struct vb2_v4l2_buffer
move struct vb2_queue to common and apply the changes related with
that Signed-off-by: Junghak Sung <jh1009.sung@xxxxxxxxxxx>
make vb2-core part with not v4l2-specific elements
Documentation/video4linux/v4l2-pci-skeleton.c | 16 +-
drivers/media/dvb-frontends/rtl2832_sdr.c | 13 +-
drivers/media/pci/cx23885/cx23885-417.c | 13 +-
drivers/media/pci/cx23885/cx23885-core.c | 4 +-
drivers/media/pci/cx23885/cx23885-dvb.c | 12 +-
drivers/media/pci/cx23885/cx23885-vbi.c | 17 +-
drivers/media/pci/cx23885/cx23885-video.c | 19 +-
drivers/media/pci/cx23885/cx23885.h | 2 +-
drivers/media/pci/cx25821/cx25821-video.c | 20 +-
drivers/media/pci/cx25821/cx25821.h | 2 +-
drivers/media/pci/cx88/cx88-blackbird.c | 15 +-
drivers/media/pci/cx88/cx88-core.c | 2 +-
drivers/media/pci/cx88/cx88-dvb.c | 13 +-
drivers/media/pci/cx88/cx88-mpeg.c | 2 +-
drivers/media/pci/cx88/cx88-vbi.c | 17 +-
drivers/media/pci/cx88/cx88-video.c | 17 +-
drivers/media/pci/cx88/cx88.h | 2 +-
drivers/media/pci/saa7134/saa7134-core.c | 4 +-
drivers/media/pci/saa7134/saa7134-ts.c | 28 +-
drivers/media/pci/saa7134/saa7134-vbi.c | 22 +-
drivers/media/pci/saa7134/saa7134-video.c | 17 +-
drivers/media/pci/saa7134/saa7134.h | 8 +-
drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 19 +-
drivers/media/pci/solo6x10/solo6x10-v4l2.c | 11 +-
drivers/media/pci/solo6x10/solo6x10.h | 4 +-
drivers/media/pci/sta2x11/sta2x11_vip.c | 24 +-
drivers/media/pci/tw68/tw68-video.c | 20 +-
drivers/media/pci/tw68/tw68.h | 2 +-
drivers/media/platform/am437x/am437x-vpfe.c | 33 +-
drivers/media/platform/am437x/am437x-vpfe.h | 2 +-
drivers/media/platform/blackfin/bfin_capture.c | 30 +-
drivers/media/platform/coda/coda-bit.c | 20 +-
drivers/media/platform/coda/coda-common.c | 28 +-
drivers/media/platform/coda/coda-jpeg.c | 2 +-
drivers/media/platform/coda/coda.h | 6 +-
drivers/media/platform/davinci/vpbe_display.c | 25 +-
drivers/media/platform/davinci/vpif_capture.c | 30 +-
drivers/media/platform/davinci/vpif_capture.h | 2 +-
drivers/media/platform/davinci/vpif_display.c | 34 +-
drivers/media/platform/davinci/vpif_display.h | 6 +-
drivers/media/platform/exynos-gsc/gsc-core.c | 2 +-
drivers/media/platform/exynos-gsc/gsc-core.h | 6 +-
drivers/media/platform/exynos-gsc/gsc-m2m.c | 12 +-
drivers/media/platform/exynos4-is/fimc-capture.c | 23 +-
drivers/media/platform/exynos4-is/fimc-core.c | 4 +-
drivers/media/platform/exynos4-is/fimc-core.h | 6 +-
drivers/media/platform/exynos4-is/fimc-is.h | 2 +-
drivers/media/platform/exynos4-is/fimc-isp-video.c | 25 +-
drivers/media/platform/exynos4-is/fimc-isp-video.h | 2 +-
drivers/media/platform/exynos4-is/fimc-isp.h | 4 +-
drivers/media/platform/exynos4-is/fimc-lite.c | 25 +-
drivers/media/platform/exynos4-is/fimc-lite.h | 4 +-
drivers/media/platform/exynos4-is/fimc-m2m.c | 14 +-
drivers/media/platform/m2m-deinterlace.c | 16 +-
drivers/media/platform/marvell-ccic/mcam-core.c | 26 +-
drivers/media/platform/marvell-ccic/mcam-core.h | 2 +-
drivers/media/platform/mx2_emmaprp.c | 16 +-
drivers/media/platform/omap3isp/ispvideo.c | 24 +-
drivers/media/platform/omap3isp/ispvideo.h | 4 +-
drivers/media/platform/s3c-camif/camif-capture.c | 23 +-
drivers/media/platform/s3c-camif/camif-core.c | 2 +-
drivers/media/platform/s3c-camif/camif-core.h | 4 +-
drivers/media/platform/s5p-g2d/g2d.c | 14 +-
drivers/media/platform/s5p-jpeg/jpeg-core.c | 34 +-
drivers/media/platform/s5p-mfc/s5p_mfc.c | 14 +-
drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 4 +-
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 36 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 57 +-
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 4 +-
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 4 +-
drivers/media/platform/s5p-tv/mixer.h | 4 +-
drivers/media/platform/s5p-tv/mixer_reg.c | 2 +-
drivers/media/platform/s5p-tv/mixer_video.c | 11 +-
drivers/media/platform/sh_veu.c | 25 +-
drivers/media/platform/soc_camera/atmel-isi.c | 26 +-
drivers/media/platform/soc_camera/mx2_camera.c | 41 +-
drivers/media/platform/soc_camera/mx3_camera.c | 34 +-
drivers/media/platform/soc_camera/rcar_vin.c | 30 +-
.../platform/soc_camera/sh_mobile_ceu_camera.c | 47 +-
drivers/media/platform/soc_camera/soc_camera.c | 2 +-
drivers/media/platform/ti-vpe/vpe.c | 30 +-
drivers/media/platform/vim2m.c | 28 +-
drivers/media/platform/vivid/vivid-core.h | 4 +-
drivers/media/platform/vivid/vivid-kthread-cap.c | 8 +-
drivers/media/platform/vivid/vivid-kthread-out.c | 8 +-
drivers/media/platform/vivid/vivid-sdr-cap.c | 18 +-
drivers/media/platform/vivid/vivid-vbi-cap.c | 14 +-
drivers/media/platform/vivid/vivid-vbi-out.c | 14 +-
drivers/media/platform/vivid/vivid-vid-cap.c | 24 +-
drivers/media/platform/vivid/vivid-vid-out.c | 21 +-
drivers/media/platform/vsp1/vsp1_rpf.c | 4 +-
drivers/media/platform/vsp1/vsp1_video.c | 20 +-
drivers/media/platform/vsp1/vsp1_video.h | 6 +-
drivers/media/platform/vsp1/vsp1_wpf.c | 4 +-
drivers/media/usb/airspy/airspy.c | 15 +-
drivers/media/usb/au0828/au0828-vbi.c | 17 +-
drivers/media/usb/au0828/au0828-video.c | 29 +-
drivers/media/usb/au0828/au0828.h | 2 +-
drivers/media/usb/em28xx/em28xx-vbi.c | 15 +-
drivers/media/usb/em28xx/em28xx-video.c | 30 +-
drivers/media/usb/em28xx/em28xx.h | 2 +-
drivers/media/usb/go7007/go7007-driver.c | 2 +-
drivers/media/usb/go7007/go7007-priv.h | 4 +-
drivers/media/usb/go7007/go7007-v4l2.c | 15 +-
drivers/media/usb/hackrf/hackrf.c | 13 +-
drivers/media/usb/msi2500/msi2500.c | 13 +-
drivers/media/usb/pwc/pwc-if.c | 26 +-
drivers/media/usb/pwc/pwc.h | 2 +-
drivers/media/usb/s2255/s2255drv.c | 16 +-
drivers/media/usb/stk1160/stk1160-v4l.c | 15 +-
drivers/media/usb/stk1160/stk1160-video.c | 2 +-
drivers/media/usb/stk1160/stk1160.h | 4 +-
drivers/media/usb/usbtv/usbtv-video.c | 11 +-
drivers/media/usb/usbtv/usbtv.h | 2 +-
drivers/media/usb/uvc/uvc_queue.c | 32 +-
drivers/media/usb/uvc/uvcvideo.h | 4 +-
drivers/media/v4l2-core/Makefile | 2 +-
drivers/media/v4l2-core/v4l2-ioctl.c | 2 +-
drivers/media/v4l2-core/v4l2-mem2mem.c | 8 +-
drivers/media/v4l2-core/videobuf2-core.c | 1888 ++------------------
drivers/media/v4l2-core/videobuf2-dma-contig.c | 2 +-
drivers/media/v4l2-core/videobuf2-dma-sg.c | 2 +-
drivers/media/v4l2-core/videobuf2-dvb.c | 2 +-
drivers/media/v4l2-core/videobuf2-memops.c | 2 +-
drivers/media/v4l2-core/videobuf2-v4l2.c | 1878 +++++++++++++++++++
drivers/media/v4l2-core/videobuf2-vmalloc.c | 2 +-
drivers/staging/media/davinci_vpfe/vpfe_video.c | 30 +-
drivers/staging/media/davinci_vpfe/vpfe_video.h | 2 +-
drivers/staging/media/dt3155v4l/dt3155v4l.c | 14 +-
drivers/staging/media/dt3155v4l/dt3155v4l.h | 2 +-
drivers/staging/media/omap4iss/iss_video.c | 16 +-
drivers/staging/media/omap4iss/iss_video.h | 4 +-
drivers/usb/gadget/function/uvc_queue.c | 14 +-
drivers/usb/gadget/function/uvc_queue.h | 4 +-
include/media/davinci/vpbe_display.h | 2 +-
include/media/soc_camera.h | 2 +-
include/media/v4l2-mem2mem.h | 10 +-
include/media/videobuf2-core.h | 235 ++-
include/media/videobuf2-dma-contig.h | 6 +-
include/media/videobuf2-dma-sg.h | 6 +-
include/media/videobuf2-dvb.h | 2 +-
include/media/videobuf2-memops.h | 2 +-
include/media/videobuf2-v4l2.h | 139 ++
include/media/videobuf2-vmalloc.h | 2 +-
144 files changed, 3290 insertions(+), 2662 deletions(-)
create mode 100644 drivers/media/v4l2-core/videobuf2-v4l2.c
create mode 100644 include/media/videobuf2-v4l2.h
--
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
