Re: [PATCH] usbaudio fixes

[Takashi Iwai <tiwai@suse.de>]

[-- 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 */