Some fixes for tuner, tvp5150 and em28xx

PATCH In review Mauro Carvalho Chehab 3 participants 6 messages Feb 22, 2011 Media (V4L/DVB)

Share & tools

Mauro Carvalho Chehab

author

2011-02-22 02:21 UTC #0 lore raw

cover Some fixes for tuner, tvp5150 and em28xx

This series contain a minor cleanup for tuner and tvp5150, and two fixes for em28xx controls. Before the em28xx patches, s_ctrl were failing on qv4l2, because it were returning a positive value of 1 for some calls.

It also contains a fix for control get/set, as it will now check if the control exists before actually calling subdev for get/set.

Mauro Carvalho Chehab (4):
[media] tuner: Remove remaining usages of T_DIGITAL_TV [media] tvp5150: device detection should be done only once [media] em28xx: Fix return value for s_ctrl [media] em28xx: properly handle subdev controls

 drivers/media/common/tuners/tuner-xc2028.c |    8 ++--
 drivers/media/video/em28xx/em28xx-video.c  |   35 ++++++++++++++++++++---
 drivers/media/video/tvp5150.c              |   42 ++++++++++++++--------------
 drivers/staging/tm6000/tm6000-cards.c      |    2 -
 include/media/tuner.h                      |    2 +-
 5 files changed, 56 insertions(+), 33 deletions(-)

Mauro Carvalho Chehab

author

2011-02-22 02:52 UTC #1 lore raw

Em 21-02-2011 23:17, Mauro Carvalho Chehab escreveu:

Quoted 8 lines

This series contain a minor cleanup for tuner and tvp5150, and two fixes for em28xx controls. Before the em28xx patches, s_ctrl were failing on qv4l2, because it were returning a positive value of 1 for some calls.
It also contains a fix for control get/set, as it will now check if the control exists before actually calling subdev for get/set.
Mauro Carvalho Chehab (4):

...

Quoted 2 lines

[media] em28xx: Fix return value for s_ctrl
[media] em28xx: properly handle subdev controls

Hans,

I discovered the issue with em28xx that I commented you on IRC.

There were, in fact, 3 issues.

One is clearly a driver problem, corrected by "em28xx: properly handle subdev controls".

The second one being partially qv4l2 and partially driver issue, fixed by "em28xx: Fix return value for s_ctrl". Basically, V4L2 API and ioctl man page says that an error is indicated by -1 value, being 0 or positive value a non-error. Well, qv4l2 understands a positive value as -EBUSY. The driver were returning a non-standard value of 1 for s_ctrl. I fixed the driver part.

The last issue is with v4l2-ctl and qv4l2. Also, the latest version of xawtv had the same issue, probably due to some changes I did at console/v4l-info.c.

What happens is that em28xx doesn't implement the control BASE+4, due to one simple reason: it is not currently defined. The ctrl loop were understanding the -EINVAL return of BASE+4 as the end of the user controls. So, on xawtv, only the 3 image controls were returned. I didn't dig into v4l2-ctl, but there, it doesn't show the first 3 controls. It shows only the audio controls:

                         volume (int)  : min=0 max=65535 step=655 default=58880 value=58880 flags=slider
                        balance (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider
                           bass (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider
                         treble (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider

mute (bool) : default=0 value=0
loudness (bool) : default=0 value=0

The xawtv fix is at:

	http://git.linuxtv.org/xawtv3.git?a=commitdiff;h=fda070af9cfd75b360db1339bde3c6d3c64ed627

A similar fix is needed for v4l2-ctl and qv4l2.

The em28xx logs after the fix are:

em28xx #0 video: VIDIOC_QUERYCTRL id=0x980900, type=1, name=Brightness, min/max=0/255, step=1, default=128, flags=0x00000000 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980901, type=1, name=Contrast, min/max=0/255, step=1, default=128, flags=0x00000000 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980902, type=1, name=Saturation, min/max=0/255, step=1, default=128, flags=0x00000000 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980903, type=1, name=Hue, min/max=-128/127, step=1, default=0, flags=0x00000000 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980904
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980905, type=1, name=Volume, min/max=0/65535, step=655, default=58880, flags=0x00000020 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980906, type=1, name=Balance, min/max=0/65535, step=655, default=32768, flags=0x00000020 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980907, type=1, name=Bass, min/max=0/65535, step=655, default=32768, flags=0x00000020 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980908, type=1, name=Treble, min/max=0/65535, step=655, default=32768, flags=0x00000020 em28xx #0 video: VIDIOC_QUERYCTRL id=0x980909, type=2, name=Mute, min/max=0/1, step=1, default=0, flags=0x00000000 em28xx #0 video: VIDIOC_QUERYCTRL id=0x98090a, type=2, name=Loudness, min/max=0/1, step=1, default=0, flags=0x00000000 em28xx #0 video: VIDIOC_QUERYCTRL id=0x98090b
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98090c
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98090d
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98090e
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98090f
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980910
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980911
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980912
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980913
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980914
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980915
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980916
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980917
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980918
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980919
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98091a
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98091b
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98091c
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98091d
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98091e
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x98091f
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980920
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x980921
em28xx #0 video: VIDIOC_QUERYCTRL error -22
em28xx #0 video: VIDIOC_QUERYCTRL id=0x8000000
em28xx #0 video: VIDIOC_QUERYCTRL error -22

And v4l-info now reports everything:

controls
VIDIOC_QUERYCTRL(BASE+0)
id : 9963776
type : INTEGER
name : "Brightness"
minimum : 0
maximum : 255
step : 1
default_value : 128
flags : 0
VIDIOC_QUERYCTRL(BASE+1)
id : 9963777
type : INTEGER
name : "Contrast"
minimum : 0
maximum : 255
step : 1
default_value : 128
flags : 0
VIDIOC_QUERYCTRL(BASE+2)
id : 9963778
type : INTEGER
name : "Saturation"
minimum : 0
maximum : 255
step : 1
default_value : 128
flags : 0
VIDIOC_QUERYCTRL(BASE+3)
id : 9963779
type : INTEGER
name : "Hue"
minimum : -128
maximum : 127
step : 1
default_value : 0
flags : 0
VIDIOC_QUERYCTRL(BASE+5)
id : 9963781
type : INTEGER
name : "Volume"
minimum : 0
maximum : 65535
step : 655
default_value : 58880
flags : 32
VIDIOC_QUERYCTRL(BASE+6)
id : 9963782
type : INTEGER
name : "Balance"
minimum : 0
maximum : 65535
step : 655
default_value : 32768
flags : 32
VIDIOC_QUERYCTRL(BASE+7)
id : 9963783
type : INTEGER
name : "Bass"
minimum : 0
maximum : 65535
step : 655
default_value : 32768
flags : 32
VIDIOC_QUERYCTRL(BASE+8)
id : 9963784
type : INTEGER
name : "Treble"
minimum : 0
maximum : 65535
step : 655
default_value : 32768
flags : 32
VIDIOC_QUERYCTRL(BASE+9)
id : 9963785
type : BOOLEAN
name : "Mute"
minimum : 0
maximum : 1
step : 1
default_value : 0
flags : 0
VIDIOC_QUERYCTRL(BASE+10)
id : 9963786
type : BOOLEAN
name : "Loudness"
minimum : 0
maximum : 1
step : 1
default_value : 0
flags : 0

Hans Verkuil

2011-02-22 07:54 UTC #2 lore raw

Quoted 44 lines · On Tuesday, February 22, 2011 03:52:11 Mauro Carvalho Chehab wrote:

Em 21-02-2011 23:17, Mauro Carvalho Chehab escreveu:
Quoted 8 lines
This series contain a minor cleanup for tuner and tvp5150, and two fixes for em28xx controls. Before the em28xx patches, s_ctrl were failing on qv4l2, because it were returning a positive value of 1 for some calls.
It also contains a fix for control get/set, as it will now check if the control exists before actually calling subdev for get/set.
Mauro Carvalho Chehab (4):
...
Quoted 2 lines
[media] em28xx: Fix return value for s_ctrl
[media] em28xx: properly handle subdev controls
Hans,
I discovered the issue with em28xx that I commented you on IRC.
There were, in fact, 3 issues.
One is clearly a driver problem, corrected by "em28xx: properly handle subdev controls".
The second one being partially qv4l2 and partially driver issue, fixed by "em28xx: Fix return value for s_ctrl". Basically, V4L2 API and ioctl man page says that an error is indicated by -1 value, being 0 or positive value a non-error. Well, qv4l2 understands a positive value as -EBUSY. The driver were returning a non-standard value of 1 for s_ctrl. I fixed the driver part.
The last issue is with v4l2-ctl and qv4l2. Also, the latest version of xawtv had the same issue, probably due to some changes I did at console/v4l-info.c.
What happens is that em28xx doesn't implement the control BASE+4, due to one simple reason: it is not currently defined. The ctrl loop were understanding the -EINVAL return of BASE+4 as the end of the user controls. So, on xawtv, only the 3 image controls were returned. I didn't dig into v4l2-ctl, but there, it doesn't show the first 3 controls. It shows only the audio controls:
                         volume (int)  : min=0 max=65535 step=655 default=58880 value=58880 flags=slider
                        balance (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider
                           bass (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider
                         treble (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider

Quoted 7 lines

mute (bool) : default=0 value=0
loudness (bool) : default=0 value=0
The xawtv fix is at:
	http://git.linuxtv.org/xawtv3.git?a=commitdiff;h=fda070af9cfd75b360db1339bde3c6d3c64ed627
A similar fix is needed for v4l2-ctl and qv4l2.

Actually, v4l2-ctrl and qv4l2 handle 'holes' correctly. I think this is a different bug relating to the handling of V4L2_CTRL_FLAG_NEXT_CTRL. Can you try this patch:

drivers/media/video/v4l2-ctrls.c +2 -0

index ef66d2a..15eda86 100644
@@ -1364,6 +1364,8 @@ EXPORT_SYMBOL(v4l2_queryctrl);
 
 int v4l2_subdev_queryctrl(struct v4l2_subdev *sd, struct v4l2_queryctrl *qc)
 {
+	if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
+		return -EINVAL;
 	return v4l2_queryctrl(sd->ctrl_handler, qc);
 }
 EXPORT_SYMBOL(v4l2_subdev_queryctrl);

v4l2-ctl and qv4l2 enumerate the controls using this flag, falling back to the old method if the flag isn't supported. The v4l2_subdev_queryctrl function will currently handle that flag, but for the controls of the subdev only. This isn't right, it should refuse this flag. Without this fix v4l2-ctl will only see the controls of the first subdev, which is exactly what you got. I never saw this bug because the HVR900 has just a single subdev.

I also suspect that s_ctrl is wrong: can you test setting a video control? I think that v4l2_device_call_until_err will always return an error. I'm not sure if there is an easy fix for this other than converting em28xx to the control framework. I need to think about this.

Regards,

Hans

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Mauro Carvalho Chehab

author

2011-02-22 11:31 UTC #3 lore raw

Em 22-02-2011 04:53, Hans Verkuil escreveu:

Quoted 71 lines · On Tuesday, February 22, 2011 03:52:11 Mauro Carvalho Chehab wrote:

Quoted 44 lines
Em 21-02-2011 23:17, Mauro Carvalho Chehab escreveu:
Quoted 8 lines
This series contain a minor cleanup for tuner and tvp5150, and two fixes for em28xx controls. Before the em28xx patches, s_ctrl were failing on qv4l2, because it were returning a positive value of 1 for some calls.
It also contains a fix for control get/set, as it will now check if the control exists before actually calling subdev for get/set.
Mauro Carvalho Chehab (4):
...
Quoted 2 lines
[media] em28xx: Fix return value for s_ctrl
[media] em28xx: properly handle subdev controls
Hans,
I discovered the issue with em28xx that I commented you on IRC.
There were, in fact, 3 issues.
One is clearly a driver problem, corrected by "em28xx: properly handle subdev controls".
The second one being partially qv4l2 and partially driver issue, fixed by "em28xx: Fix return value for s_ctrl". Basically, V4L2 API and ioctl man page says that an error is indicated by -1 value, being 0 or positive value a non-error. Well, qv4l2 understands a positive value as -EBUSY. The driver were returning a non-standard value of 1 for s_ctrl. I fixed the driver part.
The last issue is with v4l2-ctl and qv4l2. Also, the latest version of xawtv had the same issue, probably due to some changes I did at console/v4l-info.c.
What happens is that em28xx doesn't implement the control BASE+4, due to one simple reason: it is not currently defined. The ctrl loop were understanding the -EINVAL return of BASE+4 as the end of the user controls. So, on xawtv, only the 3 image controls were returned. I didn't dig into v4l2-ctl, but there, it doesn't show the first 3 controls. It shows only the audio controls:
                         volume (int)  : min=0 max=65535 step=655 default=58880 value=58880 flags=slider
                        balance (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider
                           bass (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider
                         treble (int)  : min=0 max=65535 step=655 default=32768 value=32768 flags=slider
Quoted 7 lines
mute (bool) : default=0 value=0
loudness (bool) : default=0 value=0
The xawtv fix is at:
	http://git.linuxtv.org/xawtv3.git?a=commitdiff;h=fda070af9cfd75b360db1339bde3c6d3c64ed627
A similar fix is needed for v4l2-ctl and qv4l2.
Actually, v4l2-ctrl and qv4l2 handle 'holes' correctly. I think this is a different bug relating to the handling of V4L2_CTRL_FLAG_NEXT_CTRL. Can you try this patch:
drivers/media/video/v4l2-ctrls.c +2 -0
index ef66d2a..15eda86 100644
@@ -1364,6 +1364,8 @@ EXPORT_SYMBOL(v4l2_queryctrl);
 
 int v4l2_subdev_queryctrl(struct v4l2_subdev *sd, struct v4l2_queryctrl *qc)
 {
+	if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
+		return -EINVAL;
 	return v4l2_queryctrl(sd->ctrl_handler, qc);
 }
 EXPORT_SYMBOL(v4l2_subdev_queryctrl);

Quoted 6 lines

v4l2-ctl and qv4l2 enumerate the controls using this flag, falling back to the old method if the flag isn't supported. The v4l2_subdev_queryctrl function will currently handle that flag, but for the controls of the subdev only. This isn't right, it should refuse this flag. Without this fix v4l2-ctl will only see the controls of the first subdev, which is exactly what you got. I never saw this bug because the HVR900 has just a single subdev.

Ok, that makes sense. I'll test the patch and give you a feedback.

Quoted 4 lines

I also suspect that s_ctrl is wrong: can you test setting a video control? I think that v4l2_device_call_until_err will always return an error. I'm not sure if there is an easy fix for this other than converting em28xx to the control framework. I need to think about this.

I've changed it by two subdev calls. The first one queries for the control. If control type is zero, it returns an error, otherwise, it will call v4l2_device_all_all (see patch 4/4). This is sub-optimal, but should fix the bug, and can be sent to -stable.

Cheers,
Mauro

Mauro Carvalho Chehab

author

2011-02-22 12:12 UTC #4 lore raw

In reply to Hans Verkuil

Em 22-02-2011 04:53, Hans Verkuil escreveu:

Quoted 9 lines

Actually, v4l2-ctrl and qv4l2 handle 'holes' correctly. I think this is a different bug relating to the handling of V4L2_CTRL_FLAG_NEXT_CTRL. Can you try this patch:
drivers/media/video/v4l2-ctrls.c +0 -0
index ef66d2a..15eda86 100644

drivers/media/video/v4l2-ctrls.c EXPORT_SYMBOL(v4l2_queryctrl); · line 1369

Quoted 6 lines

drivers/media/video/v4l2-ctrls.c +2 -0

@@ -1364,6 +1364,8 @@ EXPORT_SYMBOL(v4l2_queryctrl);
 
 int v4l2_subdev_queryctrl(struct v4l2_subdev *sd, struct v4l2_queryctrl *qc)
 {
+	if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
+		return -EINVAL;
 	return v4l2_queryctrl(sd->ctrl_handler, qc);

Ok, this fixed the issue:

                     brightness (int)  : min=0 max=255 step=1 default=128 value=128
                       contrast (int)  : min=0 max=255 step=1 default=128 value=128
                     saturation (int)  : min=0 max=255 step=1 default=128 value=128
                            hue (int)  : min=-128 max=127 step=1 default=0 value=0
                         volume (int)  : min=0 max=65535 step=655 default=58880 value=65500 flags=slider
                        balance (int)  : min=0 max=65535 step=655 default=32768 value=32750 flags=slider
                           bass (int)  : min=0 max=65535 step=655 default=32768 value=32750 flags=slider
                         treble (int)  : min=0 max=65535 step=655 default=32768 value=32750 flags=slider

mute (bool) : default=0 value=0
loudness (bool) : default=0 value=0

Also, v4l2-compliance is now complaining less about it.

Control ioctls:
fail: does not support V4L2_CTRL_FLAG_NEXT_CTRL
test VIDIOC_QUERYCTRL/MENU: FAIL
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: Not Supported Standard Controls: 0 Private Controls: 0

(yet, it is showing "standard controls = 0").

Could you provide your SOB to the above patch?

Thanks!
Mauro

Hans Verkuil

2011-02-22 12:27 UTC #5 lore raw

Quoted 3 lines · On Tuesday, February 22, 2011 13:12:32 Mauro Carvalho Chehab wrote:

Em 22-02-2011 04:53, Hans Verkuil escreveu:
Quoted 2 lines
Actually, v4l2-ctrl and qv4l2 handle 'holes' correctly. I think this is a different bug relating to the handling of V4L2_CTRL_FLAG_NEXT_CTRL. Can

you

Quoted 3 lines

drivers/media/video/v4l2-
Quoted 3 lines
try this patch:
drivers/media/video/v4l2- renamed from drivers/media/video/v4l2-ctrls.c +0 -0

ctrls.c

Quoted 6 lines

drivers/media/video/v4l2-ctrls.c EXPORT_SYMBOL(v4l2_queryctrl); · line 1365
Quoted 6 lines
index ef66d2a..15eda86 100644
drivers/media/video/v4l2-ctrls.c +0 -0
@@ -1364,6 +1364,8 @@ EXPORT_SYMBOL(v4l2_queryctrl);
 
 int v4l2_subdev_queryctrl(struct v4l2_subdev *sd, struct v4l2_queryctrl 

*qc)

Quoted 7 lines

drivers/media/video/v4l2-ctrls.c EXPORT_SYMBOL(v4l2_queryctrl); · line 1369
Quoted 4 lines
drivers/media/video/v4l2-ctrls.c +2 -0
 {
+	if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
+		return -EINVAL;
 	return v4l2_queryctrl(sd->ctrl_handler, qc);
Ok, this fixed the issue:
                     brightness (int)  : min=0 max=255 step=1 default=128 

value=128

Quoted 1 line

                       contrast (int)  : min=0 max=255 step=1 default=128

value=128

Quoted 1 line

                     saturation (int)  : min=0 max=255 step=1 default=128

value=128

Quoted 1 line

                            hue (int)  : min=-128 max=127 step=1 default=0

value=0

Quoted 1 line

                         volume (int)  : min=0 max=65535 step=655

default=58880 value=65500 flags=slider

Quoted 1 line

                        balance (int)  : min=0 max=65535 step=655

default=32768 value=32750 flags=slider

Quoted 1 line

                           bass (int)  : min=0 max=65535 step=655

default=32768 value=32750 flags=slider

Quoted 1 line

                         treble (int)  : min=0 max=65535 step=655

default=32768 value=32750 flags=slider

Quoted 12 lines

mute (bool) : default=0 value=0
loudness (bool) : default=0 value=0
Also, v4l2-compliance is now complaining less about it.
Control ioctls:
fail: does not support V4L2_CTRL_FLAG_NEXT_CTRL
test VIDIOC_QUERYCTRL/MENU: FAIL
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: Not Supported Standard Controls: 0 Private Controls: 0

Quoted 3 lines

(yet, it is showing "standard controls = 0").
Could you provide your SOB to the above patch?

Signed-off-by Hans Verkuil <[email protected]>

Quoted 8 lines

Thanks!
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html