Skip to contentSkip to conversation

Status of the patches under review (85 patches) and some misc notes about the devel procedures

DISCUSSION Mauro Carvalho Chehab 10 participants 28 messages May 7, 2010 – Jul 7, 2010 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

Hi,

My original idea were to send one of such emails per week, in order to allow people to review my pending list and point me for missing things. Fortunately, the number of patches for the next merge window is so high that, even passing -hg maintainership to Douglas, I was still very busy. Good to see so much work at the subsystem. Congrats to the media developers that worked really hard!

With respect to -hg/-git pull requests, my queue is currently empty.

While I was working at the patchwork queue, we've received a lot of contributions on the last few days. So, I gave up of just applying everything before sending this email. I'll just list those patches that weren't yet read. I'll likely handle those during the weekend.

Also, as usual, there are probably some patches listed that were already applied, or that I missed the current status. In this case, just ping me.

It is expected that the current development kernel (2.6.33-rc6) to be the last one before the next merge window. Let's see. If this is the case, this means that we'll stop patch receiving for 2.6.35 very soon.

With respect to -git tree, I'll likely change the procedure a little bit for 2.6.35:
instead of merging everything at master, I intend to create some topics branches, merging patches there, and reserving "master" to be in sync with the latest stable + accepted fixes (or, some -rc + fixes). This will mean that it will have a cleaner history. I expect that the merge procedure from developers tree to be simpler, as each topic maintainer could decide either to keep using the latest stable, or to go to the very latest upstream tree.

With respect to the backport tree, Douglas is the maintainer for it. He's likely having a very bad time merging those high volume of patches, while trying to keep the tree compiling with the legacy kernel. Douglas, thanks for your work! As usual, if you find something broken there, instead of complain, just send him a patch fixing it ;)

Thanks,
Mauro.

---

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] input: unlock on error paths                                   http://patchwork.kernel.org/patch/96982
May, 5 2010: [-next] dvb/stv6110x: cleanup error handling                           http://patchwork.kernel.org/patch/96983
May, 5 2010: [-next] media/mem2mem: dereferencing free memory                       http://patchwork.kernel.org/patch/96984
May, 5 2010: media/ov511: cleanup: remove unneeded null check                       http://patchwork.kernel.org/patch/96985
May, 5 2010: [-next,1/2] media/s2255drv: return if vdev not found                   http://patchwork.kernel.org/patch/96987
May, 5 2010: [-next,2/2] media/s2255drv: remove dead code                           http://patchwork.kernel.org/patch/96986
May, 5 2010: tda10048: fix the uncomplete function tda10048_read_ber                http://patchwork.kernel.org/patch/97058
May, 5 2010: media/IR: Add missing include file to rc-map.c                         http://patchwork.kernel.org/patch/97133
May, 5 2010: -next: staging/cx25821: please revert 7a02f549fcc                      http://patchwork.kernel.org/patch/97134
May, 5 2010: fix dvb frontend lockup                                                http://patchwork.kernel.org/patch/97171
May, 5 2010: videobuf: remove external function videobuf_dma_sync()                 http://patchwork.kernel.org/patch/97175
May, 5 2010: -next: staging/cx25821: please revert 7a02f549fcc                      http://patchwork.kernel.org/patch/97177
May, 5 2010: videobuf-vmalloc: remove __videobuf_sync()                             http://patchwork.kernel.org/patch/97197
May, 5 2010: [1/5] viafb: fold via_io.h into via-core.h                             http://patchwork.kernel.org/patch/97224
May, 5 2010: [2/5] viafb: get rid of i2c debug cruft                                http://patchwork.kernel.org/patch/97230
May, 5 2010: [3/5] viafb: Eliminate some global.h references                        http://patchwork.kernel.org/patch/97233
May, 5 2010: [4/5] viafb: move some include files to include/linux                  http://patchwork.kernel.org/patch/97232
May, 5 2010: [5/5] Add the viafb video capture driver                               http://patchwork.kernel.org/patch/97231
Mar,17 2010: [2/2] V4L/DVB: buf-dma-sg.c: support non-pageable user-allocated memor http://patchwork.kernel.org/patch/97263
May, 6 2010: [1/1] staging: cx25821: cx25821-alsa.c: cleanup                        http://patchwork.kernel.org/patch/97295
May, 6 2010: tda10048: fix bitmask for the transmission mode                        http://patchwork.kernel.org/patch/97340
May, 6 2010: tda10048: clear the uncorrected packet registers when saturated        http://patchwork.kernel.org/patch/97341
May, 6 2010: dvb_frontend: fix typos in comments and one function                   http://patchwork.kernel.org/patch/97343
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
May, 6 2010: tm6000: README - add vbi                                               http://patchwork.kernel.org/patch/97354
May, 6 2010: dvb-core: Fix ULE decapsulation bug when less than 4 bytes of ULE SNDU http://patchwork.kernel.org/patch/97358
May, 6 2010: TechnoTrend TT-budget T-3000                                           http://patchwork.kernel.org/patch/97436
May, 4 2010: [v2] IR/imon: remove dead IMON_KEY_RELEASE_OFFSET                      http://patchwork.kernel.org/patch/96854
May, 6 2010: [-next] IR: add header file to fix build                               http://patchwork.kernel.org/patch/97502
May, 7 2010: stv6110x Fix kernel null pointer deref when plugging two TT s2-1600    http://patchwork.kernel.org/patch/97603
May, 7 2010: TT CT-3650 DVB-C support                                               http://patchwork.kernel.org/patch/97606

May, 7 2010: [v2] stv6110x Fix kernel null pointer deref when plugging two TT s2-16 http://patchwork.kernel.org/patch/97612

		== Port mantis IR to the new API - waiting for Manu Abraham <[email protected]> ack or alternative patch == 

Apr,15 2010: [5/8] ir-core: convert mantis from ir-functions.c http://patchwork.kernel.org/patch/92961

		== dsbr100 patchess waiting for more review at the ML == 
May, 5 2010: dsbr100: implement proper locking                                      http://patchwork.kernel.org/patch/97155
May, 5 2010: dsbr100: fix potential use after free                                  http://patchwork.kernel.org/patch/97154
May, 5 2010: dsbr100: only change frequency upon success                            http://patchwork.kernel.org/patch/97156
May, 5 2010: dsbr100: remove disconnected indicator                                 http://patchwork.kernel.org/patch/97158
May, 5 2010: dsbr100: properly initialize the radio                                 http://patchwork.kernel.org/patch/97153
May, 5 2010: dsbr100: cleanup usb probe routine                                     http://patchwork.kernel.org/patch/97157
May, 5 2010: dsbr100: simplify access to radio device                               http://patchwork.kernel.org/patch/97159
		== Moorestown patches waiting for more review at the ML == 

Mar,28 2010: This patch is first part of the intel moorestown isp driver and header http://patchwork.kernel.org/patch/88734
Mar,28 2010: This patch is second part of intel moorestown isp driver and c files c http://patchwork.kernel.org/patch/88775
Mar,28 2010: This patch is second part of intel moorestown isp driver and c files c http://patchwork.kernel.org/patch/88776
Mar,28 2010: This patch is the flash subdev driver for intel moorestown camera imag http://patchwork.kernel.org/patch/88735
Mar,28 2010: this patch is 2mp camera (ov2650) sensor subdev driver for intel moore http://patchwork.kernel.org/patch/88738
Mar,28 2010: this patch is the 1.3mp camera (ov9665) sensor subdev driver fro inter http://patchwork.kernel.org/patch/88736
Mar,28 2010: this patch is the 5mp camera (ov5630) sensor subdev driver for intel m http://patchwork.kernel.org/patch/88737
Mar,28 2010: this patch is the 5mp camera (ov5630) sensor lens subdev driver for in http://patchwork.kernel.org/patch/88739
Mar,28 2010: this patch is the 5mp camera (s5k4e1) sensor subdev driver for intel m http://patchwork.kernel.org/patch/88740
Mar,28 2010: this patch is the make/kconfig files changes to enable the camera driv http://patchwork.kernel.org/patch/88741

		== Arm s5p patches waiting for more review at the ML == 
Apr,19 2010: [v1,1/3] ARM: S5P: Add FIMC driver platform helpers                    http://patchwork.kernel.org/patch/93460
Apr,19 2010: [v1,2/3] ARM: S5PC100: Add FIMC driver platform helpers                http://patchwork.kernel.org/patch/93466
Apr,19 2010: [v1,3/3] ARM: S5P: Add Camera interface (video postprocessor) driver   http://patchwork.kernel.org/patch/95064
		== Waiting for more DVB acks == 

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

		== Waiting for my fixes on Docbook == 

Feb,25 2010: DocBook/Makefile: Make it less verbose http://patchwork.kernel.org/patch/82076
Feb,25 2010: DocBook: Add rules to auto-generate some media docbooks http://patchwork.kernel.org/patch/82075
Feb,25 2010: DocBook/v4l/pixfmt.xml: Add missing formats for gspca cpia1 and sn9c20 http://patchwork.kernel.org/patch/82074
Feb,25 2010: v4l: document new Bayer and monochrome pixel formats http://patchwork.kernel.org/patch/82073

		== Waiting for Antti Palosaari <[email protected]> review == 

Mar,21 2010: af9015 : more robust eeprom parsing http://patchwork.kernel.org/patch/87243

		== Waiting for Tobias Lorenz <[email protected]> review == 

Feb, 3 2010: radio-si470x-common: -EINVAL overwritten in si470x_vidioc_s_tuner() http://patchwork.kernel.org/patch/76786

		== Waiting for its addition at linux-firmware and driver test by David Wong <[email protected]>  == 

Nov, 1 2009: lgs8gxx: remove firmware for lgs8g75 http://patchwork.kernel.org/patch/56822

		== Andy Walls <[email protected]> and Aleksandr Piskunov <[email protected]> are discussing around the solution == 

Oct,11 2009: AVerTV MCE 116 Plus radio http://patchwork.kernel.org/patch/52981

		== Waiting for Andy Walls <[email protected]> == 

Apr,10 2010: cx18: "missing audio" for analog recordings http://patchwork.kernel.org/patch/91879

		== Patches waiting for Patrick Boettcher <[email protected]> review == 

Jan,15 2010: remove obsolete conditionalizing on DVB_DIBCOM_DEBUG http://patchwork.kernel.org/patch/73147

		== Gspca patches - Waiting Hans de Goede <[email protected]> submission/review == 

Jan,29 2010: [gspca_jf,tree] gspca zc3xx: signal when unknown packet received http://patchwork.kernel.org/patch/75837

		== Gspca patches - Waiting Jean-Francois Moine <[email protected]> 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

		== soc_camera patches - Waiting Guennadi <[email protected]> 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
Mar, 5 2010: [v2] V4L/DVB: mx1-camera: compile fix http://patchwork.kernel.org/patch/83742
Apr,20 2010: pxa_camera: move fifo reset direct before dma start http://patchwork.kernel.org/patch/93619

		== Waiting for Steven Toth <[email protected]> comments on it == 

Feb, 6 2010: cx23885: Enable Message Signaled Interrupts(MSI). http://patchwork.kernel.org/patch/77492

		== Waiting for Henrik Kurelid <[email protected]> comments about an userspace patch for MythTV == 

Mar, 1 2010: firedtv: add parameter to fake ca_system_ids in CA_INFO http://patchwork.kernel.org/patch/82912

		== 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

		== Waiting for Mike Isely <[email protected]> review == 

Apr,25 2010: Problem with cx25840 and Terratec Grabster AV400 http://patchwork.kernel.org/patch/94960
Apr,27 2010: V4L: Events: Include slab.h explicitly http://patchwork.kernel.org/patch/95449

		== Waiting for Igor M. Liplianin <[email protected]> comments/review == 

Mar,10 2010: DM1105: could not attach frontend 195d:1105 http://patchwork.kernel.org/patch/84549

		== Need some fixes from Uwe Kleine-Koenig <[email protected]> == 

Mar,16 2010: mx1-camera: compile fix http://patchwork.kernel.org/patch/86106

		== Patch(broken) Lock fixes. Patch is wrong but Siano has lock problems - Waiting for developers with time to fix it == 

Nov,12 2009: Locking in Siano driver (untested) http://patchwork.kernel.org/patch/59590

		== Patch(broken) - waiting for Manu Abraham <[email protected]> submission == 

Feb,16 2010: Twinhan 1027 + IR Port support http://patchwork.kernel.org/patch/79753

		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <[email protected]> new submission == 

Apr, 5 2010: saa7134: add support for Avermedia M733A http://patchwork.kernel.org/patch/90692
Mar,19 2010: saa7134: add support for one more remote control for Avermedia M135A http://patchwork.kernel.org/patch/86989

Cheers,
Mauro

---

All patches sent to [email protected] are automatically stored at Patchwork. The driver maintainer then analyzes it it and apply or reply with comments. Others also may comment about it.

When the patch is is too trivial, or touches on a driver without a specific maintainer, I analyze and apply it directly. Otherwise, it will be taged as under review and the proper maintainer will be warned.

If you submitted or are waiting for the review of some patchset that weren't applied at the git://git.kernel.org/v4l-dvb.git nor it is at the list bellow, please check the status of the patch at the patchwork:
http://patchwork.kernel.org/project/linux-media/list/

To check for a patch that is not New or Under Review, you'll need to click at the "Filters" and pass some parameters to find the patch. Don't forget to change the "State" to "any".

Eventually, your patch were rejected for some reason, or someone asked for changes on it. If the patch is there, with a status different than New/Under Review, double check for the received comments at the mailing list. If it got Rejected, marked as RFC or as Changes Requested, please review it according with the requests and send it again. In the very unlikely case that the patch were wrongly tagged, please ping me.

If you discover any patch submitted via email that weren't caught by Patchwork, this means that the patch got mangled by your emailer. The more likely cause is that the emailer converted tabs into spaces or broke long lines.

If you're using Thunderbird, the solution is to install Asalted Patches extension, available at:
https://hg.mozilla.org/users/clarkbw_gnome.org/asalted-patches/
Other emailers will need you to disable the wrapping long lines feature.

#1 lore raw

Hi Mauro

Quoted 3 lines · Mauro Carvalho Chehab
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

Quoted 4 lines
		== soc_camera patches - Waiting Guennadi <[email protected]> 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.

Quoted 1 line

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

Quoted 1 line

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/

#2 lore raw
Quoted 22 lines · Guennadi Liakhovetski

Hi Mauro

Quoted 3 lines · Mauro Carvalho Chehab
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

Quoted 4 lines
		== soc_camera patches - Waiting Guennadi <[email protected]> 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.

Quoted 1 line

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:

Quoted 7 lines

http://git.linuxtv.org/fixes.git?a=commit;h=f6c22d4cff27a4bbb76d899b58b79dd311b7603f

Quoted 1 line

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.

Signature · 2 lines
Cheers,
Mauro
Reviewed-by Acked-by
#3 lore raw
In reply to Mauro Carvalho Chehab
Quoted 7 lines · Mauro Carvalho Chehab

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).

Quoted 7 lines

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

author Applied
#4 lore raw
Quoted 13 lines · Manu Abraham
Quoted 11 lines · Mauro Carvalho Chehab

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.

Quoted 7 lines
Quoted 3 lines
               == 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

Applied, thanks.

Signature · 2 lines
Cheers,
Mauro
#5 lore raw
In reply to Mauro Carvalho Chehab
Quoted 11 lines · Mauro Carvalho Chehab

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 ?

#6 lore raw
Quoted 11 lines · Manu Abraham
Quoted 9 lines · Mauro Carvalho Chehab

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.

Quoted 8 lines
Quoted 1 line

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.

Quoted 1 line

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.

Signature · 2 lines
Cheers,
Mauro
#7 lore raw

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

Quoted 41 lines · On Fri, 07 May 2010 22:26:13 -0300 Mauro Carvalho Chehab <[email protected]>…
Quoted 16 lines · Manu Abraham
Quoted 7 lines · Mauro Carvalho Chehab

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).

Quoted 5 lines

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

Quoted 5 lines

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.

#8 lore raw

Em Thu, 27 May 2010 16:05:48 +0200
Guy Martin <[email protected]> escreveu:

Quoted 20 lines

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);

Quoted 8 lines

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.

Quoted 51 lines

Regards,
Guy

Quoted 41 lines · On Fri, 07 May 2010 22:26:13 -0300 Mauro Carvalho Chehab <[email protected]>…
Quoted 16 lines · Manu Abraham
Quoted 7 lines · Mauro Carvalho Chehab

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).

Quoted 5 lines

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

Quoted 5 lines

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.

Signature · 2 lines
Cheers,
Mauro
#9 lore raw
In reply to Mauro Carvalho Chehab
Quoted 4 lines · Mauro Carvalho Chehab

Hi,

Quoted 8 lines

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 <[email protected]> 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

#10 lore raw
Quoted 10 lines · Manu Abraham
Quoted 7 lines · Mauro Carvalho Chehab

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).

Quoted 7 lines
Quoted 3 lines
               == Port mantis IR to the new API - waiting for Manu Abraham <[email protected]> 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.

Signature · 2 lines
Cheers,
Mauro
Reviewed-by Acked-by
#11 lore raw
In reply to Mauro Carvalho Chehab
Quoted 9 lines · Mauro Carvalho Chehab

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

#12 lore raw
Quoted 10 lines · Manu Abraham
Quoted 8 lines · Mauro Carvalho Chehab

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).

Quoted 6 lines
Quoted 1 line

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

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.

Signature · 2 lines
Cheers,
Mauro
#13 lore raw
In reply to Mauro Carvalho Chehab

Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu:

Quoted 3 lines
		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <[email protected]> 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"

Quoted 1 line

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

#14 lore raw
Quoted 7 lines · Herton Ronaldo Krzesinski

Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu:

Quoted 3 lines
		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <[email protected]> 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!

Quoted 5 lines
Quoted 1 line

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.

Signature · 2 lines
Cheers,
Mauro
#15 lore raw

Em Sáb 08 Mai 2010, às 19:41:32, Mauro Carvalho Chehab escreveu:

Quoted 17 lines
Quoted 7 lines · Herton Ronaldo Krzesinski

Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu:

Quoted 3 lines
		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <[email protected]> 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!

Quoted 5 lines
Quoted 1 line

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?

Quoted 5 lines

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

#16 lore raw
Quoted 14 lines · Herton Ronaldo Krzesinski

Em Sáb 08 Mai 2010, às 19:41:32, Mauro Carvalho Chehab escreveu:

Quoted 13 lines
Quoted 6 lines · Herton Ronaldo Krzesinski

Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu:

Quoted 3 lines
		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <[email protected]> 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!

Quoted 4 lines
Quoted 1 line

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?

Quoted 8 lines
Quoted 5 lines

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!

Signature · 2 lines
Cheers,
Mauro
#17 lore raw
In reply to Mauro Carvalho Chehab
Quoted 7 lines · On Fri, 7 May 2010 09:39:16 -0300 Mauro Carvalho Chehab <[email protected]>…
		== Gspca patches - Waiting Jean-Francois Moine

<[email protected]> 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.

Signature · 2 lines
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
#18 lore raw
Quoted 11 lines · Jean-Francois Moine
Quoted 7 lines · On Fri, 7 May 2010 09:39:16 -0300 Mauro Carvalho Chehab <[email protected]>…
		== Gspca patches - Waiting Jean-Francois Moine

<[email protected]> 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

Quoted 6 lines

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.

Signature · 2 lines
Cheers,
Mauro
#19 lore raw
Quoted 17 lines · Mauro Carvalho Chehab
Quoted 16 lines · Jean-Francois Moine
Quoted 7 lines · On Fri, 7 May 2010 09:39:16 -0300 Mauro Carvalho Chehab <[email protected]>…
		== Gspca patches - Waiting Jean-Francois Moine

<[email protected]> 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.

Quoted 8 lines
Quoted 1 line

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

#20 lore raw
Quoted 37 lines · Sarah Sharp
Quoted 13 lines · Mauro Carvalho Chehab
Quoted 12 lines · Jean-Francois Moine
Quoted 7 lines · On Fri, 7 May 2010 09:39:16 -0300 Mauro Carvalho Chehab <[email protected]>…
		== Gspca patches - Waiting Jean-Francois Moine

<[email protected]> 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,

Quoted 8 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

Signature · 2 lines
Cheers,
Mauro
#21 lore raw
Quoted 75 lines · On Mon, 10 May 2010 13:25:00 -0300 Mauro Carvalho Chehab <[email protected]>…
Quoted 40 lines · Sarah Sharp
Quoted 14 lines · Mauro Carvalho Chehab
Quoted 13 lines · Jean-Francois Moine
Quoted 8 lines · On Fri, 7 May 2010 09:39:16 -0300 Mauro Carvalho Chehab <[email protected]>…
		== Gspca patches - Waiting Jean-Francois Moine

<[email protected]> 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,

Quoted 8 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

[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 bandwidth, but I am not sure that this will not slow down the frame rate (more USB packets for the same data volume).

Hi Sarah,

Your patch should be changed to follow the global altsetting fallback mechanism. This mechanism is inside the function gspca_init_transfer(). If usb_set_interface() returns an error when the bandwidth is not wide enough (BTW, which is this error?), it should called inside the loop and, so, removed from get_ep().

Cheers.

Signature · 2 lines
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
#22 lore raw
Quoted 47 lines · Jean-Francois Moine
Quoted 44 lines · On Mon, 10 May 2010 13:25:00 -0300 Mauro Carvalho Chehab <[email protected]>…
Quoted 42 lines · Sarah Sharp
Quoted 14 lines · Mauro Carvalho Chehab
Quoted 13 lines · Jean-Francois Moine
Quoted 8 lines · On Fri, 7 May 2010 09:39:16 -0300 Mauro Carvalho Chehab <[email protected]>…
		== Gspca patches - Waiting Jean-Francois Moine

<[email protected]> 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,

Quoted 8 lines
Quoted 2 lines

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?

Quoted 7 lines
Quoted 7 lines

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.

Quoted 2 lines
Quoted 2 lines

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?

Quoted 15 lines
Quoted 15 lines

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.

Quoted 6 lines
Quoted 6 lines

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.

Quoted 4 lines
Quoted 4 lines

[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 bulk transfers are scheduled, since it is all done in hardware.

Quoted 8 lines

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.

Quoted 6 lines

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).

Hmm, can you give an accurate estimate of the maximum bandwidth you'll use? For example, if you don't know whether compression is used, just assume it won't and calculate the max bandwidth with that assumption.

If there was an API to change the required bandwidth during capture (say when the exposure time is changed), what kind of latencies could the user tolerate? Could they tolerate dropping a couple frames of data?

I ask because it takes some time to check whether the hardware can support the new bandwidth requirements. The xHCI driver must stop all transfers to the effected endpoints, issue a command, and wait for it to be processed (which takes a variable length of time, based on how many commands are already being processed). It's probably better to allocate the maximum bandwidth possible instead.

Quoted 6 lines

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.

How does it differ? Is the length in each usb_iso_packet_descriptor consistently smaller than the max packet size the endpoint advertises? Or are there bursts of the maximum packet size and then zero length transfers?

Quoted 4 lines

In an other way, a driver could search the
alternate setting which matches the best its estimated bandwidth, but I am not sure that this will not slow down the frame rate (more USB packets for the same data volume).

Can you explain what you mean by this? I understand that there's more bus bandwidth overhead for transferring many small packets versus transferring few big packets, but I'm not sure what you're getting at.

Quoted 7 lines

Hi Sarah,

Your patch should be changed to follow the global altsetting fallback mechanism. This mechanism is inside the function gspca_init_transfer(). If usb_set_interface() returns an error when the bandwidth is not wide enough (BTW, which is this error?), it should called inside the loop and, so, removed from get_ep().

I'll look at how gspca_init_transfer() does the fallback and try to update the patch. I'm fine with it being dropped, now that the reasons have been explained.

Sarah Sharp

Reviewed-by Acked-by
#23 lore raw
In reply to Mauro Carvalho Chehab
Quoted 5 lines

From: Mauro Carvalho Chehab <[email protected]>

		== 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:

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

#24 lore raw
In reply to Mauro Carvalho Chehab

Hi,

Quoted 8 lines

Mauro Carvalho Chehab <[email protected]> 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

#25 lore raw
In reply to Mauro Carvalho Chehab
Quoted 1 line · Mauro Carvalho Chehab

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

#26 lore raw

Em 01-07-2010 08:46, Bjørn Mork escreveu:

Quoted 9 lines
Quoted 1 line · Mauro Carvalho Chehab

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?

Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 7 lines

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.

#27 lore raw
Quoted 4 lines · Mauro Carvalho Chehab

Em 01-07-2010 08:46, Bjørn Mork escreveu:

Quoted 1 line

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.

Quoted 10 lines
Quoted 7 lines

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.

Quoted 3 lines

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