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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/10/2015 12:49 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 10 Aug 2015 12:11:39 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 08/10/2015 11:32 AM, Mauro Carvalho Chehab wrote:
>>> Em Mon, 10 Aug 2015 10:31:03 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> 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.
> 
> 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 :)
> 
>> 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.
> 
> Regards,
> Mauro
>