[saa7134] do not change mute state for capturing audio
Commit Message
Hi Mauro, I've finally found the time (and an energy)
to go look into the automute breakage.
With the attached automute fix I no longer have
any problems with pulseaudio.
I also attached the patch that introduces an "std"
option to limit the scan list, resulting in a faster scan.
It is completely unrelated to the automute one, it is
here just in case.
What do you think?
Comments
Em 18-09-2011 12:18, Stas Sergeev escreveu:
> Hi Mauro, I've finally found the time (and an energy)
> to go look into the automute breakage.
> With the attached automute fix I no longer have
> any problems with pulseaudio.
> I also attached the patch that introduces an "std"
> option to limit the scan list, resulting in a faster scan.
> It is completely unrelated to the automute one, it is
> here just in case.
> What do you think?
Please, one patch per email. Patchwork (or any kernel maintainer script)
won't catch more than one patch per email. See:
http://patchwork.linuxtv.org/patch/7862/
As those are the two last patches marked as new at patchwork, I've manually
uploaded it as two separate emails, in order to allow me to queue them:
http://patchwork.linuxtv.org/patch/7940/
http://patchwork.linuxtv.org/patch/7941/
With respect to this patch:
http://patchwork.linuxtv.org/patch/7941/
I don't see any sense on it. Video standard selection is done by software,
when a standards mask is passed via VIDIOC_S_STD ioctl. Drivers should not
mess it with modprobe hacks.
I'll comment later http://patchwork.linuxtv.org/patch/7940/. It seems to be
going into the right direction, but I need to take a deeper code inspection
and maybe do some tests here.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
24.09.2011 14:57, Mauro Carvalho Chehab wrote:
> Please, one patch per email. Patchwork (or any kernel maintainer script)
> won't catch more than one patch per email. See:
Sorry about that.
> With respect to this patch:
> http://patchwork.linuxtv.org/patch/7941/
>
> I don't see any sense on it. Video standard selection is done by software,
> when a standards mask is passed via VIDIOC_S_STD ioctl. Drivers should not
> mess it with modprobe hacks.
Yes, but we already have "secam=" option, and
also the first scan, that is being done on driver
init, scans too much without that option, and
sometimes, unfortunately, detects the PAL carrier
for me.
By limiting it to secam, I avoid the problem and
shorten the scan time.
But this patch is not very important, so do whatever
you think necessary with it.
> I'll comment later http://patchwork.linuxtv.org/patch/7940/. It seems to be
> going into the right direction, but I need to take a deeper code inspection
> and maybe do some tests here.
Thanks!
Of course, in my view, the _only_ right direction is
to export the mute control to the alsa mixer and then
fix mplayer. But at least I'm glad I've managed to
find the hack that satisfies your opinion and works
around the problem at the same time.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 24-09-2011 08:12, Stas Sergeev escreveu:
> 24.09.2011 14:57, Mauro Carvalho Chehab wrote:
>> Please, one patch per email. Patchwork (or any kernel maintainer script)
>> won't catch more than one patch per email. See:
> Sorry about that.
>
>> With respect to this patch:
>> http://patchwork.linuxtv.org/patch/7941/
>>
>> I don't see any sense on it. Video standard selection is done by software,
>> when a standards mask is passed via VIDIOC_S_STD ioctl. Drivers should not
>> mess it with modprobe hacks.
> Yes, but we already have "secam=" option, and
> also the first scan, that is being done on driver
> init, scans too much without that option, and
> sometimes, unfortunately, detects the PAL carrier
> for me.
> By limiting it to secam, I avoid the problem and
> shorten the scan time.
> But this patch is not very important, so do whatever
> you think necessary with it.
The scan audio logic only enables multiple audio standard detection if the userspace
application tells it to do. The right fix here is to fix the application. The secam
hack is due to a problem related to Secam L and Secam L'.
>
>> I'll comment later http://patchwork.linuxtv.org/patch/7940/. It seems to be
>> going into the right direction, but I need to take a deeper code inspection
>> and maybe do some tests here.
> Thanks!
> Of course, in my view, the _only_ right direction is
> to export the mute control to the alsa mixer and then
> fix mplayer. But at least I'm glad I've managed to
> find the hack that satisfies your opinion and works
> around the problem at the same time.
The right fix that pulseaudio should not touch at the audio mixers for the
video boards. Not all boards have an audio carrier detection like saa7134.
Regards,
Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
24.09.2011 16:12, Mauro Carvalho Chehab wrote:
> The scan audio logic only enables multiple audio standard detection if the userspace
> application tells it to do.
No: the _first_ scan is done on the driver init.
It is a multi-standard one, and a long one, too.
Do we need it at all? It seems to me the results
of that scan are not even used, or what am I
missing?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 24-09-2011 09:36, Stas Sergeev escreveu:
> 24.09.2011 16:12, Mauro Carvalho Chehab wrote:
>> The scan audio logic only enables multiple audio standard detection if the userspace
>> application tells it to do.
> No: the _first_ scan is done on the driver init.
> It is a multi-standard one, and a long one, too.
> Do we need it at all? It seems to me the results
> of that scan are not even used, or what am I
> missing?
A first scan at driver's init can be removed, IMO. The thing is that
newer versions of udev will open the device, to do a VIDIOC_QUERYCAP.
Not sure if this will wake up the tvaudio kthread to do a scan.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
24.09.2011 16:48, Mauro Carvalho Chehab wrote:
> A first scan at driver's init can be removed, IMO.
OK, that's the great news.
Will write a new patch then.
> There's nothing the driver can do if the hardware
> missdetects a carrier. Dirty tricks to try solving it
> are not good, as they'll do the wrong thing on some situations.
Well, if we assume the first scan can be removed,
then we also assume the previous "dirty trick" is
harmless, as it affects only the first scan. But I'll
better remove both the trick and the first scan then,
as the fewer the hacks, the better the code.
> If someone is using the board on an environment
> without udev and pulseaudio, this trick will break the first tuning.
I feel this somehow contradicts with your suggestion
to remove the first scan, so could you clarify?
> Well, if you think that this would solve, then just write a patch
> exporting the mute control via ALSA. I have no problems with that.
That would solve all the problems, but only if:
1. The mplayer is then moved to the use of that new
control to not depend on the autounmute hack.
I can write the patch for that too.
2. Make sure all the other apps are fixed the same way
(I hope there are none though)
3. The autounmute hack is then removed. (no
regressions if steps 1 and 2 are carefully done)
If you are fine with that plan, then I'll try to find
the time and do the things that way. Otherwise,
I'll remove the first scan, and that will do the trick
in a simpler, though less cleaner way.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 24-09-2011 10:20, Stas Sergeev escreveu:
> 24.09.2011 16:48, Mauro Carvalho Chehab wrote:
>> A first scan at driver's init can be removed, IMO.
> OK, that's the great news.
> Will write a new patch then.
OK.
>
>> There's nothing the driver can do if the hardware
>> missdetects a carrier. Dirty tricks to try solving it
>> are not good, as they'll do the wrong thing on some situations.
> Well, if we assume the first scan can be removed,
> then we also assume the previous "dirty trick" is
> harmless, as it affects only the first scan. But I'll
> better remove both the trick and the first scan then,
> as the fewer the hacks, the better the code.
Yes.
>> If someone is using the board on an environment
>> without udev and pulseaudio, this trick will break the first tuning.
> I feel this somehow contradicts with your suggestion
> to remove the first scan, so could you clarify?
What I meant to say is that both udev and pulseaudio opens the device,
and these might initialize the audio thread. The driver should be able
to work the same way with or without the first open by udev/pulseaudio.
>> Well, if you think that this would solve, then just write a patch
>> exporting the mute control via ALSA. I have no problems with that.
> That would solve all the problems, but only if:
> 1. The mplayer is then moved to the use of that new
> control to not depend on the autounmute hack.
> I can write the patch for that too.
The autounmute is not a hack. It is a logic to suppress audio when the
audio carrier is not detected. It should not be removed.
I'm not sure if it is safe to make mplayer to use the audio mixer. It
is probably a good idea doing that, as it will also work fine with webcams
that provide alsa inputs.
> 2. Make sure all the other apps are fixed the same way
> (I hope there are none though)
> 3. The autounmute hack is then removed. (no
> regressions if steps 1 and 2 are carefully done)
>
> If you are fine with that plan, then I'll try to find
> the time and do the things that way. Otherwise,
> I'll remove the first scan, and that will do the trick
> in a simpler, though less cleaner way.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
24.09.2011 19:09, Mauro Carvalho Chehab wrote:
>>> If someone is using the board on an environment
>>> without udev and pulseaudio, this trick will break the first tuning.
>> I feel this somehow contradicts with your suggestion
>> to remove the first scan, so could you clarify?
> What I meant to say is that both udev and pulseaudio opens the device,
> and these might initialize the audio thread. The driver should be able
> to work the same way with or without the first open by udev/pulseaudio.
But the first scan I was referring to, and am going
to remove, happens not on the device open, but on
the driver init (modprobe time).
open()s are safe, fortunately.
> The autounmute is not a hack. It is a logic to suppress audio when the
> audio carrier is not detected. It should not be removed.
You are confusing the automute and autoUNmute.
Autounmute is a must-die hack, and we only need
to fix mplayer first. Automute just needs a fix.
Though I'd personally remove the automute too, by
exporting some interface for an app to query the
signal strength... but that's another story. :)
> I'm not sure if it is safe to make mplayer to use the audio mixer.
Why, if otherwise it already uses alsa in our case?
The mixer control is just another part of an alsa
interface, and it is already exported to the v4l apps, so...
> It
> is probably a good idea doing that, as it will also work fine with webcams
> that provide alsa inputs.
And will make pulseaudio happy, that's for sure. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From 70709f12f7161c98cb7ebae104b520dc30c6bd53 Mon Sep 17 00:00:00 2001
From: Stas Sergeev <stsp@users.sourceforge.net>
Date: Sun, 18 Sep 2011 19:13:05 +0400
Subject: [PATCH 2/2] saa7134: introduce "std" module parameter to force video
std
---
drivers/media/video/saa7134/saa7134-video.c | 39 ++++++++++++++++++++++++--
drivers/media/video/saa7134/saa7134.h | 1 +
2 files changed, 37 insertions(+), 3 deletions(-)
@@ -40,12 +40,15 @@ static unsigned int noninterlaced; /* 0 */
static unsigned int gbufsize = 720*576*4;
static unsigned int gbufsize_max = 720*576*4;
static char secam[] = "--";
+static char std[16] = "--";
module_param(video_debug, int, 0644);
MODULE_PARM_DESC(video_debug,"enable debug messages [video]");
module_param(gbuffers, int, 0444);
MODULE_PARM_DESC(gbuffers,"number of capture buffers, range 2-32");
module_param(noninterlaced, int, 0644);
MODULE_PARM_DESC(noninterlaced,"capture non interlaced video");
+module_param_string(std, std, sizeof(std), 0644);
+MODULE_PARM_DESC(secam, "force TV standard, either PAL, SECAM or NTSC");
module_param_string(secam, secam, sizeof(secam), 0644);
MODULE_PARM_DESC(secam, "force SECAM variant, either DK,L or Lc");
@@ -1847,14 +1850,20 @@ int saa7134_s_std_internal(struct saa7134_dev *dev, struct saa7134_fh *fh, v4l2_
return -EBUSY;
}
- for (i = 0; i < TVNORMS; i++)
+ for (i = 0; i < TVNORMS; i++) {
+ if (!tvnorms[i].enabled)
+ continue;
if (*id == tvnorms[i].id)
break;
+ }
if (i == TVNORMS)
- for (i = 0; i < TVNORMS; i++)
+ for (i = 0; i < TVNORMS; i++) {
+ if (!tvnorms[i].enabled)
+ continue;
if (*id & tvnorms[i].id)
break;
+ }
if (i == TVNORMS)
return -EINVAL;
@@ -1871,6 +1880,8 @@ int saa7134_s_std_internal(struct saa7134_dev *dev, struct saa7134_fh *fh, v4l2_
fixup = V4L2_STD_SECAM;
}
for (i = 0; i < TVNORMS; i++) {
+ if (!tvnorms[i].enabled)
+ continue;
if (fixup == tvnorms[i].id)
break;
}
@@ -2579,8 +2590,30 @@ int saa7134_videoport_init(struct saa7134_dev *dev)
int saa7134_video_init2(struct saa7134_dev *dev)
{
+ int i, idx = -1;
+ for (i = 0; i < TVNORMS; i++) {
+ int enable = (std[0] == '-' || strncasecmp(std,
+ tvnorms[i].name, strlen(std)) == 0);
+
+ if (secam[0] != '-') {
+ char buf[16] = "SECAM";
+ if (strncasecmp(buf, tvnorms[i].name,
+ strlen(buf)) == 0) {
+ strcat(buf, "-");
+ strcat(buf, secam);
+ if (strncasecmp(buf, tvnorms[i].name,
+ strlen(buf)) != 0)
+ enable = 0;
+ }
+ }
+ tvnorms[i].enabled = enable;
+ if (enable && idx == -1)
+ idx = i;
+ }
+ if (idx == -1)
+ return -EINVAL;
/* init video hw */
- set_tvnorm(dev,&tvnorms[0]);
+ set_tvnorm(dev,&tvnorms[idx]);
video_mux(dev,0);
saa7134_tvaudio_setmute(dev);
saa7134_tvaudio_setvolume(dev,dev->ctl_volume);
@@ -77,6 +77,7 @@ enum saa7134_video_out {
struct saa7134_tvnorm {
char *name;
v4l2_std_id id;
+ int enabled;
/* video decoder */
unsigned int sync_control;
--
1.7.6