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

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

[Mauro Carvalho Chehab]

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 <abraham.manu@gmail.com> 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 <crope@iki.fi> review == 

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

		== Waiting for Tobias Lorenz <tobias.lorenz@gmx.net> 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 <davidtlwong@gmail.com>  == 

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

		== Andy Walls <awalls@radix.net> and Aleksandr Piskunov <aleksandr.v.piskunov@gmail.com> are discussing around the solution == 

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

		== Waiting for Andy Walls <awalls@md.metrocast.net> == 

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

		== Patches waiting for Patrick Boettcher <pboettcher@dibcom.fr> review == 

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

		== Gspca patches - Waiting Hans de Goede <hdegoede@redhat.com> 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 <moinejf@free.fr> 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 <g.liakhovetski@gmx.de> 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 <stoth@kernellabs.com> comments on it == 

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

		== Waiting for Henrik Kurelid <henrik@kurelid.se> 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 <isely@isely.net> 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 <liplianin@tut.by> 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 <u.kleine-koenig@pengutronix.de> == 

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 <abraham.manu@gmail.com> submission == 

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

		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <herton@mandriva.com.br> 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 linux-media@vger.kernel.org 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.

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

[Guennadi Liakhovetski]

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 <g.liakhovetski@gmx.de> 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/

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

[Mauro Carvalho Chehab]

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 <g.liakhovetski@gmx.de> 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

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

[Manu Abraham]

On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> 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 <manu@linuxtv.org>
Acked-by: Manu Abraham <manu@linuxtv.org>

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

[Mauro Carvalho Chehab]

Manu Abraham wrote:
> On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> 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 <manu@linuxtv.org>
> Acked-by: Manu Abraham <manu@linuxtv.org>

Applied, thanks.

-- 

Cheers,
Mauro

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

[Manu Abraham]

On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> 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 ?

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

[Mauro Carvalho Chehab]

Manu Abraham wrote:
> On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> 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

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

[Guy Martin]

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 <mchehab@redhat.com> wrote:

> Manu Abraham wrote:
> > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
> > <mchehab@redhat.com> 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.
> 

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

[Mauro Carvalho Chehab]

Em Thu, 27 May 2010 16:05:48 +0200
Guy Martin <gmsoft@tuxicoman.be> 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 <mchehab@redhat.com> wrote:
> 
> > Manu Abraham wrote:
> > > On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
> > > <mchehab@redhat.com> 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

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

[Manu Abraham]

On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> 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 <abraham.manu@gmail.com> 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

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

[Mauro Carvalho Chehab]

Manu Abraham wrote:
> On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> 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 <abraham.manu@gmail.com> 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

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

[Manu Abraham]

On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> 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 <manu@linuxtv.org>
Acked-by: Manu Abraham <manu@linuxtv.org>

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

[Mauro Carvalho Chehab]

Manu Abraham wrote:
> On Fri, May 7, 2010 at 4:39 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> 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 <manu@linuxtv.org>
> Acked-by: Manu Abraham <manu@linuxtv.org>

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

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

[Herton Ronaldo Krzesinski]

Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu:
> 		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <herton@mandriva.com.br> 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

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

[Mauro Carvalho Chehab]

Herton Ronaldo Krzesinski wrote:
> Em Sex 07 Mai 2010, às 09:39:16, Mauro Carvalho Chehab escreveu:
>> 		== Patch(broken) - waiting for Herton Ronaldo Krzesinski <herton@mandriva.com.br> 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

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

[Herton Ronaldo Krzesinski]

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 <herton@mandriva.com.br> 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

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

[Mauro Carvalho Chehab]

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 <herton@mandriva.com.br> 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

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

[Jean-Francois Moine]

On Fri, 7 May 2010 09:39:16 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> wrote:

> 		== Gspca patches - Waiting Jean-Francois Moine
> <moinejf@free.fr> 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/

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

[Mauro Carvalho Chehab]

Jean-Francois Moine wrote:
> On Fri, 7 May 2010 09:39:16 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> 
>> 		== Gspca patches - Waiting Jean-Francois Moine
>> <moinejf@free.fr> 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

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

[Sarah Sharp]

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 <mchehab@redhat.com> wrote:
> > 
> >> 		== Gspca patches - Waiting Jean-Francois Moine
> >> <moinejf@free.fr> 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

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

[Mauro Carvalho Chehab]

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 <mchehab@redhat.com> wrote:
>>>
>>>> 		== Gspca patches - Waiting Jean-Francois Moine
>>>> <moinejf@free.fr> 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

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

[Jean-Francois Moine]

On Mon, 10 May 2010 13:25:00 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> 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 <mchehab@redhat.com> wrote:
> >>>
> >>>> 		== Gspca patches - Waiting Jean-Francois Moine
> >>>> <moinejf@free.fr> 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 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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

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

[Sarah Sharp]

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 <mchehab@redhat.com> 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 <mchehab@redhat.com> wrote:
> > >>>
> > >>>> 		== Gspca patches - Waiting Jean-Francois Moine
> > >>>> <moinejf@free.fr> 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 bulk transfers are
scheduled, since it is all done in hardware.

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

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.

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

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

> 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

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

[Pawel Osciak]

>From: Mauro Carvalho Chehab <mchehab@redhat.com>

>		== 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 <p.osciak@samsung.com>
Acked-by: Pawel Osciak <p.osciak@samsung.com>


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

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

[Pawel Osciak]

Hi,

>Mauro Carvalho Chehab <mchehab@redhat.com> 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

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

[Bjørn Mork]

Mauro Carvalho Chehab <mchehab@redhat.com> 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

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

[Mauro Carvalho Chehab]

Em 01-07-2010 08:46, Bjørn Mork escreveu:
> Mauro Carvalho Chehab <mchehab@redhat.com> 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.

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

[Bjørn Mork]

Mauro Carvalho Chehab <mchehab@redhat.com> 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