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;
