Re: tw686x driver
On 03/10/2016 08:16 AM, Krzysztof Hałasa wrote: > Hans Verkuilwrites: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
Hans Verkuilwrites: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
Hans Verkuilwrites: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
On 03/04/2016 01:37 PM, Krzysztof Hałasa wrote: > Hans Verkuilwrites: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
Hans Verkuilwrites: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
On 03/04/2016 07:11 AM, Krzysztof Hałasa wrote: > Hans Verkuilwrites: > >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
Hans Verkuilwrites: >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
On 03/03/16 15:22, Krzysztof Hałasa wrote: > Hans Verkuilwrites: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
Hans Verkuilwrites: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
On 03/03/16 13:41, Krzysztof Hałasa wrote: > Hans Verkuilwrites: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
Hans Verkuilwrites: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
On 03/03/2016 07:51 AM, Krzysztof Hałasa wrote: > Hi Hans, > > Hans Verkuilwrites: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: tw686x driver
Hi Hans, Hans Verkuilwrites: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html