tw686x driver

tw686x driver

[Hans Verkuil]

Hi Krzysztof, Ezequiel,

Last Monday I compared the two proposed drivers and I decided to go with Ezequiel's
code. The reasons are that his code has audio support and FIELD_INTERLACED support,
and the reality is that most users (read applications) want full frames instead of
fields. In addition his code was a bit more mature since it had gone through a careful
code review already.

I am obviously unhappy having to choose between two drivers. But the alternative
would be to not have anything merged, and then *everyone* would be unhappy.

Ezequiel, I've asked for two small changes to your v2. Once I have v3 I will make
a pull request.

So lessons learned:

Krzysztof, next time don't wait many months before posting a new version fixing
requested changes.

Ezequiel, next time don't throw away functionality of the original code, instead
just add your own functionality to it.

Regards,

	Hans

Re: tw686x driver

[Krzysztof Hałasa]

Hi Hans,

Hans Verkuil <hverkuil@xs4all.nl> writes:

> So lessons learned:
>
> Krzysztof, next time don't wait many months before posting a new version fixing
> requested changes.

Actually, this is not how it happened.

On July 3, 2015 I posted the original driver:
http://www.spinics.net/lists/linux-media/msg91474.html

Ezequiel reviewed the code on 6 Jul 2015:
http://www.spinics.net/lists/linux-media/msg91516.html

On 27 Jul 2015, as you can easily find in your own mail archive, he
informed me that he's working on the driver and that he's going to post
updated patches himself. This was holidays time for me and I stated
that I have to suspend my work till the end of August.

I asked him to add his changes on top of my code several times (and this
is really easy with git). This never happened, and on 14 Aug 2015 he
posted:

> Problem is I've re-written the driver, taking yours as a starting point
> and reference.

> In other words, I don't have a proper git branch with a history, starting
> from the patch you submitted. Instead, I would be submitting a new
> patch for Hans and Mauro to review.

Maybe I got the meaning of this wrong, and he wasn't writing about
rewriting the driver "from scratch". Yes, after receiving this mail
I stopped my development, waiting for his driver to show up. That's
where the "many months" are. Yes, Ezequiel waited for "many months" with
his version - not me.

Only after he has posted "his" driver, I could find out what the
"rewriting" meant.

Don't get me wrong, I was never opposed to him improving my driver.
I only requested that his contributions are clearly shown, in a form
of a patch or a patch set (or a git tree etc.), and so are my own.
I really can't understand why his code can't be transparently applied
over my original patch (or the updated one, which compiles and works
fine with recent media tree).

Is it too much to ask?


The lesson I learned is thus this instead:
- don't publish code because it can be hijacked, twisted and you'll
have to fight for even getting your authorship back. Forget about proper
attribution and history.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: tw686x driver

[Hans Verkuil]

On 03/03/2016 07:51 AM, Krzysztof Hałasa wrote:
> Hi Hans,
> 
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> So lessons learned:
>>
>> Krzysztof, next time don't wait many months before posting a new version fixing
>> requested changes.
> 
> Actually, this is not how it happened.
> 
> On July 3, 2015 I posted the original driver:
> http://www.spinics.net/lists/linux-media/msg91474.html
> 
> Ezequiel reviewed the code on 6 Jul 2015:
> http://www.spinics.net/lists/linux-media/msg91516.html
> 
> On 27 Jul 2015, as you can easily find in your own mail archive, he
> informed me that he's working on the driver and that he's going to post
> updated patches himself. This was holidays time for me and I stated
> that I have to suspend my work till the end of August.
> 
> I asked him to add his changes on top of my code several times (and this
> is really easy with git). This never happened, and on 14 Aug 2015 he
> posted:
> 
>> Problem is I've re-written the driver, taking yours as a starting point
>> and reference.
> 
>> In other words, I don't have a proper git branch with a history, starting
>> from the patch you submitted. Instead, I would be submitting a new
>> patch for Hans and Mauro to review.
> 
> Maybe I got the meaning of this wrong, and he wasn't writing about
> rewriting the driver "from scratch". Yes, after receiving this mail
> I stopped my development, waiting for his driver to show up. That's
> where the "many months" are. Yes, Ezequiel waited for "many months" with
> his version - not me.
> 
> Only after he has posted "his" driver, I could find out what the
> "rewriting" meant.

Thank you for the clarification.

> Don't get me wrong, I was never opposed to him improving my driver.
> I only requested that his contributions are clearly shown, in a form
> of a patch or a patch set (or a git tree etc.), and so are my own.
> I really can't understand why his code can't be transparently applied
> over my original patch (or the updated one, which compiles and works
> fine with recent media tree).
> 
> Is it too much to ask?

When a driver is merged for the first time in the kernel it is always as
a single change, i.e. you don't include the development history as that
would pollute the kernel's history. Those earlier versions have never
been tested/reviewed to the same extent as the final version and adding
them could break git bisect. So as a general rule this isn't done.

> The lesson I learned is thus this instead:
> - don't publish code because it can be hijacked, twisted and you'll
> have to fight for even getting your authorship back. Forget about proper
> attribution and history.

Your code is attributed in the source code and your patches are all archived
in patchwork and on mailinglist archives.

And should you decide to make patches for the merged driver adding back the
lost functionality, then feel free to yell loudly at Ezequiel in the commit
log (within reason, of course :-) ). Heck, I might add some lines of my own
to that.

It is a quite unusual situation and the only way I could make it worse would
by not merging anything.

Regards,

	Hans

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> When a driver is merged for the first time in the kernel it is always as
> a single change, i.e. you don't include the development history as that
> would pollute the kernel's history.

We don't generally add new drivers with long patch series, that's right.
That's because there is usually no reason to do so. This situation is
different, there is a very good reason.

I'm not asking for doing this with a long patch set. A single patch from
me + single patch containing the Ezequiel changes would suffice.

> Those earlier versions have never
> been tested/reviewed to the same extent as the final version

This is not a real problem, given the code will be changed immediately
again.

> and adding
> them could break git bisect.

Not really. You can apply the version based on current media tree,
I have posted it a week ago or so. This doesn't require any effort.

BTW applying the thing in two consecutive patches (the old version +
changes) doesn't break git bisect at all. It only breaks the compiling,
and only in the very case when git bisect happens to stop between the
first and second patch, and the driver is configured in. Very unlikely,
though the remedy is very easy as shown in "man git-bisect".
This is a common thing when you do git bisect, though the related
patches are usually much more distant and thus the probability that the
kernel won't compile is much much greater.

Alternatively one could apply the original version to an older kernel and
merge the product. Less trivial and I don't know why one would want to
do so, given #1.

One could also disable the CONFIG_VIDEO_TW686X in Kconfig (at the
original patch). Possibilities are endless.

We have to face it: there is absolutely no problem with adding the
driver with two patches, either using my original patch or the updated
one. And it doesn't require any effort, just a couple of git commands.

> It is a quite unusual situation and the only way I could make it worse would
> by not merging anything.

Not really.
There is a very easy way out. You can just add the driver properly with
two patches.


Though I don't know why do you think my driver alone doesn't qualify to
be merged (without subsequent Ezequiel's changes). It's functional
and stable, and I have been using it (in various versions) for many
months. It's much more mature than many other drivers which make it to
the kernel regularly.

It is definitely _not_ my driver that is problematic.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: tw686x driver

[Hans Verkuil]

On 03/03/16 13:41, Krzysztof Hałasa wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> When a driver is merged for the first time in the kernel it is always as
>> a single change, i.e. you don't include the development history as that
>> would pollute the kernel's history.
> 
> We don't generally add new drivers with long patch series, that's right.
> That's because there is usually no reason to do so. This situation is
> different, there is a very good reason.
> 
> I'm not asking for doing this with a long patch set. A single patch from
> me + single patch containing the Ezequiel changes would suffice.
> 
>> Those earlier versions have never
>> been tested/reviewed to the same extent as the final version
> 
> This is not a real problem, given the code will be changed immediately
> again.
> 
>> and adding
>> them could break git bisect.
> 
> Not really. You can apply the version based on current media tree,
> I have posted it a week ago or so. This doesn't require any effort.
> 
> BTW applying the thing in two consecutive patches (the old version +
> changes) doesn't break git bisect at all. It only breaks the compiling,
> and only in the very case when git bisect happens to stop between the
> first and second patch, and the driver is configured in. Very unlikely,
> though the remedy is very easy as shown in "man git-bisect".
> This is a common thing when you do git bisect, though the related
> patches are usually much more distant and thus the probability that the
> kernel won't compile is much much greater.
> 
> Alternatively one could apply the original version to an older kernel and
> merge the product. Less trivial and I don't know why one would want to
> do so, given #1.
> 
> One could also disable the CONFIG_VIDEO_TW686X in Kconfig (at the
> original patch). Possibilities are endless.
> 
> We have to face it: there is absolutely no problem with adding the
> driver with two patches, either using my original patch or the updated
> one. And it doesn't require any effort, just a couple of git commands.
> 
>> It is a quite unusual situation and the only way I could make it worse would
>> by not merging anything.
> 
> Not really.
> There is a very easy way out. You can just add the driver properly with
> two patches.
> 
> 
> Though I don't know why do you think my driver alone doesn't qualify to
> be merged (without subsequent Ezequiel's changes). It's functional
> and stable, and I have been using it (in various versions) for many
> months. It's much more mature than many other drivers which make it to
> the kernel regularly.
> 
> It is definitely _not_ my driver that is problematic.
> 

There is no point whatsoever in committing a driver and then replacing it
with another which has a different feature set. I'm not going to do that.

One option that might be much more interesting is to add your driver to
staging with a TODO stating that the field support should be added to
the mainline driver. So the code is there and having it in staging makes
it a bit easier to do the migration. And if nothing is done about it
after 1-2 years, then it is removed again since that indicates that there
is not enough interest in the features specific to your driver version.

I'm not sure if Mauro would go for it, but I think this is a fair option.

Regards,

	Hans

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> There is no point whatsoever in committing a driver and then replacing it
> with another which has a different feature set. I'm not going to do
> that.

Sure, that's why I haven't asked you to do it.
Now there is no another driver, as Ezequiel stated - there is just one
driver.

The point is clear, showing who exactly wrote what.

> One option that might be much more interesting is to add your driver to
> staging with a TODO stating that the field support should be added to
> the mainline driver.

Field mode is one thing. What's a bit more important is that Ezequiel's
changes take away the SG DMA, and basically all DMA. The chip has to use
DMA, but his driver then simply memcpy() all the data to userspace
buffers. This doesn't work on low-power machines.

Staging is meant for completely different situation - for immature,
incomplete code. It has nothing to do with the case.

> I'm not sure if Mauro would go for it, but I think this is a fair option.

I don't expect the situation to be fair to me, anymore.

I also don't want to pursue the legal stuff, copyright laws etc.,
but a quick glance at the COPYING file at the root of the kernel sources
may be helpful:

> 2. You may modify your copy or copies of the Program or any portion
> of it, thus forming a work based on the Program, and copy and
> distribute such modifications or work under the terms of Section 1
> above, provided that you also meet all of these conditions:
>
>     a) You must cause the modified files to carry prominent notices
>     stating that you changed the files and the date of any change.

I don't even ask for that much - I only ask that the single set of
changes from Ezequiel has this very information. This is BTW one of the
reasons we switched to git.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: tw686x driver

[Hans Verkuil]

On 03/03/16 15:22, Krzysztof Hałasa wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> There is no point whatsoever in committing a driver and then replacing it
>> with another which has a different feature set. I'm not going to do
>> that.
> 
> Sure, that's why I haven't asked you to do it.
> Now there is no another driver, as Ezequiel stated - there is just one
> driver.
> 
> The point is clear, showing who exactly wrote what.
> 
>> One option that might be much more interesting is to add your driver to
>> staging with a TODO stating that the field support should be added to
>> the mainline driver.
> 
> Field mode is one thing. What's a bit more important is that Ezequiel's
> changes take away the SG DMA, and basically all DMA. The chip has to use
> DMA, but his driver then simply memcpy() all the data to userspace
> buffers. This doesn't work on low-power machines.
> 
> Staging is meant for completely different situation - for immature,
> incomplete code. It has nothing to do with the case.

It can be for anything that prevents it from being mainlined. It was (still is?)
used for mature android drivers, for example.

> 
>> I'm not sure if Mauro would go for it, but I think this is a fair option.
> 
> I don't expect the situation to be fair to me, anymore.
> 
> I also don't want to pursue the legal stuff, copyright laws etc.,
> but a quick glance at the COPYING file at the root of the kernel sources
> may be helpful:
> 
>> 2. You may modify your copy or copies of the Program or any portion
>> of it, thus forming a work based on the Program, and copy and
>> distribute such modifications or work under the terms of Section 1
>> above, provided that you also meet all of these conditions:
>>
>>     a) You must cause the modified files to carry prominent notices
>>     stating that you changed the files and the date of any change.
> 
> I don't even ask for that much - I only ask that the single set of
> changes from Ezequiel has this very information. This is BTW one of the
> reasons we switched to git.

Ezequiel, can you make a v4 and add a link to the original patch posted by
Krzysztof that you based your code on?

I think that takes care of the provenance.

Regards,

	Hans

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil <hverkuil@xs4all.nl> writes:

>> Staging is meant for completely different situation - for immature,
>> incomplete code. It has nothing to do with the case.
>
> It can be for anything that prevents it from being mainlined. It was (still is?)
> used for mature android drivers, for example.

What is preventing my driver from being mainlined?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: tw686x driver

[Hans Verkuil]

On 03/04/2016 07:11 AM, Krzysztof Hałasa wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>>> Staging is meant for completely different situation - for immature,
>>> incomplete code. It has nothing to do with the case.
>>
>> It can be for anything that prevents it from being mainlined. It was (still is?)
>> used for mature android drivers, for example.
> 
> What is preventing my driver from being mainlined?

I have two drivers with different feature sets. Only one can be active
at a time. I have to make a choice which one I'll take and Ezequiel's
version has functionality (audio, interlaced support) which matches best
with existing v4l applications and the typical use cases. I'm not going
to have two drivers for the same hw in the media subsystem since only
one can be active anyway. My decision, although Mauro can of course decide
otherwise.

I am OK with adding your driver to staging in the hope that someone will
merged the functionalities of the two to make a new and better driver.

Whether that means that Ezequiel's code is merged into yours or vice versa,
I really don't care.

My goal is to provide the end-user with the best experience, and this is
IMHO the best option given the hand I've been dealt.

I ordered a tw6869-based PCIe card so I can do testing myself once it has
arrived.

Regards,

	Hans

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> I have two drivers with different feature sets. Only one can be active
> at a time. I have to make a choice which one I'll take and Ezequiel's
> version has functionality (audio, interlaced support) which matches best
> with existing v4l applications and the typical use cases. I'm not going
> to have two drivers for the same hw in the media subsystem since only
> one can be active anyway. My decision, although Mauro can of course decide
> otherwise.

(BTW my driver supports interlace)

> I am OK with adding your driver to staging in the hope that someone will
> merged the functionalities of the two to make a new and better driver.

Then I don't really understand why there can be two drivers for the same
hw in the tree, but one has to be in "staging".
Staging isn't meant for this. My driver perfectly qualifies for being
merged in the non-staging media directory - doesn't it?

You are right, there can be two drivers in the tree for the same hw,
examples are known. You don't have to make a choice here, though you are
free to do so.

> My goal is to provide the end-user with the best experience, and this is
> IMHO the best option given the hand I've been dealt.

Then, if the moral side of the story can't be maintained, at least do it
legally as required by copyright laws (and the GPL license as well).
Doing so is not a "pollution" of git history, but your responsibility as
a maintainer.


To be honest, I still can't understand why are you afraid of adding
Ezequiel's changes on top of my driver properly. While probably far from
being a pretty changeset, it would make it legal, and this is the thing
that the author, I suppose, is entitled to.
Adding some "link" to a mail archive(?) is not a substitute.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: tw686x driver

[Hans Verkuil]

On 03/04/2016 01:37 PM, Krzysztof Hałasa wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> I have two drivers with different feature sets. Only one can be active
>> at a time. I have to make a choice which one I'll take and Ezequiel's
>> version has functionality (audio, interlaced support) which matches best
>> with existing v4l applications and the typical use cases. I'm not going
>> to have two drivers for the same hw in the media subsystem since only
>> one can be active anyway. My decision, although Mauro can of course decide
>> otherwise.
> 
> (BTW my driver supports interlace)

Sorry, I meant V4L2_FIELD_INTERLACED support. Very few applications support
FIELD_TOP/BOTTOM, let alone SEQ_BT.

> 
>> I am OK with adding your driver to staging in the hope that someone will
>> merged the functionalities of the two to make a new and better driver.
> 
> Then I don't really understand why there can be two drivers for the same
> hw in the tree, but one has to be in "staging".
> Staging isn't meant for this. My driver perfectly qualifies for being
> merged in the non-staging media directory - doesn't it?
> 
> You are right, there can be two drivers in the tree for the same hw,
> examples are known. You don't have to make a choice here, though you are
> free to do so.
> 
>> My goal is to provide the end-user with the best experience, and this is
>> IMHO the best option given the hand I've been dealt.
> 
> Then, if the moral side of the story can't be maintained, at least do it
> legally as required by copyright laws (and the GPL license as well).
> Doing so is not a "pollution" of git history, but your responsibility as
> a maintainer.
> 
> 
> To be honest, I still can't understand why are you afraid of adding
> Ezequiel's changes on top of my driver properly. While probably far from
> being a pretty changeset, it would make it legal, and this is the thing
> that the author, I suppose, is entitled to.
> Adding some "link" to a mail archive(?) is not a substitute.
> 

I don't get it. Getting your driver in staging is much better for you since
your code is in there and can be compiled for those who want to. I'm not
going to add your driver and then replace it with Ezequiel's version.

Heck, if you prefer your driver can be added to staging first, then Ezequiel's
driver commit can directly refer to the staging driver as being derived from it.

I'm not going to put both drivers in drivers/media. The functionalities of
these drivers should be merged. If I put both drivers in drivers/media then
that will never happen given human nature.

Regards,

	Hans

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> Sorry, I meant V4L2_FIELD_INTERLACED support. Very few applications support
> FIELD_TOP/BOTTOM, let alone SEQ_BT.

Well, that's doable, though not in SG mode. It still doesn't require
memcpy() of uncompressed video.

> I don't get it. Getting your driver in staging is much better for you since
> your code is in there and can be compiled for those who want to. I'm not
> going to add your driver and then replace it with Ezequiel's version.

Then simply add my driver and don't replace it.
Face it: Ezequiel's driver adds the audio support, and I guess he can
add this audio code without breaking the existing driver.
I also have (old) audio code for this driver, but it has only been
tested without an actual audio input, so it's not ready for deployment.
I simply don't use audio at the moment.

Otherwise, "his driver" is a regression - it removes critical
functionality, in exchange giving only the V4L2_FIELD_INTERLACED, which
can be easily implemented without breaking the rest.

> Heck, if you prefer your driver can be added to staging first, then Ezequiel's
> driver commit can directly refer to the staging driver as being
> derived from it.

Well, I will have to think about it. Though I wonder - if you do that,
perhaps my next request should be to swap them.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> Heck, if you prefer your driver can be added to staging first, then Ezequiel's
> driver commit can directly refer to the staging driver as being derived from it.

Ok, I guess it's fair enough for me. Would you like me to send a patch
with paths changed to staging/?


However I'm not sure if Greg will like it - staging was meant for code
not otherwise ready for mainline. Not a place for alternate drivers.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: tw686x driver

[Hans Verkuil]

On 03/10/2016 08:16 AM, Krzysztof Hałasa wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> Heck, if you prefer your driver can be added to staging first, then Ezequiel's
>> driver commit can directly refer to the staging driver as being derived from it.
> 
> Ok, I guess it's fair enough for me. Would you like me to send a patch
> with paths changed to staging/?

Yes please!

> However I'm not sure if Greg will like it - staging was meant for code
> not otherwise ready for mainline. Not a place for alternate drivers.

As I said before, it's for anything that is not considered ready for mainline for
whatever reason. In pretty sure Greg is only too happy to see an 'almost ready for
mainline' driver in staging instead of the usual crap :-)

Thanks!

	Hans