Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Tuesday, May 03, 2011 17:03:13 Mauro Carvalho Chehab wrote: > Em 03-05-2011 10:59, Hans Verkuil escreveu: > > On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote: > > > What better non-embedded driver to implement vb2 in than one that doesn't > > yet > > do stream I/O? The risks of breaking anything are much smaller and it would > > be > > a good 'gentle introduction' to vb2. > > The risk is there even on this case: existing applications should work with > vb2. > Also, you're discussing about something that we don't have: there's no vb2 > patches > for cx18 yet. > > > Also, it prevents the unnecessary > > overhead of having to replace videobuf in the future in cx18. > > This overhead already exists, as a vb1 solution is there and there's no vb2 > solution > yet. > > > The problem is no doubt different agendas. You want to have your code > > upstreamed. I want to have code upstreamed that uses the latest frameworks. > > From my side, I'm more concerned if vb2 will really support all memory modes > that > vb1 already supports, on both kernelspace and userspace API's. I'm not > confident > about that yet, and before starting spreading a solution that we don't know > for sure > that it will work on non-embedded devices, with similar or better performance > than vb1, > we need to fully test it with one complete driver, before testing on vb > subsets, > in order to fix architectural design problems (if is there any). Before that, > porting any non-embedded driver to vb2 is premature. > > > And the only way to prove that vb2 works is to use it. Saying "it's > > unproven, > > so let's not use it" is silly. > > Yes, and nobody said otherwise. > > > The right approach IMHO is to implement it in > > new drivers, and ensure that the author(s) of the framework give high > > priority > > to fixing any issues that may surface. > > This is where we diverge: while a "pure api/application compliance" might work > with a new driver, you can't compare performance if you don't have the very > same > driver using vb1 against the same driver using vb2. Even for de-facto API > compliance > tests, if you find something not working with some application and a new > driver, it > is harder to point the fingers if the issue is at VB2 or at the new driver, > as a > new driver is, per definition: NEW. So, it is untested. > > On the other hand, if you take an existing drivers, convert it to VB2, test > it with > some compliant tool and it works, and test with application X and it breaks, > you > know for sure that the error is at VB2. It sounds great, but the problem is that there is little incentive to convert existing drivers to vb2. Take cx18: someone obviously wants to get this code in. So if you require vb2 then there is an incentive to do that work. If the current version is accepted, then the incentive to move to vb2 is gone. If you are lucky you can find developers like Andy who might look at it. Look at how long it took to get rid of V4L1. Or how long it takes to convert drivers to video_ioctl2? Or the control framework? It is much easier to put such requirements on incoming code. Once it is in it can take a very long time to convert code to a newer framework. 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Em 03-05-2011 10:59, Hans Verkuil escreveu: > On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote: > What better non-embedded driver to implement vb2 in than one that doesn't yet > do stream I/O? The risks of breaking anything are much smaller and it would > be > a good 'gentle introduction' to vb2. The risk is there even on this case: existing applications should work with vb2. Also, you're discussing about something that we don't have: there's no vb2 patches for cx18 yet. > Also, it prevents the unnecessary > overhead of having to replace videobuf in the future in cx18. This overhead already exists, as a vb1 solution is there and there's no vb2 solution yet. > The problem is no doubt different agendas. You want to have your code > upstreamed. I want to have code upstreamed that uses the latest frameworks. >From my side, I'm more concerned if vb2 will really support all memory modes >that vb1 already supports, on both kernelspace and userspace API's. I'm not confident about that yet, and before starting spreading a solution that we don't know for sure that it will work on non-embedded devices, with similar or better performance than vb1, we need to fully test it with one complete driver, before testing on vb subsets, in order to fix architectural design problems (if is there any). Before that, porting any non-embedded driver to vb2 is premature. > And the only way to prove that vb2 works is to use it. Saying "it's unproven, > so let's not use it" is silly. Yes, and nobody said otherwise. > The right approach IMHO is to implement it in > new drivers, and ensure that the author(s) of the framework give high > priority > to fixing any issues that may surface. This is where we diverge: while a "pure api/application compliance" might work with a new driver, you can't compare performance if you don't have the very same driver using vb1 against the same driver using vb2. Even for de-facto API compliance tests, if you find something not working with some application and a new driver, it is harder to point the fingers if the issue is at VB2 or at the new driver, as a new driver is, per definition: NEW. So, it is untested. On the other hand, if you take an existing drivers, convert it to VB2, test it with some compliant tool and it works, and test with application X and it breaks, you know for sure that the error is at VB2. > Anyway, converting bttv to vb2 is steadily getting higher on my TODO list. > Unfortunately there is still a large number of other items that are also on > that list. I'd love to have more time for this, and things actually may > improve in the future, but not any time soon :-( > > 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Tuesday 3 May 2011, Hans Verkuil wrote: > On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote: > > Asking us to be the "guinea pig" for this new framework just because > > cx18 is the most recent driver to get a videobuf related patch just > > isn't appropriate. > > I don't get it. > > What better non-embedded driver to implement vb2 in than one that doesn't > yet do stream I/O? The risks of breaking anything are much smaller and it > would be a good 'gentle introduction' to vb2. Also, it prevents the > unnecessary overhead of having to replace videobuf in the future in cx18. > Just for everyone's information; I've been caught up in other issues here, so I'm unlikely to get onto changing videobuf to vb2 in this code in the next week or so. However, if someone else had just enough free time to convert the existing patch for me, I can free up enough time to test with our application and with GStreamer, to confirm that the conversion works adequately. -- Simon Farnsworth Software Engineer ONELAN Limited http://www.onelan.com/ -- 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote: > On Mon, May 2, 2011 at 5:31 PM, Hans Verkuil wrote: > > It's also a good idea if the author of a patch pings the list if there > > has been no feedback after one or two weeks. It's easy to forget patches, > > people can be on vacation, be sick, or in the case of Andy, have a family > > emergency. > > In principle I agree with you, and I was actually surprised to hear it > was merged. That said, what's done is done so we need to focus on > where to go from here. > > >> Likewise, I know there have indeed been cases in the past where code > >> got upstream that caused regressions (in fact, you have personally > >> been responsible for some of these if I recall). > >> > >> Let's not throw the baby out with the bathwater. If there are real > >> structural issues with the patch, then let's get them fixed. But if > >> we're just talking about a few minor "unused variable" type of > >> aesthetic issues, then that shouldn't constitute reverting the commit. > >> Do your review, and if an additional patch is needed with a half > >> dozen removals of dead/unused code, then so be it. > > > > Well, one structural thing I am not at all happy about (but it is Andy's > > call) is that it uses videobuf instead of vb2. Since this patch only deals > > with YUV it shouldn't be hard to use vb2. The problem with videobuf is that > > it violates the V4L2 spec in several places so I would prefer not to use > > videobuf in cx18. If only because converting cx18 to vb2 later will change > > the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is > > something I would like to avoid if possible. > > > > I know that Andy started work on vb2 in cx18 for all stream types (not just > > YUV). I have no idea of the current state of that work. But it might be a > > good starting point to use this patch and convert it to vb2. Later Andy can > > add vb2 support for the other stream types. > > Sure Hans. Let me just dig into my collection of 30+ products and > grab one that has already been converted to VB2 which I can use as a > reference for porting. Should be simple enough... > > cx88: nope > cx23885: nope > cx18: nope > ivtv: nope > em28xx: nope > au0828: nope > pvrusb2: nope > cx231xx: nope > saa7134: nope > saa7164: nope > tm6010: nope > dib0700: nope > bttv: nope > > Oh wait, you mean that there aren't *any* non-embedded drivers that > currently implement VB2? Vivi is the *only* example, and it's not > even real hardware so who knows what issues with the architecture we > might run into? > > And exactly what real-world applications has VB2 been validated > against? Any apps that aren't just a test harness or written by an > SOC vendor making it work against their one piece of embedded > hardware? Any consumer apps? Mplayer? VLC? Kaffeine? tvtime? > XawTV? MeTV? MythTV? > > VB2 may be the future of buffering models and it may actually be > better in the long-run, but if you want to see adoption outside of the > SOC space then you need to prove that it works against real hardware > that isn't an SOC, and demonstrate that it doesn't cause regressions > in real-world applications that people are using today. > > Let's talk about what's going to happen in the real world: the first > guy who actually ports one of the above drivers to VB2 is going to run > into bugs. He's going to have to work with you to shake out those > bugs. And it wouldn't surprise me if it exposes some bugs in some > existing applications, which are going to have to be fixed too. In > the end we'll eventually end up in a better situation, but the cost > will be non-trivial and it will be incurred by people who don't really > give a damn about VB2 since it has little end-user visible benefit. > > If you had ported any of the above drivers to the VB2 framework and > demonstrated that it works with existing applications without > modifications, then I think everybody here would breathe much easier. > But the current state today is "experimental, not implemented in any > consumer products or validated in any real-world usage outside of > SOC". > > Asking us to be the "guinea pig" for this new framework just because > cx18 is the most recent driver to get a videobuf related patch just > isn't appropriate. I don't get it. What better non-embedded driver to implement vb2 in than one that doesn't yet do stream I/O? The risks of breaking anything are much smaller and it would be a good 'gentle introduction' to vb2. Also, it prevents the unnecessary overhead of having to replace videobuf in the future in cx18. The problem is no doubt different agendas. You want to have your code upstreamed. I want to have code upstreamed that uses the latest frameworks. And the only way to prove that vb2 works is to use it. Saying "it's unproven, so let's not use it" is silly. The right approach IMHO is to implement it in new drivers, and ensure that the author(s) of the framework g
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Hi Andy, On Mon, May 2, 2011 at 10:40 PM, Andy Walls wrote: > Hi All, > > Ah crud, what a mess. Where to begin...? > > Where have I been: > > On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing > Fasciitis caused by Invasive Group A Streptococcous - otherwise known as > "Flesh-eating bacteria": > > http://en.wikipedia.org/wiki/Necrotizing_fasciitis > http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/ > > By the grace of God, my son was diagnosed very early. He only lost the > fascia on his left side and one lymph node - damage essentially > unnoticable to anyone, including my son himself. His recovery progress > is excellent and he is now back to his normal life. Yay! \O/ I am very sorry to hear about this, and am happy to hear he is doing better now. My thoughts go out to you and your family. > Naturally, Linux driver development disappeared from my mind during the > extended hospital stay, multiple surgeries, and post-hospitalization > recovery. > > As always; yard-work, house-work, work-work, choir practice, kids' > sports, kids' after school clubs, and kids' instrument lessons also > consume my time. Lest we not forget our relative priorities. LinuxTV isn't saving the world: it's making it easier for people to watch television. I think everyone here would be appalled if your priorities were reversed just for the benefit of fixing TV tuners. > History of this patch: > My thoughts: > > 1. I don't want to stop progress, so I did not NACK this patch. I don't > exactly like the patch either, so I didn't ACK it. I can certainly appreciate this, as I've been in this situation myself more than once. > 2. At a minimum someone needs to review Simon's revised patch that tried > to address my comments. That patch has to be better than this one. > Hans has already noticed a few of the bugs I pointed out to Steven and > Simon. Admittedly it's unfortunate that we didn't know there was a newer version of the patch, and it would definitely be a shame to see Simon's incremental work go to waste. That said, it would be nice to perhaps see the incremental improvements separated out into a separate patch from Steven's original, so we can understand what constitutes the original work versus what were the cleanup/improvements made by Simon. > 3. I value that this patch has been tested, but I am guessing the > use-case was limited. The toughest cx18 use-cases involve a lot of > concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple > boards (3 or 4). I had to do a lot of work with the driver to get that > concurrency reliable and performing well. Has this been tested post-BKL > removal? Have screen sizes other than the full-screen size been tested? Good questions. Simon? > 4. I do not like using videobuf(1) for this. Videobuf(1) is a buggy > dead-end IMO. I will NACK any patch that tries to fix anything due to > videobuf(1) related problems introduced into cx18 by this patch. > There's no point in throwing too much effort into fixing what would > likely be unfixable. I don't think we're trying to make videobuf1 into something it's not. The goal here is feature parity with other VB1 based devices. In fact, it's not even that: it's just being able to meet the userland API requirements related to providing video frames instead of a stream of YUV bytes. > 5. When I am done with my videobuf2 stuff for cx18, I will essentially > revert this one and add in my new implementation after sufficient > testing. Though given the amount of time I have for this, maybe the > last HVR-1600 will be dead before then. I don't see why anyone would have any objection to this provided it doesn't result in any regression in functionality. The only concerns I would have were if the VB2 cutover was submitted before fully baked, resulting in some loss of existing functionality that was considered important to the user base. And of course the more practical concern that it's unclear even by your own admission when/if this would actually happen. > Summary: > > 1. I'm not going to fix any YUV related problems merging this patch > causes. It's the YUV stream of an MPEG capture card that's more > expensive than a simple frame grabber. (I've only heard of it being > used for live play of video games and of course for Simon's > application.) Seems reasonable. To my knowledge nobody has been using the cx18 YUV support anyway other than Simon, which is what prompted his contributions in the first place. In fact in the long term I would be perfectly happy to see /dev/video32 disappear entirely since it just causes confusion for regular users trying to figure out what video device to choose. > 2. I'd at least like Simon's revised patch to be merged instead, to fix > the known deficincies in this one. Agreed. > 3. If merging this patch, means a change to videobuf2 in the future is > not allowed, than I'd prefer to NACK the patch that introduces > videobuf(1) into cx18. I cannot imagi
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Mon, May 2, 2011 at 5:31 PM, Hans Verkuil wrote: > It's also a good idea if the author of a patch pings the list if there > has been no feedback after one or two weeks. It's easy to forget patches, > people can be on vacation, be sick, or in the case of Andy, have a family > emergency. In principle I agree with you, and I was actually surprised to hear it was merged. That said, what's done is done so we need to focus on where to go from here. >> Likewise, I know there have indeed been cases in the past where code >> got upstream that caused regressions (in fact, you have personally >> been responsible for some of these if I recall). >> >> Let's not throw the baby out with the bathwater. If there are real >> structural issues with the patch, then let's get them fixed. But if >> we're just talking about a few minor "unused variable" type of >> aesthetic issues, then that shouldn't constitute reverting the commit. >> Do your review, and if an additional patch is needed with a half >> dozen removals of dead/unused code, then so be it. > > Well, one structural thing I am not at all happy about (but it is Andy's > call) is that it uses videobuf instead of vb2. Since this patch only deals > with YUV it shouldn't be hard to use vb2. The problem with videobuf is that > it violates the V4L2 spec in several places so I would prefer not to use > videobuf in cx18. If only because converting cx18 to vb2 later will change > the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is > something I would like to avoid if possible. > > I know that Andy started work on vb2 in cx18 for all stream types (not just > YUV). I have no idea of the current state of that work. But it might be a > good starting point to use this patch and convert it to vb2. Later Andy can > add vb2 support for the other stream types. Sure Hans. Let me just dig into my collection of 30+ products and grab one that has already been converted to VB2 which I can use as a reference for porting. Should be simple enough... cx88: nope cx23885: nope cx18: nope ivtv: nope em28xx: nope au0828: nope pvrusb2: nope cx231xx: nope saa7134: nope saa7164: nope tm6010: nope dib0700: nope bttv: nope Oh wait, you mean that there aren't *any* non-embedded drivers that currently implement VB2? Vivi is the *only* example, and it's not even real hardware so who knows what issues with the architecture we might run into? And exactly what real-world applications has VB2 been validated against? Any apps that aren't just a test harness or written by an SOC vendor making it work against their one piece of embedded hardware? Any consumer apps? Mplayer? VLC? Kaffeine? tvtime? XawTV? MeTV? MythTV? VB2 may be the future of buffering models and it may actually be better in the long-run, but if you want to see adoption outside of the SOC space then you need to prove that it works against real hardware that isn't an SOC, and demonstrate that it doesn't cause regressions in real-world applications that people are using today. Let's talk about what's going to happen in the real world: the first guy who actually ports one of the above drivers to VB2 is going to run into bugs. He's going to have to work with you to shake out those bugs. And it wouldn't surprise me if it exposes some bugs in some existing applications, which are going to have to be fixed too. In the end we'll eventually end up in a better situation, but the cost will be non-trivial and it will be incurred by people who don't really give a damn about VB2 since it has little end-user visible benefit. If you had ported any of the above drivers to the VB2 framework and demonstrated that it works with existing applications without modifications, then I think everybody here would breathe much easier. But the current state today is "experimental, not implemented in any consumer products or validated in any real-world usage outside of SOC". Asking us to be the "guinea pig" for this new framework just because cx18 is the most recent driver to get a videobuf related patch just isn't appropriate. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Em 03-05-2011 02:15, Hans Verkuil escreveu: > On Tuesday, May 03, 2011 05:28:02 Mauro Carvalho Chehab wrote: >>> 2. I'd at least like Simon's revised patch to be merged instead, to fix >>> the known deficincies in this one. >> >> IMO, the proper workflow would be that Simon should send his changes, as >> a diff patch against the current one. We can all review it, based on the >> comments you sent in priv and fix it. > > I disagree. The proper workflow in this particular instance is to revert the > patch, have Simon post the revised patch to the list and have it reviewed on > the list. > > As Andy noticed, in this particular case the whole procedure was a mess due > to completely understandable reasons. Nobody is to blame, it's just one of > those things that happens. > > Reading through the comments Andy made regarding this patch it is clear to > me that there are too many issues with this patch. > > Anyway, I stand by my NACK. I won't do anything before seeing the new version. Reverting a patch is painful, as it means that I need to take extra care when sending upstream, and I'm having enough headaches with all patchwork issues. I won't do it, except if we can't have this fixed before the end of the next merge window. >> As it seems that that the patch offers a subset of the desired features >> that you're planning with your approach, maybe the better would be to add >> a CONFIG var to enable YUV support, stating that such feature is >> experimental. >> >>> 3. If merging this patch, means a change to videobuf2 in the future is >>> not allowed, than I'd prefer to NACK the patch that introduces >>> videobuf(1) into cx18. >> >> The addition of VB1 first doesn't imply that VB2 would be acked or nacked. >> >> In any case, the first non-embedded VB2 driver will need a very careful >> review, to be sure that they won't break any userspace applications. >> On embedded hardware, only a limited set of applications are supported, and >> they >> are patched and bundled together with the hardware, so there's not much >> concern >> about userspace apps breakage. >> >> However, on non-embedded hardware, we should be sure that no regressions to >> existing applications will happen. So, the better would be if the first VB2 >> non-embedded driver to be a full-featured V4L2 board (e. g. saa7134 or bttv, >> as they support all types of video buffer userspace API's, including overlay >> mode), allowing us to test if VB2 is really following the specs (both the >> "de facto" and "de jure" specs). > > I fail to see why we can't implement vb2 in cx18. Where did I said that? Please, don't understand me wrong, nor put words into my mouth. All I said is that vb2 is not enough tested. > And vb2 has been tested > extensively already with respect to the spec. vivi is using it, and I'm doing > a lot of testing with that driver. There are two specs envolved here: the V4L2 API spec and the by practice spec, used by userspace apps developers when they write their stuff. It is a Linux requirement that Kernel changes should not break existing applications (no regressions). This basically means that the "by practice" spec should not be broken. I'm not saying that vb2 broke it. All I'm saying is that we don't have enough tests. vivi is nice to test new features, but only developers use it, and on a restricted environment. Embedded drivers also use it also on a restricted environment, were just one application is used, and such application is developed (or patched) to work for an specific piece of hardware. I really doubt that, except by people that work with embedded devices, people tried to use vb2 into a real environment. For example, on the early days of videobuf split into vb sub-drivers, kernel OOPSes/Panic's were noticed when channels were changed, because hardware DMA engine restarts on some hardware, and this caused some race conditions. So, before applying vb2 to a driver that will be used by the existing applications, a wide range of tests with the applications are needed. > Note that the current set of drivers behaves different already depending on > whether videobuf is used or not. Drivers like UVC follow the spec, drivers > based on videobuf don't. It's a big freakin' mess. The long term solution is to merge all vb stuff into one solution, and I have good hope that vb2 will be such solution. But before doing that, we need to be sure that vb2 will work with all kinds of situations covered by vb1, uvc-vb, gspca-vb, etc. We'll get there one day, but we should not put the cart before the horse. The proper way of doing it is to do convert a ful-featured v4l2 driver that works fine with vb1 and test it, after its conversion to vb2, if all situations are covered by vb2, and it it works properly with the existing applications, fixing it until it works properly. After having one of such drivers properly working, we can migrate the others to vb2. Doing the opposite way by adding new drivers to vb2 without ev
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Em 03-05-2011 06:03, Simon Farnsworth escreveu: > On Monday 2 May 2011, Mauro Carvalho Chehab wrote: >> Em 02-05-2011 16:11, Hans Verkuil escreveu: >>> NACK. >>> >>> For two reasons: first of all it is not signed off by Andy Walls, the >>> cx18 maintainer. I know he has had other things on his plate recently >>> which is probably why he hasn't had the chance to review this. >>> >>> Secondly, while doing a quick scan myself I noticed that this code does a >>> conversion from UYVY format to YUYV *in the driver*. Format conversion is >>> not allowed in the kernel, we have libv4lconvert for that. So at the >>> minimum this conversion code must be removed first. >> >> Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or >> andy were against it, why none of you commented it there? >> >> Now that the patch were committed, I won't revert it without a very good >> reason. >> >> With respect to the "conversion from UYVY format to YUYV", a simple patch >> could fix it, instead of removing the entire patchset. >> >> Steven/Simon, >> could you please work on such change? >> > I received feedback, which I've been working on, and have converted to a new > patch against the baseline tree without this patch applied. I can obviously > rebase (thanks, git!) so that it applies on top of this patch. It massively > cleans up the code, fixes a bug, and removes the in-kernel format conversion > (we use libv4l here anyway, so it's not needed) > > I have one more work item, requested by Andy and Hans, and that's to convert > just the YUV capture from videobuf to vb2, so that when Andy's got spare time > again, it'll be easier for him to convert the whole driver. I've been delayed > on this by other work committments, but I do have this on my schedule. > > How do you want me to proceed? Please send a rebased version. Thanks, Mauro. -- 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Monday 2 May 2011, Mauro Carvalho Chehab wrote: > Em 02-05-2011 16:11, Hans Verkuil escreveu: > > NACK. > > > > For two reasons: first of all it is not signed off by Andy Walls, the > > cx18 maintainer. I know he has had other things on his plate recently > > which is probably why he hasn't had the chance to review this. > > > > Secondly, while doing a quick scan myself I noticed that this code does a > > conversion from UYVY format to YUYV *in the driver*. Format conversion is > > not allowed in the kernel, we have libv4lconvert for that. So at the > > minimum this conversion code must be removed first. > > Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or > andy were against it, why none of you commented it there? > > Now that the patch were committed, I won't revert it without a very good > reason. > > With respect to the "conversion from UYVY format to YUYV", a simple patch > could fix it, instead of removing the entire patchset. > > Steven/Simon, > could you please work on such change? > I received feedback, which I've been working on, and have converted to a new patch against the baseline tree without this patch applied. I can obviously rebase (thanks, git!) so that it applies on top of this patch. It massively cleans up the code, fixes a bug, and removes the in-kernel format conversion (we use libv4l here anyway, so it's not needed) I have one more work item, requested by Andy and Hans, and that's to convert just the YUV capture from videobuf to vb2, so that when Andy's got spare time again, it'll be easier for him to convert the whole driver. I've been delayed on this by other work committments, but I do have this on my schedule. How do you want me to proceed? -- Simon Farnsworth Software Engineer ONELAN Limited http://www.onelan.com/ -- 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Tuesday, May 03, 2011 05:28:02 Mauro Carvalho Chehab wrote: > Em 02-05-2011 23:40, Andy Walls escreveu: > > Hi All, > > > > Ah crud, what a mess. Where to begin...? > > > > Where have I been: > > > > On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing > > Fasciitis caused by Invasive Group A Streptococcous - otherwise known as > > "Flesh-eating bacteria": > > > > http://en.wikipedia.org/wiki/Necrotizing_fasciitis > > http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/ > > Sorry to hear about that! > > > By the grace of God, my son was diagnosed very early. He only lost the > > fascia on his left side and one lymph node - damage essentially > > unnoticable to anyone, including my son himself. His recovery progress > > is excellent and he is now back to his normal life. Yay! \O/ > > Good! I hope him to fully recover from the disease. All the best for you > and your wife. Our hearts are with you. > > > Naturally, Linux driver development disappeared from my mind during the > > extended hospital stay, multiple surgeries, and post-hospitalization > > recovery. > > > > As always; yard-work, house-work, work-work, choir practice, kids' > > sports, kids' after school clubs, and kids' instrument lessons also > > consume my time. > > > > Completely understandable. > > > History of this patch: > > > > 1. Steven wrote the bulk of it 10 months ago: > > > > http://www.kernellabs.com/hg/~stoth/cx18-videobuf/ > > > > 2. At Steven's request, I took a day and reviewed it on July 10 2010 and > > provide comments off-list. (I will provide them in a follow up to Mauro > > Devin and Hans). > > Thanks! > > Next time, please answer it publicly, or if the patch author submitted > it in priv for a good reason, please c/c me on the review, in order to > warn me that you have some restrictions about a patch. > > > 3. The patch languished as Steven didn't have time to make the fixes and > > neither did I. > > > > 4. Videobuf2 came along as did good documentation on the deficiencies of > > videobuf1: > > > > http://linuxtv.org/downloads/presentations/summit_jun_2010/20100614-v4l2_summit-videobuf.pdf > > http://linuxtv.org/downloads/presentations/summit_jun_2010/Videobuf_Helsinki_June2010.pdf > > http://lwn.net/Articles/415883/ > > > > 5. I started independent work to implement videobuf2 for YUV and > > actually using zero-copy. My progress is very slow. > > > > http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18-vb2-proto > > http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18_39 > > > > 6. Simon submits the patches to the list as one big patch. > > > > 7. Off-list I forward the same 5 emails of comments to Simon as I > > provided in #2 to Steven. > > In this case, as Simon had opened the source code already for the patch, > the better would be if you had made a public statement about your nack. > I always review the ML before applying a patch. > > > 8. Simon addresses most of the comments and provides a revised patch > > off-list asking for review. I haven't had time to look at it. > > > > 9. Mauro commits the original patch that Simon submitted to the list. > > > > > > My thoughts: > > > > 1. I don't want to stop progress, so I did not NACK this patch. I don't > > exactly like the patch either, so I didn't ACK it. > > > > 2. At a minimum someone needs to review Simon's revised patch that tried > > to address my comments. That patch has to be better than this one. > > Hans has already noticed a few of the bugs I pointed out to Steven and > > Simon. > > > > 3. I value that this patch has been tested, but I am guessing the > > use-case was limited. The toughest cx18 use-cases involve a lot of > > concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple > > boards (3 or 4). I had to do a lot of work with the driver to get that > > concurrency reliable and performing well. Has this been tested post-BKL > > removal? Have screen sizes other than the full-screen size been tested? > > > > 4. I do not like using videobuf(1) for this. Videobuf(1) is a buggy > > dead-end IMO. I will NACK any patch that tries to fix anything due to > > videobuf(1) related problems introduced into cx18 by this patch. > > There's no point in throwing too much effort into fixing what would > > likely be unfixable. > > > > 5. When I am done with my videobuf2 stuff for cx18, I will essentially > > revert this one and add in my new implementation after sufficient > > testing. Though given the amount of time I have for this, maybe the > > last HVR-1600 will be dead before then. > > > > > > Summary: > > > > 1. I'm not going to fix any YUV related problems merging this patch > > causes. It's the YUV stream of an MPEG capture card that's more > > expensive than a simple frame grabber. (I've only heard of it being > > used for live play of video games and of course for Simon's > > application.) > > > > 2. I'd at least like Simon's revise
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Em 02-05-2011 23:40, Andy Walls escreveu: > Hi All, > > Ah crud, what a mess. Where to begin...? > > Where have I been: > > On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing > Fasciitis caused by Invasive Group A Streptococcous - otherwise known as > "Flesh-eating bacteria": > > http://en.wikipedia.org/wiki/Necrotizing_fasciitis > http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/ Sorry to hear about that! > By the grace of God, my son was diagnosed very early. He only lost the > fascia on his left side and one lymph node - damage essentially > unnoticable to anyone, including my son himself. His recovery progress > is excellent and he is now back to his normal life. Yay! \O/ Good! I hope him to fully recover from the disease. All the best for you and your wife. Our hearts are with you. > Naturally, Linux driver development disappeared from my mind during the > extended hospital stay, multiple surgeries, and post-hospitalization > recovery. > > As always; yard-work, house-work, work-work, choir practice, kids' > sports, kids' after school clubs, and kids' instrument lessons also > consume my time. > Completely understandable. > History of this patch: > > 1. Steven wrote the bulk of it 10 months ago: > > http://www.kernellabs.com/hg/~stoth/cx18-videobuf/ > > 2. At Steven's request, I took a day and reviewed it on July 10 2010 and > provide comments off-list. (I will provide them in a follow up to Mauro > Devin and Hans). Thanks! Next time, please answer it publicly, or if the patch author submitted it in priv for a good reason, please c/c me on the review, in order to warn me that you have some restrictions about a patch. > 3. The patch languished as Steven didn't have time to make the fixes and > neither did I. > > 4. Videobuf2 came along as did good documentation on the deficiencies of > videobuf1: > > http://linuxtv.org/downloads/presentations/summit_jun_2010/20100614-v4l2_summit-videobuf.pdf > http://linuxtv.org/downloads/presentations/summit_jun_2010/Videobuf_Helsinki_June2010.pdf > http://lwn.net/Articles/415883/ > > 5. I started independent work to implement videobuf2 for YUV and > actually using zero-copy. My progress is very slow. > > http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18-vb2-proto > http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18_39 > > 6. Simon submits the patches to the list as one big patch. > > 7. Off-list I forward the same 5 emails of comments to Simon as I > provided in #2 to Steven. In this case, as Simon had opened the source code already for the patch, the better would be if you had made a public statement about your nack. I always review the ML before applying a patch. > 8. Simon addresses most of the comments and provides a revised patch > off-list asking for review. I haven't had time to look at it. > > 9. Mauro commits the original patch that Simon submitted to the list. > > > My thoughts: > > 1. I don't want to stop progress, so I did not NACK this patch. I don't > exactly like the patch either, so I didn't ACK it. > > 2. At a minimum someone needs to review Simon's revised patch that tried > to address my comments. That patch has to be better than this one. > Hans has already noticed a few of the bugs I pointed out to Steven and > Simon. > > 3. I value that this patch has been tested, but I am guessing the > use-case was limited. The toughest cx18 use-cases involve a lot of > concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple > boards (3 or 4). I had to do a lot of work with the driver to get that > concurrency reliable and performing well. Has this been tested post-BKL > removal? Have screen sizes other than the full-screen size been tested? > > 4. I do not like using videobuf(1) for this. Videobuf(1) is a buggy > dead-end IMO. I will NACK any patch that tries to fix anything due to > videobuf(1) related problems introduced into cx18 by this patch. > There's no point in throwing too much effort into fixing what would > likely be unfixable. > > 5. When I am done with my videobuf2 stuff for cx18, I will essentially > revert this one and add in my new implementation after sufficient > testing. Though given the amount of time I have for this, maybe the > last HVR-1600 will be dead before then. > > > Summary: > > 1. I'm not going to fix any YUV related problems merging this patch > causes. It's the YUV stream of an MPEG capture card that's more > expensive than a simple frame grabber. (I've only heard of it being > used for live play of video games and of course for Simon's > application.) > > 2. I'd at least like Simon's revised patch to be merged instead, to fix > the known deficincies in this one. IMO, the proper workflow would be that Simon should send his changes, as a diff patch against the current one. We can all review it, based on the comments you sent in priv and fix it. As it seems that that the patch offers a
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Hi All, Ah crud, what a mess. Where to begin...? Where have I been: On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing Fasciitis caused by Invasive Group A Streptococcous - otherwise known as "Flesh-eating bacteria": http://en.wikipedia.org/wiki/Necrotizing_fasciitis http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/ By the grace of God, my son was diagnosed very early. He only lost the fascia on his left side and one lymph node - damage essentially unnoticable to anyone, including my son himself. His recovery progress is excellent and he is now back to his normal life. Yay! \O/ Naturally, Linux driver development disappeared from my mind during the extended hospital stay, multiple surgeries, and post-hospitalization recovery. As always; yard-work, house-work, work-work, choir practice, kids' sports, kids' after school clubs, and kids' instrument lessons also consume my time. History of this patch: 1. Steven wrote the bulk of it 10 months ago: http://www.kernellabs.com/hg/~stoth/cx18-videobuf/ 2. At Steven's request, I took a day and reviewed it on July 10 2010 and provide comments off-list. (I will provide them in a follow up to Mauro Devin and Hans). 3. The patch languished as Steven didn't have time to make the fixes and neither did I. 4. Videobuf2 came along as did good documentation on the deficiencies of videobuf1: http://linuxtv.org/downloads/presentations/summit_jun_2010/20100614-v4l2_summit-videobuf.pdf http://linuxtv.org/downloads/presentations/summit_jun_2010/Videobuf_Helsinki_June2010.pdf http://lwn.net/Articles/415883/ 5. I started independent work to implement videobuf2 for YUV and actually using zero-copy. My progress is very slow. http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18-vb2-proto http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18_39 6. Simon submits the patches to the list as one big patch. 7. Off-list I forward the same 5 emails of comments to Simon as I provided in #2 to Steven. 8. Simon addresses most of the comments and provides a revised patch off-list asking for review. I haven't had time to look at it. 9. Mauro commits the original patch that Simon submitted to the list. My thoughts: 1. I don't want to stop progress, so I did not NACK this patch. I don't exactly like the patch either, so I didn't ACK it. 2. At a minimum someone needs to review Simon's revised patch that tried to address my comments. That patch has to be better than this one. Hans has already noticed a few of the bugs I pointed out to Steven and Simon. 3. I value that this patch has been tested, but I am guessing the use-case was limited. The toughest cx18 use-cases involve a lot of concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple boards (3 or 4). I had to do a lot of work with the driver to get that concurrency reliable and performing well. Has this been tested post-BKL removal? Have screen sizes other than the full-screen size been tested? 4. I do not like using videobuf(1) for this. Videobuf(1) is a buggy dead-end IMO. I will NACK any patch that tries to fix anything due to videobuf(1) related problems introduced into cx18 by this patch. There's no point in throwing too much effort into fixing what would likely be unfixable. 5. When I am done with my videobuf2 stuff for cx18, I will essentially revert this one and add in my new implementation after sufficient testing. Though given the amount of time I have for this, maybe the last HVR-1600 will be dead before then. Summary: 1. I'm not going to fix any YUV related problems merging this patch causes. It's the YUV stream of an MPEG capture card that's more expensive than a simple frame grabber. (I've only heard of it being used for live play of video games and of course for Simon's application.) 2. I'd at least like Simon's revised patch to be merged instead, to fix the known deficincies in this one. 3. If merging this patch, means a change to videobuf2 in the future is not allowed, than I'd prefer to NACK the patch that introduces videobuf(1) into cx18. Regards, Andy On Mon, 2011-05-02 at 23:31 +0200, Hans Verkuil wrote: > On Monday, May 02, 2011 22:59:09 Devin Heitmueller wrote: > > On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil wrote: > > > It was merged without *asking* Andy. I know he has had some private stuff > > > to > > > deal with this month so I wasn't surprised that he hadn't reviewed it yet. > > > > > > It would have been nice if he was reminded first of this patch. It's a > > > fairly substantial change that also has user-visible implications. The > > > simple > > > fact is that this patch has not been reviewed and as a former cx18 > > > maintainer > > > I think that it needs a review first. > > > > > > If someone had asked and Andy wouldn't have been able to review, then I'd > > > have > > > jumped in and would have reviewed it. > > > > > > Andy, I hope you can look at it, but if not, then
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Em 02-05-2011 18:31, Hans Verkuil escreveu: > On Monday, May 02, 2011 22:59:09 Devin Heitmueller wrote: >> On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil wrote: >>> It was merged without *asking* Andy. I know he has had some private stuff to >>> deal with this month so I wasn't surprised that he hadn't reviewed it yet. >>> >>> It would have been nice if he was reminded first of this patch. It's a >>> fairly substantial change that also has user-visible implications. The >>> simple >>> fact is that this patch has not been reviewed and as a former cx18 >>> maintainer >>> I think that it needs a review first. >>> >>> If someone had asked and Andy wouldn't have been able to review, then I'd >>> have >>> jumped in and would have reviewed it. >>> >>> Andy, I hope you can look at it, but if not, then let me know and I'll do a >>> more in-depth review rather than just the simple scan I did now. >>> Now that the patch were committed, I won't revert it without a very good reason. With respect to the "conversion from UYVY format to YUYV", a simple patch could fix it, instead of removing the entire patchset. >>> >>> No, please remove the patchset because I have found two other issues: >>> >>> The patch adds this field: >>> >>>struct v4l2_framebuffer fbuf; >>> >>> This is not needed, videobuf_iolock can be called with a NULL pointer >>> instead >>> of &fbuf. >>> >>> The patch also adds tvnorm fields, but never sets s->tvnorm. And it's >>> pointless anyway since you can't change tvnorm while streaming. >>> >>> Given that I've found three things now without even trying suggests to me >>> that >>> it is too soon to commit this. Sorry. >>> >>> Regards, >>> >>>Hans >> >> Indeed comments/review are always welcome, although it would have been >> great if it had happened a month ago. It's the maintainer's >> responsibility to review patches, and if he has issues to raise them >> in a timely manner. If he doesn't care enough or is too busy to >> publicly say "hold off on this" for whatever reason, then you can >> hardly blame Mauro for merging it. > > It's also a good idea if the author of a patch pings the list if there > has been no feedback after one or two weeks. It's easy to forget patches, > people can be on vacation, be sick, or in the case of Andy, have a family > emergency. Do you know if/when he will be available to discuss about it? >> Likewise, I know there have indeed been cases in the past where code >> got upstream that caused regressions (in fact, you have personally >> been responsible for some of these if I recall). >> >> Let's not throw the baby out with the bathwater. If there are real >> structural issues with the patch, then let's get them fixed. But if >> we're just talking about a few minor "unused variable" type of >> aesthetic issues, then that shouldn't constitute reverting the commit. >> Do your review, and if an additional patch is needed with a half >> dozen removals of dead/unused code, then so be it. > > Well, one structural thing I am not at all happy about (but it is Andy's > call) is that it uses videobuf instead of vb2. Since this patch only deals > with YUV it shouldn't be hard to use vb2. The problem with videobuf is that > it violates the V4L2 spec in several places so I would prefer not to use > videobuf in cx18. If only because converting cx18 to vb2 later will change > the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is > something I would like to avoid if possible. Userspace applications works fine with vb1. Even if vb1 is not fully v4l2 compliant, it is a de-facto standard[1]. In other words, any change of behaviour for stream I/O controls that would cause applications that rely on vb1 way to break should be reverted on vb2, as otherwise it would be a regression. I'm assuming that this is not the case for vb2. However, as I was not able to test it yet, as the saa7134 port were incomplete, and didn't work, and I was not able yet to test a stream with a Samsung device, partially due to my lack of time, I'm not sure if all relevant applications work properly with vb2. > I know that Andy started work on vb2 in cx18 for all stream types (not just > YUV). I have no idea of the current state of that work. But it might be a > good starting point to use this patch and convert it to vb2. Later Andy can > add vb2 support for the other stream types. When he's done with his work, it is only a matter of replacing one code by the other. >> We're not talking about an untested board profile submitted by some >> random user. We're talking about a patch written by someone highly >> familiar with the chipset and it's *working code* that has been >> running in production for almost a year. > > It's not about that, it's about merging something substantial without the SoB > of the maintainer and without asking the maintainer. > > I'm not blaming anyone, it's just a miscommunication. What should happen with > t
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Monday, May 02, 2011 22:59:09 Devin Heitmueller wrote: > On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil wrote: > > It was merged without *asking* Andy. I know he has had some private stuff to > > deal with this month so I wasn't surprised that he hadn't reviewed it yet. > > > > It would have been nice if he was reminded first of this patch. It's a > > fairly substantial change that also has user-visible implications. The > > simple > > fact is that this patch has not been reviewed and as a former cx18 > > maintainer > > I think that it needs a review first. > > > > If someone had asked and Andy wouldn't have been able to review, then I'd > > have > > jumped in and would have reviewed it. > > > > Andy, I hope you can look at it, but if not, then let me know and I'll do a > > more in-depth review rather than just the simple scan I did now. > > > >> Now that the patch were committed, I won't revert it without a very good > >> reason. > >> > >> With respect to the "conversion from UYVY format to YUYV", a simple patch > >> could > >> fix it, instead of removing the entire patchset. > > > > No, please remove the patchset because I have found two other issues: > > > > The patch adds this field: > > > >struct v4l2_framebuffer fbuf; > > > > This is not needed, videobuf_iolock can be called with a NULL pointer > > instead > > of &fbuf. > > > > The patch also adds tvnorm fields, but never sets s->tvnorm. And it's > > pointless anyway since you can't change tvnorm while streaming. > > > > Given that I've found three things now without even trying suggests to me > > that > > it is too soon to commit this. Sorry. > > > > Regards, > > > >Hans > > Indeed comments/review are always welcome, although it would have been > great if it had happened a month ago. It's the maintainer's > responsibility to review patches, and if he has issues to raise them > in a timely manner. If he doesn't care enough or is too busy to > publicly say "hold off on this" for whatever reason, then you can > hardly blame Mauro for merging it. It's also a good idea if the author of a patch pings the list if there has been no feedback after one or two weeks. It's easy to forget patches, people can be on vacation, be sick, or in the case of Andy, have a family emergency. > Likewise, I know there have indeed been cases in the past where code > got upstream that caused regressions (in fact, you have personally > been responsible for some of these if I recall). > > Let's not throw the baby out with the bathwater. If there are real > structural issues with the patch, then let's get them fixed. But if > we're just talking about a few minor "unused variable" type of > aesthetic issues, then that shouldn't constitute reverting the commit. > Do your review, and if an additional patch is needed with a half > dozen removals of dead/unused code, then so be it. Well, one structural thing I am not at all happy about (but it is Andy's call) is that it uses videobuf instead of vb2. Since this patch only deals with YUV it shouldn't be hard to use vb2. The problem with videobuf is that it violates the V4L2 spec in several places so I would prefer not to use videobuf in cx18. If only because converting cx18 to vb2 later will change the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is something I would like to avoid if possible. I know that Andy started work on vb2 in cx18 for all stream types (not just YUV). I have no idea of the current state of that work. But it might be a good starting point to use this patch and convert it to vb2. Later Andy can add vb2 support for the other stream types. > We're not talking about an untested board profile submitted by some > random user. We're talking about a patch written by someone highly > familiar with the chipset and it's *working code* that has been > running in production for almost a year. It's not about that, it's about merging something substantial without the SoB of the maintainer and without asking the maintainer. I'm not blaming anyone, it's just a miscommunication. What should happen with this patch is up to Andy. 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil wrote: > It was merged without *asking* Andy. I know he has had some private stuff to > deal with this month so I wasn't surprised that he hadn't reviewed it yet. > > It would have been nice if he was reminded first of this patch. It's a > fairly substantial change that also has user-visible implications. The simple > fact is that this patch has not been reviewed and as a former cx18 maintainer > I think that it needs a review first. > > If someone had asked and Andy wouldn't have been able to review, then I'd have > jumped in and would have reviewed it. > > Andy, I hope you can look at it, but if not, then let me know and I'll do a > more in-depth review rather than just the simple scan I did now. > >> Now that the patch were committed, I won't revert it without a very good >> reason. >> >> With respect to the "conversion from UYVY format to YUYV", a simple patch >> could >> fix it, instead of removing the entire patchset. > > No, please remove the patchset because I have found two other issues: > > The patch adds this field: > > struct v4l2_framebuffer fbuf; > > This is not needed, videobuf_iolock can be called with a NULL pointer instead > of &fbuf. > > The patch also adds tvnorm fields, but never sets s->tvnorm. And it's > pointless anyway since you can't change tvnorm while streaming. > > Given that I've found three things now without even trying suggests to me that > it is too soon to commit this. Sorry. > > Regards, > > Hans Indeed comments/review are always welcome, although it would have been great if it had happened a month ago. It's the maintainer's responsibility to review patches, and if he has issues to raise them in a timely manner. If he doesn't care enough or is too busy to publicly say "hold off on this" for whatever reason, then you can hardly blame Mauro for merging it. Likewise, I know there have indeed been cases in the past where code got upstream that caused regressions (in fact, you have personally been responsible for some of these if I recall). Let's not throw the baby out with the bathwater. If there are real structural issues with the patch, then let's get them fixed. But if we're just talking about a few minor "unused variable" type of aesthetic issues, then that shouldn't constitute reverting the commit. Do your review, and if an additional patch is needed with a half dozen removals of dead/unused code, then so be it. We're not talking about an untested board profile submitted by some random user. We're talking about a patch written by someone highly familiar with the chipset and it's *working code* that has been running in production for almost a year. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Monday, May 02, 2011 21:35:45 Mauro Carvalho Chehab wrote:
> Em 02-05-2011 16:11, Hans Verkuil escreveu:
> > NACK.
> >
> > For two reasons: first of all it is not signed off by Andy Walls, the cx18
> > maintainer. I know he has had other things on his plate recently which is
> > probably why he hasn't had the chance to review this.
> >
> > Secondly, while doing a quick scan myself I noticed that this code does a
> > conversion from UYVY format to YUYV *in the driver*. Format conversion is
> > not allowed in the kernel, we have libv4lconvert for that. So at the minimum
> > this conversion code must be removed first.
>
> Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or
> andy were against it, why none of you commented it there?
It was merged without *asking* Andy. I know he has had some private stuff to
deal with this month so I wasn't surprised that he hadn't reviewed it yet.
It would have been nice if he was reminded first of this patch. It's a
fairly substantial change that also has user-visible implications. The simple
fact is that this patch has not been reviewed and as a former cx18 maintainer
I think that it needs a review first.
If someone had asked and Andy wouldn't have been able to review, then I'd have
jumped in and would have reviewed it.
Andy, I hope you can look at it, but if not, then let me know and I'll do a
more in-depth review rather than just the simple scan I did now.
> Now that the patch were committed, I won't revert it without a very good
> reason.
>
> With respect to the "conversion from UYVY format to YUYV", a simple patch
> could
> fix it, instead of removing the entire patchset.
No, please remove the patchset because I have found two other issues:
The patch adds this field:
struct v4l2_framebuffer fbuf;
This is not needed, videobuf_iolock can be called with a NULL pointer instead
of &fbuf.
The patch also adds tvnorm fields, but never sets s->tvnorm. And it's
pointless anyway since you can't change tvnorm while streaming.
Given that I've found three things now without even trying suggests to me that
it is too soon to commit this. Sorry.
Regards,
Hans
>
> Thanks,
> Mauro.
>
> >
> > Regards,
> >
> > Hans
> >
> > On Monday, May 02, 2011 19:17:57 Mauro Carvalho Chehab wrote:
> >> This is an automatic generated email to let you know that the following
> >> patch were queued at the
> >> http://git.linuxtv.org/media_tree.git tree:
> >>
> >> Subject: [media] cx18: mmap() support for raw YUV video capture
> >> Author: Steven Toth
> >> Date:Wed Apr 6 08:32:56 2011 -0300
> >>
> >> Add support for mmap method streaming of raw YUV video on cx18-based
> >> hardware, in addition to the existing support for read() streaming of
> >> raw YUV and MPEG-2 encoded video.
> >>
> >> [simon.farnswo...@onelan.co.uk: I forward-ported this from Steven's
> >> original work,
> >> done under contract to ONELAN. The original code is at
> >> http://www.kernellabs.com/hg/~stoth/cx18-videobuf]
> >>
> >> Signed-off-by: Steven Toth
> >> Signed-off-by: Simon Farnsworth
> >> Signed-off-by: Mauro Carvalho Chehab
> >>
> >> drivers/media/video/cx18/Kconfig|2 +
> >> drivers/media/video/cx18/cx18-driver.h | 25
> >> drivers/media/video/cx18/cx18-fileops.c | 214
> >> +++
> >> drivers/media/video/cx18/cx18-fileops.h |2 +
> >> drivers/media/video/cx18/cx18-ioctl.c | 136 ++--
> >> drivers/media/video/cx18/cx18-mailbox.c | 70 ++
> >> drivers/media/video/cx18/cx18-streams.c | 23
> >> drivers/media/video/cx18/cx23418.h |6 +
> >> 8 files changed, 466 insertions(+), 12 deletions(-)
> >>
> >> ---
> >>
> >> http://git.linuxtv.org/media_tree.git?a=commitdiff;h=fa8f1381764d83222333cb67b8d93b9cb1605bf3
> >>
> >> diff --git a/drivers/media/video/cx18/Kconfig
> >> b/drivers/media/video/cx18/Kconfig
> >> index d788ad6..9c23202 100644
> >> --- a/drivers/media/video/cx18/Kconfig
> >> +++ b/drivers/media/video/cx18/Kconfig
> >> @@ -2,6 +2,8 @@ config VIDEO_CX18
> >>tristate "Conexant cx23418 MPEG encoder support"
> >>depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
> >>select I2C_ALGOBIT
> >> + select VIDEOBUF_DVB
> >> + select VIDEOBUF_VMALLOC
> >>depends on RC_CORE
> >>select VIDEO_TUNER
> >>select VIDEO_TVEEPROM
> >> diff --git a/drivers/media/video/cx18/cx18-driver.h
> >> b/drivers/media/video/cx18/cx18-driver.h
> >> index b86a740..70e1e04 100644
> >> --- a/drivers/media/video/cx18/cx18-driver.h
> >> +++ b/drivers/media/video/cx18/cx18-driver.h
> >> @@ -65,6 +65,10 @@
> >> #include "dvb_net.h"
> >> #include "dvbdev.h"
> >>
> >> +/* Videobuf / YUV support */
> >> +#include
> >> +#include
> >> +
> >> #ifndef CONFIG_PCI
> >> # error "This driver requires kernel PCI support."
> >> #endif
> >> @@ -403,6 +407,23 @@ struct cx18_stream {
> >>struct cx18_queue q_idle; /* idle - not in rotation
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Mon, May 2, 2011 at 3:35 PM, Mauro Carvalho Chehab wrote: > Em 02-05-2011 16:11, Hans Verkuil escreveu: >> NACK. >> >> For two reasons: first of all it is not signed off by Andy Walls, the cx18 >> maintainer. I know he has had other things on his plate recently which is >> probably why he hasn't had the chance to review this. >> >> Secondly, while doing a quick scan myself I noticed that this code does a >> conversion from UYVY format to YUYV *in the driver*. Format conversion is >> not allowed in the kernel, we have libv4lconvert for that. So at the minimum >> this conversion code must be removed first. > > Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or > andy were against it, why none of you commented it there? > > Now that the patch were committed, I won't revert it without a very good > reason. > > With respect to the "conversion from UYVY format to YUYV", a simple patch > could > fix it, instead of removing the entire patchset. > > Steven/Simon, > could you please work on such change? Simon, If you're willing to do a bit of work to actually prepare the patch and test the results, I can walk you through pretty much exactly what needs to change (basically you just need to remove one block of code and change a #define). Steven has been really busy with other stuff, so I don't think we should count on his participation in this process. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Em 02-05-2011 16:11, Hans Verkuil escreveu:
> NACK.
>
> For two reasons: first of all it is not signed off by Andy Walls, the cx18
> maintainer. I know he has had other things on his plate recently which is
> probably why he hasn't had the chance to review this.
>
> Secondly, while doing a quick scan myself I noticed that this code does a
> conversion from UYVY format to YUYV *in the driver*. Format conversion is
> not allowed in the kernel, we have libv4lconvert for that. So at the minimum
> this conversion code must be removed first.
Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or
andy were against it, why none of you commented it there?
Now that the patch were committed, I won't revert it without a very good reason.
With respect to the "conversion from UYVY format to YUYV", a simple patch could
fix it, instead of removing the entire patchset.
Steven/Simon,
could you please work on such change?
Thanks,
Mauro.
>
> Regards,
>
> Hans
>
> On Monday, May 02, 2011 19:17:57 Mauro Carvalho Chehab wrote:
>> This is an automatic generated email to let you know that the following
>> patch were queued at the
>> http://git.linuxtv.org/media_tree.git tree:
>>
>> Subject: [media] cx18: mmap() support for raw YUV video capture
>> Author: Steven Toth
>> Date:Wed Apr 6 08:32:56 2011 -0300
>>
>> Add support for mmap method streaming of raw YUV video on cx18-based
>> hardware, in addition to the existing support for read() streaming of
>> raw YUV and MPEG-2 encoded video.
>>
>> [simon.farnswo...@onelan.co.uk: I forward-ported this from Steven's original
>> work,
>> done under contract to ONELAN. The original code is at
>> http://www.kernellabs.com/hg/~stoth/cx18-videobuf]
>>
>> Signed-off-by: Steven Toth
>> Signed-off-by: Simon Farnsworth
>> Signed-off-by: Mauro Carvalho Chehab
>>
>> drivers/media/video/cx18/Kconfig|2 +
>> drivers/media/video/cx18/cx18-driver.h | 25
>> drivers/media/video/cx18/cx18-fileops.c | 214
>> +++
>> drivers/media/video/cx18/cx18-fileops.h |2 +
>> drivers/media/video/cx18/cx18-ioctl.c | 136 ++--
>> drivers/media/video/cx18/cx18-mailbox.c | 70 ++
>> drivers/media/video/cx18/cx18-streams.c | 23
>> drivers/media/video/cx18/cx23418.h |6 +
>> 8 files changed, 466 insertions(+), 12 deletions(-)
>>
>> ---
>>
>> http://git.linuxtv.org/media_tree.git?a=commitdiff;h=fa8f1381764d83222333cb67b8d93b9cb1605bf3
>>
>> diff --git a/drivers/media/video/cx18/Kconfig
>> b/drivers/media/video/cx18/Kconfig
>> index d788ad6..9c23202 100644
>> --- a/drivers/media/video/cx18/Kconfig
>> +++ b/drivers/media/video/cx18/Kconfig
>> @@ -2,6 +2,8 @@ config VIDEO_CX18
>> tristate "Conexant cx23418 MPEG encoder support"
>> depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
>> select I2C_ALGOBIT
>> +select VIDEOBUF_DVB
>> +select VIDEOBUF_VMALLOC
>> depends on RC_CORE
>> select VIDEO_TUNER
>> select VIDEO_TVEEPROM
>> diff --git a/drivers/media/video/cx18/cx18-driver.h
>> b/drivers/media/video/cx18/cx18-driver.h
>> index b86a740..70e1e04 100644
>> --- a/drivers/media/video/cx18/cx18-driver.h
>> +++ b/drivers/media/video/cx18/cx18-driver.h
>> @@ -65,6 +65,10 @@
>> #include "dvb_net.h"
>> #include "dvbdev.h"
>>
>> +/* Videobuf / YUV support */
>> +#include
>> +#include
>> +
>> #ifndef CONFIG_PCI
>> # error "This driver requires kernel PCI support."
>> #endif
>> @@ -403,6 +407,23 @@ struct cx18_stream {
>> struct cx18_queue q_idle; /* idle - not in rotation */
>>
>> struct work_struct out_work_order;
>> +
>> +/* Videobuf for YUV video */
>> +u32 pixelformat;
>> +struct list_head vb_capture;/* video capture queue */
>> +spinlock_t vb_lock;
>> +struct v4l2_framebuffer fbuf;
>> +v4l2_std_id tvnorm; /* selected tv norm */
>> +struct timer_list vb_timeout;
>> +int vbwidth;
>> +int vbheight;
>> +};
>> +
>> +struct cx18_videobuf_buffer {
>> +/* Common video buffer sub-system struct */
>> +struct videobuf_buffer vb;
>> +v4l2_std_id tvnorm; /* selected tv norm */
>> +u32 bytes_used;
>> };
>>
>> struct cx18_open_id {
>> @@ -410,6 +431,10 @@ struct cx18_open_id {
>> u32 open_id;
>> int type;
>> struct cx18 *cx;
>> +
>> +struct videobuf_queue vbuf_q;
>> +spinlock_t s_lock; /* Protect vbuf_q */
>> +enum v4l2_buf_type vb_type;
>> };
>>
>> static inline struct cx18_open_id *fh2id(struct v4l2_fh *fh)
>> diff --git a/drivers/media/video/cx18/cx18-fileops.c
>> b/drivers/media/video/cx18/cx18-fileops.c
>> index e9802d9..c74eafd 100644
>> --- a/drivers/media/video/cx18/cx18-fileops.c
>> +++ b/drivers/media/video/cx18/cx18-fileops.c
>> @@ -597,6 +597,13 @@ ssize_t cx18_v4l2_read(struct file *filp, char __user
>> *buf, size_t count,
>> mutex_unlock(&cx->serialize_lock);
>> if (rc)
>> return rc;
>> +
>>
Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
On Mon, May 2, 2011 at 3:11 PM, Hans Verkuil wrote: > NACK. > > For two reasons: first of all it is not signed off by Andy Walls, the cx18 > maintainer. I know he has had other things on his plate recently which is > probably why he hasn't had the chance to review this. > > Secondly, while doing a quick scan myself I noticed that this code does a > conversion from UYVY format to YUYV *in the driver*. Format conversion is > not allowed in the kernel, we have libv4lconvert for that. So at the minimum > this conversion code must be removed first. Hi Hans, Cutting the code that does UYVY to YUYV shouldn't be a problem, since there are other devices which only support UYVY and thus applications do support the format (the HVR-950q for one). Should just need to remove the offending code block and adjust the advertised formats list. That said, Andy hasn't provided any feedback onlist at all, which is a bit disconcerting (and probably calls for "why won't Andy comment?" instead of an arbitrary NACK). I did speak to Andy about this patch series several months ago, and he was generally not in favor of it because he was planning on converting to videobuf2. While I agree this would be good in the long term, this patch provides a great deal of value in the meantime, and I've always been a fan of the notion that "perfect is the enemy of good". Who knows when we'll actually see a videobuf2 conversion, and this patch doesn't really prevent any of that from happening. I would hate to see yet another situation where a solution stays out-of-tree for years because of some totally awesome better approach which might possibly get integrated at some unknown point in the future. In other words, let's get this merged in (sans the UYVY/YUYV conversion), and if/when Andy eventually does a videobuf2 conversion, then we will all rejoice. Actually, nobody except us driver developers will rejoice since it's an internal architecture change which provides no user-visible value. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
NACK.
For two reasons: first of all it is not signed off by Andy Walls, the cx18
maintainer. I know he has had other things on his plate recently which is
probably why he hasn't had the chance to review this.
Secondly, while doing a quick scan myself I noticed that this code does a
conversion from UYVY format to YUYV *in the driver*. Format conversion is
not allowed in the kernel, we have libv4lconvert for that. So at the minimum
this conversion code must be removed first.
Regards,
Hans
On Monday, May 02, 2011 19:17:57 Mauro Carvalho Chehab wrote:
> This is an automatic generated email to let you know that the following patch
> were queued at the
> http://git.linuxtv.org/media_tree.git tree:
>
> Subject: [media] cx18: mmap() support for raw YUV video capture
> Author: Steven Toth
> Date:Wed Apr 6 08:32:56 2011 -0300
>
> Add support for mmap method streaming of raw YUV video on cx18-based
> hardware, in addition to the existing support for read() streaming of
> raw YUV and MPEG-2 encoded video.
>
> [simon.farnswo...@onelan.co.uk: I forward-ported this from Steven's original
> work,
> done under contract to ONELAN. The original code is at
> http://www.kernellabs.com/hg/~stoth/cx18-videobuf]
>
> Signed-off-by: Steven Toth
> Signed-off-by: Simon Farnsworth
> Signed-off-by: Mauro Carvalho Chehab
>
> drivers/media/video/cx18/Kconfig|2 +
> drivers/media/video/cx18/cx18-driver.h | 25
> drivers/media/video/cx18/cx18-fileops.c | 214
> +++
> drivers/media/video/cx18/cx18-fileops.h |2 +
> drivers/media/video/cx18/cx18-ioctl.c | 136 ++--
> drivers/media/video/cx18/cx18-mailbox.c | 70 ++
> drivers/media/video/cx18/cx18-streams.c | 23
> drivers/media/video/cx18/cx23418.h |6 +
> 8 files changed, 466 insertions(+), 12 deletions(-)
>
> ---
>
> http://git.linuxtv.org/media_tree.git?a=commitdiff;h=fa8f1381764d83222333cb67b8d93b9cb1605bf3
>
> diff --git a/drivers/media/video/cx18/Kconfig
> b/drivers/media/video/cx18/Kconfig
> index d788ad6..9c23202 100644
> --- a/drivers/media/video/cx18/Kconfig
> +++ b/drivers/media/video/cx18/Kconfig
> @@ -2,6 +2,8 @@ config VIDEO_CX18
> tristate "Conexant cx23418 MPEG encoder support"
> depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
> select I2C_ALGOBIT
> + select VIDEOBUF_DVB
> + select VIDEOBUF_VMALLOC
> depends on RC_CORE
> select VIDEO_TUNER
> select VIDEO_TVEEPROM
> diff --git a/drivers/media/video/cx18/cx18-driver.h
> b/drivers/media/video/cx18/cx18-driver.h
> index b86a740..70e1e04 100644
> --- a/drivers/media/video/cx18/cx18-driver.h
> +++ b/drivers/media/video/cx18/cx18-driver.h
> @@ -65,6 +65,10 @@
> #include "dvb_net.h"
> #include "dvbdev.h"
>
> +/* Videobuf / YUV support */
> +#include
> +#include
> +
> #ifndef CONFIG_PCI
> # error "This driver requires kernel PCI support."
> #endif
> @@ -403,6 +407,23 @@ struct cx18_stream {
> struct cx18_queue q_idle; /* idle - not in rotation */
>
> struct work_struct out_work_order;
> +
> + /* Videobuf for YUV video */
> + u32 pixelformat;
> + struct list_head vb_capture;/* video capture queue */
> + spinlock_t vb_lock;
> + struct v4l2_framebuffer fbuf;
> + v4l2_std_id tvnorm; /* selected tv norm */
> + struct timer_list vb_timeout;
> + int vbwidth;
> + int vbheight;
> +};
> +
> +struct cx18_videobuf_buffer {
> + /* Common video buffer sub-system struct */
> + struct videobuf_buffer vb;
> + v4l2_std_id tvnorm; /* selected tv norm */
> + u32 bytes_used;
> };
>
> struct cx18_open_id {
> @@ -410,6 +431,10 @@ struct cx18_open_id {
> u32 open_id;
> int type;
> struct cx18 *cx;
> +
> + struct videobuf_queue vbuf_q;
> + spinlock_t s_lock; /* Protect vbuf_q */
> + enum v4l2_buf_type vb_type;
> };
>
> static inline struct cx18_open_id *fh2id(struct v4l2_fh *fh)
> diff --git a/drivers/media/video/cx18/cx18-fileops.c
> b/drivers/media/video/cx18/cx18-fileops.c
> index e9802d9..c74eafd 100644
> --- a/drivers/media/video/cx18/cx18-fileops.c
> +++ b/drivers/media/video/cx18/cx18-fileops.c
> @@ -597,6 +597,13 @@ ssize_t cx18_v4l2_read(struct file *filp, char __user
> *buf, size_t count,
> mutex_unlock(&cx->serialize_lock);
> if (rc)
> return rc;
> +
> + if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> + (id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> + return videobuf_read_stream(&id->vbuf_q, buf, count, pos, 0,
> + filp->f_flags & O_NONBLOCK);
> + }
> +
> return cx18_read_pos(s, buf, count, pos, filp->f_flags & O_NONBLOCK);
> }
>
> @@ -622,6 +629,11 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp,
> poll_table *wait)
> CX18_DEBUG_FILE("Encoder poll started capture\n");
> }
>
> + if ((id->vb_type == V4L2_BUF_TYPE
