All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] Add duplex sound support for USB devices using implicit feedback
@ 2020-05-09 18:10 Erwin Burema
  2020-05-10 17:31 ` Takashi Iwai
  0 siblings, 1 reply; 3+ messages in thread
From: Erwin Burema @ 2020-05-09 18:10 UTC (permalink / raw)
  To: alsa-devel

Hi,

Since I had problems getting duplex audio to work on my motu m2 I created the following patch to get duplex audio to work. This is based on my understanding that when implicit feedback is used the size of the next output is determined by the previous input, for this the content of the input is ignored but in case of duplex audio the content can actually contain audio information, so long as both input and output are configured the same both sould be usable.

With the above in mind an endpoint can be opened/configured a second time so long as it is an implicit sync endpoint and the config does not change. So that is what this patch does, the check if the config does not change might be a bit more complex then needed, but it seems to work on my motu m2.

Fixes bug 207023 and should fix 103751 as well

Kind Regards,

Erwin Burema

---
 sound/usb/card.h     |   1 +
 sound/usb/endpoint.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
 sound/usb/pcm.c      |   5 ++
 3 files changed, 197 insertions(+), 4 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 820e564656ed..d6219fba9699 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -108,6 +108,7 @@ struct snd_usb_endpoint {
 	int iface, altsetting;
 	int skip_packets;		/* quirks for devices to ignore the first n packets
 					   in a stream */
+	bool is_implicit_feedback;      /* This endpoint is used as implicit feedback */
 
 	spinlock_t lock;
 	struct list_head list;
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 50104f658ed4..9bea7d3f99f8 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -522,6 +522,8 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
 
 	list_add_tail(&ep->list, &chip->ep_list);
 
+	ep->is_implicit_feedback = 0;
+
 __exit_unlock:
 	mutex_unlock(&chip->mutex);
 
@@ -621,6 +623,178 @@ static void release_urbs(struct snd_usb_endpoint *ep, int force)
 	ep->nurbs = 0;
 }
 
+/*
+ * Check data endpoint for format differences
+ */
+static bool check_ep_params(struct snd_usb_endpoint *ep,
+			      snd_pcm_format_t pcm_format,
+			      unsigned int channels,
+			      unsigned int period_bytes,
+			      unsigned int frames_per_period,
+			      unsigned int periods_per_buffer,
+			      struct audioformat *fmt,
+			      struct snd_usb_endpoint *sync_ep)
+{
+	unsigned int maxsize, minsize, packs_per_ms, max_packs_per_urb;
+	unsigned int max_packs_per_period, urbs_per_period, urb_packs;
+	unsigned int max_urbs;
+	int frame_bits = snd_pcm_format_physical_width(pcm_format) * channels;
+	int tx_length_quirk = (ep->chip->tx_length_quirk &&
+			       usb_pipeout(ep->pipe));
+	bool ret = 1;
+
+	if (pcm_format == SNDRV_PCM_FORMAT_DSD_U16_LE && fmt->dsd_dop) {
+		/*
+		 * When operating in DSD DOP mode, the size of a sample frame
+		 * in hardware differs from the actual physical format width
+		 * because we need to make room for the DOP markers.
+		 */
+		frame_bits += channels << 3;
+	}
+
+	ret = ret && (ep->datainterval == fmt->datainterval);
+	ret = ret && (ep->stride == frame_bits >> 3);
+
+	switch (pcm_format) {
+	case SNDRV_PCM_FORMAT_U8:
+		ret = ret && (ep->silence_value == 0x80);
+		break;
+	case SNDRV_PCM_FORMAT_DSD_U8:
+	case SNDRV_PCM_FORMAT_DSD_U16_LE:
+	case SNDRV_PCM_FORMAT_DSD_U32_LE:
+	case SNDRV_PCM_FORMAT_DSD_U16_BE:
+	case SNDRV_PCM_FORMAT_DSD_U32_BE:
+		ret = ret && (ep->silence_value == 0x69);
+		break;
+	default:
+		ret = ret && (ep->silence_value == 0);
+	}
+
+	/* assume max. frequency is 50% higher than nominal */
+	ret = ret && (ep->freqmax == ep->freqn + (ep->freqn >> 1));
+	/* Round up freqmax to nearest integer in order to calculate maximum
+	 * packet size, which must represent a whole number of frames.
+	 * This is accomplished by adding 0x0.ffff before converting the
+	 * Q16.16 format into integer.
+	 * In order to accurately calculate the maximum packet size when
+	 * the data interval is more than 1 (i.e. ep->datainterval > 0),
+	 * multiply by the data interval prior to rounding. For instance,
+	 * a freqmax of 41 kHz will result in a max packet size of 6 (5.125)
+	 * frames with a data interval of 1, but 11 (10.25) frames with a
+	 * data interval of 2.
+	 * (ep->freqmax << ep->datainterval overflows at 8.192 MHz for the
+	 * maximum datainterval value of 3, at USB full speed, higher for
+	 * USB high speed, noting that ep->freqmax is in units of
+	 * frames per packet in Q16.16 format.)
+	 */
+	maxsize = (((ep->freqmax << ep->datainterval) + 0xffff) >> 16) *
+			 (frame_bits >> 3);
+	if (tx_length_quirk)
+		maxsize += sizeof(__le32); /* Space for length descriptor */
+	/* but wMaxPacketSize might reduce this */
+	if (ep->maxpacksize && ep->maxpacksize < maxsize) {
+		/* whatever fits into a max. size packet */
+		unsigned int data_maxsize = maxsize = ep->maxpacksize;
+
+		if (tx_length_quirk)
+			/* Need to remove the length descriptor to calc freq */
+			data_maxsize -= sizeof(__le32);
+		ret = ret && (ep->freqmax == (data_maxsize / (frame_bits >> 3))
+				<< (16 - ep->datainterval));
+	}
+
+	if (ep->fill_max)
+		ret = ret && (ep->curpacksize == ep->maxpacksize);
+	else
+		ret = ret && (ep->curpacksize == maxsize);
+
+	if (snd_usb_get_speed(ep->chip->dev) != USB_SPEED_FULL) {
+		packs_per_ms = 8 >> ep->datainterval;
+		max_packs_per_urb = MAX_PACKS_HS;
+	} else {
+		packs_per_ms = 1;
+		max_packs_per_urb = MAX_PACKS;
+	}
+	if (sync_ep && !snd_usb_endpoint_implicit_feedback_sink(ep))
+		max_packs_per_urb = min(max_packs_per_urb,
+					1U << sync_ep->syncinterval);
+	max_packs_per_urb = max(1u, max_packs_per_urb >> ep->datainterval);
+
+	/*
+	 * Capture endpoints need to use small URBs because there's no way
+	 * to tell in advance where the next period will end, and we don't
+	 * want the next URB to complete much after the period ends.
+	 *
+	 * Playback endpoints with implicit sync much use the same parameters
+	 * as their corresponding capture endpoint.
+	 */
+	if (usb_pipein(ep->pipe) ||
+			snd_usb_endpoint_implicit_feedback_sink(ep)) {
+
+		urb_packs = packs_per_ms;
+		/*
+		 * Wireless devices can poll at a max rate of once per 4ms.
+		 * For dataintervals less than 5, increase the packet count to
+		 * allow the host controller to use bursting to fill in the
+		 * gaps.
+		 */
+		if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_WIRELESS) {
+			int interval = ep->datainterval;
+
+			while (interval < 5) {
+				urb_packs <<= 1;
+				++interval;
+			}
+		}
+		/* make capture URBs <= 1 ms and smaller than a period */
+		urb_packs = min(max_packs_per_urb, urb_packs);
+		while (urb_packs > 1 && urb_packs * maxsize >= period_bytes)
+			urb_packs >>= 1;
+		ret = ret && (ep->nurbs == MAX_URBS);
+
+	/*
+	 * Playback endpoints without implicit sync are adjusted so that
+	 * a period fits as evenly as possible in the smallest number of
+	 * URBs.  The total number of URBs is adjusted to the size of the
+	 * ALSA buffer, subject to the MAX_URBS and MAX_QUEUE limits.
+	 */
+	} else {
+		/* determine how small a packet can be */
+		minsize = (ep->freqn >> (16 - ep->datainterval)) *
+				(frame_bits >> 3);
+		/* with sync from device, assume it can be 12% lower */
+		if (sync_ep)
+			minsize -= minsize >> 3;
+		minsize = max(minsize, 1u);
+
+		/* how many packets will contain an entire ALSA period? */
+		max_packs_per_period = DIV_ROUND_UP(period_bytes, minsize);
+
+		/* how many URBs will contain a period? */
+		urbs_per_period = DIV_ROUND_UP(max_packs_per_period,
+				max_packs_per_urb);
+		/* how many packets are needed in each URB? */
+		urb_packs = DIV_ROUND_UP(max_packs_per_period, urbs_per_period);
+
+		/* limit the number of frames in a single URB */
+		ret = ret && (ep->max_urb_frames ==
+			DIV_ROUND_UP(frames_per_period, urbs_per_period));
+
+		/* try to use enough URBs to contain an entire ALSA buffer */
+		max_urbs = min((unsigned) MAX_URBS,
+				MAX_QUEUE * packs_per_ms / urb_packs);
+		ret = ret && (ep->nurbs == min(max_urbs,
+				urbs_per_period * periods_per_buffer));
+	}
+
+	ret = ret && (ep->datainterval == fmt->datainterval);
+	ret = ret && (ep->maxpacksize == fmt->maxpacksize);
+	ret = ret &&
+		(ep->fill_max == !!(fmt->attributes & UAC_EP_CS_ATTR_FILL_MAX));
+
+	return ret;
+}
+
 /*
  * configure a data endpoint
  */
@@ -886,10 +1060,23 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 	int err;
 
 	if (ep->use_count != 0) {
-		usb_audio_warn(ep->chip,
-			 "Unable to change format on ep #%x: already in use\n",
-			 ep->ep_num);
-		return -EBUSY;
+		bool check = ep->is_implicit_feedback &&
+			check_ep_params(ep, pcm_format,
+					     channels, period_bytes,
+					     period_frames, buffer_periods,
+					     fmt, sync_ep);
+
+		if (!check) {
+			usb_audio_warn(ep->chip,
+				"Unable to change format on ep #%x: already in use\n",
+				ep->ep_num);
+			return -EBUSY;
+		}
+
+		usb_audio_dbg(ep->chip,
+			      "Ep #%x already in use as implicit feedback but format not changed\n",
+			      ep->ep_num);
+		return 0;
 	}
 
 	/* release old buffers, if any */
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b50965ab3b3a..d61c2f1095b5 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -404,6 +404,8 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 	if (!subs->sync_endpoint)
 		return -EINVAL;
 
+	subs->sync_endpoint->is_implicit_feedback = 1;
+
 	subs->data_endpoint->sync_master = subs->sync_endpoint;
 
 	return 1;
@@ -502,12 +504,15 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
 						   implicit_fb ?
 							SND_USB_ENDPOINT_TYPE_DATA :
 							SND_USB_ENDPOINT_TYPE_SYNC);
+
 	if (!subs->sync_endpoint) {
 		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
 			return 0;
 		return -EINVAL;
 	}
 
+	subs->sync_endpoint->is_implicit_feedback = implicit_fb;
+
 	subs->data_endpoint->sync_master = subs->sync_endpoint;
 
 	return 0;
-- 
2.26.2





^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [RFC] Add duplex sound support for USB devices using implicit feedback
  2020-05-09 18:10 [RFC] Add duplex sound support for USB devices using implicit feedback Erwin Burema
@ 2020-05-10 17:31 ` Takashi Iwai
  2020-05-10 18:30   ` Erwin Burema
  0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2020-05-10 17:31 UTC (permalink / raw)
  To: Erwin Burema; +Cc: alsa-devel

On Sat, 09 May 2020 20:10:47 +0200,
Erwin Burema wrote:
> 
> Hi,
> 
> Since I had problems getting duplex audio to work on my motu m2 I created the following patch to get duplex audio to work. This is based on my understanding that when implicit feedback is used the size of the next output is determined by the previous input, for this the content of the input is ignored but in case of duplex audio the content can actually contain audio information, so long as both input and output are configured the same both sould be usable.
> 
> With the above in mind an endpoint can be opened/configured a second time so long as it is an implicit sync endpoint and the config does not change. So that is what this patch does, the check if the config does not change might be a bit more complex then needed, but it seems to work on my motu m2.
> 
> Fixes bug 207023 and should fix 103751 as well

The code changes look good and even if it's not perfect, it can't be
worse than the current situation, so I'd happily take it.

But could you submit in a formal way, especially with your
Signed-off-by line, and with the proper patch description?


thanks,

Takashi

> 
> Kind Regards,
> 
> Erwin Burema
> 
> ---
>  sound/usb/card.h     |   1 +
>  sound/usb/endpoint.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
>  sound/usb/pcm.c      |   5 ++
>  3 files changed, 197 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index 820e564656ed..d6219fba9699 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -108,6 +108,7 @@ struct snd_usb_endpoint {
>  	int iface, altsetting;
>  	int skip_packets;		/* quirks for devices to ignore the first n packets
>  					   in a stream */
> +	bool is_implicit_feedback;      /* This endpoint is used as implicit feedback */
>  
>  	spinlock_t lock;
>  	struct list_head list;
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index 50104f658ed4..9bea7d3f99f8 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -522,6 +522,8 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
>  
>  	list_add_tail(&ep->list, &chip->ep_list);
>  
> +	ep->is_implicit_feedback = 0;
> +
>  __exit_unlock:
>  	mutex_unlock(&chip->mutex);
>  
> @@ -621,6 +623,178 @@ static void release_urbs(struct snd_usb_endpoint *ep, int force)
>  	ep->nurbs = 0;
>  }
>  
> +/*
> + * Check data endpoint for format differences
> + */
> +static bool check_ep_params(struct snd_usb_endpoint *ep,
> +			      snd_pcm_format_t pcm_format,
> +			      unsigned int channels,
> +			      unsigned int period_bytes,
> +			      unsigned int frames_per_period,
> +			      unsigned int periods_per_buffer,
> +			      struct audioformat *fmt,
> +			      struct snd_usb_endpoint *sync_ep)
> +{
> +	unsigned int maxsize, minsize, packs_per_ms, max_packs_per_urb;
> +	unsigned int max_packs_per_period, urbs_per_period, urb_packs;
> +	unsigned int max_urbs;
> +	int frame_bits = snd_pcm_format_physical_width(pcm_format) * channels;
> +	int tx_length_quirk = (ep->chip->tx_length_quirk &&
> +			       usb_pipeout(ep->pipe));
> +	bool ret = 1;
> +
> +	if (pcm_format == SNDRV_PCM_FORMAT_DSD_U16_LE && fmt->dsd_dop) {
> +		/*
> +		 * When operating in DSD DOP mode, the size of a sample frame
> +		 * in hardware differs from the actual physical format width
> +		 * because we need to make room for the DOP markers.
> +		 */
> +		frame_bits += channels << 3;
> +	}
> +
> +	ret = ret && (ep->datainterval == fmt->datainterval);
> +	ret = ret && (ep->stride == frame_bits >> 3);
> +
> +	switch (pcm_format) {
> +	case SNDRV_PCM_FORMAT_U8:
> +		ret = ret && (ep->silence_value == 0x80);
> +		break;
> +	case SNDRV_PCM_FORMAT_DSD_U8:
> +	case SNDRV_PCM_FORMAT_DSD_U16_LE:
> +	case SNDRV_PCM_FORMAT_DSD_U32_LE:
> +	case SNDRV_PCM_FORMAT_DSD_U16_BE:
> +	case SNDRV_PCM_FORMAT_DSD_U32_BE:
> +		ret = ret && (ep->silence_value == 0x69);
> +		break;
> +	default:
> +		ret = ret && (ep->silence_value == 0);
> +	}
> +
> +	/* assume max. frequency is 50% higher than nominal */
> +	ret = ret && (ep->freqmax == ep->freqn + (ep->freqn >> 1));
> +	/* Round up freqmax to nearest integer in order to calculate maximum
> +	 * packet size, which must represent a whole number of frames.
> +	 * This is accomplished by adding 0x0.ffff before converting the
> +	 * Q16.16 format into integer.
> +	 * In order to accurately calculate the maximum packet size when
> +	 * the data interval is more than 1 (i.e. ep->datainterval > 0),
> +	 * multiply by the data interval prior to rounding. For instance,
> +	 * a freqmax of 41 kHz will result in a max packet size of 6 (5.125)
> +	 * frames with a data interval of 1, but 11 (10.25) frames with a
> +	 * data interval of 2.
> +	 * (ep->freqmax << ep->datainterval overflows at 8.192 MHz for the
> +	 * maximum datainterval value of 3, at USB full speed, higher for
> +	 * USB high speed, noting that ep->freqmax is in units of
> +	 * frames per packet in Q16.16 format.)
> +	 */
> +	maxsize = (((ep->freqmax << ep->datainterval) + 0xffff) >> 16) *
> +			 (frame_bits >> 3);
> +	if (tx_length_quirk)
> +		maxsize += sizeof(__le32); /* Space for length descriptor */
> +	/* but wMaxPacketSize might reduce this */
> +	if (ep->maxpacksize && ep->maxpacksize < maxsize) {
> +		/* whatever fits into a max. size packet */
> +		unsigned int data_maxsize = maxsize = ep->maxpacksize;
> +
> +		if (tx_length_quirk)
> +			/* Need to remove the length descriptor to calc freq */
> +			data_maxsize -= sizeof(__le32);
> +		ret = ret && (ep->freqmax == (data_maxsize / (frame_bits >> 3))
> +				<< (16 - ep->datainterval));
> +	}
> +
> +	if (ep->fill_max)
> +		ret = ret && (ep->curpacksize == ep->maxpacksize);
> +	else
> +		ret = ret && (ep->curpacksize == maxsize);
> +
> +	if (snd_usb_get_speed(ep->chip->dev) != USB_SPEED_FULL) {
> +		packs_per_ms = 8 >> ep->datainterval;
> +		max_packs_per_urb = MAX_PACKS_HS;
> +	} else {
> +		packs_per_ms = 1;
> +		max_packs_per_urb = MAX_PACKS;
> +	}
> +	if (sync_ep && !snd_usb_endpoint_implicit_feedback_sink(ep))
> +		max_packs_per_urb = min(max_packs_per_urb,
> +					1U << sync_ep->syncinterval);
> +	max_packs_per_urb = max(1u, max_packs_per_urb >> ep->datainterval);
> +
> +	/*
> +	 * Capture endpoints need to use small URBs because there's no way
> +	 * to tell in advance where the next period will end, and we don't
> +	 * want the next URB to complete much after the period ends.
> +	 *
> +	 * Playback endpoints with implicit sync much use the same parameters
> +	 * as their corresponding capture endpoint.
> +	 */
> +	if (usb_pipein(ep->pipe) ||
> +			snd_usb_endpoint_implicit_feedback_sink(ep)) {
> +
> +		urb_packs = packs_per_ms;
> +		/*
> +		 * Wireless devices can poll at a max rate of once per 4ms.
> +		 * For dataintervals less than 5, increase the packet count to
> +		 * allow the host controller to use bursting to fill in the
> +		 * gaps.
> +		 */
> +		if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_WIRELESS) {
> +			int interval = ep->datainterval;
> +
> +			while (interval < 5) {
> +				urb_packs <<= 1;
> +				++interval;
> +			}
> +		}
> +		/* make capture URBs <= 1 ms and smaller than a period */
> +		urb_packs = min(max_packs_per_urb, urb_packs);
> +		while (urb_packs > 1 && urb_packs * maxsize >= period_bytes)
> +			urb_packs >>= 1;
> +		ret = ret && (ep->nurbs == MAX_URBS);
> +
> +	/*
> +	 * Playback endpoints without implicit sync are adjusted so that
> +	 * a period fits as evenly as possible in the smallest number of
> +	 * URBs.  The total number of URBs is adjusted to the size of the
> +	 * ALSA buffer, subject to the MAX_URBS and MAX_QUEUE limits.
> +	 */
> +	} else {
> +		/* determine how small a packet can be */
> +		minsize = (ep->freqn >> (16 - ep->datainterval)) *
> +				(frame_bits >> 3);
> +		/* with sync from device, assume it can be 12% lower */
> +		if (sync_ep)
> +			minsize -= minsize >> 3;
> +		minsize = max(minsize, 1u);
> +
> +		/* how many packets will contain an entire ALSA period? */
> +		max_packs_per_period = DIV_ROUND_UP(period_bytes, minsize);
> +
> +		/* how many URBs will contain a period? */
> +		urbs_per_period = DIV_ROUND_UP(max_packs_per_period,
> +				max_packs_per_urb);
> +		/* how many packets are needed in each URB? */
> +		urb_packs = DIV_ROUND_UP(max_packs_per_period, urbs_per_period);
> +
> +		/* limit the number of frames in a single URB */
> +		ret = ret && (ep->max_urb_frames ==
> +			DIV_ROUND_UP(frames_per_period, urbs_per_period));
> +
> +		/* try to use enough URBs to contain an entire ALSA buffer */
> +		max_urbs = min((unsigned) MAX_URBS,
> +				MAX_QUEUE * packs_per_ms / urb_packs);
> +		ret = ret && (ep->nurbs == min(max_urbs,
> +				urbs_per_period * periods_per_buffer));
> +	}
> +
> +	ret = ret && (ep->datainterval == fmt->datainterval);
> +	ret = ret && (ep->maxpacksize == fmt->maxpacksize);
> +	ret = ret &&
> +		(ep->fill_max == !!(fmt->attributes & UAC_EP_CS_ATTR_FILL_MAX));
> +
> +	return ret;
> +}
> +
>  /*
>   * configure a data endpoint
>   */
> @@ -886,10 +1060,23 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
>  	int err;
>  
>  	if (ep->use_count != 0) {
> -		usb_audio_warn(ep->chip,
> -			 "Unable to change format on ep #%x: already in use\n",
> -			 ep->ep_num);
> -		return -EBUSY;
> +		bool check = ep->is_implicit_feedback &&
> +			check_ep_params(ep, pcm_format,
> +					     channels, period_bytes,
> +					     period_frames, buffer_periods,
> +					     fmt, sync_ep);
> +
> +		if (!check) {
> +			usb_audio_warn(ep->chip,
> +				"Unable to change format on ep #%x: already in use\n",
> +				ep->ep_num);
> +			return -EBUSY;
> +		}
> +
> +		usb_audio_dbg(ep->chip,
> +			      "Ep #%x already in use as implicit feedback but format not changed\n",
> +			      ep->ep_num);
> +		return 0;
>  	}
>  
>  	/* release old buffers, if any */
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index b50965ab3b3a..d61c2f1095b5 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -404,6 +404,8 @@ static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
>  	if (!subs->sync_endpoint)
>  		return -EINVAL;
>  
> +	subs->sync_endpoint->is_implicit_feedback = 1;
> +
>  	subs->data_endpoint->sync_master = subs->sync_endpoint;
>  
>  	return 1;
> @@ -502,12 +504,15 @@ static int set_sync_endpoint(struct snd_usb_substream *subs,
>  						   implicit_fb ?
>  							SND_USB_ENDPOINT_TYPE_DATA :
>  							SND_USB_ENDPOINT_TYPE_SYNC);
> +
>  	if (!subs->sync_endpoint) {
>  		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
>  			return 0;
>  		return -EINVAL;
>  	}
>  
> +	subs->sync_endpoint->is_implicit_feedback = implicit_fb;
> +
>  	subs->data_endpoint->sync_master = subs->sync_endpoint;
>  
>  	return 0;
> -- 
> 2.26.2
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RFC] Add duplex sound support for USB devices using implicit feedback
  2020-05-10 17:31 ` Takashi Iwai
@ 2020-05-10 18:30   ` Erwin Burema
  0 siblings, 0 replies; 3+ messages in thread
From: Erwin Burema @ 2020-05-10 18:30 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

On zondag 10 mei 2020 19:31:51 CEST Takashi Iwai wrote:
> On Sat, 09 May 2020 20:10:47 +0200,
> 
> Erwin Burema wrote:
> > Hi,
> > 
> > Since I had problems getting duplex audio to work on my motu m2 I created
> > the following patch to get duplex audio to work. This is based on my
> > understanding that when implicit feedback is used the size of the next
> > output is determined by the previous input, for this the content of the
> > input is ignored but in case of duplex audio the content can actually
> > contain audio information, so long as both input and output are
> > configured the same both sould be usable.
> > 
> > With the above in mind an endpoint can be opened/configured a second time
> > so long as it is an implicit sync endpoint and the config does not
> > change. So that is what this patch does, the check if the config does not
> > change might be a bit more complex then needed, but it seems to work on
> > my motu m2.
> > 
> > Fixes bug 207023 and should fix 103751 as well
> 
> The code changes look good and even if it's not perfect, it can't be
> worse than the current situation, so I'd happily take it.
> 
> But could you submit in a formal way, especially with your
> Signed-off-by line, and with the proper patch description?
> 
> 

Send as patch with signed-off-by line and hopefully a proper patch description, 
if anything is wrong please let me know (I think this is my first patch to the 
kernel to be honest)

Kind Regards

Erwin Burema

> thanks,
> 
> Takashi
> 
> > Kind Regards,
> > 
> > Erwin Burema
> > 
> > ---
> > 
> >  sound/usb/card.h     |   1 +
> >  sound/usb/endpoint.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
> >  sound/usb/pcm.c      |   5 ++
> >  3 files changed, 197 insertions(+), 4 deletions(-)
> > 
> > diff --git a/sound/usb/card.h b/sound/usb/card.h
> > index 820e564656ed..d6219fba9699 100644
> > --- a/sound/usb/card.h
> > +++ b/sound/usb/card.h
> > @@ -108,6 +108,7 @@ struct snd_usb_endpoint {
> > 
> >  	int iface, altsetting;
> >  	int skip_packets;		/* quirks for devices to 
ignore the first n packets
> >  	
> >  					   in a stream */
> > 
> > +	bool is_implicit_feedback;      /* This endpoint is used as 
implicit
> > feedback */> 
> >  	spinlock_t lock;
> >  	struct list_head list;
> > 
> > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> > index 50104f658ed4..9bea7d3f99f8 100644
> > --- a/sound/usb/endpoint.c
> > +++ b/sound/usb/endpoint.c
> > @@ -522,6 +522,8 @@ struct snd_usb_endpoint *snd_usb_add_endpoint(struct
> > snd_usb_audio *chip,> 
> >  	list_add_tail(&ep->list, &chip->ep_list);
> > 
> > +	ep->is_implicit_feedback = 0;
> > +
> > 
> >  __exit_unlock:
> >  	mutex_unlock(&chip->mutex);
> > 
> > @@ -621,6 +623,178 @@ static void release_urbs(struct snd_usb_endpoint
> > *ep, int force)> 
> >  	ep->nurbs = 0;
> >  
> >  }
> > 
> > +/*
> > + * Check data endpoint for format differences
> > + */
> > +static bool check_ep_params(struct snd_usb_endpoint *ep,
> > +			      snd_pcm_format_t pcm_format,
> > +			      unsigned int channels,
> > +			      unsigned int period_bytes,
> > +			      unsigned int frames_per_period,
> > +			      unsigned int periods_per_buffer,
> > +			      struct audioformat *fmt,
> > +			      struct snd_usb_endpoint *sync_ep)
> > +{
> > +	unsigned int maxsize, minsize, packs_per_ms, max_packs_per_urb;
> > +	unsigned int max_packs_per_period, urbs_per_period, urb_packs;
> > +	unsigned int max_urbs;
> > +	int frame_bits = snd_pcm_format_physical_width(pcm_format) * 
channels;
> > +	int tx_length_quirk = (ep->chip->tx_length_quirk &&
> > +			       usb_pipeout(ep->pipe));
> > +	bool ret = 1;
> > +
> > +	if (pcm_format == SNDRV_PCM_FORMAT_DSD_U16_LE && fmt->dsd_dop) {
> > +		/*
> > +		 * When operating in DSD DOP mode, the size of a sample 
frame
> > +		 * in hardware differs from the actual physical format 
width
> > +		 * because we need to make room for the DOP markers.
> > +		 */
> > +		frame_bits += channels << 3;
> > +	}
> > +
> > +	ret = ret && (ep->datainterval == fmt->datainterval);
> > +	ret = ret && (ep->stride == frame_bits >> 3);
> > +
> > +	switch (pcm_format) {
> > +	case SNDRV_PCM_FORMAT_U8:
> > +		ret = ret && (ep->silence_value == 0x80);
> > +		break;
> > +	case SNDRV_PCM_FORMAT_DSD_U8:
> > +	case SNDRV_PCM_FORMAT_DSD_U16_LE:
> > +	case SNDRV_PCM_FORMAT_DSD_U32_LE:
> > +	case SNDRV_PCM_FORMAT_DSD_U16_BE:
> > +	case SNDRV_PCM_FORMAT_DSD_U32_BE:
> > +		ret = ret && (ep->silence_value == 0x69);
> > +		break;
> > +	default:
> > +		ret = ret && (ep->silence_value == 0);
> > +	}
> > +
> > +	/* assume max. frequency is 50% higher than nominal */
> > +	ret = ret && (ep->freqmax == ep->freqn + (ep->freqn >> 1));
> > +	/* Round up freqmax to nearest integer in order to calculate 
maximum
> > +	 * packet size, which must represent a whole number of frames.
> > +	 * This is accomplished by adding 0x0.ffff before converting the
> > +	 * Q16.16 format into integer.
> > +	 * In order to accurately calculate the maximum packet size when
> > +	 * the data interval is more than 1 (i.e. ep->datainterval > 0),
> > +	 * multiply by the data interval prior to rounding. For instance,
> > +	 * a freqmax of 41 kHz will result in a max packet size of 6 
(5.125)
> > +	 * frames with a data interval of 1, but 11 (10.25) frames with a
> > +	 * data interval of 2.
> > +	 * (ep->freqmax << ep->datainterval overflows at 8.192 MHz for the
> > +	 * maximum datainterval value of 3, at USB full speed, higher for
> > +	 * USB high speed, noting that ep->freqmax is in units of
> > +	 * frames per packet in Q16.16 format.)
> > +	 */
> > +	maxsize = (((ep->freqmax << ep->datainterval) + 0xffff) >> 16) *
> > +			 (frame_bits >> 3);
> > +	if (tx_length_quirk)
> > +		maxsize += sizeof(__le32); /* Space for length 
descriptor */
> > +	/* but wMaxPacketSize might reduce this */
> > +	if (ep->maxpacksize && ep->maxpacksize < maxsize) {
> > +		/* whatever fits into a max. size packet */
> > +		unsigned int data_maxsize = maxsize = ep->maxpacksize;
> > +
> > +		if (tx_length_quirk)
> > +			/* Need to remove the length descriptor to 
calc freq */
> > +			data_maxsize -= sizeof(__le32);
> > +		ret = ret && (ep->freqmax == (data_maxsize / 
(frame_bits >> 3))
> > +				<< (16 - ep->datainterval));
> > +	}
> > +
> > +	if (ep->fill_max)
> > +		ret = ret && (ep->curpacksize == ep->maxpacksize);
> > +	else
> > +		ret = ret && (ep->curpacksize == maxsize);
> > +
> > +	if (snd_usb_get_speed(ep->chip->dev) != USB_SPEED_FULL) {
> > +		packs_per_ms = 8 >> ep->datainterval;
> > +		max_packs_per_urb = MAX_PACKS_HS;
> > +	} else {
> > +		packs_per_ms = 1;
> > +		max_packs_per_urb = MAX_PACKS;
> > +	}
> > +	if (sync_ep && !snd_usb_endpoint_implicit_feedback_sink(ep))
> > +		max_packs_per_urb = min(max_packs_per_urb,
> > +					1U << sync_ep-
>syncinterval);
> > +	max_packs_per_urb = max(1u, max_packs_per_urb >> ep-
>datainterval);
> > +
> > +	/*
> > +	 * Capture endpoints need to use small URBs because there's no way
> > +	 * to tell in advance where the next period will end, and we don't
> > +	 * want the next URB to complete much after the period ends.
> > +	 *
> > +	 * Playback endpoints with implicit sync much use the same 
parameters
> > +	 * as their corresponding capture endpoint.
> > +	 */
> > +	if (usb_pipein(ep->pipe) ||
> > +			snd_usb_endpoint_implicit_feedback_sink(ep)) 
{
> > +
> > +		urb_packs = packs_per_ms;
> > +		/*
> > +		 * Wireless devices can poll at a max rate of once per 
4ms.
> > +		 * For dataintervals less than 5, increase the packet 
count to
> > +		 * allow the host controller to use bursting to fill in 
the
> > +		 * gaps.
> > +		 */
> > +		if (snd_usb_get_speed(ep->chip->dev) == 
USB_SPEED_WIRELESS) {
> > +			int interval = ep->datainterval;
> > +
> > +			while (interval < 5) {
> > +				urb_packs <<= 1;
> > +				++interval;
> > +			}
> > +		}
> > +		/* make capture URBs <= 1 ms and smaller than a period 
*/
> > +		urb_packs = min(max_packs_per_urb, urb_packs);
> > +		while (urb_packs > 1 && urb_packs * maxsize >= 
period_bytes)
> > +			urb_packs >>= 1;
> > +		ret = ret && (ep->nurbs == MAX_URBS);
> > +
> > +	/*
> > +	 * Playback endpoints without implicit sync are adjusted so that
> > +	 * a period fits as evenly as possible in the smallest number of
> > +	 * URBs.  The total number of URBs is adjusted to the size of the
> > +	 * ALSA buffer, subject to the MAX_URBS and MAX_QUEUE limits.
> > +	 */
> > +	} else {
> > +		/* determine how small a packet can be */
> > +		minsize = (ep->freqn >> (16 - ep->datainterval)) *
> > +				(frame_bits >> 3);
> > +		/* with sync from device, assume it can be 12% lower */
> > +		if (sync_ep)
> > +			minsize -= minsize >> 3;
> > +		minsize = max(minsize, 1u);
> > +
> > +		/* how many packets will contain an entire ALSA period? 
*/
> > +		max_packs_per_period = DIV_ROUND_UP(period_bytes, 
minsize);
> > +
> > +		/* how many URBs will contain a period? */
> > +		urbs_per_period = DIV_ROUND_UP(max_packs_per_period,
> > +				max_packs_per_urb);
> > +		/* how many packets are needed in each URB? */
> > +		urb_packs = DIV_ROUND_UP(max_packs_per_period, 
urbs_per_period);
> > +
> > +		/* limit the number of frames in a single URB */
> > +		ret = ret && (ep->max_urb_frames ==
> > +			DIV_ROUND_UP(frames_per_period, 
urbs_per_period));
> > +
> > +		/* try to use enough URBs to contain an entire ALSA 
buffer */
> > +		max_urbs = min((unsigned) MAX_URBS,
> > +				MAX_QUEUE * packs_per_ms / 
urb_packs);
> > +		ret = ret && (ep->nurbs == min(max_urbs,
> > +				urbs_per_period * 
periods_per_buffer));
> > +	}
> > +
> > +	ret = ret && (ep->datainterval == fmt->datainterval);
> > +	ret = ret && (ep->maxpacksize == fmt->maxpacksize);
> > +	ret = ret &&
> > +		(ep->fill_max == !!(fmt->attributes & 
UAC_EP_CS_ATTR_FILL_MAX));
> > +
> > +	return ret;
> > +}
> > +
> > 
> >  /*
> >  
> >   * configure a data endpoint
> >   */
> > 
> > @@ -886,10 +1060,23 @@ int snd_usb_endpoint_set_params(struct
> > snd_usb_endpoint *ep,> 
> >  	int err;
> >  	
> >  	if (ep->use_count != 0) {
> > 
> > -		usb_audio_warn(ep->chip,
> > -			 "Unable to change format on ep #%x: already 
in use\n",
> > -			 ep->ep_num);
> > -		return -EBUSY;
> > +		bool check = ep->is_implicit_feedback &&
> > +			check_ep_params(ep, pcm_format,
> > +					     channels, 
period_bytes,
> > +					     period_frames, 
buffer_periods,
> > +					     fmt, sync_ep);
> > +
> > +		if (!check) {
> > +			usb_audio_warn(ep->chip,
> > +				"Unable to change format on ep 
#%x: already in use\n",
> > +				ep->ep_num);
> > +			return -EBUSY;
> > +		}
> > +
> > +		usb_audio_dbg(ep->chip,
> > +			      "Ep #%x already in use as implicit 
feedback but format not
> > changed\n", +			      ep->ep_num);
> > +		return 0;
> > 
> >  	}
> >  	
> >  	/* release old buffers, if any */
> > 
> > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> > index b50965ab3b3a..d61c2f1095b5 100644
> > --- a/sound/usb/pcm.c
> > +++ b/sound/usb/pcm.c
> > @@ -404,6 +404,8 @@ static int set_sync_ep_implicit_fb_quirk(struct
> > snd_usb_substream *subs,> 
> >  	if (!subs->sync_endpoint)
> >  	
> >  		return -EINVAL;
> > 
> > +	subs->sync_endpoint->is_implicit_feedback = 1;
> > +
> > 
> >  	subs->data_endpoint->sync_master = subs->sync_endpoint;
> >  	
> >  	return 1;
> > 
> > @@ -502,12 +504,15 @@ static int set_sync_endpoint(struct
> > snd_usb_substream *subs,> 
> >  						   
implicit_fb ?
> >  							
> >  							
SND_USB_ENDPOINT_TYPE_DATA :
> >  							
SND_USB_ENDPOINT_TYPE_SYNC);
> > 
> > +
> > 
> >  	if (!subs->sync_endpoint) {
> >  	
> >  		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
> >  		
> >  			return 0;
> >  		
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > +	subs->sync_endpoint->is_implicit_feedback = implicit_fb;
> > +
> > 
> >  	subs->data_endpoint->sync_master = subs->sync_endpoint;
> >  	
> >  	return 0;





^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-05-10 18:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-09 18:10 [RFC] Add duplex sound support for USB devices using implicit feedback Erwin Burema
2020-05-10 17:31 ` Takashi Iwai
2020-05-10 18:30   ` Erwin Burema

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.