* [PATCH 0/4] Fix em28xx audio when using a xHCI USB port
@ 2014-01-10 18:45 Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 1/4] em28xx: use bInterval on em28xx-audio Mauro Carvalho Chehab
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-01-10 18:45 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Mauro Carvalho Chehab
There are some bugs preventing the usage of audio with a xHCI port:
1) The URB interval equal to 1 means a IRQ rate of 64 x 125 us, with
xHCI (a period of 8ms), while it means a period of 64 ms with EHCI;
2) The period value was never 64 bytes.
Properly address the two above issues and prepare the driver to estimate
all parameters in realtime.
Mauro Carvalho Chehab (4):
em28xx: use bInterval on em28xx-audio
em28xx-audio: Fix error path
em28xx: don't hardcode audio URB calculus
em28x-audio: fix the period size in bytes
drivers/media/usb/em28xx/em28xx-audio.c | 187 +++++++++++++++++++++++++++-----
drivers/media/usb/em28xx/em28xx.h | 8 +-
2 files changed, 160 insertions(+), 35 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/4] em28xx: use bInterval on em28xx-audio
2014-01-10 18:45 [PATCH 0/4] Fix em28xx audio when using a xHCI USB port Mauro Carvalho Chehab
@ 2014-01-10 18:45 ` Mauro Carvalho Chehab
2014-01-11 13:37 ` Frank Schäfer
2014-01-10 18:45 ` [PATCH 2/4] em28xx-audio: Fix error path Mauro Carvalho Chehab
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-01-10 18:45 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Mauro Carvalho Chehab
Just filling urb->interval with 1 is wrong, and causes a different
behaviour with xHCI.
With EHCI, the URB size is typically 192 bytes. However, as
xHCI specifies intervals in microframes, the URB size becomes
too short (24 bytes).
With this patch, the interval will be properly initialized, and
the device will behave the same if connected into a xHCI or an
EHCI device port.
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
drivers/media/usb/em28xx/em28xx-audio.c | 39 ++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 30ee389a07f0..8e6f04873422 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -620,10 +620,13 @@ static int em28xx_audio_init(struct em28xx *dev)
struct em28xx_audio *adev = &dev->adev;
struct snd_pcm *pcm;
struct snd_card *card;
+ struct usb_interface *intf;
+ struct usb_endpoint_descriptor *e, *ep = NULL;
static int devnr;
int err, i;
const int sb_size = EM28XX_NUM_AUDIO_PACKETS *
EM28XX_AUDIO_MAX_PACKET_SIZE;
+ u8 alt;
if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
/* This device does not support the extension (in this case
@@ -679,6 +682,34 @@ static int em28xx_audio_init(struct em28xx *dev)
em28xx_cvol_new(card, dev, "Surround", AC97_SURROUND_MASTER);
}
+ if (dev->audio_ifnum)
+ alt = 1;
+ else
+ alt = 7;
+
+ intf = usb_ifnum_to_if(dev->udev, dev->audio_ifnum);
+
+ if (intf->num_altsetting <= alt) {
+ em28xx_errdev("alt %d doesn't exist on interface %d\n",
+ dev->audio_ifnum, alt);
+ return -ENODEV;
+ }
+
+ for (i = 0; i < intf->altsetting[alt].desc.bNumEndpoints; i++) {
+ e = &intf->altsetting[alt].endpoint[i].desc;
+ if (!usb_endpoint_dir_in(e))
+ continue;
+ if (e->bEndpointAddress == EM28XX_EP_AUDIO) {
+ ep = e;
+ break;
+ }
+ }
+
+ if (!ep) {
+ em28xx_errdev("Couldn't find an audio endpoint");
+ return -ENODEV;
+ }
+
/* Alloc URB and transfer buffers */
for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
struct urb *urb;
@@ -707,11 +738,17 @@ static int em28xx_audio_init(struct em28xx *dev)
urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
urb->transfer_buffer = dev->adev.transfer_buffer[i];
- urb->interval = 1;
+ urb->interval = 1 << (ep->bInterval - 1);
urb->complete = em28xx_audio_isocirq;
urb->number_of_packets = EM28XX_NUM_AUDIO_PACKETS;
urb->transfer_buffer_length = sb_size;
+ if (!i)
+ dprintk("Will use ep 0x%02x on intf %d alt %d interval = %d (rcv isoc pipe: 0x%08x)\n",
+ EM28XX_EP_AUDIO, dev->audio_ifnum, alt,
+ urb->interval,
+ urb->pipe);
+
for (j = k = 0; j < EM28XX_NUM_AUDIO_PACKETS;
j++, k += EM28XX_AUDIO_MAX_PACKET_SIZE) {
urb->iso_frame_desc[j].offset = k;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/4] em28xx-audio: Fix error path
2014-01-10 18:45 [PATCH 0/4] Fix em28xx audio when using a xHCI USB port Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 1/4] em28xx: use bInterval on em28xx-audio Mauro Carvalho Chehab
@ 2014-01-10 18:45 ` Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 3/4] em28xx: don't hardcode audio URB calculus Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 4/4] em28x-audio: fix the period size in bytes Mauro Carvalho Chehab
3 siblings, 0 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-01-10 18:45 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Mauro Carvalho Chehab
De-allocate memory and free sound if an error happens.
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
drivers/media/usb/em28xx/em28xx-audio.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 8e6f04873422..4ee3488960e1 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -692,6 +692,7 @@ static int em28xx_audio_init(struct em28xx *dev)
if (intf->num_altsetting <= alt) {
em28xx_errdev("alt %d doesn't exist on interface %d\n",
dev->audio_ifnum, alt);
+ snd_card_free(card);
return -ENODEV;
}
@@ -707,6 +708,7 @@ static int em28xx_audio_init(struct em28xx *dev)
if (!ep) {
em28xx_errdev("Couldn't find an audio endpoint");
+ snd_card_free(card);
return -ENODEV;
}
@@ -759,6 +761,7 @@ static int em28xx_audio_init(struct em28xx *dev)
err = snd_card_register(card);
if (err < 0) {
+ em28xx_audio_free_urb(dev);
snd_card_free(card);
return err;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/4] em28xx: don't hardcode audio URB calculus
2014-01-10 18:45 [PATCH 0/4] Fix em28xx audio when using a xHCI USB port Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 1/4] em28xx: use bInterval on em28xx-audio Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 2/4] em28xx-audio: Fix error path Mauro Carvalho Chehab
@ 2014-01-10 18:45 ` Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 4/4] em28x-audio: fix the period size in bytes Mauro Carvalho Chehab
3 siblings, 0 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-01-10 18:45 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Mauro Carvalho Chehab
The current code hardcodes the number of audio URBs, the number
of packets per URB and the maximum URB size.
This is not a good idea, as it:
- wastes more bandwidth than necessary, by using a very
large number of packets;
- those constants are bound to an specific scenario, with
a bandwidth of 48 kHz;
- don't take the maximum endpoint size into account;
- with urb->interval = 1 on xHCI, those constraints cause a "funny"
setup: URBs with 64 packets inside, with only 24 bytes total. E. g.
a complete waste of space.
Change the code to do dynamic URB audio calculus and allocation.
For now, use the same constraints as used before this patch, to
avoid regressions.
A good scenario (tested) seems to use those defines, instead:
#define EM28XX_MAX_AUDIO_BUFS 8
#define EM28XX_MIN_AUDIO_PACKETS 2
But let's not do such change here, letting the optimization to
happen on latter patches, after more tests.
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
drivers/media/usb/em28xx/em28xx-audio.c | 141 ++++++++++++++++++++++++--------
drivers/media/usb/em28xx/em28xx.h | 8 +-
2 files changed, 111 insertions(+), 38 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 4ee3488960e1..6c7d34600294 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -50,6 +50,8 @@ static int debug;
module_param(debug, int, 0644);
MODULE_PARM_DESC(debug, "activates debug info");
+#define EM28XX_MAX_AUDIO_BUFS 5
+#define EM28XX_MIN_AUDIO_PACKETS 64
#define dprintk(fmt, arg...) do { \
if (debug) \
printk(KERN_INFO "em28xx-audio %s: " fmt, \
@@ -63,7 +65,7 @@ static int em28xx_deinit_isoc_audio(struct em28xx *dev)
int i;
dprintk("Stopping isoc\n");
- for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
+ for (i = 0; i < dev->adev.num_urb; i++) {
struct urb *urb = dev->adev.urb[i];
if (!irqs_disabled())
@@ -168,7 +170,7 @@ static int em28xx_init_audio_isoc(struct em28xx *dev)
dprintk("Starting isoc transfers\n");
/* Start streaming */
- for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
+ for (i = 0; i < dev->adev.num_urb; i++) {
memset(dev->adev.transfer_buffer[i], 0x80,
dev->adev.urb[i]->transfer_buffer_length);
@@ -598,21 +600,35 @@ static void em28xx_audio_free_urb(struct em28xx *dev)
{
int i;
- for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
+ for (i = 0; i < dev->adev.num_urb; i++) {
struct urb *urb = dev->adev.urb[i];
- if (!dev->adev.urb[i])
+ if (!urb)
continue;
- usb_free_coherent(dev->udev,
- urb->transfer_buffer_length,
- dev->adev.transfer_buffer[i],
- urb->transfer_dma);
+ if (dev->adev.transfer_buffer[i])
+ usb_free_coherent(dev->udev,
+ urb->transfer_buffer_length,
+ dev->adev.transfer_buffer[i],
+ urb->transfer_dma);
usb_free_urb(urb);
- dev->adev.urb[i] = NULL;
- dev->adev.transfer_buffer[i] = NULL;
}
+ kfree(dev->adev.urb);
+ kfree(dev->adev.transfer_buffer);
+ dev->adev.num_urb = 0;
+}
+
+/* high bandwidth multiplier, as encoded in highspeed endpoint descriptors */
+static int em28xx_audio_ep_packet_size(struct usb_device *udev,
+ struct usb_endpoint_descriptor *e)
+{
+ int size = le16_to_cpu(e->wMaxPacketSize);
+
+ if (udev->speed == USB_SPEED_HIGH)
+ return (size & 0x7ff) * (1 + (((size) >> 11) & 0x03));
+
+ return size & 0x7ff;
}
static int em28xx_audio_init(struct em28xx *dev)
@@ -623,9 +639,8 @@ static int em28xx_audio_init(struct em28xx *dev)
struct usb_interface *intf;
struct usb_endpoint_descriptor *e, *ep = NULL;
static int devnr;
- int err, i;
- const int sb_size = EM28XX_NUM_AUDIO_PACKETS *
- EM28XX_AUDIO_MAX_PACKET_SIZE;
+ int err, i, ep_size, interval, num_urb, npackets;
+ int urb_size, bytes_per_transfer;
u8 alt;
if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
@@ -648,6 +663,9 @@ static int em28xx_audio_init(struct em28xx *dev)
return err;
spin_lock_init(&adev->slock);
+ adev->sndcard = card;
+ adev->udev = dev->udev;
+
err = snd_pcm_new(card, "Em28xx Audio", 0, 0, 1, &pcm);
if (err < 0) {
snd_card_free(card);
@@ -712,25 +730,92 @@ static int em28xx_audio_init(struct em28xx *dev)
return -ENODEV;
}
- /* Alloc URB and transfer buffers */
- for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
+ ep_size = em28xx_audio_ep_packet_size(dev->udev, ep);
+ interval = 1 << (ep->bInterval - 1);
+
+ em28xx_info("Endpoint 0x%02x %s on intf %d alt %d interval = %d, size %d\n",
+ EM28XX_EP_AUDIO, usb_speed_string(dev->udev->speed),
+ dev->audio_ifnum, alt,
+ interval,
+ ep_size);
+
+ /* Calculate the number and size of URBs to better fit the audio samples */
+
+ /*
+ * Estimate the number of bytes per DMA transfer.
+ *
+ * This is given by the bit rate (for now, only 48000 Hz) multiplied
+ * by 2 channels and 2 bytes/sample divided by the number of microframe
+ * intervals and by the microframe rate (125 us)
+ */
+ bytes_per_transfer = DIV_ROUND_UP(48000 * 2 * 2, 125 * interval);
+
+ /*
+ * Estimate the number of transfer URBs. Don't let it go past the
+ * maximum number of URBs that is known to be supported by the device.
+ */
+ num_urb = DIV_ROUND_UP(bytes_per_transfer, ep_size);
+ if (num_urb > EM28XX_MAX_AUDIO_BUFS)
+ num_urb = EM28XX_MAX_AUDIO_BUFS;
+
+ /*
+ * Now that we know the number of bytes per transfer and the number of
+ * URBs, estimate the typical size of an URB, in order to adjust the
+ * minimal number of packets.
+ */
+ urb_size = bytes_per_transfer / num_urb;
+
+ /*
+ * Now, calculate the amount of audio packets to be filled on each
+ * URB. In order to preserve the old behaviour, use a minimal
+ * threshold for this value.
+ */
+ npackets = EM28XX_MIN_AUDIO_PACKETS;
+ if (urb_size > ep_size * npackets)
+ npackets = DIV_ROUND_UP(urb_size, ep_size);
+
+ em28xx_info("Number of URBs: %d, with %d packets and %d size",
+ num_urb, npackets, urb_size);
+
+ /* Allocate space to store the number of URBs to be used */
+
+ dev->adev.transfer_buffer = kcalloc(num_urb,
+ sizeof(*dev->adev.transfer_buffer),
+ GFP_ATOMIC);
+ if (!dev->adev.transfer_buffer) {
+ snd_card_free(card);
+ return -ENOMEM;
+ }
+
+ dev->adev.urb = kcalloc(num_urb, sizeof(*dev->adev.urb), GFP_ATOMIC);
+ if (!dev->adev.urb) {
+ snd_card_free(card);
+ kfree(dev->adev.transfer_buffer);
+ return -ENOMEM;
+ }
+
+ /* Alloc memory for each URB and for each transfer buffer */
+ dev->adev.num_urb = num_urb;
+ for (i = 0; i < num_urb; i++) {
struct urb *urb;
int j, k;
void *buf;
- urb = usb_alloc_urb(EM28XX_NUM_AUDIO_PACKETS, GFP_ATOMIC);
+ urb = usb_alloc_urb(npackets, GFP_ATOMIC);
if (!urb) {
em28xx_errdev("usb_alloc_urb failed!\n");
em28xx_audio_free_urb(dev);
+ snd_card_free(card);
return -ENOMEM;
}
dev->adev.urb[i] = urb;
- buf = usb_alloc_coherent(dev->udev, sb_size, GFP_ATOMIC,
+ buf = usb_alloc_coherent(dev->udev, npackets * ep_size, GFP_ATOMIC,
&urb->transfer_dma);
if (!buf) {
em28xx_errdev("usb_alloc_coherent failed!\n");
em28xx_audio_free_urb(dev);
+ snd_card_free(card);
return -ENOMEM;
}
dev->adev.transfer_buffer[i] = buf;
@@ -739,23 +824,15 @@ static int em28xx_audio_init(struct em28xx *dev)
urb->context = dev;
urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
- urb->transfer_buffer = dev->adev.transfer_buffer[i];
- urb->interval = 1 << (ep->bInterval - 1);
+ urb->transfer_buffer = buf;
+ urb->interval = interval;
urb->complete = em28xx_audio_isocirq;
- urb->number_of_packets = EM28XX_NUM_AUDIO_PACKETS;
- urb->transfer_buffer_length = sb_size;
-
- if (!i)
- dprintk("Will use ep 0x%02x on intf %d alt %d interval = %d (rcv isoc pipe: 0x%08x)\n",
- EM28XX_EP_AUDIO, dev->audio_ifnum, alt,
- urb->interval,
- urb->pipe);
+ urb->number_of_packets = npackets;
+ urb->transfer_buffer_length = ep_size * npackets;
- for (j = k = 0; j < EM28XX_NUM_AUDIO_PACKETS;
- j++, k += EM28XX_AUDIO_MAX_PACKET_SIZE) {
+ for (j = k = 0; j < npackets; j++, k += ep_size) {
urb->iso_frame_desc[j].offset = k;
- urb->iso_frame_desc[j].length =
- EM28XX_AUDIO_MAX_PACKET_SIZE;
+ urb->iso_frame_desc[j].length = ep_size;
}
}
@@ -765,8 +842,6 @@ static int em28xx_audio_init(struct em28xx *dev)
snd_card_free(card);
return err;
}
- adev->sndcard = card;
- adev->udev = dev->udev;
em28xx_info("Audio extension successfully initialized\n");
return 0;
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index efdf38642768..e76f993e3195 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -481,9 +481,6 @@ struct em28xx_eeprom {
u8 string_idx_table;
};
-#define EM28XX_AUDIO_BUFS 5
-#define EM28XX_NUM_AUDIO_PACKETS 64
-#define EM28XX_AUDIO_MAX_PACKET_SIZE 196 /* static value */
#define EM28XX_CAPTURE_STREAM_EN 1
/* em28xx extensions */
@@ -498,8 +495,9 @@ struct em28xx_eeprom {
struct em28xx_audio {
char name[50];
- char *transfer_buffer[EM28XX_AUDIO_BUFS];
- struct urb *urb[EM28XX_AUDIO_BUFS];
+ unsigned num_urb;
+ char **transfer_buffer;
+ struct urb **urb;
struct usb_device *udev;
unsigned int capture_transfer_done;
struct snd_pcm_substream *capture_pcm_substream;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/4] em28x-audio: fix the period size in bytes
2014-01-10 18:45 [PATCH 0/4] Fix em28xx audio when using a xHCI USB port Mauro Carvalho Chehab
` (2 preceding siblings ...)
2014-01-10 18:45 ` [PATCH 3/4] em28xx: don't hardcode audio URB calculus Mauro Carvalho Chehab
@ 2014-01-10 18:45 ` Mauro Carvalho Chehab
3 siblings, 0 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-01-10 18:45 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Mauro Carvalho Chehab
If the period size is wrong, userspace will assume a wrong delay
any may negociate an inadequate value.
The em28xx devices use 8 for URB interval, in microframes,
and the driver programs it to have 64 packets.
That means that the IRQ sampling period is 125 * 8 * 64,
with is equal to 64 ms.
So, that's the minimal latency with the current settings. It is
possible to program a lower latency, by using less than 64 packets,
but that increases the amount of bandwitdh used, and the number of
IRQ events per second.
In any case, in order to support it, the driver logic should be
changed to fill those parameters in realtime.
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
drivers/media/usb/em28xx/em28xx-audio.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 6c7d34600294..f6fcee3d4fb9 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -52,6 +52,7 @@ MODULE_PARM_DESC(debug, "activates debug info");
#define EM28XX_MAX_AUDIO_BUFS 5
#define EM28XX_MIN_AUDIO_PACKETS 64
+
#define dprintk(fmt, arg...) do { \
if (debug) \
printk(KERN_INFO "em28xx-audio %s: " fmt, \
@@ -217,15 +218,26 @@ static struct snd_pcm_hardware snd_em28xx_hw_capture = {
.formats = SNDRV_PCM_FMTBIT_S16_LE,
- .rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_KNOT,
+ .rates = SNDRV_PCM_RATE_48000,
.rate_min = 48000,
.rate_max = 48000,
.channels_min = 2,
.channels_max = 2,
.buffer_bytes_max = 62720 * 8, /* just about the value in usbaudio.c */
- .period_bytes_min = 64, /* 12544/2, */
- .period_bytes_max = 12544,
+
+
+ /*
+ * The period is 12.288 bytes. Allow a 10% of variation along its
+ * value, in order to avoid overruns/underruns due to some clock
+ * drift.
+ *
+ * FIXME: This period assumes 64 packets, and a 48000 PCM rate.
+ * Calculate it dynamically.
+ */
+ .period_bytes_min = 11059,
+ .period_bytes_max = 13516,
+
.periods_min = 2,
.periods_max = 98, /* 12544, */
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] em28xx: use bInterval on em28xx-audio
2014-01-10 18:45 ` [PATCH 1/4] em28xx: use bInterval on em28xx-audio Mauro Carvalho Chehab
@ 2014-01-11 13:37 ` Frank Schäfer
2014-01-11 20:53 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 8+ messages in thread
From: Frank Schäfer @ 2014-01-11 13:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab
Am 10.01.2014 19:45, schrieb Mauro Carvalho Chehab:
> Just filling urb->interval with 1 is wrong, and causes a different
> behaviour with xHCI.
>
> With EHCI, the URB size is typically 192 bytes. However, as
> xHCI specifies intervals in microframes, the URB size becomes
> too short (24 bytes).
>
> With this patch, the interval will be properly initialized, and
> the device will behave the same if connected into a xHCI or an
> EHCI device port.
>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> ---
> drivers/media/usb/em28xx/em28xx-audio.c | 39 ++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> index 30ee389a07f0..8e6f04873422 100644
> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> @@ -620,10 +620,13 @@ static int em28xx_audio_init(struct em28xx *dev)
> struct em28xx_audio *adev = &dev->adev;
> struct snd_pcm *pcm;
> struct snd_card *card;
> + struct usb_interface *intf;
> + struct usb_endpoint_descriptor *e, *ep = NULL;
> static int devnr;
> int err, i;
> const int sb_size = EM28XX_NUM_AUDIO_PACKETS *
> EM28XX_AUDIO_MAX_PACKET_SIZE;
> + u8 alt;
>
> if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
> /* This device does not support the extension (in this case
> @@ -679,6 +682,34 @@ static int em28xx_audio_init(struct em28xx *dev)
> em28xx_cvol_new(card, dev, "Surround", AC97_SURROUND_MASTER);
> }
>
> + if (dev->audio_ifnum)
> + alt = 1;
> + else
> + alt = 7;
> +
> + intf = usb_ifnum_to_if(dev->udev, dev->audio_ifnum);
> +
> + if (intf->num_altsetting <= alt) {
> + em28xx_errdev("alt %d doesn't exist on interface %d\n",
> + dev->audio_ifnum, alt);
> + return -ENODEV;
> + }
Hmm... yeah, this the alt setting code looks suspicious.
Take a look at snd_em28xx_capture_open():
if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
if (dev->audio_ifnum)
dev->alt = 1;
else
dev->alt = 7;
...
}
I've been thinking about this for a while and I think this code is based
on the following assumptions:
1.) Video endpoints are always at interface 0
2.) Hence, if the audio endpoints are on a separate interface, the
interface number is > 0
3.) Video interfaces always have alt settings 0-7 and 7 is the one with
the highest bandwith.
1.) is definitely wrong, the VAD Laplace webcam for example has video on
interface #3.
This needs to be fixed in the core, too.
Because of that, the (dev->audio_ifnum > 0) check also needs to be fixed
and dev->is_audio_only should be checked instead.
3.) matches what I've seen so far, but seems to be safer to do the same
what we are doing in em28xx_usb_probe() for the dvb video endpoints.
Whether alt=1 and alt=max are a good choice is a separate question.
How many altternate settings does the audio only interface of you em2860
have and how do they look ?
In case of vendor audio endpoints on the same interface as the video
endpoints, wMaxPacketSize and bInterval seem to be the same for all alt
settings.
(The only device I know so far is the HVR-930c: wMaxPacketSize =1x 196
bytes, bInterval = 4).
> +
> + for (i = 0; i < intf->altsetting[alt].desc.bNumEndpoints; i++) {
> + e = &intf->altsetting[alt].endpoint[i].desc;
> + if (!usb_endpoint_dir_in(e))
> + continue;
> + if (e->bEndpointAddress == EM28XX_EP_AUDIO) {
> + ep = e;
> + break;
> + }
> + }
> +
> + if (!ep) {
> + em28xx_errdev("Couldn't find an audio endpoint");
> + return -ENODEV;
> + }
> +
> /* Alloc URB and transfer buffers */
> for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
> struct urb *urb;
> @@ -707,11 +738,17 @@ static int em28xx_audio_init(struct em28xx *dev)
> urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
> urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> urb->transfer_buffer = dev->adev.transfer_buffer[i];
> - urb->interval = 1;
> + urb->interval = 1 << (ep->bInterval - 1);
> urb->complete = em28xx_audio_isocirq;
> urb->number_of_packets = EM28XX_NUM_AUDIO_PACKETS;
> urb->transfer_buffer_length = sb_size;
>
> + if (!i)
> + dprintk("Will use ep 0x%02x on intf %d alt %d interval = %d (rcv isoc pipe: 0x%08x)\n",
> + EM28XX_EP_AUDIO, dev->audio_ifnum, alt,
> + urb->interval,
> + urb->pipe);
> +
> for (j = k = 0; j < EM28XX_NUM_AUDIO_PACKETS;
> j++, k += EM28XX_AUDIO_MAX_PACKET_SIZE) {
> urb->iso_frame_desc[j].offset = k;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] em28xx: use bInterval on em28xx-audio
2014-01-11 13:37 ` Frank Schäfer
@ 2014-01-11 20:53 ` Mauro Carvalho Chehab
2014-01-12 15:13 ` Frank Schäfer
0 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-01-11 20:53 UTC (permalink / raw)
To: Frank Schäfer; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab
Em Sat, 11 Jan 2014 14:37:30 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Am 10.01.2014 19:45, schrieb Mauro Carvalho Chehab:
> > Just filling urb->interval with 1 is wrong, and causes a different
> > behaviour with xHCI.
> >
> > With EHCI, the URB size is typically 192 bytes. However, as
> > xHCI specifies intervals in microframes, the URB size becomes
> > too short (24 bytes).
> >
> > With this patch, the interval will be properly initialized, and
> > the device will behave the same if connected into a xHCI or an
> > EHCI device port.
> >
> > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > ---
> > drivers/media/usb/em28xx/em28xx-audio.c | 39 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> > index 30ee389a07f0..8e6f04873422 100644
> > --- a/drivers/media/usb/em28xx/em28xx-audio.c
> > +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> > @@ -620,10 +620,13 @@ static int em28xx_audio_init(struct em28xx *dev)
> > struct em28xx_audio *adev = &dev->adev;
> > struct snd_pcm *pcm;
> > struct snd_card *card;
> > + struct usb_interface *intf;
> > + struct usb_endpoint_descriptor *e, *ep = NULL;
> > static int devnr;
> > int err, i;
> > const int sb_size = EM28XX_NUM_AUDIO_PACKETS *
> > EM28XX_AUDIO_MAX_PACKET_SIZE;
> > + u8 alt;
> >
> > if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
> > /* This device does not support the extension (in this case
> > @@ -679,6 +682,34 @@ static int em28xx_audio_init(struct em28xx *dev)
> > em28xx_cvol_new(card, dev, "Surround", AC97_SURROUND_MASTER);
> > }
> >
> > + if (dev->audio_ifnum)
> > + alt = 1;
> > + else
> > + alt = 7;
> > +
> > + intf = usb_ifnum_to_if(dev->udev, dev->audio_ifnum);
> > +
> > + if (intf->num_altsetting <= alt) {
> > + em28xx_errdev("alt %d doesn't exist on interface %d\n",
> > + dev->audio_ifnum, alt);
> > + return -ENODEV;
> > + }
> Hmm... yeah, this the alt setting code looks suspicious.
>
> Take a look at snd_em28xx_capture_open():
>
> if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
> if (dev->audio_ifnum)
> dev->alt = 1;
> else
> dev->alt = 7;
> ...
> }
>
> I've been thinking about this for a while and I think this code is based
> on the following assumptions:
> 1.) Video endpoints are always at interface 0
> 2.) Hence, if the audio endpoints are on a separate interface, the
> interface number is > 0
> 3.) Video interfaces always have alt settings 0-7 and 7 is the one with
> the highest bandwith.
> 1.) is definitely wrong, the VAD Laplace webcam for example has video on
> interface #3.
> This needs to be fixed in the core, too.
> Because of that, the (dev->audio_ifnum > 0) check also needs to be fixed
> and dev->is_audio_only should be checked instead.
> 3.) matches what I've seen so far, but seems to be safer to do the same
> what we are doing in em28xx_usb_probe() for the dvb video endpoints.
>
> Whether alt=1 and alt=max are a good choice is a separate question.
alt = 1 for audio is because the audio-only interface on em2861 has only 5
alternates, and alt = 1 has the biggest wMaxPacketSize.
This code can of course be optimized.
alt = <max> for mixed video/audio interface was chosen to avoid the
need of recalculate the video + audio bandwidth.
It would be possible to rework on this code to create a single function
that would adjust the bandwidth considering both video and audio
constraints, but it takes time, and testing. So, when the audio driver
was written, the developer just hardcoded alt = 7, and didn't bother
with that.
Nobody had time to review it so far.
> How many altternate settings does the audio only interface of you em2860
> have and how do they look ?
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 4
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 1
bNumEndpoints 1
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 2
bNumEndpoints 1
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00b4 1x 180 bytes
bInterval 4
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 3
bNumEndpoints 1
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0084 1x 132 bytes
bInterval 4
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 4
bNumEndpoints 1
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0044 1x 68 bytes
bInterval 4
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 5
bNumEndpoints 1
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0024 1x 36 bytes
bInterval 4
>
> In case of vendor audio endpoints on the same interface as the video
> endpoints, wMaxPacketSize and bInterval seem to be the same for all alt
> settings.
> (The only device I know so far is the HVR-930c: wMaxPacketSize =1x 196
> bytes, bInterval = 4).
Yes, but the interface to be selected, in this case, should estimate both
audio and video, if both are active, in order to select the right
alternate.
In any case, this patch doesn't change anything related to it, and it is
a first step to add a proper code here to detect and select the proper
setup.
> > +
> > + for (i = 0; i < intf->altsetting[alt].desc.bNumEndpoints; i++) {
> > + e = &intf->altsetting[alt].endpoint[i].desc;
> > + if (!usb_endpoint_dir_in(e))
> > + continue;
> > + if (e->bEndpointAddress == EM28XX_EP_AUDIO) {
> > + ep = e;
> > + break;
> > + }
> > + }
> > +
> > + if (!ep) {
> > + em28xx_errdev("Couldn't find an audio endpoint");
> > + return -ENODEV;
> > + }
> > +
> > /* Alloc URB and transfer buffers */
> > for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
> > struct urb *urb;
> > @@ -707,11 +738,17 @@ static int em28xx_audio_init(struct em28xx *dev)
> > urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
> > urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> > urb->transfer_buffer = dev->adev.transfer_buffer[i];
> > - urb->interval = 1;
> > + urb->interval = 1 << (ep->bInterval - 1);
> > urb->complete = em28xx_audio_isocirq;
> > urb->number_of_packets = EM28XX_NUM_AUDIO_PACKETS;
> > urb->transfer_buffer_length = sb_size;
> >
> > + if (!i)
> > + dprintk("Will use ep 0x%02x on intf %d alt %d interval = %d (rcv isoc pipe: 0x%08x)\n",
> > + EM28XX_EP_AUDIO, dev->audio_ifnum, alt,
> > + urb->interval,
> > + urb->pipe);
> > +
> > for (j = k = 0; j < EM28XX_NUM_AUDIO_PACKETS;
> > j++, k += EM28XX_AUDIO_MAX_PACKET_SIZE) {
> > urb->iso_frame_desc[j].offset = k;
>
--
Cheers,
Mauro
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/4] em28xx: use bInterval on em28xx-audio
2014-01-11 20:53 ` Mauro Carvalho Chehab
@ 2014-01-12 15:13 ` Frank Schäfer
0 siblings, 0 replies; 8+ messages in thread
From: Frank Schäfer @ 2014-01-12 15:13 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab
On 11.01.2014 21:53, Mauro Carvalho Chehab wrote:
> Em Sat, 11 Jan 2014 14:37:30 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 10.01.2014 19:45, schrieb Mauro Carvalho Chehab:
>>> Just filling urb->interval with 1 is wrong, and causes a different
>>> behaviour with xHCI.
>>>
>>> With EHCI, the URB size is typically 192 bytes. However, as
>>> xHCI specifies intervals in microframes, the URB size becomes
>>> too short (24 bytes).
>>>
>>> With this patch, the interval will be properly initialized, and
>>> the device will behave the same if connected into a xHCI or an
>>> EHCI device port.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
>>> ---
>>> drivers/media/usb/em28xx/em28xx-audio.c | 39 ++++++++++++++++++++++++++++++++-
>>> 1 file changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>>> index 30ee389a07f0..8e6f04873422 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>>> @@ -620,10 +620,13 @@ static int em28xx_audio_init(struct em28xx *dev)
>>> struct em28xx_audio *adev = &dev->adev;
>>> struct snd_pcm *pcm;
>>> struct snd_card *card;
>>> + struct usb_interface *intf;
>>> + struct usb_endpoint_descriptor *e, *ep = NULL;
>>> static int devnr;
>>> int err, i;
>>> const int sb_size = EM28XX_NUM_AUDIO_PACKETS *
>>> EM28XX_AUDIO_MAX_PACKET_SIZE;
>>> + u8 alt;
>>>
>>> if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
>>> /* This device does not support the extension (in this case
>>> @@ -679,6 +682,34 @@ static int em28xx_audio_init(struct em28xx *dev)
>>> em28xx_cvol_new(card, dev, "Surround", AC97_SURROUND_MASTER);
>>> }
>>>
>>> + if (dev->audio_ifnum)
>>> + alt = 1;
>>> + else
>>> + alt = 7;
>>> +
>>> + intf = usb_ifnum_to_if(dev->udev, dev->audio_ifnum);
>>> +
>>> + if (intf->num_altsetting <= alt) {
>>> + em28xx_errdev("alt %d doesn't exist on interface %d\n",
>>> + dev->audio_ifnum, alt);
>>> + return -ENODEV;
>>> + }
>> Hmm... yeah, this the alt setting code looks suspicious.
>>
>> Take a look at snd_em28xx_capture_open():
>>
>> if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
>> if (dev->audio_ifnum)
>> dev->alt = 1;
>> else
>> dev->alt = 7;
>> ...
>> }
>>
>> I've been thinking about this for a while and I think this code is based
>> on the following assumptions:
>> 1.) Video endpoints are always at interface 0
>> 2.) Hence, if the audio endpoints are on a separate interface, the
>> interface number is > 0
>> 3.) Video interfaces always have alt settings 0-7 and 7 is the one with
>> the highest bandwith.
>> 1.) is definitely wrong, the VAD Laplace webcam for example has video on
>> interface #3.
>> This needs to be fixed in the core, too.
>> Because of that, the (dev->audio_ifnum > 0) check also needs to be fixed
>> and dev->is_audio_only should be checked instead.
>> 3.) matches what I've seen so far, but seems to be safer to do the same
>> what we are doing in em28xx_usb_probe() for the dvb video endpoints.
>>
>> Whether alt=1 and alt=max are a good choice is a separate question.
> alt = 1 for audio is because the audio-only interface on em2861 has only 5
> alternates, and alt = 1 has the biggest wMaxPacketSize.
Ahh, ok, that explains a lot.
> This code can of course be optimized.
>
> alt = <max> for mixed video/audio interface was chosen to avoid the
> need of recalculate the video + audio bandwidth.
>
> It would be possible to rework on this code to create a single function
> that would adjust the bandwidth considering both video and audio
> constraints, but it takes time, and testing. So, when the audio driver
> was written, the developer just hardcoded alt = 7, and didn't bother
> with that.
We are already doing such an improved alt setting determination in the
core (em28xx_set_alternate() in em28xx-video.c).
It doesn't consider the audio endpoints, but given that all devices with
mixed interfaces are using the same audio bandwidth parameters for all
alt settings like my HVR930c does, everything is fine.
> Nobody had time to review it so far.
>
>> How many altternate settings does the audio only interface of you em2860
>> have and how do they look ?
>
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 0
> bNumEndpoints 1
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass 0
> bInterfaceProtocol 255
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x83 EP 3 IN
> bmAttributes 1
> Transfer Type Isochronous
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0000 1x 0 bytes
> bInterval 4
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 1
> bNumEndpoints 1
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass 0
> bInterfaceProtocol 255
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x83 EP 3 IN
> bmAttributes 1
> Transfer Type Isochronous
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x00c4 1x 196 bytes
> bInterval 4
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 2
> bNumEndpoints 1
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass 0
> bInterfaceProtocol 255
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x83 EP 3 IN
> bmAttributes 1
> Transfer Type Isochronous
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x00b4 1x 180 bytes
> bInterval 4
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 3
> bNumEndpoints 1
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass 0
> bInterfaceProtocol 255
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x83 EP 3 IN
> bmAttributes 1
> Transfer Type Isochronous
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0084 1x 132 bytes
> bInterval 4
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 4
> bNumEndpoints 1
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass 0
> bInterfaceProtocol 255
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x83 EP 3 IN
> bmAttributes 1
> Transfer Type Isochronous
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0044 1x 68 bytes
> bInterval 4
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 5
> bNumEndpoints 1
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass 0
> bInterfaceProtocol 255
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x83 EP 3 IN
> bmAttributes 1
> Transfer Type Isochronous
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0024 1x 36 bytes
> bInterval 4
Interesting, thanks !
I assume there are 5 alt settings because the device supports 5 sample
rates ?
>
>> In case of vendor audio endpoints on the same interface as the video
>> endpoints, wMaxPacketSize and bInterval seem to be the same for all alt
>> settings.
>> (The only device I know so far is the HVR-930c: wMaxPacketSize =1x 196
>> bytes, bInterval = 4).
> Yes, but the interface to be selected, in this case, should estimate both
> audio and video, if both are active, in order to select the right
> alternate.
>
> In any case, this patch doesn't change anything related to it, and it is
> a first step to add a proper code here to detect and select the proper
> setup.
Yes, the patch looks reasonable, I didn't want to question that.
I'm not familiar with all these ALSA und USB details, so I can't review
this series in detail.
I also have no em28xx device with a working vendor audio interface.
But I can't see anything which is obviously wrong. :)
>
>>> +
>>> + for (i = 0; i < intf->altsetting[alt].desc.bNumEndpoints; i++) {
>>> + e = &intf->altsetting[alt].endpoint[i].desc;
>>> + if (!usb_endpoint_dir_in(e))
>>> + continue;
>>> + if (e->bEndpointAddress == EM28XX_EP_AUDIO) {
>>> + ep = e;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (!ep) {
>>> + em28xx_errdev("Couldn't find an audio endpoint");
>>> + return -ENODEV;
>>> + }
>>> +
>>> /* Alloc URB and transfer buffers */
>>> for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
>>> struct urb *urb;
>>> @@ -707,11 +738,17 @@ static int em28xx_audio_init(struct em28xx *dev)
>>> urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
>>> urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
>>> urb->transfer_buffer = dev->adev.transfer_buffer[i];
>>> - urb->interval = 1;
>>> + urb->interval = 1 << (ep->bInterval - 1);
>>> urb->complete = em28xx_audio_isocirq;
>>> urb->number_of_packets = EM28XX_NUM_AUDIO_PACKETS;
>>> urb->transfer_buffer_length = sb_size;
>>>
>>> + if (!i)
>>> + dprintk("Will use ep 0x%02x on intf %d alt %d interval = %d (rcv isoc pipe: 0x%08x)\n",
>>> + EM28XX_EP_AUDIO, dev->audio_ifnum, alt,
>>> + urb->interval,
>>> + urb->pipe);
>>> +
>>> for (j = k = 0; j < EM28XX_NUM_AUDIO_PACKETS;
>>> j++, k += EM28XX_AUDIO_MAX_PACKET_SIZE) {
>>> urb->iso_frame_desc[j].offset = k;
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-01-12 15:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-10 18:45 [PATCH 0/4] Fix em28xx audio when using a xHCI USB port Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 1/4] em28xx: use bInterval on em28xx-audio Mauro Carvalho Chehab
2014-01-11 13:37 ` Frank Schäfer
2014-01-11 20:53 ` Mauro Carvalho Chehab
2014-01-12 15:13 ` Frank Schäfer
2014-01-10 18:45 ` [PATCH 2/4] em28xx-audio: Fix error path Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 3/4] em28xx: don't hardcode audio URB calculus Mauro Carvalho Chehab
2014-01-10 18:45 ` [PATCH 4/4] em28x-audio: fix the period size in bytes Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).