Re: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Mauro Carvalho Chehab writes: > Em 01-07-2010 08:46, Bjørn Mork escreveu: >> Any chance of a new status update anytime soon? > > Updated today, after two or three weeks spent to handle the backlog. Great! Thanks. It's really appreciated, and I do note that it made quite a few people finally ack/nak the patches they were supposed to review. >> I'm particularily >> interested in getting a forced status change on any patch which was >> "under review" at the time of the last status message. I believe it's >> reasonable to expect two months "review" to be more than enough. If >> the patches are found unacceptable, then it's much better to have them >> rejected with a "please fix foo and resubmit" than the current total >> silence called "review". > > The patches marked as under review means that I'm expecting an action > from someone else (the patch author or the driver author/maintainer). Well, I'm of course not in a position to tell you how to do your job, so please regard this as a humble suggestion only... But I believe you make your job much harder by defining a number of "unofficial" driver maintainers and giving them indefinite slack, while at the same time *you* are the one having to keep track of all their outstanding patches. Either you delegate the maintainance properly, documenting it in MAINTAINERS and pointing there whenever someone sends a patch directly to you, or you might as well just do the ack/nak yourself based on the mailing list feedback. Putting yourself in the middle, taking the patch queue responsibility, but not the ack/nak responsibility, is just wasting your time on accounting and other boring work... I do believe that having the original author(s) maintain a driver is a very good idea as long as they are still actively maintaining it. But this must be based on actual maintainance, and not some misunderstood "ownership based on previous contributions". That's what the CREDITS file is for. Please look at other subsystems with a large number of old drivers, like e.g. networking. It's not like it's possible to have every tiny patch approved by the original author all the time. This does not hinder some newer drivers having very active official maintainers, like the Intel e1000(e) drivers, nor does it hinder the original authors from participating on the mailing list giving their comments and ack/nak if they want. But if they don't respond on the list, davem will just make a decision for himself without waiting for it. > So, if you have patches there still under review, you're helping us > if you direct your complains to the one that it is sitting on the top > of them. Oh, it's not so much my submissions bothering me (I have received some very good feedback on this list), but the fact that some drivers do not get any updates at all, even though patches are submitted to this mailing list. Not to mention the problem that patch submissions will (and do) stop due to the lack of any feedback whatsoever. Most people have better things to do than writing to /dev/null, and that's the feeling this queuing-for-original-author-review system leaves. Bjørn -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Em 01-07-2010 08:46, Bjørn Mork escreveu: > Mauro Carvalho Chehab writes: > > >> My original idea were to send one of such emails per week, > > Nearly two months has passed since this message. I apologize if I > missed something, but I have not seen another status update. Is it just > me? > > Anyway, since the last status I've seen, 2.6.34 has been released, the > 2.6.35 merge window has been open and then closed, and a number of new > patches have been collected in > https://patchwork.kernel.org/project/linux-media/list/ > , many of which seem rather trivial. Ideally, the patches that are OK should already be catched by a driver maintainer and submitted me via git or mercurial pull request, and the bad patches should already be nacked. The ones that are not merged from the normal process are the ones that are not trivial, among others, due to one of the reasons bellow: - there's no driver maintainer; - the driver maintainer is lazy or overloaded; - there are some concerns about that patch; - there are some changes undergoing that will affect/be affected by the patch; - the patch got forgotten/lost in the middle of the emails. The fact is that the number of those patches are very high, causes me a lot of overload to handle them and to send emails to people requesting the review of those patches. > Any chance of a new status update anytime soon? Updated today, after two or three weeks spent to handle the backlog. I still have a few pull requests pending for 2.6.35-rc (bug fixes) that I'll be handling this week. > I'm particularily > interested in getting a forced status change on any patch which was > "under review" at the time of the last status message. I believe it's > reasonable to expect two months "review" to be more than enough. If > the patches are found unacceptable, then it's much better to have them > rejected with a "please fix foo and resubmit" than the current total > silence called "review". The patches marked as under review means that I'm expecting an action from someone else (the patch author or the driver author/maintainer). So, if you have patches there still under review, you're helping us if you direct your complains to the one that it is sitting on the top of them. Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Mauro Carvalho Chehab writes: > My original idea were to send one of such emails per week, Nearly two months has passed since this message. I apologize if I missed something, but I have not seen another status update. Is it just me? Anyway, since the last status I've seen, 2.6.34 has been released, the 2.6.35 merge window has been open and then closed, and a number of new patches have been collected in https://patchwork.kernel.org/project/linux-media/list/ , many of which seem rather trivial. Any chance of a new status update anytime soon? I'm particularily interested in getting a forced status change on any patch which was "under review" at the time of the last status message. I believe it's reasonable to expect two months "review" to be more than enough. If the patches are found unacceptable, then it's much better to have them rejected with a "please fix foo and resubmit" than the current total silence called "review". Thanks, Bjørn -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Em Thu, 27 May 2010 16:05:48 +0200 Guy Martin escreveu: > > Hi Mauro, > > > Sorry for the delay, I was abroad. > Let me detail the issue a bit more. > > In budget.c, the the frontend is attached this way : > > budget->dvb_frontend = dvb_attach(stv090x_attach, > &tt1600_stv090x_config, &budget->i2c_adap, STV090x_DEMODULATOR_0); > > This means that the tt1600_stv090x_config structure will be common for > all the cards. > > Then the tuner is attached to the frontend : > > ctl = dvb_attach(stv6110x_attach, budget->dvb_frontend, > &tt1600_stv6110x_config, &budget->i2c_adap); > > Once the tuner is attached, the ops are copied to the config : > tt1600_stv090x_config.tuner_sleep = ctl->tuner_sleep; > > This results in the ops being set for subsequently attached cards while > fe->tuner_priv is NULL. > > This is why a check for tuner_priv being set is mandatory when calling > tuner_sleep(). However as pointed out, it may not be the best fix. Ok. I'll commit it for now, as it is a simple patch and can be easily be applied at stable. The proper patch would be to avoid calling the function at the second board with tuner_priv = NULL. > > Regards, > Guy > > > > On Fri, 07 May 2010 22:26:13 -0300 > Mauro Carvalho Chehab wrote: > > > Manu Abraham wrote: > > > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab > > > wrote: > > >> Hi, > > >> > > > > > >> This is the summary of the patches that are currently under review. > > >> Each patch is represented by its submission date, the subject (up > > >> to 70 chars) and the patchwork link (if submitted via email). > > >> > > >> P.S.: This email is c/c to the developers that some review action > > >> is expected. > > >> > > >> May, 7 2010: [v2] stv6110x Fix kernel null pointer deref when > > >> plugging two TT s2-16 http://patchwork.kernel.org/patch/97612 > > > > > > > > > How is this patch going to fix a NULL ptr dereference when more > > > than 1 card is plugged in ? The patch doesn't seem to do what the > > > patch title implies. At least the patch title seems to be wrong. > > > Maybe the patch is supposed to check for a possible NULL ptr > > > dereference when put to sleep ? > > > > (c/c patch author, to be sure that he'll see your explanation request) > > > > His original patch is at: > > https://patchwork.kernel.org/patch/91929/ > > > > The original description with the bug were much better than version 2. > > > > From his OOPS log and description, I suspect that he's facing some > > sort of race condition with the two cards. > > > > This fix seems still valid (with an updated comment), as his dump > > proofed that there are some cases where fe->tuner_priv can be null, > > generating an OOPS, but it seems that his patch is combating > > the effect, and not the cause. > > > > So, I am for adding his patch for now, and then work on a more > > complete approach for the two cards environment. > > > -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Hi Mauro, Sorry for the delay, I was abroad. Let me detail the issue a bit more. In budget.c, the the frontend is attached this way : budget->dvb_frontend = dvb_attach(stv090x_attach, &tt1600_stv090x_config, &budget->i2c_adap, STV090x_DEMODULATOR_0); This means that the tt1600_stv090x_config structure will be common for all the cards. Then the tuner is attached to the frontend : ctl = dvb_attach(stv6110x_attach, budget->dvb_frontend, &tt1600_stv6110x_config, &budget->i2c_adap); Once the tuner is attached, the ops are copied to the config : tt1600_stv090x_config.tuner_sleep = ctl->tuner_sleep; This results in the ops being set for subsequently attached cards while fe->tuner_priv is NULL. This is why a check for tuner_priv being set is mandatory when calling tuner_sleep(). However as pointed out, it may not be the best fix. Regards, Guy On Fri, 07 May 2010 22:26:13 -0300 Mauro Carvalho Chehab wrote: > Manu Abraham wrote: > > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab > > wrote: > >> Hi, > >> > > > >> This is the summary of the patches that are currently under review. > >> Each patch is represented by its submission date, the subject (up > >> to 70 chars) and the patchwork link (if submitted via email). > >> > >> P.S.: This email is c/c to the developers that some review action > >> is expected. > >> > >> May, 7 2010: [v2] stv6110x Fix kernel null pointer deref when > >> plugging two TT s2-16 http://patchwork.kernel.org/patch/97612 > > > > > > How is this patch going to fix a NULL ptr dereference when more > > than 1 card is plugged in ? The patch doesn't seem to do what the > > patch title implies. At least the patch title seems to be wrong. > > Maybe the patch is supposed to check for a possible NULL ptr > > dereference when put to sleep ? > > (c/c patch author, to be sure that he'll see your explanation request) > > His original patch is at: > https://patchwork.kernel.org/patch/91929/ > > The original description with the bug were much better than version 2. > > From his OOPS log and description, I suspect that he's facing some > sort of race condition with the two cards. > > This fix seems still valid (with an updated comment), as his dump > proofed that there are some cases where fe->tuner_priv can be null, > generating an OOPS, but it seems that his patch is combating > the effect, and not the cause. > > So, I am for adding his patch for now, and then work on a more > complete approach for the two cards environment. > -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Mon, May 10, 2010 at 07:39:37PM +0200, Jean-Francois Moine wrote: > On Mon, 10 May 2010 13:25:00 -0300 > Mauro Carvalho Chehab wrote: > > > Sarah Sharp wrote: > > > On Sat, May 08, 2010 at 03:45:41PM -0700, Mauro Carvalho Chehab > > > wrote: > > >> Jean-Francois Moine wrote: > > >>> On Fri, 7 May 2010 09:39:16 -0300 > > >>> Mauro Carvalho Chehab wrote: > > >>> > > == Gspca patches - Waiting Jean-Francois Moine > > submission/review == > > > > Feb,24 2010: gspca pac7302: add USB PID range based on > > heuristics > > http://patchwork.kernel.org/patch/81612 May, 3 2010: gspca: Try > > a less bandwidth-intensive alt setting. > > http://patchwork.kernel.org/patch/96514 > > >>> Hello Mauro, > > >>> > > >>> I don't think the patch about pac7302 should be applied. > > >>> The patch about the gspca main is in my last git pull request. > > >> (c/c Sarah) > > >> > > >> I also didn't like this patch strategy. It seems a sort of > > >> workaround for xHCI, instead of a proper fix. > > >> > > >> I'll mark this patch as rejected, and wait for a proper fix. > > > > > > This isn't a work around for a bug in the xHCI host. The bandwidth > > > checking is a feature. It allows the host to reject a new > > > interface if other devices are already taking up too much of the > > > bus bandwidth. I expect that all drivers that use interrupt or > > > isochronous will have to fall back to a different alternate > > > interface setting if they can. > > > > > > Now, Alan Stern and I have been talking about making a different > > > API for drivers to request a specific polling rate and frame list > > > length for an endpoint. However, I expect that the call would have > > > to be either before or part of the call to usb_set_interface(), > > > because of how the xHCI hardware tracks endpoints. So even if we > > > had the ideal interface, the drivers would still need code like > > > this to fall back to less-bandwidth intensive alternate settings. > > > > > > Is there a different way you think we should handle running out of > > > bandwidth? > > > > Sarah, > > > > A loop like the above doesn't work for video streams. The point is > > that the hardware got programmed to some resolution and frame rate. > > For example, a typical case is to program the hardware to do > > 640x480x30fps. Assuming a device with 16 bits per pixel, this means a > > bandwidth of 18.432 Mbps. The raw bandwidth number (18.432 Mbps) isn't useful to the xHCI hardware. It wants to know the maximum packet size the driver needs, along with the number of additional opportunities per microframe the device can handle and the periodic interval to poll the endpoint at. > > With V4L API, the resolution is configured before starting stream > > (so, before the place where usb_set_interface is called). Are you talking about programming the USB device to have a specific resolution and frame rate? If so, is that done through control transfers or some other method? > > After > > having all video stream parameters configured via several different > > ioctls, a call to VIDIOC_REQBUFS is done, causing the allocation of > > the streaming buffers and the corresponding USB interface setup. On > > that moment, it is too late to negotiate the bandwidth. So, if xHCI > > is not capable of supporting at least 18.432 Mbps (assuming my > > example), the call should simply fail. > > > > In thesis, all V4L/DVB USB drivers have a code where it plays with > > USB interface alternates, until they found the minimum alternate that > > is capable of handing the bandwidth needed by the stream. As there's > > no standard way, at USB core, to set an isoc bandwidth [1], each > > driver has its own logic for doing that, and maybe some strategies > > are better than the others. > > > > I'm not sure if it would be simpler or possible, but an alternative > > way would be to have something like usb_request_bandwidth(), where > > the USB core would have the logic to set the interface alternates to > > fulfill the bandwidth needs, or return -EINVAL if it is not possible. > > The advantage is that you won't need to fix all the different logics > > used by the V4L/DVB drivers. That could be do-able. Can you guarantee that the drivers won't attempt to use the old alternate setting between a call to usb_request_bandwidth() in V4L and a call to usb_set_interface() in the drivers? The problem is that we actually need to install the new alternate interface in the xHCI hardware to find out if we have enough bandwidth. Drivers can't issue URBs to the old alternate setting after this bandwidth check. > > [1] On some devices, even needing a constant bandwitdh, they only > > support bulk transfers, due to hardware limitation, but, from the > > driver's perspective, the seek for the alternate has an identical > > need. Bulk transfers can't have guaranteed bandwidth under xHCI. The xHCI host controller driver has no control over when the bul
Re: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Herton Ronaldo Krzesinski wrote: > Em Sáb 08 Mai 2010, às 19:41:32, Mauro Carvalho Chehab escreveu: >> Herton Ronaldo Krzesinski wrote: >>> Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu: == Patch(broken) - waiting for Herton Ronaldo Krzesinski new submission == Apr, 5 2010: saa7134: add support for Avermedia M733A http://patchwork.kernel.org/patch/90692 >>> Hi, I submitted now a new fixed version of the patch to mailing list, under >>> title "[PATCH v2] saa7134: add support for Avermedia M733A" >> OK, thanks! >> Mar,19 2010: saa7134: add support for one more remote control for Avermedia M135A http://patchwork.kernel.org/patch/86989 >>> This was the addition of RM-K6 remote control to M135A too, I think we can >>> drop >>> this, since adding this to the kernel is deprecated now in favour of >>> assigning >>> keymaps in userspace (keytable tool from v4l-utils), right? >> For now, I prefer to add the keytab there, since there are some scripts that >> syncs v4l-util >> keytables with the kernel ones. If you prefer, you may the put RM-K6 table >> together >> with the other M135A keytable. I intend to group the non-conflicting >> keytables soon, >> and it makes kense to group both the original and the RM-K6 into the same >> table, in order to >> provide an easier hot-plug support for this device. > > Ok, I updated and tested a new patch now, and sent with title > "[PATCH] saa7134: add RM-K6 remote control support for Avermedia M135A" Thanks! -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Em Sáb 08 Mai 2010, às 19:41:32, Mauro Carvalho Chehab escreveu: > Herton Ronaldo Krzesinski wrote: > > Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu: > >>== Patch(broken) - waiting for Herton Ronaldo Krzesinski > >> new submission == > >> > >> Apr, 5 2010: saa7134: add support for Avermedia M733A > >> http://patchwork.kernel.org/patch/90692 > > > > Hi, I submitted now a new fixed version of the patch to mailing list, under > > title "[PATCH v2] saa7134: add support for Avermedia M733A" > > OK, thanks! > > >> Mar,19 2010: saa7134: add support for one more remote control for > >> Avermedia M135A http://patchwork.kernel.org/patch/86989 > > > > This was the addition of RM-K6 remote control to M135A too, I think we can > > drop > > this, since adding this to the kernel is deprecated now in favour of > > assigning > > keymaps in userspace (keytable tool from v4l-utils), right? > > For now, I prefer to add the keytab there, since there are some scripts that > syncs v4l-util > keytables with the kernel ones. If you prefer, you may the put RM-K6 table > together > with the other M135A keytable. I intend to group the non-conflicting > keytables soon, > and it makes kense to group both the original and the RM-K6 into the same > table, in order to > provide an easier hot-plug support for this device. Ok, I updated and tested a new patch now, and sent with title "[PATCH] saa7134: add RM-K6 remote control support for Avermedia M135A" -- []'s Herton -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Mon, 10 May 2010 13:25:00 -0300 Mauro Carvalho Chehab wrote: > Sarah Sharp wrote: > > On Sat, May 08, 2010 at 03:45:41PM -0700, Mauro Carvalho Chehab > > wrote: > >> Jean-Francois Moine wrote: > >>> On Fri, 7 May 2010 09:39:16 -0300 > >>> Mauro Carvalho Chehab wrote: > >>> > == Gspca patches - Waiting Jean-Francois Moine > submission/review == > > Feb,24 2010: gspca pac7302: add USB PID range based on > heuristics > http://patchwork.kernel.org/patch/81612 May, 3 2010: gspca: Try > a less bandwidth-intensive alt setting. > http://patchwork.kernel.org/patch/96514 > >>> Hello Mauro, > >>> > >>> I don't think the patch about pac7302 should be applied. > >>> The patch about the gspca main is in my last git pull request. > >> (c/c Sarah) > >> > >> I also didn't like this patch strategy. It seems a sort of > >> workaround for xHCI, instead of a proper fix. > >> > >> I'll mark this patch as rejected, and wait for a proper fix. > > > > This isn't a work around for a bug in the xHCI host. The bandwidth > > checking is a feature. It allows the host to reject a new > > interface if other devices are already taking up too much of the > > bus bandwidth. I expect that all drivers that use interrupt or > > isochronous will have to fall back to a different alternate > > interface setting if they can. > > > > Now, Alan Stern and I have been talking about making a different > > API for drivers to request a specific polling rate and frame list > > length for an endpoint. However, I expect that the call would have > > to be either before or part of the call to usb_set_interface(), > > because of how the xHCI hardware tracks endpoints. So even if we > > had the ideal interface, the drivers would still need code like > > this to fall back to less-bandwidth intensive alternate settings. > > > > Is there a different way you think we should handle running out of > > bandwidth? > > Sarah, > > A loop like the above doesn't work for video streams. The point is > that the hardware got programmed to some resolution and frame rate. > For example, a typical case is to program the hardware to do > 640x480x30fps. Assuming a device with 16 bits per pixel, this means a > bandwidth of 18.432 Mbps. > > With V4L API, the resolution is configured before starting stream > (so, before the place where usb_set_interface is called). After > having all video stream parameters configured via several different > ioctls, a call to VIDIOC_REQBUFS is done, causing the allocation of > the streaming buffers and the corresponding USB interface setup. On > that moment, it is too late to negotiate the bandwidth. So, if xHCI > is not capable of supporting at least 18.432 Mbps (assuming my > example), the call should simply fail. > > In thesis, all V4L/DVB USB drivers have a code where it plays with > USB interface alternates, until they found the minimum alternate that > is capable of handing the bandwidth needed by the stream. As there's > no standard way, at USB core, to set an isoc bandwidth [1], each > driver has its own logic for doing that, and maybe some strategies > are better than the others. > > I'm not sure if it would be simpler or possible, but an alternative > way would be to have something like usb_request_bandwidth(), where > the USB core would have the logic to set the interface alternates to > fulfill the bandwidth needs, or return -EINVAL if it is not possible. > The advantage is that you won't need to fix all the different logics > used by the V4L/DVB drivers. > > [1] On some devices, even needing a constant bandwitdh, they only > support bulk transfers, due to hardware limitation, but, from the > driver's perspective, the seek for the alternate has an identical > need. Hi Mauro, Contrary to other webcam drivers, gspca already has a fallback to a different altsetting. Actually, this is done at URB submit time and it works correctly, mainly with USB-1.1. It seems that the bandwidth may be now checked when setting the altsetting. The proposed patch does not change the gspca logic at all. Maybe it could be done in a clearer way. A first problem with webcams is that the bandwidth is not fully known when starting the capture. The frame rate may be changed during streaming either by direct video control (rare) or by changing the exposure time (manual or automatic - AEC). Also, the video stream is often compressed and the compression ratio is not well known (some webcams automatically change the JPEG quality). The second problem is the USB interface. If a driver could calculate a good estimation of the needed bandwidth, it cannot actually tell it to the USB subsystem. The USB subsystem uses the packet length and the message interval to calculate the bandwidth that the device will use. This is indeed not correct because the device does not send data at the full interface speed. In an other way, a driver could search the alternate setting which matches the best its estimated b
Re: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Sarah Sharp wrote: > On Sat, May 08, 2010 at 03:45:41PM -0700, Mauro Carvalho Chehab wrote: >> Jean-Francois Moine wrote: >>> On Fri, 7 May 2010 09:39:16 -0300 >>> Mauro Carvalho Chehab wrote: >>> == Gspca patches - Waiting Jean-Francois Moine submission/review == Feb,24 2010: gspca pac7302: add USB PID range based on heuristics http://patchwork.kernel.org/patch/81612 May, 3 2010: gspca: Try a less bandwidth-intensive alt setting. http://patchwork.kernel.org/patch/96514 >>> Hello Mauro, >>> >>> I don't think the patch about pac7302 should be applied. >>> The patch about the gspca main is in my last git pull request. >> (c/c Sarah) >> >> I also didn't like this patch strategy. It seems a sort of workaround >> for xHCI, instead of a proper fix. >> >> I'll mark this patch as rejected, and wait for a proper fix. > > This isn't a work around for a bug in the xHCI host. The bandwidth > checking is a feature. It allows the host to reject a new interface if > other devices are already taking up too much of the bus bandwidth. I > expect that all drivers that use interrupt or isochronous will have to > fall back to a different alternate interface setting if they can. > > Now, Alan Stern and I have been talking about making a different API for > drivers to request a specific polling rate and frame list length for an > endpoint. However, I expect that the call would have to be either > before or part of the call to usb_set_interface(), because of how the > xHCI hardware tracks endpoints. So even if we had the ideal interface, > the drivers would still need code like this to fall back to > less-bandwidth intensive alternate settings. > > Is there a different way you think we should handle running out of > bandwidth? Sarah, A loop like the above doesn't work for video streams. The point is that the hardware got programmed to some resolution and frame rate. For example, a typical case is to program the hardware to do 640x480x30fps. Assuming a device with 16 bits per pixel, this means a bandwidth of 18.432 Mbps. With V4L API, the resolution is configured before starting stream (so, before the place where usb_set_interface is called). After having all video stream parameters configured via several different ioctls, a call to VIDIOC_REQBUFS is done, causing the allocation of the streaming buffers and the corresponding USB interface setup. On that moment, it is too late to negotiate the bandwidth. So, if xHCI is not capable of supporting at least 18.432 Mbps (assuming my example), the call should simply fail. In thesis, all V4L/DVB USB drivers have a code where it plays with USB interface alternates, until they found the minimum alternate that is capable of handing the bandwidth needed by the stream. As there's no standard way, at USB core, to set an isoc bandwidth [1], each driver has its own logic for doing that, and maybe some strategies are better than the others. I'm not sure if it would be simpler or possible, but an alternative way would be to have something like usb_request_bandwidth(), where the USB core would have the logic to set the interface alternates to fulfill the bandwidth needs, or return -EINVAL if it is not possible. The advantage is that you won't need to fix all the different logics used by the V4L/DVB drivers. [1] On some devices, even needing a constant bandwitdh, they only support bulk transfers, due to hardware limitation, but, from the driver's perspective, the seek for the alternate has an identical need. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Sat, May 08, 2010 at 03:45:41PM -0700, Mauro Carvalho Chehab wrote: > Jean-Francois Moine wrote: > > On Fri, 7 May 2010 09:39:16 -0300 > > Mauro Carvalho Chehab wrote: > > > >>== Gspca patches - Waiting Jean-Francois Moine > >> submission/review == > >> > >> Feb,24 2010: gspca pac7302: add USB PID range based on > >> heuristics http://patchwork.kernel.org/patch/81612 > >> May, 3 2010: gspca: Try a less bandwidth-intensive alt > >> setting. http://patchwork.kernel.org/patch/96514 > > > > Hello Mauro, > > > > I don't think the patch about pac7302 should be applied. > > > > > The patch about the gspca main is in my last git pull request. > > (c/c Sarah) > > I also didn't like this patch strategy. It seems a sort of workaround > for xHCI, instead of a proper fix. > > I'll mark this patch as rejected, and wait for a proper fix. This isn't a work around for a bug in the xHCI host. The bandwidth checking is a feature. It allows the host to reject a new interface if other devices are already taking up too much of the bus bandwidth. I expect that all drivers that use interrupt or isochronous will have to fall back to a different alternate interface setting if they can. Now, Alan Stern and I have been talking about making a different API for drivers to request a specific polling rate and frame list length for an endpoint. However, I expect that the call would have to be either before or part of the call to usb_set_interface(), because of how the xHCI hardware tracks endpoints. So even if we had the ideal interface, the drivers would still need code like this to fall back to less-bandwidth intensive alternate settings. Is there a different way you think we should handle running out of bandwidth? Sarah Sharp -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Hi, >Mauro Carvalho Chehab wrote: > == Videobuf patches - Need more tests before committing it - > Volunteers? == > >Apr,21 2010: [v1, 1/2] v4l: videobuf: Add support for out-of-order buffer >dequeuing > http://patchwork.kernel.org/patch/93901 >Apr,21 2010: [v1, 2/2] v4l: vivi: adapt to out-of-order buffer dequeuing in >videobu http://patchwork.kernel.org/patch/93903 > it'd be great if there was a driver/device that would be benefiting from this, i.e. that may want to: - return buffers in a different order than queued, - process them in parallel, - process them in a different order, or anybody interested in those features. Is anybody aware of any such devices in the V4L tree? This is also the first step to simplifying videobuf and making it lighter by removing superficial (to my best knowledge) waitqueues in every videobuf_buffer structure, as discussed at the Oslo meeting. Best regards -- Pawel Osciak Linux Platform Group Samsung Poland R&D Center -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
>From: Mauro Carvalho Chehab > == New or updated patches starting from May, 5 or newer - not > handled yet == > >May, 5 2010: [-next] media/mem2mem: dereferencing free memory http://patchwork.kernel.org/patch/96984 This one is obvious, but might as well: Reviewed-by: Pawel Osciak Acked-by: Pawel Osciak Best regards -- Pawel Osciak Linux Platform Group Samsung Poland R&D Center -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Jean-Francois Moine wrote: > On Fri, 7 May 2010 09:39:16 -0300 > Mauro Carvalho Chehab wrote: > >> == Gspca patches - Waiting Jean-Francois Moine >> submission/review == >> >> Feb,24 2010: gspca pac7302: add USB PID range based on >> heuristics http://patchwork.kernel.org/patch/81612 >> May, 3 2010: gspca: Try a less bandwidth-intensive alt >> setting. http://patchwork.kernel.org/patch/96514 > > Hello Mauro, > > I don't think the patch about pac7302 should be applied. > > The patch about the gspca main is in my last git pull request. (c/c Sarah) I also didn't like this patch strategy. It seems a sort of workaround for xHCI, instead of a proper fix. I'll mark this patch as rejected, and wait for a proper fix. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Herton Ronaldo Krzesinski wrote: > Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu: >> == Patch(broken) - waiting for Herton Ronaldo Krzesinski >> new submission == >> >> Apr, 5 2010: saa7134: add support for Avermedia M733A >>http://patchwork.kernel.org/patch/90692 > > Hi, I submitted now a new fixed version of the patch to mailing list, under > title "[PATCH v2] saa7134: add support for Avermedia M733A" OK, thanks! >> Mar,19 2010: saa7134: add support for one more remote control for Avermedia >> M135A http://patchwork.kernel.org/patch/86989 > > This was the addition of RM-K6 remote control to M135A too, I think we can > drop > this, since adding this to the kernel is deprecated now in favour of assigning > keymaps in userspace (keytable tool from v4l-utils), right? For now, I prefer to add the keytab there, since there are some scripts that syncs v4l-util keytables with the kernel ones. If you prefer, you may the put RM-K6 table together with the other M135A keytable. I intend to group the non-conflicting keytables soon, and it makes kense to group both the original and the RM-K6 into the same table, in order to provide an easier hot-plug support for this device. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Fri, 7 May 2010 09:39:16 -0300 Mauro Carvalho Chehab wrote: > == Gspca patches - Waiting Jean-Francois Moine > submission/review == > > Feb,24 2010: gspca pac7302: add USB PID range based on > heuristics http://patchwork.kernel.org/patch/81612 > May, 3 2010: gspca: Try a less bandwidth-intensive alt > setting. http://patchwork.kernel.org/patch/96514 Hello Mauro, I don't think the patch about pac7302 should be applied. The patch about the gspca main is in my last git pull request. Cheers. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu: > == Patch(broken) - waiting for Herton Ronaldo Krzesinski > new submission == > > Apr, 5 2010: saa7134: add support for Avermedia M733A > http://patchwork.kernel.org/patch/90692 Hi, I submitted now a new fixed version of the patch to mailing list, under title "[PATCH v2] saa7134: add support for Avermedia M733A" > Mar,19 2010: saa7134: add support for one more remote control for Avermedia > M135A http://patchwork.kernel.org/patch/86989 This was the addition of RM-K6 remote control to M135A too, I think we can drop this, since adding this to the kernel is deprecated now in favour of assigning keymaps in userspace (keytable tool from v4l-utils), right? -- []'s Herton -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Manu Abraham wrote: > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab > wrote: >> Hi, >> > >> This is the summary of the patches that are currently under review. >> Each patch is represented by its submission date, the subject (up to 70 >> chars) and the patchwork link (if submitted via email). >> >> >> May, 2 2010: Add FE_CAN_TURBO_FEC (was: Add FE_CAN_PSK_8 to allow apps to >> identify http://patchwork.kernel.org/patch/96341 > > > > Reviewed-by: Manu Abraham > Acked-by: Manu Abraham Could you please reply with your reviewed-by tag at the original thread at the ML? I intend to add for a while for more review, and, by replying at the thread, Patchwork will automatically add your tag to the patch. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Manu Abraham wrote: > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab > wrote: >> Hi, >> > >> This is the summary of the patches that are currently under review. >> Each patch is represented by its submission date, the subject (up to 70 >> chars) and the patchwork link (if submitted via email). >> > >>== Port mantis IR to the new API - waiting for Manu Abraham >> ack or alternative patch == >> >> Apr,15 2010: [5/8] ir-core: convert mantis from ir-functions.c > > > This needs to wait for an alternate patch, which depends on > linuxtv.org/hg/v4l-dvb proper build Ok. I think Douglas already backported the IR changes. Not sure if his tree is now in sync with git. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Manu Abraham wrote: > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab > wrote: >> Hi, >> > >> This is the summary of the patches that are currently under review. >> Each patch is represented by its submission date, the subject (up to 70 >> chars) and the patchwork link (if submitted via email). >> >> P.S.: This email is c/c to the developers that some review action is >> expected. >> >> >> >>== New or updated patches starting from May, 5 or newer - not >> handled yet == >> >> May, 5 2010: [-next] dvb/stv6110x: cleanup error handling >>http://patchwork.kernel.org/patch/96983 > > > Reviewed-by: Manu Abraham > Acked-by: Manu Abraham Applied, thanks. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Manu Abraham wrote: > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab > wrote: >> Hi, >> > >> This is the summary of the patches that are currently under review. >> Each patch is represented by its submission date, the subject (up to 70 >> chars) and the patchwork link (if submitted via email). >> >> P.S.: This email is c/c to the developers that some review action is >> expected. >> >> May, 7 2010: [v2] stv6110x Fix kernel null pointer deref when plugging two >> TT s2-16 http://patchwork.kernel.org/patch/97612 > > > How is this patch going to fix a NULL ptr dereference when more than 1 > card is plugged in ? The patch doesn't seem to do what the patch title > implies. At least the patch title seems to be wrong. Maybe the patch > is supposed to check for a possible NULL ptr dereference when put to > sleep ? (c/c patch author, to be sure that he'll see your explanation request) His original patch is at: https://patchwork.kernel.org/patch/91929/ The original description with the bug were much better than version 2. >From his OOPS log and description, I suspect that he's facing some sort of race condition with the two cards. This fix seems still valid (with an updated comment), as his dump proofed that there are some cases where fe->tuner_priv can be null, generating an OOPS, but it seems that his patch is combating the effect, and not the cause. So, I am for adding his patch for now, and then work on a more complete approach for the two cards environment. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Guennadi Liakhovetski wrote: > Hi Mauro > > On Fri, 7 May 2010, Mauro Carvalho Chehab wrote: > >> May, 6 2010: [1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 >>http://patchwork.kernel.org/patch/97345 >> May, 6 2010: [2/3] mx27: add support for the CSI device >>http://patchwork.kernel.org/patch/97352 >> May, 6 2010: [3/3] mx25: add support for the CSI device >>http://patchwork.kernel.org/patch/97353 > > I'll be reviewing these > >> == soc_camera patches - Waiting Guennadi >> submission/review == >> >> Feb, 2 2010: [2/3] soc-camera: mt9t112: modify delay time after initialize >>http://patchwork.kernel.org/patch/76213 >> Feb, 2 2010: [3/3] soc-camera: mt9t112: The flag which control camera-init >> is remov http://patchwork.kernel.org/patch/76214 > > These two are still on hold, I think, I'll have to ask the author if we > can drop them. > >> Mar, 5 2010: [v2] V4L/DVB: mx1-camera: compile fix >>http://patchwork.kernel.org/patch/83742 > > An updated version of this one is already in your fixes tree: > > http://git.linuxtv.org/fixes.git?a=commit;h=f6c22d4cff27a4bbb76d899b58b79dd311b7603f > >> Apr,20 2010: pxa_camera: move fifo reset direct before dma start >>http://patchwork.kernel.org/patch/93619 > > Ditto for this one: > > http://git.linuxtv.org/fixes.git?a=commit;h=80cef8eb49c9689664a31b8a21f83517042d9763 Thanks for the input. I've updated patchwork accordingly. -- Cheers, 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab wrote: > Hi, > > This is the summary of the patches that are currently under review. > Each patch is represented by its submission date, the subject (up to 70 > chars) and the patchwork link (if submitted via email). > > > May, 2 2010: Add FE_CAN_TURBO_FEC (was: Add FE_CAN_PSK_8 to allow apps to > identify http://patchwork.kernel.org/patch/96341 Reviewed-by: Manu Abraham Acked-by: Manu Abraham -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab wrote: > Hi, > > > This is the summary of the patches that are currently under review. > Each patch is represented by its submission date, the subject (up to 70 > chars) and the patchwork link (if submitted via email). > > == Port mantis IR to the new API - waiting for Manu Abraham > ack or alternative patch == > > Apr,15 2010: [5/8] ir-core: convert mantis from ir-functions.c This needs to wait for an alternate patch, which depends on linuxtv.org/hg/v4l-dvb proper build -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab wrote: > Hi, > > > This is the summary of the patches that are currently under review. > Each patch is represented by its submission date, the subject (up to 70 > chars) and the patchwork link (if submitted via email). > > P.S.: This email is c/c to the developers that some review action is expected. > > May, 7 2010: [v2] stv6110x Fix kernel null pointer deref when plugging two TT > s2-16 http://patchwork.kernel.org/patch/97612 How is this patch going to fix a NULL ptr dereference when more than 1 card is plugged in ? The patch doesn't seem to do what the patch title implies. At least the patch title seems to be wrong. Maybe the patch is supposed to check for a possible NULL ptr dereference when put to sleep ? -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab wrote: > Hi, > > This is the summary of the patches that are currently under review. > Each patch is represented by its submission date, the subject (up to 70 > chars) and the patchwork link (if submitted via email). > > P.S.: This email is c/c to the developers that some review action is expected. > > > > == New or updated patches starting from May, 5 or newer - not > handled yet == > > May, 5 2010: [-next] dvb/stv6110x: cleanup error handling > http://patchwork.kernel.org/patch/96983 Reviewed-by: Manu Abraham Acked-by: Manu Abraham -- 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: Status of the patches under review (85 patches) and some misc notes about the devel procedures
Hi Mauro On Fri, 7 May 2010, Mauro Carvalho Chehab wrote: > May, 6 2010: [1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27 > http://patchwork.kernel.org/patch/97345 > May, 6 2010: [2/3] mx27: add support for the CSI device > http://patchwork.kernel.org/patch/97352 > May, 6 2010: [3/3] mx25: add support for the CSI device > http://patchwork.kernel.org/patch/97353 I'll be reviewing these > == soc_camera patches - Waiting Guennadi > submission/review == > > Feb, 2 2010: [2/3] soc-camera: mt9t112: modify delay time after initialize > http://patchwork.kernel.org/patch/76213 > Feb, 2 2010: [3/3] soc-camera: mt9t112: The flag which control camera-init is > remov http://patchwork.kernel.org/patch/76214 These two are still on hold, I think, I'll have to ask the author if we can drop them. > Mar, 5 2010: [v2] V4L/DVB: mx1-camera: compile fix > http://patchwork.kernel.org/patch/83742 An updated version of this one is already in your fixes tree: http://git.linuxtv.org/fixes.git?a=commit;h=f6c22d4cff27a4bbb76d899b58b79dd311b7603f > Apr,20 2010: pxa_camera: move fifo reset direct before dma start > http://patchwork.kernel.org/patch/93619 Ditto for this one: http://git.linuxtv.org/fixes.git?a=commit;h=80cef8eb49c9689664a31b8a21f83517042d9763 Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
