Am 16.01.2014 00:11, schrieb Mauro Carvalho Chehab:
Em Wed, 15 Jan 2014 22:36:25 +0100
Frank Schäfer<fschaefer.oss@xxxxxxxxxxxxxx> escreveu:
Am 15.01.2014 22:31, schrieb Frank Schäfer:
em28xx-audio currently switches to usb alternate setting #7 in case of a mixed
interface. This may overwrite the setting made by the video part and break video
streaming.
As far as we know, there is no difference between the alt settings with regards
to the audio endpoint if the interface is a mixed interface, the audio part only
has to make sure that alt is > 0, which is fortunately only the case when video
streaming is off.
Signed-off-by: Frank Schäfer<fschaefer.oss@xxxxxxxxxxxxxx>
---
drivers/media/usb/em28xx/em28xx-audio.c | 41 ++++++++++++-------------------
1 Datei geändert, 16 Zeilen hinzugefügt(+), 25 Zeilen entfernt(-)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 05e9bd1..2e7a3ad 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -266,33 +266,30 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
dprintk("opening device and trying to acquire exclusive lock\n");
runtime->hw = snd_em28xx_hw_capture;
- if ((dev->alt == 0 || dev->is_audio_only) && dev->adev.users == 0) {
- int nonblock = !!(substream->f_flags & O_NONBLOCK);
+ if (dev->adev.users == 0) {
+ int nonblock = !!(substream->f_flags & O_NONBLOCK);
if (nonblock) {
if (!mutex_trylock(&dev->lock))
return -EAGAIN;
} else
mutex_lock(&dev->lock);
- if (dev->is_audio_only)
- /* vendor audio is on a separate interface */
+
+ /* Select initial alternate setting (if necessary) */
+ if (dev->alt == 0) {
dev->alt = 1;
- else
- /* vendor audio is on the same interface as video */
- dev->alt = 7;
/*
- * FIXME: The intention seems to be to select the alt
- * setting with the largest wMaxPacketSize for the video
- * endpoint.
- * At least dev->alt should be used instead, but we
- * should probably not touch it at all if it is
- * already >0, because wMaxPacketSize of the audio
- * endpoints seems to be the same for all.
+ * NOTE: in case of a mixed (audio+video) interface, we
+ * don't want to touch the alt setting made by the video
+ * part. There is no difference between the alt settings
+ * with regards to the audio endpoint.
+ * TODO: in case of a pure audio interface, this could
+ * be improved. The alt settings are different here.
*/
-
- dprintk("changing alternate number on interface %d to %d\n",
- dev->ifnum, dev->alt);
- usb_set_interface(dev->udev, dev->ifnum, dev->alt);
+ dprintk("changing alternate number on interface %d to %d\n",
+ dev->ifnum, dev->alt);
+ usb_set_interface(dev->udev, dev->ifnum, dev->alt);
+ }
/* Sets volume, mute, etc */
dev->mute = 0;
@@ -740,15 +737,9 @@ static int em28xx_audio_urb_init(struct em28xx *dev)
struct usb_endpoint_descriptor *e, *ep = NULL;
int i, ep_size, interval, num_urb, npackets;
int urb_size, bytes_per_transfer;
- u8 alt;
-
- if (dev->ifnum)
- alt = 1;
- else
- alt = 7;
+ u8 alt = 1;
intf = usb_ifnum_to_if(dev->udev, dev->ifnum);
-
if (intf->num_altsetting <= alt) {
em28xx_errdev("alt %d doesn't exist on interface %d\n",
dev->ifnum, alt);
Please note that this is actually just a minor fix.
What's really evil with the current alternate setting code is that the
video part may switch the alt setting while audio streaming is in progress.
I'm not sure how to fix this. Maybe we shouldn't start audio streaming
before video streaming.
This patch will very likely break em28xx audio. The change to use alt=7
was added there because em28xx can only deliver a certain number of URBs
per a given period of time. In other words, if the video-only calculated
alternate is used, when audio starts, the em28xx DMA engine half-fills
some video URBs.
In case of a mixed (audio+video) interface:
If the audio part changes the alt setting and starts streaming before
the video part, it doesn't matter which alt setting is selected as long
as it is > 0.
If the video part changes the alt setting and starts streaming before
the audio part, it doesn care about audio at all and overwrites alt=7
with it's own calculated value.
As I said, the right fix here is to have a bandwidth estimator that will
take both traffics into account (when both are activated), and select
the right alternate.
More than that: both parts (audio and video) need to consider that the
other one is already streaming at the current alt setting.
Such patch should be tested with more than one device type, as I think
that em284x are somewhat different than em286x and em288x with this
regards.
Indeed, that's why I marked it as RFT. :-)
Anyway, I've misread the code. The current code of course already makes
sure that the audio part doesn't touch the alt setting for mixed
interfaces as long as it is > 0.
The only thing it really changes is that alt=1 is selected for both
interface types but that's probably not worth beeing fixed. Most devices
seem to provide 7 alt settings.
So please disregard this patch.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
