[PATCH] usbaudio fixes

[PATCH] usbaudio fixes

[Clemens Ladisch]

I botched my previous patch for the UA-5: the altsettings of the audio
interface would be ignored because bInterfaceSubClass isn't set to
USB_SUBCLASS_AUDIO_STREAMING on that device. This patch corrects this.

And some devices (e.g. the Logitech QuickCam Web) have an endpoint
with wMaxPacketSize == 0, and no class-specific descriptors in their
first altsetting (instead of no endpoints). The additional check now
suppresses an unnecessary warning message.


Index: alsa-kernel/usb/usbaudio.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/usb/usbaudio.c,v
retrieving revision 1.56
diff -u -r1.56 usbaudio.c
--- alsa-kernel/usb/usbaudio.c	4 Jun 2003 12:43:55 -0000	1.56
+++ alsa-kernel/usb/usbaudio.c	19 Jun 2003 06:51:04 -0000
@@ -2163,8 +2163,10 @@
 		/* skip invalid one */
 		if ((altsd->bInterfaceClass != USB_CLASS_AUDIO &&
 		     altsd->bInterfaceClass != USB_CLASS_VENDOR_SPEC) ||
-		    altsd->bInterfaceSubClass != USB_SUBCLASS_AUDIO_STREAMING ||
-		    altsd->bNumEndpoints < 1)
+		    (altsd->bInterfaceSubClass != USB_SUBCLASS_AUDIO_STREAMING &&
+		     altsd->bInterfaceSubClass != USB_SUBCLASS_VENDOR_SPEC) ||
+		    altsd->bNumEndpoints < 1 ||
+		    get_endpoint(alts, 0)->wMaxPacketSize == 0)
 			continue;
 		/* must be isochronous */
 		if ((get_endpoint(alts, 0)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) !=
Index: alsa-kernel/usb/usbaudio.h
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/usb/usbaudio.h,v
retrieving revision 1.17
diff -u -r1.17 usbaudio.h
--- alsa-kernel/usb/usbaudio.h	13 May 2003 10:44:09 -0000	1.17
+++ alsa-kernel/usb/usbaudio.h	19 Jun 2003 06:51:04 -0000
@@ -28,6 +28,7 @@
 #define USB_SUBCLASS_AUDIO_CONTROL	0x01
 #define USB_SUBCLASS_AUDIO_STREAMING	0x02
 #define USB_SUBCLASS_MIDI_STREAMING	0x03
+#define USB_SUBCLASS_VENDOR_SPEC	0xff

 #define USB_DT_CS_DEVICE                0x21
 #define USB_DT_CS_CONFIG                0x22




-------------------------------------------------------
This SF.Net email is sponsored by: INetU
Attention Web Developers & Consultants: Become An INetU Hosting Partner.
Refer Dedicated Servers. We Manage Them. You Get 10% Monthly Commission!
INetU Dedicated Managed Hosting http://www.inetu.net/partner/index.php

Re: [PATCH] usbaudio fixes

[Takashi Iwai]

At Thu, 19 Jun 2003 09:00:47 +0200 (METDST),
Clemens Ladisch wrote:
> 
> 
> I botched my previous patch for the UA-5: the altsettings of the audio
> interface would be ignored because bInterfaceSubClass isn't set to
> USB_SUBCLASS_AUDIO_STREAMING on that device. This patch corrects this.
> 
> And some devices (e.g. the Logitech QuickCam Web) have an endpoint
> with wMaxPacketSize == 0, and no class-specific descriptors in their
> first altsetting (instead of no endpoints). The additional check now
> suppresses an unnecessary warning message.

thanks, applied to cvs now.


Takashi


-------------------------------------------------------
This SF.Net email is sponsored by: INetU
Attention Web Developers & Consultants: Become An INetU Hosting Partner.
Refer Dedicated Servers. We Manage Them. You Get 10% Monthly Commission!
INetU Dedicated Managed Hosting http://www.inetu.net/partner/index.php

[PATCH] usbaudio fixes

[Clemens Ladisch]

- don't clear active_mask bits until it's clear that the URB is _not_
  resubmitted, to prevent a race with unlinking
- initialize active_mask and unlink_mask each time before URBs are
  started
- don't call sleeping functions in trigger callback


Index: alsa-kernel/usb/usbaudio.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/usb/usbaudio.c,v
retrieving revision 1.70
diff -u -r1.70 usbaudio.c
--- alsa-kernel/usb/usbaudio.c	20 Nov 2003 16:08:26 -0000	1.70
+++ alsa-kernel/usb/usbaudio.c	24 Nov 2003 07:40:19 -0000
@@ -573,21 +573,18 @@
 	snd_urb_ctx_t *ctx = (snd_urb_ctx_t *)urb->context;
 	snd_usb_substream_t *subs = ctx->subs;
 	snd_pcm_substream_t *substream = ctx->subs->pcm_substream;
-	int err;
+	int err = 0;

-	clear_bit(ctx->index, &subs->active_mask);
-	clear_bit(ctx->index, &subs->unlink_mask);
-	if (subs->running && subs->ops.retire(subs, substream->runtime, urb))
-		return;
-	if (! subs->running) /* can be stopped during retire callback */
-		return;
-	if ((err = subs->ops.prepare(subs, substream->runtime, urb)) < 0 ||
+	if ((subs->running && subs->ops.retire(subs, substream->runtime, urb)) ||
+	    ! subs->running || /* can be stopped during retire callback */
+	    (err = subs->ops.prepare(subs, substream->runtime, urb)) < 0 ||
 	    (err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
-		snd_printd(KERN_ERR "cannot submit urb (err = %d)\n", err);
-		snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
-		return;
+		clear_bit(ctx->index, &subs->active_mask);
+		if (err < 0) {
+			snd_printd(KERN_ERR "cannot submit urb (err = %d)\n", err);
+			snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
+		}
 	}
-	set_bit(ctx->index, &subs->active_mask);
 }


@@ -599,21 +596,18 @@
 	snd_urb_ctx_t *ctx = (snd_urb_ctx_t *)urb->context;
 	snd_usb_substream_t *subs = ctx->subs;
 	snd_pcm_substream_t *substream = ctx->subs->pcm_substream;
-	int err;
+	int err = 0;

-	clear_bit(ctx->index + 16, &subs->active_mask);
-	clear_bit(ctx->index + 16, &subs->unlink_mask);
-	if (subs->running && subs->ops.retire_sync(subs, substream->runtime, urb))
-		return;
-	if (! subs->running) /* can be stopped during retire callback */
-		return;
-	if ((err = subs->ops.prepare_sync(subs, substream->runtime, urb))  < 0 ||
+	if ((subs->running && subs->ops.retire_sync(subs, substream->runtime, urb)) ||
+	    ! subs->running || /* can be stopped during retire callback */
+	    (err = subs->ops.prepare_sync(subs, substream->runtime, urb)) < 0 ||
 	    (err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
-		snd_printd(KERN_ERR "cannot submit sync urb (err = %d)\n", err);
-		snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
-		return;
+		clear_bit(ctx->index + 16, &subs->active_mask);
+		if (err < 0) {
+			snd_printd(KERN_ERR "cannot submit sync urb (err = %d)\n", err);
+			snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
+		}
 	}
-	set_bit(ctx->index + 16, &subs->active_mask);
 }


@@ -694,9 +688,11 @@
 		}
 	}

+	subs->active_mask = 0;
+	subs->unlink_mask = 0;
 	subs->running = 1;
 	for (i = 0; i < subs->nurbs; i++) {
-		if ((err = usb_submit_urb(subs->dataurb[i].urb, GFP_KERNEL)) < 0) {
+		if ((err = usb_submit_urb(subs->dataurb[i].urb, GFP_ATOMIC)) < 0) {
 			snd_printk(KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
 			goto __error;
 		}
@@ -704,7 +700,7 @@
 	}
 	if (subs->syncpipe) {
 		for (i = 0; i < SYNC_URBS; i++) {
-			if ((err = usb_submit_urb(subs->syncurb[i].urb, GFP_KERNEL)) < 0) {
+			if ((err = usb_submit_urb(subs->syncurb[i].urb, GFP_ATOMIC)) < 0) {
 				snd_printk(KERN_ERR "cannot submit syncpipe for urb %d, err = %d\n", i, err);
 				goto __error;
 			}
@@ -843,8 +839,6 @@
 	subs->hwptr_done = 0;
 	subs->transfer_sched = 0;
 	subs->transfer_done = 0;
-	subs->active_mask = 0;
-	subs->unlink_mask = 0;

 	/* calculate the max. size of packet */
 	maxsize = ((subs->freqmax + 0x3fff) * (frame_bits >> 3)) >> 14;
@@ -1282,9 +1276,6 @@
 	/* some unit conversions in runtime */
 	subs->maxframesize = bytes_to_frames(runtime, subs->maxpacksize);
 	subs->curframesize = bytes_to_frames(runtime, subs->curpacksize);
-
-	/* deactivate urbs to be sure */
-	deactivate_urbs(subs, 0, 0);

 	return 0;
 }




-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [PATCH] usbaudio fixes

[Takashi Iwai]

At Mon, 24 Nov 2003 08:46:08 +0100 (MET),
Clemens Ladisch wrote:
> 
> 
> - don't clear active_mask bits until it's clear that the URB is _not_
>   resubmitted, to prevent a race with unlinking
> - initialize active_mask and unlink_mask each time before URBs are
>   started

we still need to check here whether the urbs are really free, since
a path like trigger stop -> prepare -> trigger_start is possible. 
in this case, the operation can be done quickly enough before urbs are
really unlinked.

the question is then where we can do a long wait.  perpare would be a
better place than trigger, but unfortunately, prepare callback is also
regarded as atomic because of linked streams.

the patch looks nice, btw.  i already have a similar version on my
tree for testing, but wasn't applied because of the reason above.
i'll apply your patch now to cvs, and let's solve the prepare problem
later.


Takashi


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [PATCH] usbaudio fixes

[Takashi Iwai]

At Mon, 24 Nov 2003 08:46:08 +0100 (MET),
Clemens Ladisch wrote:
> 
> 
> - don't clear active_mask bits until it's clear that the URB is _not_
>   resubmitted, to prevent a race with unlinking
> - initialize active_mask and unlink_mask each time before URBs are
>   started

we still need to check here whether the urbs are really free, since
a path like trigger stop -> prepare -> trigger_start is possible. 
in this case, the operation can be done quickly enough before urbs are
really unlinked.

the question is then where we can do a long wait.  perpare would be a
better place than trigger, but unfortunately, prepare callback is also
regarded as atomic because of linked streams.

the patch looks nice, btw.  i already have a similar version on my
tree for testing, but wasn't applied because of the reason above.
i'll apply your patch now to cvs, and let's solve the prepare problem
later.


Takashi


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [PATCH] usbaudio fixes

[Clemens Ladisch]

Takashi Iwai wrote:

> Clemens Ladisch wrote:
> >
> > - initialize active_mask and unlink_mask each time before URBs are
> >   started
>
> we still need to check here whether the urbs are really free, since
> a path like trigger stop -> prepare -> trigger_start is possible.
> in this case, the operation can be done quickly enough before urbs are
> really unlinked.
>
> the question is then where we can do a long wait.  perpare would be a
> better place than trigger, but unfortunately, prepare callback is also
> regarded as atomic because of linked streams.

If we cannot sleep until the URBs are completely unlinked, we have to
1) busy-wait until the URBs are inactive (*ouch!*), or
2) let trigger_start fail with "device not yet ready", or
3) allocate a new set of URBs, and put the old ones in some list to be
   freed later.

(1) would be really evil; (2) a quick hack; (3) would be most useful,
but it's anything but elegant.


Regards,
Clemens




-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [PATCH] usbaudio fixes

[Takashi Iwai]

At Mon, 24 Nov 2003 13:10:55 +0100 (MET),
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> 
> > Clemens Ladisch wrote:
> > >
> > > - initialize active_mask and unlink_mask each time before URBs are
> > >   started
> >
> > we still need to check here whether the urbs are really free, since
> > a path like trigger stop -> prepare -> trigger_start is possible.
> > in this case, the operation can be done quickly enough before urbs are
> > really unlinked.
> >
> > the question is then where we can do a long wait.  perpare would be a
> > better place than trigger, but unfortunately, prepare callback is also
> > regarded as atomic because of linked streams.
> 
> If we cannot sleep until the URBs are completely unlinked, we have to
> 1) busy-wait until the URBs are inactive (*ouch!*), or
> 2) let trigger_start fail with "device not yet ready", or
> 3) allocate a new set of URBs, and put the old ones in some list to be
>    freed later.
> 
> (1) would be really evil; (2) a quick hack; (3) would be most useful,
> but it's anything but elegant.

(1) is difficult because the callback is performed with irq disabled.
thus complete callback will be never called inside the prepare or
trigger callback.

(2) will bring many complains, i guess :)

(3) looks like a good idea as a workaround.


meanwhile, i'm trying to implement:

(4) allow prepare callback to sleep with a special flag.

this will be useful for other drivers, too, such as vx and korg1212
drivers which require the handshaking.
but what i'm doing is still a hack, and will be a fundamental rewrite
later.


Takashi


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [PATCH] usbaudio fixes

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

At Mon, 24 Nov 2003 13:36:42 +0100,
I wrote:
> 
> meanwhile, i'm trying to implement:
> 
> (4) allow prepare callback to sleep with a special flag.
> 
> this will be useful for other drivers, too, such as vx and korg1212
> drivers which require the handshaking.
> but what i'm doing is still a hack, and will be a fundamental rewrite
> later.

the attached is a quick-hacked version.
it's untested for linked streams.  we need a test case here.


Takashi

[-- Attachment #2: nonatomic-prepare.dif --]
[-- Type: application/octet-stream, Size: 6888 bytes --]

Index: alsa-kernel/core/pcm_native.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/core/pcm_native.c,v
retrieving revision 1.63
diff -u -r1.63 pcm_native.c
--- alsa-kernel/core/pcm_native.c	21 Oct 2003 09:49:34 -0000	1.63
+++ alsa-kernel/core/pcm_native.c	24 Nov 2003 12:42:32 -0000
@@ -620,7 +620,7 @@
  */
 static int snd_pcm_action_group(struct action_ops *ops,
 				snd_pcm_substream_t *substream,
-				int state)
+				int state, int atomic_only)
 {
 	struct list_head *pos;
 	snd_pcm_substream_t *s = NULL;
@@ -628,6 +628,8 @@
 
 	snd_pcm_group_for_each(pos, substream) {
 		s = snd_pcm_group_substream_entry(pos);
+		if (atomic_only && (s->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS))
+			continue;
 		if (s != substream)
 			spin_lock(&s->self_group.lock);
 		res = ops->pre_action(s, state);
@@ -637,6 +639,8 @@
 	if (res >= 0) {
 		snd_pcm_group_for_each(pos, substream) {
 			s = snd_pcm_group_substream_entry(pos);
+			if (atomic_only && (s->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS))
+				continue;
 			err = ops->do_action(s, state);
 			if (err < 0) {
 				if (res == 0)
@@ -652,7 +656,9 @@
 		/* unlock all streams */
 		snd_pcm_group_for_each(pos, substream) {
 			s1 = snd_pcm_group_substream_entry(pos);
-			if (s1 != substream)
+			if (atomic_only && (s1->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS))
+				;
+			else if (s1 != substream)
 				spin_unlock(&s1->self_group.lock);
 			if (s1 == s)	/* end */
 				break;
@@ -682,6 +688,8 @@
 
 /*
  *  Note: call with stream lock
+ *
+ * NB2: this won't handle the non-atomic callbacks
  */
 static int snd_pcm_action(struct action_ops *ops,
 			  snd_pcm_substream_t *substream,
@@ -695,7 +703,7 @@
 			spin_lock(&substream->group->lock);
 			spin_lock(&substream->self_group.lock);
 		}
-		res = snd_pcm_action_group(ops, substream, state);
+		res = snd_pcm_action_group(ops, substream, state, 0);
 		spin_unlock(&substream->group->lock);
 	} else {
 		res = snd_pcm_action_single(ops, substream, state);
@@ -705,10 +713,14 @@
 
 /*
  *  Note: don't use any locks before
+ *
+ * NB2: this can handle the non-atomic callbacks if allow_nonatomic = 1
+ *      when the pcm->info_flags has NONATOMIC_OPS bit, it's handled
+ *      ouside the lock to allow sleep in the callback.
  */
 static int snd_pcm_action_lock_irq(struct action_ops *ops,
 				   snd_pcm_substream_t *substream,
-				   int state)
+				   int state, int allow_nonatomic)
 {
 	int res;
 
@@ -716,10 +728,42 @@
 	if (snd_pcm_stream_linked(substream)) {
 		spin_lock(&substream->group->lock);
 		spin_lock(&substream->self_group.lock);
-		res = snd_pcm_action_group(ops, substream, state);
+		res = snd_pcm_action_group(ops, substream, state, allow_nonatomic);
 		spin_unlock(&substream->self_group.lock);
 		spin_unlock(&substream->group->lock);
+		if (res >= 0 && allow_nonatomic) {
+			/* now process the non-atomic substreams separately
+			 * outside the lock
+			 */
+#define MAX_LINKED_STREAMS	16	/* FIXME: should be variable */
+
+			struct list_head *pos;
+			int i, num_s = 0;
+			snd_pcm_substream_t *s;
+			snd_pcm_substream_t *subs[MAX_LINKED_STREAMS];
+			snd_pcm_group_for_each(pos, substream) {
+				if (num_s >= MAX_LINKED_STREAMS) {
+					res = -ENOMEM;
+					num_s = 0; /* don't proceed */
+					break;
+				}
+				s = snd_pcm_group_substream_entry(pos);
+				if (s->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS)
+					subs[num_s++] = s;
+			}
+			if (num_s > 0) {
+				read_unlock_irq(&snd_pcm_link_rwlock);
+				for (i = 0; i < num_s && res >= 0; i++)
+					res = snd_pcm_action_single(ops, subs[i], state);
+				return res;
+			}
+		}
 	} else {
+		if (allow_nonatomic &&
+		    (substream->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS)) {
+			read_unlock_irq(&snd_pcm_link_rwlock);
+			return snd_pcm_action_single(ops, substream, state);
+		}
 		spin_lock(&substream->self_group.lock);
 		res = snd_pcm_action_single(ops, substream, state);
 		spin_unlock(&substream->self_group.lock);
@@ -993,7 +1037,7 @@
 
 	snd_power_lock(card);
 	if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0, substream->ffile)) >= 0)
-		return snd_pcm_action_lock_irq(&snd_pcm_action_resume, substream, 0);
+		return snd_pcm_action_lock_irq(&snd_pcm_action_resume, substream, 0, 0);
 	snd_power_unlock(card);
 	return res;
 }
@@ -1083,7 +1127,7 @@
 
 static int snd_pcm_reset(snd_pcm_substream_t *substream)
 {
-	return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0);
+	return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0, 0);
 }
 
 static int snd_pcm_pre_prepare(snd_pcm_substream_t * substream, int state)
@@ -1131,7 +1175,7 @@
 
 	snd_power_lock(card);
 	if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0, substream->ffile)) >= 0)
-		res = snd_pcm_action_lock_irq(&snd_pcm_action_prepare, substream, 0);
+		res = snd_pcm_action_lock_irq(&snd_pcm_action_prepare, substream, 0, 1); /* allow sleep if specified */
 	snd_power_unlock(card);
 	return res;
 }
Index: alsa-kernel/include/asound.h
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/include/asound.h,v
retrieving revision 1.24
diff -u -r1.24 asound.h
--- alsa-kernel/include/asound.h	22 Oct 2003 09:41:06 -0000	1.24
+++ alsa-kernel/include/asound.h	24 Nov 2003 12:23:47 -0000
@@ -272,6 +272,7 @@
 #define SNDRV_PCM_INFO_HALF_DUPLEX	0x00100000	/* only half duplex */
 #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
 #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
+#define SNDRV_PCM_INFO_NONATOMIC_OPS	0x00800000	/* non-atomic prepare callback */
 
 enum sndrv_pcm_state {
 	SNDRV_PCM_STATE_OPEN = 0,	/* stream is open */
Index: alsa-kernel/usb/usbaudio.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/usb/usbaudio.c,v
retrieving revision 1.77
diff -u -r1.77 usbaudio.c
--- alsa-kernel/usb/usbaudio.c	24 Nov 2003 11:51:02 -0000	1.77
+++ alsa-kernel/usb/usbaudio.c	24 Nov 2003 12:44:47 -0000
@@ -804,8 +804,7 @@
 	int i;
 
 	/* stop urbs (to be sure) */
-	deactivate_urbs(subs, force, 1);
-	if (async_unlink)
+	if (deactivate_urbs(subs, force, 1) > 0)
 		wait_clear_urbs(subs);
 
 	for (i = 0; i < MAX_URBS; i++)
@@ -1277,6 +1276,10 @@
 	subs->maxframesize = bytes_to_frames(runtime, subs->maxpacksize);
 	subs->curframesize = bytes_to_frames(runtime, subs->curpacksize);
 
+	/* clear urbs (to be sure) */
+	if (deactivate_urbs(subs, 0, 1) > 0)
+		wait_clear_urbs(subs);
+
 	return 0;
 }
 
@@ -2002,7 +2005,7 @@
 	as->pcm = pcm;
 	pcm->private_data = as;
 	pcm->private_free = snd_usb_audio_pcm_free;
-	pcm->info_flags = 0;
+	pcm->info_flags = SNDRV_PCM_INFO_NONATOMIC_OPS;
 	if (chip->pcm_devs > 0)
 		sprintf(pcm->name, "USB Audio #%d", chip->pcm_devs);
 	else

Re: [PATCH] usbaudio fixes

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

At Mon, 24 Nov 2003 13:51:50 +0100,
I wrote:
> 
> At Mon, 24 Nov 2003 13:36:42 +0100,
> I wrote:
> > 
> > meanwhile, i'm trying to implement:
> > 
> > (4) allow prepare callback to sleep with a special flag.
> > 
> > this will be useful for other drivers, too, such as vx and korg1212
> > drivers which require the handshaking.
> > but what i'm doing is still a hack, and will be a fundamental rewrite
> > later.
> 
> the attached is a quick-hacked version.
> it's untested for linked streams.  we need a test case here.

ok, here is the final version.  this one seems working fine.

Jaroslav, could you check whether it's ok?
the changes in pcm_native.c shouldn't affect other cards.


Takashi

[-- Attachment #2: Type: text/plain, Size: 11056 bytes --]

Index: alsa-kernel/core/pcm_native.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/core/pcm_native.c,v
retrieving revision 1.63
diff -u -r1.63 pcm_native.c
--- alsa-kernel/core/pcm_native.c	21 Oct 2003 09:49:34 -0000	1.63
+++ alsa-kernel/core/pcm_native.c	24 Nov 2003 17:46:04 -0000
@@ -620,7 +620,7 @@
  */
 static int snd_pcm_action_group(struct action_ops *ops,
 				snd_pcm_substream_t *substream,
-				int state)
+				int state, int atomic_only)
 {
 	struct list_head *pos;
 	snd_pcm_substream_t *s = NULL;
@@ -628,6 +628,8 @@
 
 	snd_pcm_group_for_each(pos, substream) {
 		s = snd_pcm_group_substream_entry(pos);
+		if (atomic_only && (s->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS))
+			continue;
 		if (s != substream)
 			spin_lock(&s->self_group.lock);
 		res = ops->pre_action(s, state);
@@ -637,6 +639,8 @@
 	if (res >= 0) {
 		snd_pcm_group_for_each(pos, substream) {
 			s = snd_pcm_group_substream_entry(pos);
+			if (atomic_only && (s->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS))
+				continue;
 			err = ops->do_action(s, state);
 			if (err < 0) {
 				if (res == 0)
@@ -652,7 +656,9 @@
 		/* unlock all streams */
 		snd_pcm_group_for_each(pos, substream) {
 			s1 = snd_pcm_group_substream_entry(pos);
-			if (s1 != substream)
+			if (atomic_only && (s1->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS))
+				;
+			else if (s1 != substream)
 				spin_unlock(&s1->self_group.lock);
 			if (s1 == s)	/* end */
 				break;
@@ -682,6 +688,8 @@
 
 /*
  *  Note: call with stream lock
+ *
+ * NB2: this won't handle the non-atomic callbacks
  */
 static int snd_pcm_action(struct action_ops *ops,
 			  snd_pcm_substream_t *substream,
@@ -695,7 +703,7 @@
 			spin_lock(&substream->group->lock);
 			spin_lock(&substream->self_group.lock);
 		}
-		res = snd_pcm_action_group(ops, substream, state);
+		res = snd_pcm_action_group(ops, substream, state, 0);
 		spin_unlock(&substream->group->lock);
 	} else {
 		res = snd_pcm_action_single(ops, substream, state);
@@ -705,10 +713,14 @@
 
 /*
  *  Note: don't use any locks before
+ *
+ * NB2: this can handle the non-atomic callbacks if allow_nonatomic = 1
+ *      when the pcm->info_flags has NONATOMIC_OPS bit, it's handled
+ *      ouside the lock to allow sleep in the callback.
  */
 static int snd_pcm_action_lock_irq(struct action_ops *ops,
 				   snd_pcm_substream_t *substream,
-				   int state)
+				   int state, int allow_nonatomic)
 {
 	int res;
 
@@ -716,10 +728,43 @@
 	if (snd_pcm_stream_linked(substream)) {
 		spin_lock(&substream->group->lock);
 		spin_lock(&substream->self_group.lock);
-		res = snd_pcm_action_group(ops, substream, state);
+		res = snd_pcm_action_group(ops, substream, state, allow_nonatomic);
 		spin_unlock(&substream->self_group.lock);
 		spin_unlock(&substream->group->lock);
+		if (res >= 0 && allow_nonatomic) {
+			/* now process the non-atomic substreams separately
+			 * outside the lock
+			 */
+#define MAX_LINKED_STREAMS	16	/* FIXME: should be variable */
+
+			struct list_head *pos;
+			int i, num_s = 0;
+			snd_pcm_substream_t *s;
+			snd_pcm_substream_t *subs[MAX_LINKED_STREAMS];
+			snd_pcm_group_for_each(pos, substream) {
+				if (num_s >= MAX_LINKED_STREAMS) {
+					res = -ENOMEM;
+					num_s = 0; /* don't proceed */
+					break;
+				}
+				s = snd_pcm_group_substream_entry(pos);
+				if (s->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS)
+					subs[num_s++] = s;
+			}
+			if (num_s > 0) {
+				read_unlock_irq(&snd_pcm_link_rwlock);
+				for (i = 0; i < num_s && res >= 0; i++)
+					res = snd_pcm_action_single(ops, subs[i], state);
+				return res;
+			}
+		}
 	} else {
+		if (allow_nonatomic &&
+		    (substream->pcm->info_flags & SNDRV_PCM_INFO_NONATOMIC_OPS)) {
+			read_unlock_irq(&snd_pcm_link_rwlock);
+			/* process outside the lock */
+			return snd_pcm_action_single(ops, substream, state);
+		}
 		spin_lock(&substream->self_group.lock);
 		res = snd_pcm_action_single(ops, substream, state);
 		spin_unlock(&substream->self_group.lock);
@@ -993,7 +1038,7 @@
 
 	snd_power_lock(card);
 	if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0, substream->ffile)) >= 0)
-		return snd_pcm_action_lock_irq(&snd_pcm_action_resume, substream, 0);
+		return snd_pcm_action_lock_irq(&snd_pcm_action_resume, substream, 0, 0);
 	snd_power_unlock(card);
 	return res;
 }
@@ -1083,7 +1128,7 @@
 
 static int snd_pcm_reset(snd_pcm_substream_t *substream)
 {
-	return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0);
+	return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0, 0);
 }
 
 static int snd_pcm_pre_prepare(snd_pcm_substream_t * substream, int state)
@@ -1131,7 +1176,7 @@
 
 	snd_power_lock(card);
 	if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0, substream->ffile)) >= 0)
-		res = snd_pcm_action_lock_irq(&snd_pcm_action_prepare, substream, 0);
+		res = snd_pcm_action_lock_irq(&snd_pcm_action_prepare, substream, 0, 1); /* allow sleep if specified */
 	snd_power_unlock(card);
 	return res;
 }
@@ -2330,7 +2375,7 @@
 	case SNDRV_PCM_IOCTL_RESET:
 		return snd_pcm_reset(substream);
 	case SNDRV_PCM_IOCTL_START:
-		return snd_pcm_action(&snd_pcm_action_start, substream, 0);
+		return snd_pcm_action_lock_irq(&snd_pcm_action_start, substream, 0, 0);
 	case SNDRV_PCM_IOCTL_LINK:
 		return snd_pcm_link(substream, (long) arg);
 	case SNDRV_PCM_IOCTL_UNLINK:
Index: alsa-kernel/include/asound.h
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/include/asound.h,v
retrieving revision 1.24
diff -u -r1.24 asound.h
--- alsa-kernel/include/asound.h	22 Oct 2003 09:41:06 -0000	1.24
+++ alsa-kernel/include/asound.h	24 Nov 2003 12:23:47 -0000
@@ -272,6 +272,7 @@
 #define SNDRV_PCM_INFO_HALF_DUPLEX	0x00100000	/* only half duplex */
 #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
 #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
+#define SNDRV_PCM_INFO_NONATOMIC_OPS	0x00800000	/* non-atomic prepare callback */
 
 enum sndrv_pcm_state {
 	SNDRV_PCM_STATE_OPEN = 0,	/* stream is open */
Index: alsa-kernel/usb/usbaudio.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/usb/usbaudio.c,v
retrieving revision 1.77
diff -u -r1.77 usbaudio.c
--- alsa-kernel/usb/usbaudio.c	24 Nov 2003 11:51:02 -0000	1.77
+++ alsa-kernel/usb/usbaudio.c	24 Nov 2003 17:49:59 -0000
@@ -28,9 +28,8 @@
  *  NOTES:
  *
  *   - async unlink should be used for avoiding the sleep inside lock.
- *     however, it causes oops by unknown reason on usb-uhci, and
- *     disabled as default.  the feature is enabled by async_unlink=1
- *     option (especially when preempt is used).
+ *     2.4.22 usb-uhci seems buggy for async unlinking and results in
+ *     oops.  in such a cse, pass async_unlink=0 option.
  *   - the linked URBs would be preferred but not used so far because of
  *     the instability of unlinking.
  *   - type II is not supported properly.  there is no device which supports
@@ -69,7 +68,7 @@
 static int vid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 }; /* Vendor ID for this card */
 static int pid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 }; /* Product ID for this card */
 static int nrpacks = 4;		/* max. number of packets per urb */
-static int async_unlink = 0;
+static int async_unlink = 1;
 
 MODULE_PARM(index, "1-" __MODULE_STRING(SNDRV_CARDS) "i");
 MODULE_PARM_DESC(index, "Index value for the USB audio adapter.");
@@ -804,8 +803,7 @@
 	int i;
 
 	/* stop urbs (to be sure) */
-	deactivate_urbs(subs, force, 1);
-	if (async_unlink)
+	if (deactivate_urbs(subs, force, 1) > 0)
 		wait_clear_urbs(subs);
 
 	for (i = 0; i < MAX_URBS; i++)
@@ -834,12 +832,6 @@
 	subs->freqmax = subs->freqn + (subs->freqn >> 2); /* max. allowed frequency */
 	subs->phase = 0;
 
-	/* reset the pointer */
-	subs->hwptr = 0;
-	subs->hwptr_done = 0;
-	subs->transfer_sched = 0;
-	subs->transfer_done = 0;
-
 	/* calculate the max. size of packet */
 	maxsize = ((subs->freqmax + 0x3fff) * (frame_bits >> 3)) >> 14;
 	if (subs->maxpacksize && maxsize > subs->maxpacksize) {
@@ -1277,6 +1269,17 @@
 	subs->maxframesize = bytes_to_frames(runtime, subs->maxpacksize);
 	subs->curframesize = bytes_to_frames(runtime, subs->curpacksize);
 
+	/* reset the pointer */
+	subs->hwptr = 0;
+	subs->hwptr_done = 0;
+	subs->transfer_sched = 0;
+	subs->transfer_done = 0;
+	subs->phase = 0;
+
+	/* clear urbs (to be sure) */
+	if (deactivate_urbs(subs, 0, 0) > 0)
+		wait_clear_urbs(subs);
+
 	return 0;
 }
 
@@ -2002,7 +2005,7 @@
 	as->pcm = pcm;
 	pcm->private_data = as;
 	pcm->private_free = snd_usb_audio_pcm_free;
-	pcm->info_flags = 0;
+	pcm->info_flags = SNDRV_PCM_INFO_NONATOMIC_OPS;
 	if (chip->pcm_devs > 0)
 		sprintf(pcm->name, "USB Audio #%d", chip->pcm_devs);
 	else
Index: alsa-driver/usb/usbaudio.patch
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-driver/usb/usbaudio.patch,v
retrieving revision 1.1
diff -u -r1.1 usbaudio.patch
--- alsa-driver/usb/usbaudio.patch	2 Jun 2003 10:07:21 -0000	1.1
+++ alsa-driver/usb/usbaudio.patch	24 Nov 2003 17:51:55 -0000
@@ -1,11 +1,25 @@
---- usbaudio.c	2003-06-01 20:32:24.000000000 +0200
-+++ usbaudio.c.old	2003-06-01 20:32:42.000000000 +0200
+--- usbaudio.c	2003-11-24 18:49:59.000000000 +0100
++++ usbaudio.c	2003-11-24 18:51:33.000000000 +0100
 @@ -1,3 +1,4 @@
 +#include "usbaudio.inc"
  /*
   *   (Tentative) USB Audio Driver for ALSA
   *
-@@ -1656,9 +1657,11 @@
+@@ -68,7 +69,12 @@
+ static int vid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 }; /* Vendor ID for this card */
+ static int pid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 }; /* Product ID for this card */
+ static int nrpacks = 4;		/* max. number of packets per urb */
+-static int async_unlink = 1;
++static int async_unlink =
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 0)
++1;
++#else
++0; /* disabled as default for buggy async-unlink handling */
++#endif
+ 
+ MODULE_PARM(index, "1-" __MODULE_STRING(SNDRV_CARDS) "i");
+ MODULE_PARM_DESC(index, "Index value for the USB audio adapter.");
+@@ -1761,9 +1767,11 @@
   * entry point for linux usb interface
   */
  
@@ -17,7 +31,7 @@
  
  static struct usb_device_id usb_audio_ids [] = {
  #include "usbquirks.h"
-@@ -1671,10 +1674,15 @@
+@@ -1776,10 +1784,15 @@
  MODULE_DEVICE_TABLE (usb, usb_audio_ids);
  
  static struct usb_driver usb_audio_driver = {
@@ -33,7 +47,7 @@
  	.id_table =	usb_audio_ids,
  };
  
-@@ -2744,6 +2752,7 @@
+@@ -2918,6 +2931,7 @@
  	}
  }
  
@@ -41,7 +55,7 @@
  /*
   * new 2.5 USB kernel API
   */
-@@ -2764,6 +2773,8 @@
+@@ -2938,6 +2952,8 @@
  	snd_usb_audio_disconnect(interface_to_usbdev(intf),
  				 dev_get_drvdata(&intf->dev));
  }
@@ -50,7 +64,7 @@
  
  
  static int __init snd_usb_audio_init(void)
-@@ -2803,3 +2814,5 @@
+@@ -2981,3 +2997,5 @@
  __setup("snd-usb-audio=", snd_usb_audio_module_setup);
  
  #endif /* !MODULE */

Re: [PATCH] usbaudio fixes

[Jaroslav Kysela]

On Mon, 24 Nov 2003, Takashi Iwai wrote:

> At Mon, 24 Nov 2003 13:51:50 +0100,
> I wrote:
> > 
> > At Mon, 24 Nov 2003 13:36:42 +0100,
> > I wrote:
> > > 
> > > meanwhile, i'm trying to implement:
> > > 
> > > (4) allow prepare callback to sleep with a special flag.
> > > 
> > > this will be useful for other drivers, too, such as vx and korg1212
> > > drivers which require the handshaking.
> > > but what i'm doing is still a hack, and will be a fundamental rewrite
> > > later.
> > 
> > the attached is a quick-hacked version.
> > it's untested for linked streams.  we need a test case here.
> 
> ok, here is the final version.  this one seems working fine.
> 
> Jaroslav, could you check whether it's ok?
> the changes in pcm_native.c shouldn't affect other cards.

It's ok, but it still looks like an ugly hack, but I don't see any other 
way to solve this problem so you can apply this code to CVS.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/

Re: [PATCH] usbaudio fixes

[Takashi Iwai]

At Tue, 25 Nov 2003 12:51:11 +0100 (CET),
Jaroslav wrote:
> 
> On Mon, 24 Nov 2003, Takashi Iwai wrote:
> 
> > At Mon, 24 Nov 2003 13:51:50 +0100,
> > I wrote:
> > > 
> > > At Mon, 24 Nov 2003 13:36:42 +0100,
> > > I wrote:
> > > > 
> > > > meanwhile, i'm trying to implement:
> > > > 
> > > > (4) allow prepare callback to sleep with a special flag.
> > > > 
> > > > this will be useful for other drivers, too, such as vx and korg1212
> > > > drivers which require the handshaking.
> > > > but what i'm doing is still a hack, and will be a fundamental rewrite
> > > > later.
> > > 
> > > the attached is a quick-hacked version.
> > > it's untested for linked streams.  we need a test case here.
> > 
> > ok, here is the final version.  this one seems working fine.
> > 
> > Jaroslav, could you check whether it's ok?
> > the changes in pcm_native.c shouldn't affect other cards.
> 
> It's ok, but it still looks like an ugly hack, but I don't see any other 
> way to solve this problem so you can apply this code to CVS.

yeah, agreed, it's a quick'n'dirty hack :)

the better way would be to move all prepare callback in every driver
as non-atomic, and call it inside a mutex.  linking/unlinking streams
will need to issue both a mutex and a spinlock for prepare and trigger
callbacks.
but, for this, we have to audit all drivers.  let's beautify later.

ok, i'll commit the changes now.


thanks,

Takashi


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/