[alsa-devel] [PATCH v4 00/15] ALSA: usb-audio: fix playback/capture concurrent usage
Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices.
Now the following work properly on my system: * Full-duplex with jack :)
* All tests I ran with my tool. Will post it in case anyone want to try it, it takes about 15 minutes to run all the tests.
* Starting a capture-only jackd instance, e.g.: jackd -n 1 -d alsa -d hw:2,0 -C and then playback-only: jackd -n 2 -d alsa -d hw:2,0 -P and then stop and start the playback. The same with playback running and restarting capture.
* Trying to start a second substream with parameter mismatch (rate or period bytes is all I can test with my device) will fail, without ill effects.
With this series applied there are now 3 usage tracking variables: - substream usage flag - endpoint param_set (to protect the parameters until the endpoint is started) - the existing endpoint use_count
Changes from v3: * Added substream "used" flag, to prevent failure in some combinations of ops. (#13) * Moved start_endpoints for capture to prepare (#14) * Added more constraints to the endpoint_may_set_params function (#10) * Cleanups (#12, #15)
v3: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065744.html
Applies against for-next, tested on upstream 3.12-rc3 (needs Alan Stern's recent patch "improve buffer size computations for USB PCM audio" to apply cleanly on current mainline).
Daniel and Nikolay: if you can test again with this series I'd appreciate it.
Cheers, Eldad
Eldad Zack (15): ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: correct ep use_count semantics (add set_param flag) ALSA: usb-audio: rename alt_idx to altsetting ALSA: usb-audio: conditional interface altsetting ALSA: usb-audio: conditional concurrent usage of endpoint ALSA: usb-audio: remove altset_idx from snd_usb_substream ALSA: usb-audio: remove unused endpoint flag EP_FLAG_ACTIVATED ALSA: usb-audio: clear sync subs hw_params ALSA: usb-audio: always wait in start_endpoints ALSA: usb-audio: improve logging messages
sound/usb/card.h | 9 ++- sound/usb/endpoint.c | 122 ++++++++++++++++++++--------- sound/usb/endpoint.h | 13 +++- sound/usb/pcm.c | 214 ++++++++++++++++++++++++++++++++++++--------------- sound/usb/pcm.h | 2 + sound/usb/proc.c | 4 +- 6 files changed, 258 insertions(+), 106 deletions(-)
Since the format is not actually used in sync_ep_set_params(), there is no need to pass it down.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/endpoint.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 21dc642..5dd51af 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -714,8 +714,7 @@ out_of_memory: /* * configure a sync endpoint */ -static int sync_ep_set_params(struct snd_usb_endpoint *ep, - struct audioformat *fmt) +static int sync_ep_set_params(struct snd_usb_endpoint *ep) { int i;
@@ -812,7 +811,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, buffer_periods, fmt, sync_ep); break; case SND_USB_ENDPOINT_TYPE_SYNC: - err = sync_ep_set_params(ep, fmt); + err = sync_ep_set_params(ep); break; default: err = -EINVAL;
The only call site for deactivate_endpoints() at snd_usb_hw_free(). The return value is not checked there, as it is irrelevant if it fails on hw_free. This patch moves the deactivation of the endpoints directly into snd_usb_hw_free().
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 19e7995..1a9a018 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -282,22 +282,6 @@ static void stop_endpoints(struct snd_usb_substream *subs, bool wait) } }
-static int deactivate_endpoints(struct snd_usb_substream *subs) -{ - int reta, retb; - - reta = snd_usb_endpoint_deactivate(subs->sync_endpoint); - retb = snd_usb_endpoint_deactivate(subs->data_endpoint); - - if (reta < 0) - return reta; - - if (retb < 0) - return retb; - - return 0; -} - static int search_roland_implicit_fb(struct usb_device *dev, int ifnum, unsigned int altsetting, struct usb_host_interface **alts, @@ -736,7 +720,8 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) down_read(&subs->stream->chip->shutdown_rwsem); if (!subs->stream->chip->shutdown) { stop_endpoints(subs, true); - deactivate_endpoints(subs); + snd_usb_endpoint_deactivate(subs->sync_endpoint); + snd_usb_endpoint_deactivate(subs->data_endpoint); } up_read(&subs->stream->chip->shutdown_rwsem); return snd_pcm_lib_free_vmalloc_buffer(substream);
If an endpoint uses another endpoint for synchronization, and the other endpoint is stopped, an oops will occur on NULL dereference. Clearing the prepare/retire callbacks solves this issue.
v2: Thanks to Daniel Mack, fixed (an ironic) NULL dereference when the pcm substream is opened and closed immediately.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 1a9a018..525bc8c 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -1205,6 +1205,11 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) subs->interface = -1; }
+ if (subs->data_endpoint) { + subs->data_endpoint->prepare_data_urb = NULL; + subs->data_endpoint->retire_data_urb = NULL; + } + subs->pcm_substream = NULL; snd_usb_autosuspend(subs->stream->chip);
@@ -1535,6 +1540,8 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP: + subs->data_endpoint->prepare_data_urb = NULL; + subs->data_endpoint->retire_data_urb = NULL; stop_endpoints(subs, false); subs->running = 0; return 0; @@ -1565,6 +1572,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP: + subs->data_endpoint->retire_data_urb = NULL; stop_endpoints(subs, false); subs->running = 0; return 0;
At Sun, 6 Oct 2013 22:31:08 +0200, Eldad Zack wrote:
If an endpoint uses another endpoint for synchronization, and the other endpoint is stopped, an oops will occur on NULL dereference. Clearing the prepare/retire callbacks solves this issue.
v2: Thanks to Daniel Mack, fixed (an ironic) NULL dereference when the pcm substream is opened and closed immediately.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
sound/usb/pcm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 1a9a018..525bc8c 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -1205,6 +1205,11 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) subs->interface = -1; }
- if (subs->data_endpoint) {
subs->data_endpoint->prepare_data_urb = NULL;subs->data_endpoint->retire_data_urb = NULL;- }
This is superfluous as the close callback is always called after tigger stop and hw_free.
Takashi
subs->pcm_substream = NULL; snd_usb_autosuspend(subs->stream->chip);
@@ -1535,6 +1540,8 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP:
subs->data_endpoint->prepare_data_urb = NULL;subs->data_endpoint->retire_data_urb = NULL;@@ -1565,6 +1572,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP:
subs->data_endpoint->retire_data_urb = NULL;-- 1.8.1.5
If an endpoint in use, its associated URBs should not be deactivated.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/endpoint.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 5dd51af..e84732c 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -959,12 +959,12 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep) if (!ep) return -EINVAL;
- deactivate_urbs(ep, true); - wait_clear_urbs(ep); - if (ep->use_count != 0) return 0;
+ deactivate_urbs(ep, true); + wait_clear_urbs(ep); + clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
return 0;
The return value of snd_usb_endpoint_deactivate() is not used, make the function have no return value. Update the documentation to reflect what the function is actually doing.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/endpoint.c | 15 +++++---------- sound/usb/endpoint.h | 2 +- 2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index e84732c..2685660 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -946,28 +946,23 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) * * @ep: the endpoint to deactivate * - * If the endpoint is not currently in use, this functions will select the - * alternate interface setting 0 for the interface of this endpoint. + * If the endpoint is not currently in use, this functions will + * deactivate its associated URBs. * * In case of any active users, this functions does nothing. - * - * Returns an error if usb_set_interface() failed, 0 in all other - * cases. */ -int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep) +void snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep) { if (!ep) - return -EINVAL; + return;
if (ep->use_count != 0) - return 0; + return;
deactivate_urbs(ep, true); wait_clear_urbs(ep);
clear_bit(EP_FLAG_ACTIVATED, &ep->flags); - - return 0; }
/** diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 3bd02f0..1c7e8ee 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -22,7 +22,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); -int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep); +void snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep); void snd_usb_endpoint_free(struct list_head *head);
int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
If setting the interface fails, the SUBSTREAM_FLAG_SYNC_EP_STARTED should be cleared.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/pcm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 525bc8c..e38961e 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -246,6 +246,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) subs->sync_endpoint->iface, subs->sync_endpoint->alt_idx); if (err < 0) { + clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); snd_printk(KERN_ERR "%d:%d:%d: cannot set interface (%d)\n", subs->dev->devnum,
Currently, use_count is used in snd_usb_endpoint_set_params to ensure the parameters don't get changed for an in-use endpoint.
However, there is a subtle condition where this check fails - if hw_params is called on both substreams before calling prepare (for playback) or start trigger (for capture): the endpoint is not yet started, i.e., snd_usb_endpoint_start() does not yet increment use_count.
This adds a flag to check if the parameters are set, but does not omit checking the use_count.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
--- v2: Cleaned up the patch, now it does only one thing (add the flag and the check). --- sound/usb/card.h | 1 + sound/usb/endpoint.c | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index ca98a9b..1014136 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -51,6 +51,7 @@ struct snd_usb_endpoint { struct snd_usb_audio *chip;
int use_count; + bool param_set; int ep_num; /* the referenced endpoint number */ int type; /* SND_USB_ENDPOINT_TYPE_* */ unsigned long flags; diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 2685660..9acc864 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -472,6 +472,8 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, ep->syncmaxsize = le16_to_cpu(get_endpoint(alts, 1)->wMaxPacketSize); }
+ ep->param_set = false; + list_add_tail(&ep->list, &chip->ep_list);
__exit_unlock: @@ -780,11 +782,12 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, { int err;
- if (ep->use_count != 0) { + if (ep->param_set || ep->use_count != 0) { snd_printk(KERN_WARNING "Unable to change format on ep #%x: already in use\n", ep->ep_num); return -EBUSY; } + ep->param_set = true;
/* release old buffers, if any */ release_urbs(ep, 0); @@ -937,6 +940,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) ep->sync_slave = NULL; ep->retire_data_urb = NULL; ep->prepare_data_urb = NULL; + ep->param_set = false; set_bit(EP_FLAG_STOPPING, &ep->flags); } }
At Sun, 6 Oct 2013 22:31:12 +0200, Eldad Zack wrote:
Currently, use_count is used in snd_usb_endpoint_set_params to ensure the parameters don't get changed for an in-use endpoint.
However, there is a subtle condition where this check fails - if hw_params is called on both substreams before calling prepare (for playback) or start trigger (for capture): the endpoint is not yet started, i.e., snd_usb_endpoint_start() does not yet increment use_count.
This adds a flag to check if the parameters are set, but does not omit checking the use_count.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
v2: Cleaned up the patch, now it does only one thing (add the flag and the check).
I guess this logic doesn't work if a capture stream is re-prepared before started. I know that you'll change the capture stream starting in the later patch, but then this patch must be applied after that, because it implicitly assumes the scheme.
Takashi
sound/usb/card.h | 1 + sound/usb/endpoint.c | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index ca98a9b..1014136 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -51,6 +51,7 @@ struct snd_usb_endpoint { struct snd_usb_audio *chip;
int use_count;
- bool param_set; int ep_num; /* the referenced endpoint number */ int type; /* SND_USB_ENDPOINT_TYPE_* */ unsigned long flags;
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 2685660..9acc864 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -472,6 +472,8 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, ep->syncmaxsize = le16_to_cpu(get_endpoint(alts, 1)->wMaxPacketSize); }
- ep->param_set = false;
- list_add_tail(&ep->list, &chip->ep_list);
__exit_unlock: @@ -780,11 +782,12 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, { int err;
- if (ep->use_count != 0) {
if (ep->param_set || ep->use_count != 0) { snd_printk(KERN_WARNING "Unable to change format on ep #%x: already in use\n", ep->ep_num); return -EBUSY; }
ep->param_set = true;
/* release old buffers, if any */ release_urbs(ep, 0);
@@ -937,6 +940,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) ep->sync_slave = NULL; ep->retire_data_urb = NULL; ep->prepare_data_urb = NULL;
ep->param_set = false;}
1.8.1.5
On Mon, 7 Oct 2013, Takashi Iwai wrote:
At Sun, 6 Oct 2013 22:31:12 +0200, Eldad Zack wrote:
Currently, use_count is used in snd_usb_endpoint_set_params to ensure the parameters don't get changed for an in-use endpoint.
However, there is a subtle condition where this check fails - if hw_params is called on both substreams before calling prepare (for playback) or start trigger (for capture): the endpoint is not yet started, i.e., snd_usb_endpoint_start() does not yet increment use_count.
This adds a flag to check if the parameters are set, but does not omit checking the use_count.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
v2: Cleaned up the patch, now it does only one thing (add the flag and the check).
I guess this logic doesn't work if a capture stream is re-prepared before started. I know that you'll change the capture stream starting in the later patch, but then this patch must be applied after that, because it implicitly assumes the scheme.
Yes and it causes a regression too, it prevents full-duplex usage (with jack or client that use the same sequence). Sorry, I forgot to add that to the commit message.
I left this so the change is clear, I figured it helps when reading the history. Should I combine this with the later patch? Or should I put a notice in this commit message that "this patch breaks... will be fixed later by..."?
BTW - sorry if that's a trivial question - is it possible for ops to race, or do the ops get serialized? If it is possible, I believe there's a few places that need locking.
Cheers, Eldad
At Mon, 7 Oct 2013 21:31:35 +0200 (CEST), Eldad Zack wrote:
On Mon, 7 Oct 2013, Takashi Iwai wrote:
At Sun, 6 Oct 2013 22:31:12 +0200, Eldad Zack wrote:
Currently, use_count is used in snd_usb_endpoint_set_params to ensure the parameters don't get changed for an in-use endpoint.
However, there is a subtle condition where this check fails - if hw_params is called on both substreams before calling prepare (for playback) or start trigger (for capture): the endpoint is not yet started, i.e., snd_usb_endpoint_start() does not yet increment use_count.
This adds a flag to check if the parameters are set, but does not omit checking the use_count.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
v2: Cleaned up the patch, now it does only one thing (add the flag and the check).
I guess this logic doesn't work if a capture stream is re-prepared before started. I know that you'll change the capture stream starting in the later patch, but then this patch must be applied after that, because it implicitly assumes the scheme.
Yes and it causes a regression too, it prevents full-duplex usage (with jack or client that use the same sequence). Sorry, I forgot to add that to the commit message.
I left this so the change is clear, I figured it helps when reading the history. Should I combine this with the later patch? Or should I put a notice in this commit message that "this patch breaks... will be fixed later by..."?
At best shuffle the order of patches so that this one is applied after the required patches.
BTW - sorry if that's a trivial question - is it possible for ops to race, or do the ops get serialized? If it is possible, I believe there's a few places that need locking.
Which ops specifically? For a PCM substream, it's serialized. For duplex substreams, both can be called at the same time, thus you'd need to protect concurrent accesses, if any.
Takashi
As Clemens Ladisch kindly explained: "Please note that there are two methods to identify alternate settings: the number, which is the value in bAlternateSetting, and the index, which is the index in the descriptor array. There might be some wording in the USB spec that these two values must be the same, but in reality, [insert standard rant about firmware writers], bAlternateSetting must be treated as a random ID value."
This patch changes the name to express the correct usage semantics. No functional change.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/card.h | 2 +- sound/usb/endpoint.c | 6 +++--- sound/usb/pcm.c | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 1014136..7ccacb4 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -97,7 +97,7 @@ struct snd_usb_endpoint { unsigned int syncinterval; /* P for adaptive mode, 0 otherwise */ unsigned char silence_value; unsigned int stride; - int iface, alt_idx; + int iface, altsetting; int skip_packets; /* quirks for devices to ignore the first n packets in a stream */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 9acc864..c90d065 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -426,9 +426,9 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, list_for_each_entry(ep, &chip->ep_list, list) { if (ep->ep_num == ep_num && ep->iface == alts->desc.bInterfaceNumber && - ep->alt_idx == alts->desc.bAlternateSetting) { + ep->altsetting == alts->desc.bAlternateSetting) { snd_printdd(KERN_DEBUG "Re-using EP %x in iface %d,%d @%p\n", - ep_num, ep->iface, ep->alt_idx, ep); + ep_num, ep->iface, ep->altsetting, ep); goto __exit_unlock; } } @@ -447,7 +447,7 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, ep->type = type; ep->ep_num = ep_num; ep->iface = alts->desc.bInterfaceNumber; - ep->alt_idx = alts->desc.bAlternateSetting; + ep->altsetting = alts->desc.bAlternateSetting; INIT_LIST_HEAD(&ep->ready_playback_urbs); ep_num &= USB_ENDPOINT_NUMBER_MASK;
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index e38961e..89381ad 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -241,17 +241,17 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) struct snd_usb_endpoint *ep = subs->sync_endpoint;
if (subs->data_endpoint->iface != subs->sync_endpoint->iface || - subs->data_endpoint->alt_idx != subs->sync_endpoint->alt_idx) { + subs->data_endpoint->altsetting != subs->sync_endpoint->altsetting) { err = usb_set_interface(subs->dev, subs->sync_endpoint->iface, - subs->sync_endpoint->alt_idx); + subs->sync_endpoint->altsetting); if (err < 0) { clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); snd_printk(KERN_ERR "%d:%d:%d: cannot set interface (%d)\n", subs->dev->devnum, subs->sync_endpoint->iface, - subs->sync_endpoint->alt_idx, err); + subs->sync_endpoint->altsetting, err); return -EIO; } }
On some devices, if the endpoint for the other direction is in use, setting one interface will shutdown the other (in use) endpoint. This patch moves all of the alternate setting operations for pcm ops to one function which checks if we can do so.
If current alternate is 0, it is safe to set.
Thanks to Clemens Ladisch for pointing out that cur_altsetting in usb_interface can be used to determine the current altsetting index as well as the correct naming convention for the altsetting number.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
--- v2: * Removed get_interface usage. * Fixed NULL dereference for explicit feedback - now other_subs will be set only after testing that subs is not NULL. v3: * Changed "altset_idx" to "altsetting". v4: * Removed helper functions --- sound/usb/pcm.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 8 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 89381ad..c165ccf 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -216,6 +216,60 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, } }
+static int get_cur_altsetting(struct usb_device *dev, int ifnum) +{ + struct usb_interface *iface; + struct usb_interface_descriptor *altsd; + + if (ifnum == -1) + return 0; + + iface = usb_ifnum_to_if(dev, ifnum); + if (!iface) + return -EINVAL; + + if (!iface->cur_altsetting) + return -EINVAL; + altsd = &(iface->cur_altsetting->desc); + + return altsd->bAlternateSetting; +} + +static int subs_set_interface(struct snd_usb_substream *subs, int ifnum, + int altsetting) +{ + struct snd_usb_substream *other_subs; + int err; + + snd_printdd(KERN_DEBUG "%s: Requested set interface %d alts %d\n", + __func__, ifnum, altsetting); + + if (subs == NULL) + return usb_set_interface(subs->dev, ifnum, altsetting); + other_subs = &(subs->stream->substream[subs->direction ^ 1]); + + if (other_subs->data_endpoint && + other_subs->data_endpoint->use_count != 0) { + int cur_altsetting = get_cur_altsetting(subs->dev, ifnum); + + if (cur_altsetting == altsetting) + return 0; + + if (cur_altsetting > 0 && altsetting == 0) + return 0; + } + + err = usb_set_interface(subs->dev, ifnum, altsetting); + snd_printdd(KERN_DEBUG "%s: Interface %d alts set to %d (err %d)\n", + __func__, ifnum, altsetting, err); + if (err < 0) + return err; + + subs->interface = altsetting == 0 ? -1 : ifnum; + + return 0; +} + static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) { int err; @@ -242,9 +296,9 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep)
if (subs->data_endpoint->iface != subs->sync_endpoint->iface || subs->data_endpoint->altsetting != subs->sync_endpoint->altsetting) { - err = usb_set_interface(subs->dev, - subs->sync_endpoint->iface, - subs->sync_endpoint->altsetting); + err = subs_set_interface(subs, + subs->sync_endpoint->iface, + subs->sync_endpoint->altsetting); if (err < 0) { clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); snd_printk(KERN_ERR @@ -467,20 +521,19 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
/* close the old interface */ if (subs->interface >= 0 && subs->interface != fmt->iface) { - err = usb_set_interface(subs->dev, subs->interface, 0); + err = subs_set_interface(subs, subs->interface, 0); if (err < 0) { snd_printk(KERN_ERR "%d:%d:%d: return to setting 0 failed (%d)\n", dev->devnum, fmt->iface, fmt->altsetting, err); return -EIO; } - subs->interface = -1; subs->altset_idx = 0; }
/* set interface */ if (subs->interface != fmt->iface || subs->altset_idx != fmt->altset_idx) { - err = usb_set_interface(dev, fmt->iface, fmt->altsetting); + err = subs_set_interface(subs, fmt->iface, fmt->altsetting); if (err < 0) { snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n", dev->devnum, fmt->iface, fmt->altsetting, err); @@ -488,7 +541,6 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) } snd_printdd(KERN_INFO "setting usb interface %d:%d\n", fmt->iface, fmt->altsetting); - subs->interface = fmt->iface; subs->altset_idx = fmt->altset_idx;
snd_usb_set_interface_quirk(dev); @@ -1202,7 +1254,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) stop_endpoints(subs, true);
if (!as->chip->shutdown && subs->interface >= 0) { - usb_set_interface(subs->dev, subs->interface, 0); + subs_set_interface(subs, subs->interface, 0); subs->interface = -1; }
At Sun, 6 Oct 2013 22:31:14 +0200, Eldad Zack wrote:
On some devices, if the endpoint for the other direction is in use, setting one interface will shutdown the other (in use) endpoint. This patch moves all of the alternate setting operations for pcm ops to one function which checks if we can do so.
If current alternate is 0, it is safe to set.
Is this check needed for all devices?
The combination of playback and capture streams in snd_usb_substream is casual, just depending on the enumeration. It doesn't always mean that these are coupled. For non-implicit-fb devices, won't this result in a false positive?
Also...
+static int subs_set_interface(struct snd_usb_substream *subs, int ifnum,
int altsetting)+{
- struct snd_usb_substream *other_subs;
- int err;
- snd_printdd(KERN_DEBUG "%s: Requested set interface %d alts %d\n",
__func__, ifnum, altsetting);- if (subs == NULL)
return usb_set_interface(subs->dev, ifnum, altsetting);
It must lead to Oops.
thanks,
Takashi
On Mon, 7 Oct 2013, Takashi Iwai wrote:
At Sun, 6 Oct 2013 22:31:14 +0200, Eldad Zack wrote:
On some devices, if the endpoint for the other direction is in use, setting one interface will shutdown the other (in use) endpoint. This patch moves all of the alternate setting operations for pcm ops to one function which checks if we can do so.
If current alternate is 0, it is safe to set.
Is this check needed for all devices?
I'm not sure, I only have this one device to test with. I asked once on the list if this behavior (when the alts is set to 0, the endpoint stops if it is running) or does it only happen on implicit feedback, but got no response.
The combination of playback and capture streams in snd_usb_substream is casual, just depending on the enumeration. It doesn't always mean that these are coupled. For non-implicit-fb devices, won't this result in a false positive?
The worst that can happen is that the altsetting will not go back to 0, if the other substream is in use. Is that an issue?
Also...
+static int subs_set_interface(struct snd_usb_substream *subs, int ifnum,
int altsetting)+{
- struct snd_usb_substream *other_subs;
- int err;
- snd_printdd(KERN_DEBUG "%s: Requested set interface %d alts %d\n",
__func__, ifnum, altsetting);- if (subs == NULL)
return usb_set_interface(subs->dev, ifnum, altsetting);It must lead to Oops.
Yes, that's bad code. :/ Thank you for pointing it out!
Although it won't get triggered, since it won't get any NULL subs from any callsite as far as I see. I'll remove this and add WARN_ON(!subs).
Cheers, Eldad
Eldad Zack wrote:
On Mon, 7 Oct 2013, Takashi Iwai wrote:
Eldad Zack wrote:
On some devices, if the endpoint for the other direction is in use, setting one interface will shutdown the other (in use) endpoint. This patch moves all of the alternate setting operations for pcm ops to one function which checks if we can do so.
If current alternate is 0, it is safe to set.
Is this check needed for all devices?
I'm not sure, I only have this one device to test with.
In theory, interfaces are independent, but in practice, the firmware does whatever it does. For implicit feedback devices, it's likely that they were expected to be used in full duplex mode only.
Regards, Clemens
At Mon, 07 Oct 2013 20:23:38 +0200, Clemens Ladisch wrote:
Eldad Zack wrote:
On Mon, 7 Oct 2013, Takashi Iwai wrote:
Eldad Zack wrote:
On some devices, if the endpoint for the other direction is in use, setting one interface will shutdown the other (in use) endpoint. This patch moves all of the alternate setting operations for pcm ops to one function which checks if we can do so.
If current alternate is 0, it is safe to set.
Is this check needed for all devices?
I'm not sure, I only have this one device to test with.
In theory, interfaces are independent, but in practice, the firmware does whatever it does. For implicit feedback devices, it's likely that they were expected to be used in full duplex mode only.
For the implicit feedback devices, I find the change being good, too. But my concern is rather that this filtering is applied to all cases, no matter whether it's implicit fb or not. In such cases, the assignment of full duplex streams is just casual, and they are likely individual.
thanks,
Takashi
If the current endpoint is in use, check if the hardware parameters match. If they do, allow the usage. This also requires setting the parameters in case an data endpoint is used as a synchornization source, and it is first set by its sink endpoint.
Also check the data endpoint on the substream in the other direction. If it is in use, the rate and format must match. The channel count may differ - but the period bytes must match. If the channel count is different, adjust the expected period bytes by the ratio of the channel count.
Add the same check to hw_params, to prevent overwriting the current params. Clear the hardware parameters on hw_free only if the other endpoint is not in use.
No change for sync endpoints (explicit feedback).
Signed-off-by: Eldad Zack eldad@fogrefinery.com
--- v2: added checking the data endpoint on the substream in the other direction. --- sound/usb/endpoint.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---- sound/usb/endpoint.h | 9 +++++++- sound/usb/pcm.c | 59 ++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 114 insertions(+), 17 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index c90d065..6cb3137 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -753,6 +753,51 @@ out_of_memory: return -ENOMEM; }
+bool snd_usb_endpoint_may_set_params(struct snd_usb_substream *subs, + snd_pcm_format_t pcm_format, + unsigned int channels, + unsigned int period_bytes, + unsigned int rate) +{ + struct snd_usb_substream *other_subs = + &subs->stream->substream[subs->direction ^ 1]; + struct snd_usb_endpoint *other_ep = other_subs->data_endpoint; + + /* no associated substream */ + if (!subs) + return true; + + if (other_ep && other_ep->use_count != 0) { + int other_pb; + if (other_subs->pcm_format != pcm_format || + other_subs->cur_rate != rate) + return false; + + other_pb = (other_subs->channels == channels ? + other_subs->period_bytes : + other_subs->period_bytes * channels / other_subs->channels); + if (other_pb != period_bytes) + return false; + } + + /* data_endpoint not yet initialized */ + if (!subs->data_endpoint) + return true; + + if (subs->data_endpoint->use_count == 0) + return true; + + if (subs->pcm_format != pcm_format || + subs->channels != channels || + subs->period_bytes != period_bytes || + subs->cur_rate != rate) + return false; + + snd_printdd(KERN_INFO "ep #%x in use, parameters match\n", + subs->data_endpoint->ep_num); + return true; +} + /** * snd_usb_endpoint_set_params: configure an snd_usb_endpoint * @@ -765,6 +810,7 @@ out_of_memory: * @rate: the frame rate. * @fmt: the USB audio format information * @sync_ep: the sync endpoint to use, if any + * @subs: the substream that uses this endpoint as data endpoint * * Determine the number of URBs to be used on this endpoint. * An endpoint must be configured before it can be started. @@ -778,14 +824,23 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, unsigned int buffer_periods, unsigned int rate, struct audioformat *fmt, - struct snd_usb_endpoint *sync_ep) + struct snd_usb_endpoint *sync_ep, + struct snd_usb_substream *subs) { int err;
if (ep->param_set || ep->use_count != 0) { - snd_printk(KERN_WARNING "Unable to change format on ep #%x: already in use\n", - ep->ep_num); - return -EBUSY; + if (!subs || + !snd_usb_endpoint_may_set_params(subs, pcm_format, + channels, period_bytes, + rate)) { + snd_printk(KERN_WARNING + "Unable to change format on ep #%x: already in use\n", + ep->ep_num); + return -EBUSY; + } + /* Endpoint already set with the same parameters */ + return 0; } ep->param_set = true;
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 1c7e8ee..188675d 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -16,7 +16,8 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, unsigned int buffer_periods, unsigned int rate, struct audioformat *fmt, - struct snd_usb_endpoint *sync_ep); + struct snd_usb_endpoint *sync_ep, + struct snd_usb_substream *subs);
int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); @@ -32,4 +33,10 @@ void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep, struct snd_usb_endpoint *sender, const struct urb *urb);
+bool snd_usb_endpoint_may_set_params(struct snd_usb_substream *subs, + snd_pcm_format_t pcm_format, + unsigned int channels, + unsigned int period_bytes, + unsigned int rate); + #endif /* __USBAUDIO_ENDPOINT_H */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index c165ccf..7895c81 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -635,6 +635,7 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs) 0, 0, subs->cur_rate, subs->cur_audiofmt, + NULL, NULL);
/* Try to find the best matching audioformat. */ @@ -672,9 +673,18 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs) 0, 0, subs->cur_rate, sync_fp, - NULL); + NULL, + sync_subs); + if (ret < 0) + return ret;
- return ret; + sync_subs->pcm_format = subs->pcm_format; + sync_subs->period_bytes = sync_period_bytes; + sync_subs->channels = sync_fp->channels; + sync_subs->cur_rate = subs->cur_rate; + sync_subs->data_endpoint = subs->sync_endpoint; + + return 0; }
/* @@ -696,7 +706,8 @@ static int configure_endpoint(struct snd_usb_substream *subs) subs->buffer_periods, subs->cur_rate, subs->cur_audiofmt, - subs->sync_endpoint); + subs->sync_endpoint, + subs); if (ret < 0) return ret;
@@ -722,18 +733,38 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, struct snd_usb_substream *subs = substream->runtime->private_data; struct audioformat *fmt; int ret; + snd_pcm_format_t pcm_format; + unsigned int period_bytes; + unsigned int period_frames; + unsigned int buffer_periods; + unsigned int channels; + unsigned int rate;
ret = snd_pcm_lib_alloc_vmalloc_buffer(substream, params_buffer_bytes(hw_params)); if (ret < 0) return ret;
- subs->pcm_format = params_format(hw_params); - subs->period_bytes = params_period_bytes(hw_params); - subs->period_frames = params_period_size(hw_params); - subs->buffer_periods = params_periods(hw_params); - subs->channels = params_channels(hw_params); - subs->cur_rate = params_rate(hw_params); + pcm_format = params_format(hw_params); + period_bytes = params_period_bytes(hw_params); + period_frames = params_period_size(hw_params); + buffer_periods = params_periods(hw_params); + channels = params_channels(hw_params); + rate = params_rate(hw_params); + + if (!snd_usb_endpoint_may_set_params(subs, pcm_format, channels, + period_bytes, rate)) { + snd_printk(KERN_WARNING "Unable to set hw_params on substream (dir: %d), data EP in use\n", + subs->direction); + return -EBUSY; + } + + subs->pcm_format = pcm_format; + subs->period_bytes = period_bytes; + subs->period_frames = period_frames; + subs->buffer_periods = buffer_periods; + subs->channels = channels; + subs->cur_rate = rate;
fmt = find_format(subs); if (!fmt) { @@ -767,9 +798,13 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) { struct snd_usb_substream *subs = substream->runtime->private_data;
- subs->cur_audiofmt = NULL; - subs->cur_rate = 0; - subs->period_bytes = 0; + if (!subs->data_endpoint || subs->data_endpoint->use_count == 0) { + subs->cur_audiofmt = NULL; + subs->cur_rate = 0; + subs->period_bytes = 0; + subs->channels = 0; + } + down_read(&subs->stream->chip->shutdown_rwsem); if (!subs->stream->chip->shutdown) { stop_endpoints(subs, true);
We only need to check the current altsetting in one place, so there's no benefit from tracking it. This patch replaces that one check with get_cur_altset_idx() added in a previous patch and in the proc substream dump routine.
Thanks to Clemens Ladisch for pointing out the difference between altset_idx and altsetting - the check is changed accordingly to match against fmt->altsetting.
Note that the proc dump now returns the altsetting number and not the array index.
This allows removing altset_idx from snd_usb_substream.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
--- v2: corrected check. --- sound/usb/card.h | 1 - sound/usb/pcm.c | 8 ++------ sound/usb/pcm.h | 2 ++ sound/usb/proc.c | 4 +++- 4 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index 7ccacb4..eb38b0a 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -120,7 +120,6 @@ struct snd_usb_substream { unsigned int period_bytes; /* current period bytes (for hw_params callback) */ unsigned int period_frames; /* current frames per period */ unsigned int buffer_periods; /* current periods per buffer */ - unsigned int altset_idx; /* USB data format: index of alternate setting */ unsigned int txfr_quirk:1; /* allow sub-frame alignment */ unsigned int fmt_type; /* USB audio format type (1-3) */ unsigned int pkt_offset_adj; /* Bytes to drop from beginning of packets (for non-compliant devices) */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 7895c81..8854ee5 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -216,7 +216,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, } }
-static int get_cur_altsetting(struct usb_device *dev, int ifnum) +int get_cur_altsetting(struct usb_device *dev, int ifnum) { struct usb_interface *iface; struct usb_interface_descriptor *altsd; @@ -527,12 +527,11 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) dev->devnum, fmt->iface, fmt->altsetting, err); return -EIO; } - subs->altset_idx = 0; }
/* set interface */ if (subs->interface != fmt->iface || - subs->altset_idx != fmt->altset_idx) { + get_cur_altsetting(subs->dev, subs->interface) != fmt->altsetting) { err = subs_set_interface(subs, fmt->iface, fmt->altsetting); if (err < 0) { snd_printk(KERN_ERR "%d:%d:%d: usb_set_interface failed (%d)\n", @@ -541,7 +540,6 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) } snd_printdd(KERN_INFO "setting usb interface %d:%d\n", fmt->iface, fmt->altsetting); - subs->altset_idx = fmt->altset_idx;
snd_usb_set_interface_quirk(dev); } @@ -783,7 +781,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, return ret;
subs->interface = fmt->iface; - subs->altset_idx = fmt->altset_idx; subs->need_setup_ep = true;
return 0; @@ -1267,7 +1264,6 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction) struct snd_usb_substream *subs = &as->substream[direction];
subs->interface = -1; - subs->altset_idx = 0; runtime->hw = snd_usb_hardware; runtime->private_data = subs; subs->pcm_substream = substream; diff --git a/sound/usb/pcm.h b/sound/usb/pcm.h index df7a003..0c9cb8c 100644 --- a/sound/usb/pcm.h +++ b/sound/usb/pcm.h @@ -10,5 +10,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, struct usb_host_interface *alts, struct audioformat *fmt);
+int get_cur_altsetting(struct usb_device *dev, int ifnum); +
#endif /* __USBAUDIO_PCM_H */ diff --git a/sound/usb/proc.c b/sound/usb/proc.c index 5f761ab..ab6aafe 100644 --- a/sound/usb/proc.c +++ b/sound/usb/proc.c @@ -27,6 +27,7 @@ #include "card.h" #include "endpoint.h" #include "proc.h" +#include "pcm.h"
/* convert our full speed USB rate into sampling rate in Hz */ static inline unsigned get_full_speed_hz(unsigned int usb_rate) @@ -140,7 +141,8 @@ static void proc_dump_substream_status(struct snd_usb_substream *subs, struct sn if (subs->running) { snd_iprintf(buffer, " Status: Running\n"); snd_iprintf(buffer, " Interface = %d\n", subs->interface); - snd_iprintf(buffer, " Altset = %d\n", subs->altset_idx); + snd_iprintf(buffer, " Altset = %d\n", + get_cur_altsetting(subs->dev, subs->interface)); proc_dump_ep_status(subs, subs->data_endpoint, subs->sync_endpoint, buffer); } else { snd_iprintf(buffer, " Status: Stop\n");
EP_FLAG_ACTIVATED is never tested for, remove it.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/endpoint.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 6cb3137..8379989 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -33,7 +33,6 @@ #include "pcm.h" #include "quirks.h"
-#define EP_FLAG_ACTIVATED 0 #define EP_FLAG_RUNNING 1 #define EP_FLAG_STOPPING 2
@@ -1020,8 +1019,6 @@ void snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep)
deactivate_urbs(ep, true); wait_clear_urbs(ep); - - clear_bit(EP_FLAG_ACTIVATED, &ep->flags); }
/**
If the snd_usb_substream associated with the sync endpoint is not in use as a data endpoint, clear its hw_params.
For implicit feedback, this prevents a failure condition when the capture substream is closed while the playback substream is open (here the capture substream does not clear its parameters, since use_count is 1), the playback is then closed and capture is opened again. The code will then assume that the endpoint is already running and will fail later.
Instead of just using the use_count of the endpoint, add a usage bit to the struct to track if the substream is opened, to allow clearing the parameters of the other substream if and only if it is not opened.
This flag must be set as soon as the substream is opened, since the endpoint is started only later (at prepare).
Also group together the bit flags.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/card.h | 5 +++-- sound/usb/pcm.c | 27 ++++++++++++++++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h index eb38b0a..d9cb674 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -120,11 +120,12 @@ struct snd_usb_substream { unsigned int period_bytes; /* current period bytes (for hw_params callback) */ unsigned int period_frames; /* current frames per period */ unsigned int buffer_periods; /* current periods per buffer */ - unsigned int txfr_quirk:1; /* allow sub-frame alignment */ unsigned int fmt_type; /* USB audio format type (1-3) */ unsigned int pkt_offset_adj; /* Bytes to drop from beginning of packets (for non-compliant devices) */
- unsigned int running: 1; /* running status */ + unsigned int txfr_quirk:1; /* allow sub-frame alignment */ + unsigned int running:1; /* running status */ + unsigned int used:1; /* substream used */
unsigned int hwptr_done; /* processed byte position in the buffer */ unsigned int transfer_done; /* processed frames since last period update */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 8854ee5..7ca9b47 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -786,6 +786,20 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, return 0; }
+static void clear_substream(struct snd_usb_substream *subs) +{ + if (subs->used != 0) + return; + + if (subs->data_endpoint && subs->data_endpoint->use_count != 0) + return; + + subs->cur_audiofmt = NULL; + subs->cur_rate = 0; + subs->period_bytes = 0; + subs->channels = 0; +} + /* * hw_free callback * @@ -795,11 +809,13 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) { struct snd_usb_substream *subs = substream->runtime->private_data;
- if (!subs->data_endpoint || subs->data_endpoint->use_count == 0) { - subs->cur_audiofmt = NULL; - subs->cur_rate = 0; - subs->period_bytes = 0; - subs->channels = 0; + subs->used = 0; + clear_substream(subs); + + if (subs->sync_endpoint) { + struct snd_usb_substream *sync_subs = + &subs->stream->substream[subs->direction ^ 1]; + clear_substream(sync_subs); }
down_read(&subs->stream->chip->shutdown_rwsem); @@ -1264,6 +1280,7 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction) struct snd_usb_substream *subs = &as->substream[direction];
subs->interface = -1; + subs->used = 1; runtime->hw = snd_usb_hardware; runtime->private_data = subs; subs->pcm_substream = substream;
Start the endpoints at prepare also for capture endpoints, since it might be needed to wait for the URBs to be unlinked.
If an implicit feedback source endpoint stops being used by its sink endpoint, but immediately used as a data endpoint, usb_submit_urb will return -EBUSY.
Merge two trigger cases since they are now the same.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/endpoint.c | 8 +++----- sound/usb/endpoint.h | 2 +- sound/usb/pcm.c | 28 ++++++++++++---------------- 3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 8379989..81056b6 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -884,18 +884,17 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, * snd_usb_endpoint_start: start an snd_usb_endpoint * * @ep: the endpoint to start - * @can_sleep: flag indicating whether the operation is executed in - * non-atomic context * * A call to this function will increment the use count of the endpoint. * In case it is not already running, the URBs for this endpoint will be * submitted. Otherwise, this function does nothing. * * Must be balanced to calls of snd_usb_endpoint_stop(). + * This operation can sleep and must not be executed in atomic context. * * Returns an error if the URB submission failed, 0 in all other cases. */ -int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) { int err; unsigned int i; @@ -909,8 +908,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep)
/* just to be sure */ deactivate_urbs(ep, false); - if (can_sleep) - wait_clear_urbs(ep); + wait_clear_urbs(ep);
ep->active_mask = 0; ep->unlink_mask = 0; diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 188675d..22baec8 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -19,7 +19,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, struct snd_usb_endpoint *sync_ep, struct snd_usb_substream *subs);
-int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep); void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 7ca9b47..f3d48d4 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -270,7 +270,7 @@ static int subs_set_interface(struct snd_usb_substream *subs, int ifnum, return 0; }
-static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) +static int start_endpoints(struct snd_usb_substream *subs) { int err;
@@ -283,7 +283,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) snd_printdd(KERN_DEBUG "Starting data EP @%p\n", ep);
ep->data_subs = subs; - err = snd_usb_endpoint_start(ep, can_sleep); + err = snd_usb_endpoint_start(ep); if (err < 0) { clear_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags); return err; @@ -313,7 +313,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) snd_printdd(KERN_DEBUG "Starting sync EP @%p\n", ep);
ep->sync_slave = subs->data_endpoint; - err = snd_usb_endpoint_start(ep, can_sleep); + err = snd_usb_endpoint_start(ep); if (err < 0) { clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); return err; @@ -893,10 +893,14 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) subs->last_frame_number = 0; runtime->delay = 0;
- /* for playback, submit the URBs now; otherwise, the first hwptr_done - * updates for all URBs would happen at the same time when starting */ - if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) - ret = start_endpoints(subs, true); + /* + * for playback, submit the URBs now; otherwise, the first hwptr_done + * updates for all URBs would happen at the same time when starting. + * for capture, waiting is required in case the endpoint is an implicit + * feedback source and it is has just been stopped (by the playback + * substream). + */ + ret = start_endpoints(subs);
unlock: up_read(&subs->stream->chip->shutdown_rwsem); @@ -1660,15 +1664,11 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream, int cmd) { - int err; struct snd_usb_substream *subs = substream->runtime->private_data;
switch (cmd) { case SNDRV_PCM_TRIGGER_START: - err = start_endpoints(subs, false); - if (err < 0) - return err; - + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: subs->data_endpoint->retire_data_urb = retire_capture_urb; subs->running = 1; return 0; @@ -1681,10 +1681,6 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream subs->data_endpoint->retire_data_urb = NULL; subs->running = 0; return 0; - case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - subs->data_endpoint->retire_data_urb = retire_capture_urb; - subs->running = 1; - return 0; }
return -EINVAL;
At Sun, 6 Oct 2013 22:31:19 +0200, Eldad Zack wrote:
Start the endpoints at prepare also for capture endpoints, since it might be needed to wait for the URBs to be unlinked.
If an implicit feedback source endpoint stops being used by its sink endpoint, but immediately used as a data endpoint, usb_submit_urb will return -EBUSY.
Merge two trigger cases since they are now the same.
This change worries me about the timing. This change means that the capture stream isn't started at the moment the trigger callback is called but at the next urb handling. It means a possible regression in the case of realtime usage.
Is there any reason to do this except for clean up? IOW, does this fix any problem by itself?
Takashi
Signed-off-by: Eldad Zack eldad@fogrefinery.com
sound/usb/endpoint.c | 8 +++----- sound/usb/endpoint.h | 2 +- sound/usb/pcm.c | 28 ++++++++++++---------------- 3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 8379989..81056b6 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -884,18 +884,17 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
- snd_usb_endpoint_start: start an snd_usb_endpoint
- @ep: the endpoint to start
- @can_sleep: flag indicating whether the operation is executed in
non-atomic context- A call to this function will increment the use count of the endpoint.
- In case it is not already running, the URBs for this endpoint will be
- submitted. Otherwise, this function does nothing.
- Must be balanced to calls of snd_usb_endpoint_stop().
*/
- This operation can sleep and must not be executed in atomic context.
- Returns an error if the URB submission failed, 0 in all other cases.
-int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) { int err; unsigned int i; @@ -909,8 +908,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep)
/* just to be sure */ deactivate_urbs(ep, false);
- if (can_sleep)
wait_clear_urbs(ep);
wait_clear_urbs(ep);
ep->active_mask = 0; ep->unlink_mask = 0;
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 188675d..22baec8 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -19,7 +19,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, struct snd_usb_endpoint *sync_ep, struct snd_usb_substream *subs);
-int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep); void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 7ca9b47..f3d48d4 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -270,7 +270,7 @@ static int subs_set_interface(struct snd_usb_substream *subs, int ifnum, return 0; }
-static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) +static int start_endpoints(struct snd_usb_substream *subs) { int err;
@@ -283,7 +283,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) snd_printdd(KERN_DEBUG "Starting data EP @%p\n", ep);
ep->data_subs = subs;
err = snd_usb_endpoint_start(ep, can_sleep);
err = snd_usb_endpoint_start(ep);@@ -313,7 +313,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) snd_printdd(KERN_DEBUG "Starting sync EP @%p\n", ep);
ep->sync_slave = subs->data_endpoint;
err = snd_usb_endpoint_start(ep, can_sleep);
err = snd_usb_endpoint_start(ep);@@ -893,10 +893,14 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) subs->last_frame_number = 0; runtime->delay = 0;
- /* for playback, submit the URBs now; otherwise, the first hwptr_done
* updates for all URBs would happen at the same time when starting */- if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
ret = start_endpoints(subs, true);
/*
* for playback, submit the URBs now; otherwise, the first hwptr_done* updates for all URBs would happen at the same time when starting.* for capture, waiting is required in case the endpoint is an implicit* feedback source and it is has just been stopped (by the playback* substream).*/ret = start_endpoints(subs);
unlock: up_read(&subs->stream->chip->shutdown_rwsem);
@@ -1660,15 +1664,11 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream, int cmd) {
int err; struct snd_usb_substream *subs = substream->runtime->private_data;
switch (cmd) { case SNDRV_PCM_TRIGGER_START:
err = start_endpoints(subs, false);if (err < 0)return err;
- case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: subs->data_endpoint->retire_data_urb = retire_capture_urb; subs->running = 1; return 0;
@@ -1681,10 +1681,6 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream subs->data_endpoint->retire_data_urb = NULL; subs->running = 0; return 0;
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
subs->data_endpoint->retire_data_urb = retire_capture_urb;subs->running = 1;return 0;}
return -EINVAL;
-- 1.8.1.5
On Mon, 7 Oct 2013, Takashi Iwai wrote:
At Sun, 6 Oct 2013 22:31:19 +0200, Eldad Zack wrote:
Start the endpoints at prepare also for capture endpoints, since it might be needed to wait for the URBs to be unlinked.
If an implicit feedback source endpoint stops being used by its sink endpoint, but immediately used as a data endpoint, usb_submit_urb will return -EBUSY.
Merge two trigger cases since they are now the same.
This change worries me about the timing. This change means that the capture stream isn't started at the moment the trigger callback is called but at the next urb handling. It means a possible regression in the case of realtime usage.
I'm not sure I understand. Do you mean it might cause the delay between capture and playback to vary at each startup?
Is there any reason to do this except for clean up? IOW, does this fix any problem by itself?
Yes, I only became aware of it since I bumped into it with my test tool. Attached here - I hope the mailing list accepts attachments.
I reverted the patch right now (mainline rc4 + this series) and this is the exact sequence: $ ./atest --device hw:2 stream --first 0x0038f0 atest v1 Testing device hw:2, rate 96000, range 38f0:0 * Test stream, Steps: [pcm_open] [pcm_hw_params] [pcm_prepare] [pcm_start] [waitsleep] [pcm_stop] [pcm_close] ++ Test: stream, permutation: 0x0038f0 * [1/14] playback => pcm_open * [2/14] playback => pcm_hw_params * [3/14] playback => pcm_prepare * [4/14] playback => pcm_start * [5/14] capture => pcm_open * [6/14] capture => pcm_hw_params * [7/14] capture => pcm_prepare * [8/14] capture => pcm_start * [9/14] playback => waitsleep * [10/14] playback => pcm_stop ++ Frames [playback]: 58560 * [11/14] playback => pcm_close * [12/14] capture => waitsleep * [13/14] capture => pcm_stop ++ Frames [capture]: 5760 * [14/14] capture => pcm_close ++ permutation: 0x0038f0, runtime: 0.228940 sec ++ Test: stream, permutation: 0x003907 * [1/14] capture => pcm_open * [2/14] capture => pcm_hw_params * [3/14] capture => pcm_prepare * [4/14] playback => pcm_open * [5/14] playback => pcm_hw_params * [6/14] playback => pcm_prepare * [7/14] playback => pcm_start * [8/14] playback => waitsleep * [9/14] capture => pcm_start * [10/14] playback => pcm_stop ++ Frames [playback]: 58560 * [11/14] playback => pcm_close capture_thread:209: error -32 on readi 0 ** IO Error (capture) ++ permutation: 0x003907, runtime: 0.142909 sec !! Test stream permutation 0x003907 failed !!
...and dmesg shows:
snd_usb_endpoint_start: cannot submit urb 0, error -16: device busy
After I removed the revert, no test fail.
Cheers, Eldad
At Mon, 7 Oct 2013 21:26:57 +0200 (CEST), Eldad Zack wrote:
On Mon, 7 Oct 2013, Takashi Iwai wrote:
At Sun, 6 Oct 2013 22:31:19 +0200, Eldad Zack wrote:
Start the endpoints at prepare also for capture endpoints, since it might be needed to wait for the URBs to be unlinked.
If an implicit feedback source endpoint stops being used by its sink endpoint, but immediately used as a data endpoint, usb_submit_urb will return -EBUSY.
Merge two trigger cases since they are now the same.
This change worries me about the timing. This change means that the capture stream isn't started at the moment the trigger callback is called but at the next urb handling. It means a possible regression in the case of realtime usage.
I'm not sure I understand. Do you mean it might cause the delay between capture and playback to vary at each startup?
No, I mean that the actual begin of the recording will be delayed in comparison with the driver without your patch. The delay can't be avoided in this style.
Is there any reason to do this except for clean up? IOW, does this fix any problem by itself?
Yes, I only became aware of it since I bumped into it with my test tool. Attached here - I hope the mailing list accepts attachments.
I understand that this will be required for the implicit feedback case. But why applying this to *all* other cases, too, although we know that this may regress slightly with respect to the latency? This is my biggest concern.
thanks,
Takashi
I reverted the patch right now (mainline rc4 + this series) and this is the exact sequence: $ ./atest --device hw:2 stream --first 0x0038f0 atest v1 Testing device hw:2, rate 96000, range 38f0:0
- Test stream, Steps: [pcm_open] [pcm_hw_params] [pcm_prepare] [pcm_start] [waitsleep] [pcm_stop] [pcm_close]
++ Test: stream, permutation: 0x0038f0
- [1/14] playback => pcm_open
- [2/14] playback => pcm_hw_params
- [3/14] playback => pcm_prepare
- [4/14] playback => pcm_start
- [5/14] capture => pcm_open
- [6/14] capture => pcm_hw_params
- [7/14] capture => pcm_prepare
- [8/14] capture => pcm_start
- [9/14] playback => waitsleep
- [10/14] playback => pcm_stop
++ Frames [playback]: 58560
- [11/14] playback => pcm_close
- [12/14] capture => waitsleep
- [13/14] capture => pcm_stop
++ Frames [capture]: 5760
- [14/14] capture => pcm_close
++ permutation: 0x0038f0, runtime: 0.228940 sec ++ Test: stream, permutation: 0x003907
- [1/14] capture => pcm_open
- [2/14] capture => pcm_hw_params
- [3/14] capture => pcm_prepare
- [4/14] playback => pcm_open
- [5/14] playback => pcm_hw_params
- [6/14] playback => pcm_prepare
- [7/14] playback => pcm_start
- [8/14] playback => waitsleep
- [9/14] capture => pcm_start
- [10/14] playback => pcm_stop
++ Frames [playback]: 58560
- [11/14] playback => pcm_close
capture_thread:209: error -32 on readi 0 ** IO Error (capture) ++ permutation: 0x003907, runtime: 0.142909 sec !! Test stream permutation 0x003907 failed !!
...and dmesg shows:
snd_usb_endpoint_start: cannot submit urb 0, error -16: device busy
After I removed the revert, no test fail.
Cheers, Eldad
On Tue, 8 Oct 2013, Takashi Iwai wrote:
At Mon, 7 Oct 2013 21:26:57 +0200 (CEST), Eldad Zack wrote:
On Mon, 7 Oct 2013, Takashi Iwai wrote:
At Sun, 6 Oct 2013 22:31:19 +0200, Eldad Zack wrote:
Start the endpoints at prepare also for capture endpoints, since it might be needed to wait for the URBs to be unlinked.
If an implicit feedback source endpoint stops being used by its sink endpoint, but immediately used as a data endpoint, usb_submit_urb will return -EBUSY.
Merge two trigger cases since they are now the same.
This change worries me about the timing. This change means that the capture stream isn't started at the moment the trigger callback is called but at the next urb handling. It means a possible regression in the case of realtime usage.
I'm not sure I understand. Do you mean it might cause the delay between capture and playback to vary at each startup?
No, I mean that the actual begin of the recording will be delayed in comparison with the driver without your patch. The delay can't be avoided in this style.
Ah. I see what you mean.
Is there any reason to do this except for clean up? IOW, does this fix any problem by itself?
Yes, I only became aware of it since I bumped into it with my test tool. Attached here - I hope the mailing list accepts attachments.
I understand that this will be required for the implicit feedback case. But why applying this to *all* other cases, too, although we know that this may regress slightly with respect to the latency? This is my biggest concern.
Yes, I understand now. I'll of course change that along with all the rest and post a new set. Thanks!
Cheers, Eldad
Add -EBUSY to the list of returned USB error strings. Change some debugging messages to KERN_DEBUG. Add the calling function for usb_submit_urb related errors.
Signed-off-by: Eldad Zack eldad@fogrefinery.com --- sound/usb/endpoint.c | 12 +++++++----- sound/usb/pcm.c | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 81056b6..ac4dedf 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -114,6 +114,8 @@ static const char *usb_error_string(int err) case -EFBIG: case -EMSGSIZE: return "internal error"; + case -EBUSY: + return "device busy"; default: return "unknown error"; } @@ -333,8 +335,8 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep)
err = usb_submit_urb(ctx->urb, GFP_ATOMIC); if (err < 0) - snd_printk(KERN_ERR "Unable to submit urb #%d: %d (urb %p)\n", - ctx->index, err, ctx->urb); + snd_printk(KERN_ERR "%s: cannot submit urb #%d: %d (urb %p)\n", + __func__, ctx->index, err, ctx->urb); else set_bit(ctx->index, &ep->active_mask); } @@ -387,7 +389,7 @@ static void snd_complete_urb(struct urb *urb) if (err == 0) return;
- snd_printk(KERN_ERR "cannot submit urb (err = %d)\n", err); + snd_printk(KERN_ERR "%s: cannot submit urb (err = %d)\n", __func__, err); //snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
exit_clear: @@ -948,8 +950,8 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
err = usb_submit_urb(urb, GFP_ATOMIC); if (err < 0) { - snd_printk(KERN_ERR "cannot submit urb %d, error %d: %s\n", - i, err, usb_error_string(err)); + snd_printk(KERN_ERR "%s: cannot submit urb %d, error %d: %s\n", + __func__, i, err, usb_error_string(err)); goto __error; } set_bit(i, &ep->active_mask); diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index f3d48d4..8897da7 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -606,7 +606,7 @@ static int match_endpoint_audioformats(struct audioformat *fp, if (fp->channels == match->channels) score++;
- snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score); + snd_printdd(KERN_DEBUG "%s: (fmt @%p) score %d\n", __func__, fp, score);
return score; } @@ -660,7 +660,7 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs) if (sync_fp->channels != subs->channels) { sync_period_bytes = (subs->period_bytes / subs->channels) * sync_fp->channels; - snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n", + snd_printdd(KERN_DEBUG "%s: adjusted sync ep period bytes (%d -> %d)\n", __func__, subs->period_bytes, sync_period_bytes); }
At Sun, 6 Oct 2013 22:31:20 +0200, Eldad Zack wrote:
Add -EBUSY to the list of returned USB error strings. Change some debugging messages to KERN_DEBUG.
snd_printdd() implies KERN_DEBUG as default, so it's superfluous.
Takashi
Add the calling function for usb_submit_urb related errors.
Signed-off-by: Eldad Zack eldad@fogrefinery.com
sound/usb/endpoint.c | 12 +++++++----- sound/usb/pcm.c | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 81056b6..ac4dedf 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -114,6 +114,8 @@ static const char *usb_error_string(int err) case -EFBIG: case -EMSGSIZE: return "internal error";
- case -EBUSY:
return "device busy";@@ -333,8 +335,8 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep)
err = usb_submit_urb(ctx->urb, GFP_ATOMIC); if (err < 0)
snd_printk(KERN_ERR "Unable to submit urb #%d: %d (urb %p)\n",ctx->index, err, ctx->urb);
snd_printk(KERN_ERR "%s: cannot submit urb #%d: %d (urb %p)\n",__func__, ctx->index, err, ctx->urb);@@ -387,7 +389,7 @@ static void snd_complete_urb(struct urb *urb) if (err == 0) return;
- snd_printk(KERN_ERR "cannot submit urb (err = %d)\n", err);
- snd_printk(KERN_ERR "%s: cannot submit urb (err = %d)\n", __func__, err); //snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
exit_clear: @@ -948,8 +950,8 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
err = usb_submit_urb(urb, GFP_ATOMIC); if (err < 0) {
snd_printk(KERN_ERR "cannot submit urb %d, error %d: %s\n",i, err, usb_error_string(err));
snd_printk(KERN_ERR "%s: cannot submit urb %d, error %d: %s\n",__func__, i, err, usb_error_string(err)); goto __error;diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index f3d48d4..8897da7 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -606,7 +606,7 @@ static int match_endpoint_audioformats(struct audioformat *fp, if (fp->channels == match->channels) score++;
- snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
snd_printdd(KERN_DEBUG "%s: (fmt @%p) score %d\n", __func__, fp, score);
return score;
} @@ -660,7 +660,7 @@ static int configure_sync_endpoint(struct snd_usb_substream *subs) if (sync_fp->channels != subs->channels) { sync_period_bytes = (subs->period_bytes / subs->channels) * sync_fp->channels;
snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
snd_printdd(KERN_DEBUG "%s: adjusted sync ep period bytes (%d -> %d)\n", __func__, subs->period_bytes, sync_period_bytes);-- 1.8.1.5
At Sun, 6 Oct 2013 22:31:05 +0200, Eldad Zack wrote:
Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices.
Now the following work properly on my system:
Full-duplex with jack :)
All tests I ran with my tool. Will post it in case anyone want to try it, it takes about 15 minutes to run all the tests.
Starting a capture-only jackd instance, e.g.: jackd -n 1 -d alsa -d hw:2,0 -C and then playback-only: jackd -n 2 -d alsa -d hw:2,0 -P and then stop and start the playback. The same with playback running and restarting capture.
Trying to start a second substream with parameter mismatch (rate or period bytes is all I can test with my device) will fail, without ill effects.
With this series applied there are now 3 usage tracking variables:
- substream usage flag
- endpoint param_set (to protect the parameters until the endpoint is started)
- the existing endpoint use_count
Changes from v3:
- Added substream "used" flag, to prevent failure in some combinations of ops. (#13)
- Moved start_endpoints for capture to prepare (#14)
- Added more constraints to the endpoint_may_set_params function (#10)
- Cleanups (#12, #15)
v3: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065744.html
Applies against for-next, tested on upstream 3.12-rc3 (needs Alan Stern's recent patch "improve buffer size computations for USB PCM audio" to apply cleanly on current mainline).
Daniel and Nikolay: if you can test again with this series I'd appreciate it.
Cheers, Eldad
Eldad Zack (15): ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: correct ep use_count semantics (add set_param flag) ALSA: usb-audio: rename alt_idx to altsetting ALSA: usb-audio: conditional interface altsetting ALSA: usb-audio: conditional concurrent usage of endpoint ALSA: usb-audio: remove altset_idx from snd_usb_substream ALSA: usb-audio: remove unused endpoint flag EP_FLAG_ACTIVATED ALSA: usb-audio: clear sync subs hw_params ALSA: usb-audio: always wait in start_endpoints ALSA: usb-audio: improve logging messages
For reducing the further development, I applied trivial cleanup / fix patches now, namely, below have been merged now: ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: rename alt_idx to altsetting ALSA: usb-audio: remove unused endpoint flag EP_FLAG_ACTIVATED
The patches below need trivial fixes: ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: improve logging messages
And these need reconsideration: ALSA: usb-audio: always wait in start_endpoints ALSA: usb-audio: correct ep use_count semantics (add set_param flag)
I'll check the rest later.
thanks,
Takashi
sound/usb/card.h | 9 ++- sound/usb/endpoint.c | 122 ++++++++++++++++++++--------- sound/usb/endpoint.h | 13 +++- sound/usb/pcm.c | 214 ++++++++++++++++++++++++++++++++++++--------------- sound/usb/pcm.h | 2 + sound/usb/proc.c | 4 +- 6 files changed, 258 insertions(+), 106 deletions(-)
-- 1.8.1.5
On Mon, 7 Oct 2013, Takashi Iwai wrote:
At Sun, 6 Oct 2013 22:31:05 +0200, Eldad Zack wrote:
Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices.
[snip]
For reducing the further development, I applied trivial cleanup / fix patches now, namely, below have been merged now:
Thanks, Takashi!
ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: rename alt_idx to altsetting ALSA: usb-audio: remove unused endpoint flag EP_FLAG_ACTIVATED
The patches below need trivial fixes: ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: improve logging messages
And these need reconsideration: ALSA: usb-audio: always wait in start_endpoints ALSA: usb-audio: correct ep use_count semantics (add set_param flag)
I'll check the rest later.
Thanks for the very fast response and the review! I'll try to fix all the issues for the rest and repost.
Cheers, Eldad
Hi.
I've tried this series applied against 3.11.6 in ubuntu with MS headset - seems to be working fine with Skype.
Thanks!
2013/10/6 Eldad Zack eldad@fogrefinery.com
Hi,
This patch series attempts to fix a long standing problem with the concurrent usage of playback and capture on implicit feedback devices.
Now the following work properly on my system:
Full-duplex with jack :)
All tests I ran with my tool. Will post it in case anyone want to try it, it takes about 15 minutes to run all the tests.
Starting a capture-only jackd instance, e.g.: jackd -n 1 -d alsa -d hw:2,0 -C and then playback-only: jackd -n 2 -d alsa -d hw:2,0 -P and then stop and start the playback. The same with playback running and restarting capture.
Trying to start a second substream with parameter mismatch (rate or period bytes is all I can test with my device) will fail, without ill effects.
With this series applied there are now 3 usage tracking variables:
- substream usage flag
- endpoint param_set (to protect the parameters until the endpoint is
started)
- the existing endpoint use_count
Changes from v3:
- Added substream "used" flag, to prevent failure in some combinations of
ops. (#13)
- Moved start_endpoints for capture to prepare (#14)
- Added more constraints to the endpoint_may_set_params function (#10)
- Cleanups (#12, #15)
v3:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-August/065744.html
Applies against for-next, tested on upstream 3.12-rc3 (needs Alan Stern's recent patch "improve buffer size computations for USB PCM audio" to apply cleanly on current mainline).
Daniel and Nikolay: if you can test again with this series I'd appreciate it.
Cheers, Eldad
Eldad Zack (15): ALSA: usb-audio: remove unused parameter from sync_ep_set_params ALSA: usb-audio: remove deactivate_endpoints() ALSA: usb-audio: prevent NULL dereference on stop trigger ALSA: usb-audio: don't deactivate URBs on in-use EP ALSA: usb-audio: void return type of snd_usb_endpoint_deactivate() ALSA: usb-audio: clear SUBSTREAM_FLAG_SYNC_EP_STARTED on error ALSA: usb-audio: correct ep use_count semantics (add set_param flag) ALSA: usb-audio: rename alt_idx to altsetting ALSA: usb-audio: conditional interface altsetting ALSA: usb-audio: conditional concurrent usage of endpoint ALSA: usb-audio: remove altset_idx from snd_usb_substream ALSA: usb-audio: remove unused endpoint flag EP_FLAG_ACTIVATED ALSA: usb-audio: clear sync subs hw_params ALSA: usb-audio: always wait in start_endpoints ALSA: usb-audio: improve logging messages
sound/usb/card.h | 9 ++- sound/usb/endpoint.c | 122 ++++++++++++++++++++--------- sound/usb/endpoint.h | 13 +++- sound/usb/pcm.c | 214 ++++++++++++++++++++++++++++++++++++--------------- sound/usb/pcm.h | 2 + sound/usb/proc.c | 4 +- 6 files changed, 258 insertions(+), 106 deletions(-)
-- 1.8.1.5
participants (4)
-
Clemens Ladisch -
Eldad Zack -
Nikolay Martynov -
Takashi Iwai