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@xxxxxxxxxxx>
> ---
> 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;
--
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
