Re: [RFC PATCH v2 0/5] Refactoring Videobuf2 for common use

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

 




Hi, Hans and Mauro.

First of all, thank you for your reviews.

On 08/10/2015 08:44 PM, Hans Verkuil wrote:
On 08/10/2015 12:49 PM, Mauro Carvalho Chehab wrote:
Em Mon, 10 Aug 2015 12:11:39 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:

On 08/10/2015 11:32 AM, Mauro Carvalho Chehab wrote:
Em Mon, 10 Aug 2015 10:31:03 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:

Hi Jungsak,

On 07/31/2015 10:44 AM, Junghak Sung wrote:
Hello everybody,

This is the 2nd round for refactoring Videobuf2(a.k.a VB2).
The purpose of this patch series is to separate existing VB2 framework
into core part and V4L2 specific part. So that not only V4L2 but also other
frameworks can use them to manage buffer and utilize queue.

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.

We have a plan for this work as follows:
1. Separate existing VB2 framework into three parts - VB2 common, VB2-v4l2.
    Of course, this change will not affect other v4l2-based
    device drivers. This patch series corresponds to this step.

2. Add and implement new APIs for DVB streaming I/O.
    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.

This patch series is the first step for it.
The previous version of this patch series can be found at [1].

[1] RFC PATCH v1 - http://www.spinics.net/lists/linux-media/msg90688.html

This v2 looks much better, but, as per my comment to patch 1/5, it needs a bit
more work before I can do a really good review. I think things will be much
clearer once patch 3 shows the code moving from core.c/h to v4l2.c/h instead
of the other way around. That shouldn't be too difficult.

Hans,

I suggested Junkhak to do that. On his previous patchset, he did what
you're suggestiong, e. g moving things from vb2-core into vb2-v4l2, and
that resulted on patches big enough to not be catched by vger.

Actually, that wasn't the reason why the patches became so big. I just
reorganized the patch series as I suggested above (pretty easy to do) and
the size of patch 3 went down.

Considering only size of the patch, it is better to move from vb2-v4l2 to vb2-core as this patch. Because the file size of videobuf2-v4l2.c (56k) is bigger that videobuf2-core.c (50k). :P


Ah, ok.

Also, IMHO, it is cleared this way, as we can see what parts of VB2 will
actually be shared, as there are lots of things that won't be shared:
overlay, userptr, multiplanar.

That's why I prefer to see what moves *out* from the core.

To be honest, it depends on what your preference is.

Yeah. I actually prefer to see what will be shared, because the
non-shared code won't have changes (except for minor kABI things),
while the shared code will be more affected :)


As Mauro said, I focus on what code can be shared. In other words,
what code is NOT dependent on V4L2 and what code is really necessary for vb2-core even though it is depend on V4l2. And then, those codes
are moved from vb2-v4l2 to vb2-core.

Junghak, just leave the patch as-is. However, for v3 you should run
checkpatch.pl over the diff since it complained about various things.

There are two things here:

1) on patches that just move things around, we should not
run checkpatch, as otherwise it would be a nightmare for
review. Ok, as we're doing a remanufacturing, it is a good
idea to run checkpatch at the end and see what should be fixed
(or do it before), but this is out of the scope of the manufacturing.
I can do that myself when applying the series.

It was actually complaining about new code.

	Hans


2) For all other patches that are adding/changing the code, then
Junghak should run checkpatch and fix (most) stuff complained there.

Ok. for v3, I will run checkpatch for added/changed code.


Regards,
Mauro


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