ALSA: usb-audio: Always apply the hw constraints for implicit fb sync

Since the commit 5a6c3e11c9c9 ("ALSA: usb-audio: Add hw constraint for
implicit fb sync"), we apply the hw constraints for the implicit
feedback sync to make the secondary open aligned with the already
opened stream setup.  This change assumed that the secondary open is
performed after the first stream has been already set up, and adds the
hw constraints to sync with the first stream's parameters only when
the EP setup for the first stream was confirmed at the open time.
However, most of applications handling the full-duplex operations do
open both playback and capture streams at first, then set up both
streams.  This results in skipping the additional hw constraints since
the counter-part stream hasn't been set up yet at the open of the
second stream, and it eventually leads to "incompatible EP" error in
the end.

This patch corrects the behavior by always applying the hw constraints
for the implicit fb sync.  The hw constraint rules are defined so that
they check the sync EP dynamically at each invocation, instead.  This
covers the concurrent stream setups better and lets the hw refine
calls resolving to the right configuration.

Also this patch corrects a minor error that has existed in the debug
print that isn't built as default.

Fixes: 5a6c3e11c9c9 ("ALSA: usb-audio: Add hw constraint for implicit fb sync")
Link: https://lore.kernel.org/r/20210111081611.12790-1-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>

1 files changed, 108 insertions, 63 deletions

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 56079901769f..f71965bf815f 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -663,7 +663,7 @@ static int hw_check_valid_format(struct snd_usb_substream *subs,
 	check_fmts.bits[1] = (u32)(fp->formats >> 32);
 	snd_mask_intersect(&check_fmts, fmts);
 	if (snd_mask_empty(&check_fmts)) {
-		hwc_debug("   > check: no supported format %d\n", fp->format);
+		hwc_debug("   > check: no supported format 0x%llx\n", fp->formats);
 		return 0;
 	}
 	/* check the channels */
@@ -775,24 +775,11 @@ static int hw_rule_channels(struct snd_pcm_hw_params *params,
 	return apply_hw_params_minmax(it, rmin, rmax);
 }
 
-static int hw_rule_format(struct snd_pcm_hw_params *params,
-			  struct snd_pcm_hw_rule *rule)
+static int apply_hw_params_format_bits(struct snd_mask *fmt, u64 fbits)
 {
-	struct snd_usb_substream *subs = rule->private;
-	const struct audioformat *fp;
-	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
-	u64 fbits;
 	u32 oldbits[2];
 	int changed;
 
-	hwc_debug("hw_rule_format: %x:%x\n", fmt->bits[0], fmt->bits[1]);
-	fbits = 0;
-	list_for_each_entry(fp, &subs->fmt_list, list) {
-		if (!hw_check_valid_format(subs, params, fp))
-			continue;
-		fbits |= fp->formats;
-	}
-
 	oldbits[0] = fmt->bits[0];
 	oldbits[1] = fmt->bits[1];
 	fmt->bits[0] &= (u32)fbits;
@@ -806,6 +793,24 @@ static int hw_rule_format(struct snd_pcm_hw_params *params,
 	return changed;
 }
 
+static int hw_rule_format(struct snd_pcm_hw_params *params,
+			  struct snd_pcm_hw_rule *rule)
+{
+	struct snd_usb_substream *subs = rule->private;
+	const struct audioformat *fp;
+	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
+	u64 fbits;
+
+	hwc_debug("hw_rule_format: %x:%x\n", fmt->bits[0], fmt->bits[1]);
+	fbits = 0;
+	list_for_each_entry(fp, &subs->fmt_list, list) {
+		if (!hw_check_valid_format(subs, params, fp))
+			continue;
+		fbits |= fp->formats;
+	}
+	return apply_hw_params_format_bits(fmt, fbits);
+}
+
 static int hw_rule_period_time(struct snd_pcm_hw_params *params,
 			       struct snd_pcm_hw_rule *rule)
 {
@@ -833,64 +838,92 @@ static int hw_rule_period_time(struct snd_pcm_hw_params *params,
 	return apply_hw_params_minmax(it, pmin, UINT_MAX);
 }
 
-/* apply PCM hw constraints from the concurrent sync EP */
-static int apply_hw_constraint_from_sync(struct snd_pcm_runtime *runtime,
-					 struct snd_usb_substream *subs)
+/* get the EP or the sync EP for implicit fb when it's already set up */
+static const struct snd_usb_endpoint *
+get_sync_ep_from_substream(struct snd_usb_substream *subs)
 {
 	struct snd_usb_audio *chip = subs->stream->chip;
-	struct snd_usb_endpoint *ep;
 	const struct audioformat *fp;
-	int err;
+	const struct snd_usb_endpoint *ep;
 
 	list_for_each_entry(fp, &subs->fmt_list, list) {
 		ep = snd_usb_get_endpoint(chip, fp->endpoint);
 		if (ep && ep->cur_rate)
-			goto found;
+			return ep;
 		if (!fp->implicit_fb)
 			continue;
 		/* for the implicit fb, check the sync ep as well */
 		ep = snd_usb_get_endpoint(chip, fp->sync_ep);
 		if (ep && ep->cur_rate)
-			goto found;
+			return ep;
 	}
-	return 0;
+	return NULL;
+}
+
+/* additional hw constraints for implicit feedback mode */
+static int hw_rule_format_implicit_fb(struct snd_pcm_hw_params *params,
+				      struct snd_pcm_hw_rule *rule)
+{
+	struct snd_usb_substream *subs = rule->private;
+	const struct snd_usb_endpoint *ep;
+	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
 
- found:
-	if (!find_format(&subs->fmt_list, ep->cur_format, ep->cur_rate,
-			 ep->cur_channels, false, NULL)) {
-		usb_audio_dbg(chip, "EP 0x%x being used, but not applicable\n",
-			      ep->ep_num);
+	ep = get_sync_ep_from_substream(subs);
+	if (!ep)
 		return 0;
-	}
 
-	usb_audio_dbg(chip, "EP 0x%x being used, using fixed params:\n",
-		      ep->ep_num);
-	usb_audio_dbg(chip, "rate=%d, period_size=%d, periods=%d\n",
-		      ep->cur_rate, ep->cur_period_frames,
-		      ep->cur_buffer_periods);
+	hwc_debug("applying %s\n", __func__);
+	return apply_hw_params_format_bits(fmt, pcm_format_to_bits(ep->cur_format));
+}
 
-	runtime->hw.formats = subs->formats;
-	runtime->hw.rate_min = runtime->hw.rate_max = ep->cur_rate;
-	runtime->hw.rates = SNDRV_PCM_RATE_KNOT;
-	runtime->hw.periods_min = runtime->hw.periods_max =
-		ep->cur_buffer_periods;
+static int hw_rule_rate_implicit_fb(struct snd_pcm_hw_params *params,
+				    struct snd_pcm_hw_rule *rule)
+{
+	struct snd_usb_substream *subs = rule->private;
+	const struct snd_usb_endpoint *ep;
+	struct snd_interval *it;
 
-	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
-				  hw_rule_channels, subs,
-				  SNDRV_PCM_HW_PARAM_FORMAT,
-				  SNDRV_PCM_HW_PARAM_RATE,
-				  -1);
-	if (err < 0)
-		return err;
+	ep = get_sync_ep_from_substream(subs);
+	if (!ep)
+		return 0;
 
-	err = snd_pcm_hw_constraint_minmax(runtime,
-					   SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
-					   ep->cur_period_frames,
-					   ep->cur_period_frames);
-	if (err < 0)
-		return err;
+	hwc_debug("applying %s\n", __func__);
+	it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
+	return apply_hw_params_minmax(it, ep->cur_rate, ep->cur_rate);
+}
+
+static int hw_rule_period_size_implicit_fb(struct snd_pcm_hw_params *params,
+					   struct snd_pcm_hw_rule *rule)
+{
+	struct snd_usb_substream *subs = rule->private;
+	const struct snd_usb_endpoint *ep;
+	struct snd_interval *it;
 
-	return 1; /* notify the finding */
+	ep = get_sync_ep_from_substream(subs);
+	if (!ep)
+		return 0;
+
+	hwc_debug("applying %s\n", __func__);
+	it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE);
+	return apply_hw_params_minmax(it, ep->cur_period_frames,
+				      ep->cur_period_frames);
+}
+
+static int hw_rule_periods_implicit_fb(struct snd_pcm_hw_params *params,
+				       struct snd_pcm_hw_rule *rule)
+{
+	struct snd_usb_substream *subs = rule->private;
+	const struct snd_usb_endpoint *ep;
+	struct snd_interval *it;
+
+	ep = get_sync_ep_from_substream(subs);
+	if (!ep)
+		return 0;
+
+	hwc_debug("applying %s\n", __func__);
+	it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_PERIODS);
+	return apply_hw_params_minmax(it, ep->cur_buffer_periods,
+				      ep->cur_buffer_periods);
 }
 
 /*
@@ -899,20 +932,11 @@ static int apply_hw_constraint_from_sync(struct snd_pcm_runtime *runtime,
 
 static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substream *subs)
 {
-	struct snd_usb_audio *chip = subs->stream->chip;
 	const struct audioformat *fp;
 	unsigned int pt, ptmin;
 	int param_period_time_if_needed = -1;
 	int err;
 
-	mutex_lock(&chip->mutex);
-	err = apply_hw_constraint_from_sync(runtime, subs);
-	mutex_unlock(&chip->mutex);
-	if (err < 0)
-		return err;
-	if (err > 0) /* found the matching? */
-		goto add_extra_rules;
-
 	runtime->hw.formats = subs->formats;
 
 	runtime->hw.rate_min = 0x7fffffff;
@@ -964,7 +988,6 @@ static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substre
 	if (err < 0)
 		return err;
 
-add_extra_rules:
 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
 				  hw_rule_channels, subs,
 				  SNDRV_PCM_HW_PARAM_FORMAT,
@@ -993,6 +1016,28 @@ add_extra_rules:
 			return err;
 	}
 
+	/* additional hw constraints for implicit fb */
+	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,
+				  hw_rule_format_implicit_fb, subs,
+				  SNDRV_PCM_HW_PARAM_FORMAT, -1);
+	if (err < 0)
+		return err;
+	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
+				  hw_rule_rate_implicit_fb, subs,
+				  SNDRV_PCM_HW_PARAM_RATE, -1);
+	if (err < 0)
+		return err;
+	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
+				  hw_rule_period_size_implicit_fb, subs,
+				  SNDRV_PCM_HW_PARAM_PERIOD_SIZE, -1);
+	if (err < 0)
+		return err;
+	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_PERIODS,
+				  hw_rule_periods_implicit_fb, subs,
+				  SNDRV_PCM_HW_PARAM_PERIODS, -1);
+	if (err < 0)
+		return err;
+
 	return 0;
 }