[PATCH 0/3] ALSA: usb-audio: Fix a few implicit fb sync issues

[PATCH 0/3] ALSA: usb-audio: Fix a few implicit fb sync issues

[Takashi Iwai]

Hi,

this is a series of patches for addressing the remaining problem with
the implicit fb sync.  The first one is to cover the left over EP run
state at re-configuring PCM.  The second one is a workaround for a
regression with the hw constraints, and the last one is the
refactoring and fix for the multiple EPs assigned in the format list.


Takashi

===

Takashi Iwai (3):
  ALSA: usb-audio: Make sure to stop endpoints before closing EPs
  ALSA: usb-audio: Relax hw constraints for implicit fb sync
  ALSA: usb-audio: More refactoring of hw constraint rules

 sound/usb/pcm.c | 217 ++++++++++++++++++++++++++++++------------------
 1 file changed, 134 insertions(+), 83 deletions(-)

-- 
2.35.3

[PATCH 1/3] ALSA: usb-audio: Make sure to stop endpoints before closing EPs

[Takashi Iwai]

At the PCM hw params, we may re-configure the endpoints and it's done
by a temporary EP close followed by re-open.  A potential problem
there is that the EP might be already running internally at the PCM
prepare stage; it's seen typically in the playback stream with the
implicit feedback sync.  As this stream start isn't tracked by the
core PCM layer, we'd need to stop it explicitly, and that's the
missing piece.

This patch adds the stop_endpoints() call at snd_usb_hw_params() to
assure the stream stop before closing the EPs.

Fixes: bf6313a0ff76 ("ALSA: usb-audio: Refactor endpoint management")
Link: https://lore.kernel.org/r/4e509aea-e563-e592-e652-ba44af6733fe@veniogames.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/pcm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 99a66d0ef5b2..7fc95ae9b2f0 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -525,6 +525,8 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 		if (snd_usb_endpoint_compatible(chip, subs->data_endpoint,
 						fmt, hw_params))
 			goto unlock;
+		if (stop_endpoints(subs, false))
+			sync_pending_stops(subs);
 		close_endpoints(chip, subs);
 	}
 
-- 
2.35.3

[PATCH 2/3] ALSA: usb-audio: Relax hw constraints for implicit fb sync

[Takashi Iwai]

The fix commit the commit e4ea77f8e53f ("ALSA: usb-audio: Always apply
the hw constraints for implicit fb sync") tried to address the bug
where an incorrect PCM parameter is chosen when two (implicit fb)
streams are set up at the same time.  This change had, however, some
side effect: once when the sync endpoint is chosen and set up, this
restriction is applied at the next hw params unless it's freed via hw
free explicitly.

This patch is a workaround for the problem by relaxing the hw
constraints a bit for the implicit fb sync.  We still keep applying
the hw constraints for implicit fb sync, but only when the matching
sync EP is being used by other streams.

Fixes: e4ea77f8e53f ("ALSA: usb-audio: Always apply the hw constraints for implicit fb sync")
Link: https://lore.kernel.org/r/4e509aea-e563-e592-e652-ba44af6733fe@veniogames.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/pcm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 7fc95ae9b2f0..2fd4ecc1b25a 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -937,8 +937,13 @@ get_sync_ep_from_substream(struct snd_usb_substream *subs)
 			continue;
 		/* for the implicit fb, check the sync ep as well */
 		ep = snd_usb_get_endpoint(chip, fp->sync_ep);
-		if (ep && ep->cur_audiofmt)
-			return ep;
+		if (ep && ep->cur_audiofmt) {
+			/* ditto, if the sync (data) ep is used by others,
+			 * this stream is restricted by the sync ep
+			 */
+			if (ep != subs->sync_endpoint || ep->opened > 1)
+				return ep;
+		}
 	}
 	return NULL;
 }
-- 
2.35.3

[PATCH 3/3] ALSA: usb-audio: More refactoring of hw constraint rules

[Takashi Iwai]

Although we applied a workaround for the hw constraints code with the
implicit feedback sync, it still has a potential problem.  Namely, as
the code treats only the first matching (sync) endpoint, it might be
too restrictive when multiple endpoints are listed in the substream's
format list.

This patch is another attempt to improve the hw constraint handling
for the implicit feedback sync.  The code is rewritten and the sync EP
handling for the rate and the format is put inside the fmt_list loop
in each hw_rule_*() function instead of the additional rules.  The
rules for the period size and periods are extended to loop over the
fmt_list like others, and they apply the constraints only if needed.

Link: https://lore.kernel.org/r/4e509aea-e563-e592-e652-ba44af6733fe@veniogames.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/pcm.c | 218 +++++++++++++++++++++++++++++-------------------
 1 file changed, 131 insertions(+), 87 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 2fd4ecc1b25a..fbd4798834e5 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -789,11 +789,27 @@ static int apply_hw_params_minmax(struct snd_interval *it, unsigned int rmin,
 	return changed;
 }
 
+/* get the specified endpoint object that is being used by other streams
+ * (i.e. the parameter is locked)
+ */
+static const struct snd_usb_endpoint *
+get_endpoint_in_use(struct snd_usb_audio *chip, int endpoint,
+		    const struct snd_usb_endpoint *ref_ep)
+{
+	const struct snd_usb_endpoint *ep;
+
+	ep = snd_usb_get_endpoint(chip, endpoint);
+	if (ep && ep->cur_audiofmt && (ep != ref_ep || ep->opened > 1))
+		return ep;
+	return NULL;
+}
+
 static int hw_rule_rate(struct snd_pcm_hw_params *params,
 			struct snd_pcm_hw_rule *rule)
 {
 	struct snd_usb_substream *subs = rule->private;
 	struct snd_usb_audio *chip = subs->stream->chip;
+	const struct snd_usb_endpoint *ep;
 	const struct audioformat *fp;
 	struct snd_interval *it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
 	unsigned int rmin, rmax, r;
@@ -805,6 +821,29 @@ static int hw_rule_rate(struct snd_pcm_hw_params *params,
 	list_for_each_entry(fp, &subs->fmt_list, list) {
 		if (!hw_check_valid_format(subs, params, fp))
 			continue;
+
+		ep = get_endpoint_in_use(chip, fp->endpoint,
+					 subs->data_endpoint);
+		if (ep) {
+			hwc_debug("rate limit %d for ep#%x\n",
+				  ep->cur_rate, fp->endpoint);
+			rmin = min(rmin, ep->cur_rate);
+			rmax = max(rmax, ep->cur_rate);
+			continue;
+		}
+
+		if (fp->implicit_fb) {
+			ep = get_endpoint_in_use(chip, fp->sync_ep,
+						 subs->sync_endpoint);
+			if (ep) {
+				hwc_debug("rate limit %d for sync_ep#%x\n",
+					  ep->cur_rate, fp->sync_ep);
+				rmin = min(rmin, ep->cur_rate);
+				rmax = max(rmax, ep->cur_rate);
+				continue;
+			}
+		}
+
 		r = snd_usb_endpoint_get_clock_rate(chip, fp->clock);
 		if (r > 0) {
 			if (!snd_interval_test(it, r))
@@ -874,6 +913,8 @@ static int hw_rule_format(struct snd_pcm_hw_params *params,
 			  struct snd_pcm_hw_rule *rule)
 {
 	struct snd_usb_substream *subs = rule->private;
+	struct snd_usb_audio *chip = subs->stream->chip;
+	const struct snd_usb_endpoint *ep;
 	const struct audioformat *fp;
 	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
 	u64 fbits;
@@ -883,6 +924,27 @@ static int hw_rule_format(struct snd_pcm_hw_params *params,
 	list_for_each_entry(fp, &subs->fmt_list, list) {
 		if (!hw_check_valid_format(subs, params, fp))
 			continue;
+
+		ep = get_endpoint_in_use(chip, fp->endpoint,
+					 subs->data_endpoint);
+		if (ep) {
+			hwc_debug("format limit %d for ep#%x\n",
+				  ep->cur_format, fp->endpoint);
+			fbits |= pcm_format_to_bits(ep->cur_format);
+			continue;
+		}
+
+		if (fp->implicit_fb) {
+			ep = get_endpoint_in_use(chip, fp->sync_ep,
+						 subs->sync_endpoint);
+			if (ep) {
+				hwc_debug("format limit %d for sync_ep#%x\n",
+					  ep->cur_format, fp->sync_ep);
+				fbits |= pcm_format_to_bits(ep->cur_format);
+				continue;
+			}
+		}
+
 		fbits |= fp->formats;
 	}
 	return apply_hw_params_format_bits(fmt, fbits);
@@ -915,103 +977,95 @@ static int hw_rule_period_time(struct snd_pcm_hw_params *params,
 	return apply_hw_params_minmax(it, pmin, UINT_MAX);
 }
 
-/* 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)
+/* additional hw constraints for implicit feedback mode */
+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;
 	struct snd_usb_audio *chip = subs->stream->chip;
 	const struct audioformat *fp;
 	const struct snd_usb_endpoint *ep;
+	struct snd_interval *it;
+	unsigned int rmin, rmax;
 
+	it = hw_param_interval(params, SNDRV_PCM_HW_PARAM_PERIOD_SIZE);
+	hwc_debug("hw_rule_period_size: (%u,%u)\n", it->min, it->max);
+	rmin = UINT_MAX;
+	rmax = 0;
 	list_for_each_entry(fp, &subs->fmt_list, list) {
-		ep = snd_usb_get_endpoint(chip, fp->endpoint);
-		if (ep && ep->cur_audiofmt) {
-			/* if EP is already opened solely for this substream,
-			 * we still allow us to change the parameter; otherwise
-			 * this substream has to follow the existing parameter
-			 */
-			if (ep->cur_audiofmt != subs->cur_audiofmt || ep->opened > 1)
-				return ep;
-		}
-		if (!fp->implicit_fb)
+		if (!hw_check_valid_format(subs, params, fp))
+			continue;
+		ep = get_endpoint_in_use(chip, fp->endpoint,
+					 subs->data_endpoint);
+		if (ep) {
+			hwc_debug("period size limit %d for ep#%x\n",
+				  ep->cur_period_frames, fp->endpoint);
+			rmin = min(rmin, ep->cur_period_frames);
+			rmax = max(rmax, ep->cur_period_frames);
 			continue;
-		/* for the implicit fb, check the sync ep as well */
-		ep = snd_usb_get_endpoint(chip, fp->sync_ep);
-		if (ep && ep->cur_audiofmt) {
-			/* ditto, if the sync (data) ep is used by others,
-			 * this stream is restricted by the sync ep
-			 */
-			if (ep != subs->sync_endpoint || ep->opened > 1)
-				return ep;
 		}
-	}
-	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);
-
-	ep = get_sync_ep_from_substream(subs);
-	if (!ep)
-		return 0;
-
-	hwc_debug("applying %s\n", __func__);
-	return apply_hw_params_format_bits(fmt, pcm_format_to_bits(ep->cur_format));
-}
-
-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;
-
-	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_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;
-
-	ep = get_sync_ep_from_substream(subs);
-	if (!ep)
-		return 0;
+		if (fp->implicit_fb) {
+			ep = get_endpoint_in_use(chip, fp->sync_ep,
+						 subs->sync_endpoint);
+			if (ep) {
+				hwc_debug("period size limit %d for sync_ep#%x\n",
+					  ep->cur_period_frames, fp->sync_ep);
+				rmin = min(rmin, ep->cur_period_frames);
+				rmax = max(rmax, ep->cur_period_frames);
+				continue;
+			}
+		}
+	}
 
-	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);
+	if (!rmax)
+		return 0; /* no limit by implicit fb */
+	return apply_hw_params_minmax(it, rmin, rmax);
 }
 
 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;
+	struct snd_usb_audio *chip = subs->stream->chip;
+	const struct audioformat *fp;
 	const struct snd_usb_endpoint *ep;
 	struct snd_interval *it;
+	unsigned int rmin, rmax;
 
-	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);
+	hwc_debug("hw_rule_periods: (%u,%u)\n", it->min, it->max);
+	rmin = UINT_MAX;
+	rmax = 0;
+	list_for_each_entry(fp, &subs->fmt_list, list) {
+		if (!hw_check_valid_format(subs, params, fp))
+			continue;
+		ep = get_endpoint_in_use(chip, fp->endpoint,
+					 subs->data_endpoint);
+		if (ep) {
+			hwc_debug("periods limit %d for ep#%x\n",
+				  ep->cur_buffer_periods, fp->endpoint);
+			rmin = min(rmin, ep->cur_buffer_periods);
+			rmax = max(rmax, ep->cur_buffer_periods);
+			continue;
+		}
+
+		if (fp->implicit_fb) {
+			ep = get_endpoint_in_use(chip, fp->sync_ep,
+						 subs->sync_endpoint);
+			if (ep) {
+				hwc_debug("periods limit %d for sync_ep#%x\n",
+					  ep->cur_buffer_periods, fp->sync_ep);
+				rmin = min(rmin, ep->cur_buffer_periods);
+				rmax = max(rmax, ep->cur_buffer_periods);
+				continue;
+			}
+		}
+	}
+
+	if (!rmax)
+		return 0; /* no limit by implicit fb */
+	return apply_hw_params_minmax(it, rmin, rmax);
 }
 
 /*
@@ -1120,16 +1174,6 @@ static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substre
 		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);
-- 
2.35.3

Re: [PATCH 0/3] ALSA: usb-audio: Fix a few implicit fb sync issues

[Geraldo Nascimento]

On Mon, Jan 02, 2023 at 06:07:56PM +0100, Takashi Iwai wrote:
> Hi,

Hi Takashi,

> 
> this is a series of patches for addressing the remaining problem with
> the implicit fb sync.  The first one is to cover the left over EP run
> state at re-configuring PCM.  The second one is a workaround for a
> regression with the hw constraints, and the last one is the
> refactoring and fix for the multiple EPs assigned in the format list.
>

Perhaps it would be wise to include tag
"Reported-by: Ruud van Asseldonk <ruud@veniogames.com>"
in the relevants commits tied to this series, as Ruud did share a small
reproducing C program and went so far as to bisect the regression.

Thanks,
Geraldo Nascimento

> 
> Takashi
> 
> ===
> 
> Takashi Iwai (3):
>   ALSA: usb-audio: Make sure to stop endpoints before closing EPs
>   ALSA: usb-audio: Relax hw constraints for implicit fb sync
>   ALSA: usb-audio: More refactoring of hw constraint rules
> 
>  sound/usb/pcm.c | 217 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 134 insertions(+), 83 deletions(-)
> 
> -- 
> 2.35.3
> 

Re: [PATCH 0/3] ALSA: usb-audio: Fix a few implicit fb sync issues

[Takashi Iwai]

On Mon, 02 Jan 2023 22:15:25 +0100,
Geraldo Nascimento wrote:
> 
> On Mon, Jan 02, 2023 at 06:07:56PM +0100, Takashi Iwai wrote:
> > Hi,
> 
> Hi Takashi,
> 
> > 
> > this is a series of patches for addressing the remaining problem with
> > the implicit fb sync.  The first one is to cover the left over EP run
> > state at re-configuring PCM.  The second one is a workaround for a
> > regression with the hw constraints, and the last one is the
> > refactoring and fix for the multiple EPs assigned in the format list.
> >
> 
> Perhaps it would be wise to include tag
> "Reported-by: Ruud van Asseldonk <ruud@veniogames.com>"
> in the relevants commits tied to this series, as Ruud did share a small
> reproducing C program and went so far as to bisect the regression.

Yes, it makes sense.


Takashi