From: Alexander Tsoy <[email protected]>
[ Upstream commit f0bd62b64016508938df9babe47f65c2c727d25c ]
For computation of the the next frame size current value of fs/fps and
accumulated fractional parts of fs/fps are used, where values are stored
in Q16.16 format. This is quite natural for computing frame size for
asynchronous endpoints driven by explicit feedback, since in this case
fs/fps is a value provided by the feedback endpoint and it's already in
the Q format. If an error is accumulated over time, the device can
adjust fs/fps value to prevent buffer overruns/underruns.
But for synchronous endpoints the accuracy provided by these computations
is not enough. Due to accumulated error the driver periodically produces
frames with incorrect size (+/- 1 audio sample).
This patch fixes this issue by implementing a different algorithm for
frame size computation. It is based on accumulating of the remainders
from division fs/fps and it doesn't accumulate errors over time. This
new method is enabled for synchronous and adaptive playback endpoints.
Signed-off-by: Alexander Tsoy <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Takashi Iwai <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
sound/usb/card.h | 4 ++++
sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++-----
sound/usb/endpoint.h | 1 +
sound/usb/pcm.c | 2 ++
4 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h
index 111b0f009afa4..c4599cf0ddc94 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -80,6 +80,10 @@ struct snd_usb_endpoint {
dma_addr_t sync_dma; /* DMA address of syncbuf */
unsigned int pipe; /* the data i/o pipe */
+ unsigned int framesize[2]; /* small/large frame sizes in samples */
+ unsigned int sample_rem; /* remainder from division fs/fps */
+ unsigned int sample_accum; /* sample accumulator */
+ unsigned int fps; /* frames per second */
unsigned int freqn; /* nominal sampling rate in fs/fps in Q16.16 format */
unsigned int freqm; /* momentary sampling rate in fs/fps in Q16.16 format */
int freqshift; /* how much to shift the feedback value to get Q16.16 */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 30aa5f2df6da5..b5207e71ed720 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -137,12 +137,12 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep)
/*
* For streaming based on information derived from sync endpoints,
- * prepare_outbound_urb_sizes() will call next_packet_size() to
+ * prepare_outbound_urb_sizes() will call slave_next_packet_size() to
* determine the number of samples to be sent in the next packet.
*
- * For implicit feedback, next_packet_size() is unused.
+ * For implicit feedback, slave_next_packet_size() is unused.
*/
-int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep)
+int snd_usb_endpoint_slave_next_packet_size(struct snd_usb_endpoint *ep)
{
unsigned long flags;
int ret;
@@ -159,6 +159,29 @@ int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep)
return ret;
}
+/*
+ * For adaptive and synchronous endpoints, prepare_outbound_urb_sizes()
+ * will call next_packet_size() to determine the number of samples to be
+ * sent in the next packet.
+ */
+int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep)
+{
+ int ret;
+
+ if (ep->fill_max)
+ return ep->maxframesize;
+
+ ep->sample_accum += ep->sample_rem;
+ if (ep->sample_accum >= ep->fps) {
+ ep->sample_accum -= ep->fps;
+ ret = ep->framesize[1];
+ } else {
+ ret = ep->framesize[0];
+ }
+
+ return ret;
+}
+
static void retire_outbound_urb(struct snd_usb_endpoint *ep,
struct snd_urb_ctx *urb_ctx)
{
@@ -203,6 +226,8 @@ static void prepare_silent_urb(struct snd_usb_endpoint *ep,
if (ctx->packet_size[i])
counts = ctx->packet_size[i];
+ else if (ep->sync_master)
+ counts = snd_usb_endpoint_slave_next_packet_size(ep);
else
counts = snd_usb_endpoint_next_packet_size(ep);
@@ -875,10 +900,17 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
ep->maxpacksize = fmt->maxpacksize;
ep->fill_max = !!(fmt->attributes & UAC_EP_CS_ATTR_FILL_MAX);
- if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_FULL)
+ if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_FULL) {
ep->freqn = get_usb_full_speed_rate(rate);
- else
+ ep->fps = 1000;
+ } else {
ep->freqn = get_usb_high_speed_rate(rate);
+ ep->fps = 8000;
+ }
+
+ ep->sample_rem = rate % ep->fps;
+ ep->framesize[0] = rate / ep->fps;
+ ep->framesize[1] = (rate + (ep->fps - 1)) / ep->fps;
/* calculate the frequency in 16.16 format */
ep->freqm = ep->freqn;
@@ -937,6 +969,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
ep->active_mask = 0;
ep->unlink_mask = 0;
ep->phase = 0;
+ ep->sample_accum = 0;
snd_usb_endpoint_start_quirk(ep);
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index 584f295d7c773..4aad49cbeb5f1 100644
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -27,6 +27,7 @@ void snd_usb_endpoint_release(struct snd_usb_endpoint *ep);
void snd_usb_endpoint_free(struct snd_usb_endpoint *ep);
int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
+int snd_usb_endpoint_slave_next_packet_size(struct snd_usb_endpoint *ep);
int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep);
void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep,
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 9bc995f9b4e17..615213aeda338 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1483,6 +1483,8 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
for (i = 0; i < ctx->packets; i++) {
if (ctx->packet_size[i])
counts = ctx->packet_size[i];
+ else if (ep->sync_master)
+ counts = snd_usb_endpoint_slave_next_packet_size(ep);
else
counts = snd_usb_endpoint_next_packet_size(ep);
--
2.25.1
В Пн, 29/06/2020 в 11:37 -0400, Sasha Levin пишет:
> From: Alexander Tsoy <[email protected]>
>
> [ Upstream commit f0bd62b64016508938df9babe47f65c2c727d25c ]
>
> For computation of the the next frame size current value of fs/fps
> and
> accumulated fractional parts of fs/fps are used, where values are
> stored
> in Q16.16 format. This is quite natural for computing frame size for
> asynchronous endpoints driven by explicit feedback, since in this
> case
> fs/fps is a value provided by the feedback endpoint and it's already
> in
> the Q format. If an error is accumulated over time, the device can
> adjust fs/fps value to prevent buffer overruns/underruns.
>
> But for synchronous endpoints the accuracy provided by these
> computations
> is not enough. Due to accumulated error the driver periodically
> produces
> frames with incorrect size (+/- 1 audio sample).
>
> This patch fixes this issue by implementing a different algorithm for
> frame size computation. It is based on accumulating of the remainders
> from division fs/fps and it doesn't accumulate errors over time. This
> new method is enabled for synchronous and adaptive playback
> endpoints.
>
> Signed-off-by: Alexander Tsoy <[email protected]>
> Link:
> https://lore.kernel.org/r/[email protected]
> Signed-off-by: Takashi Iwai <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> sound/usb/card.h | 4 ++++
> sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++--
> ---
> sound/usb/endpoint.h | 1 +
> sound/usb/pcm.c | 2 ++
> 4 files changed, 45 insertions(+), 5 deletions(-)
Please drop this patch from the queue for now (and for 4.4 as well). It
introduced a regression for some devices. The fix is available, but not
accepted yet.
On Tue, Jun 30, 2020 at 01:49:50PM +0300, Alexander Tsoy wrote:
>В Пн, 29/06/2020 в 11:37 -0400, Sasha Levin пишет:
>> From: Alexander Tsoy <[email protected]>
>>
>> [ Upstream commit f0bd62b64016508938df9babe47f65c2c727d25c ]
>>
>> For computation of the the next frame size current value of fs/fps
>> and
>> accumulated fractional parts of fs/fps are used, where values are
>> stored
>> in Q16.16 format. This is quite natural for computing frame size for
>> asynchronous endpoints driven by explicit feedback, since in this
>> case
>> fs/fps is a value provided by the feedback endpoint and it's already
>> in
>> the Q format. If an error is accumulated over time, the device can
>> adjust fs/fps value to prevent buffer overruns/underruns.
>>
>> But for synchronous endpoints the accuracy provided by these
>> computations
>> is not enough. Due to accumulated error the driver periodically
>> produces
>> frames with incorrect size (+/- 1 audio sample).
>>
>> This patch fixes this issue by implementing a different algorithm for
>> frame size computation. It is based on accumulating of the remainders
>> from division fs/fps and it doesn't accumulate errors over time. This
>> new method is enabled for synchronous and adaptive playback
>> endpoints.
>>
>> Signed-off-by: Alexander Tsoy <[email protected]>
>> Link:
>> https://lore.kernel.org/r/[email protected]
>> Signed-off-by: Takashi Iwai <[email protected]>
>> Signed-off-by: Sasha Levin <[email protected]>
>> ---
>> sound/usb/card.h | 4 ++++
>> sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++--
>> ---
>> sound/usb/endpoint.h | 1 +
>> sound/usb/pcm.c | 2 ++
>> 4 files changed, 45 insertions(+), 5 deletions(-)
>
>Please drop this patch from the queue for now (and for 4.4 as well). It
>introduced a regression for some devices. The fix is available, but not
>accepted yet.
I've dropped it from the older branches, but note that it's already in
newer released stable kernels. Should it be reverted or should we wait
for the fix?
--
Thanks,
Sasha
On Tue, Jun 30, 2020 at 12:54:07PM -0400, Sasha Levin wrote:
> On Tue, Jun 30, 2020 at 01:49:50PM +0300, Alexander Tsoy wrote:
> > В Пн, 29/06/2020 в 11:37 -0400, Sasha Levin пишет:
> > > From: Alexander Tsoy <[email protected]>
> > >
> > > [ Upstream commit f0bd62b64016508938df9babe47f65c2c727d25c ]
> > >
> > > For computation of the the next frame size current value of fs/fps
> > > and
> > > accumulated fractional parts of fs/fps are used, where values are
> > > stored
> > > in Q16.16 format. This is quite natural for computing frame size for
> > > asynchronous endpoints driven by explicit feedback, since in this
> > > case
> > > fs/fps is a value provided by the feedback endpoint and it's already
> > > in
> > > the Q format. If an error is accumulated over time, the device can
> > > adjust fs/fps value to prevent buffer overruns/underruns.
> > >
> > > But for synchronous endpoints the accuracy provided by these
> > > computations
> > > is not enough. Due to accumulated error the driver periodically
> > > produces
> > > frames with incorrect size (+/- 1 audio sample).
> > >
> > > This patch fixes this issue by implementing a different algorithm for
> > > frame size computation. It is based on accumulating of the remainders
> > > from division fs/fps and it doesn't accumulate errors over time. This
> > > new method is enabled for synchronous and adaptive playback
> > > endpoints.
> > >
> > > Signed-off-by: Alexander Tsoy <[email protected]>
> > > Link:
> > > https://lore.kernel.org/r/[email protected]
> > > Signed-off-by: Takashi Iwai <[email protected]>
> > > Signed-off-by: Sasha Levin <[email protected]>
> > > ---
> > > sound/usb/card.h | 4 ++++
> > > sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++--
> > > ---
> > > sound/usb/endpoint.h | 1 +
> > > sound/usb/pcm.c | 2 ++
> > > 4 files changed, 45 insertions(+), 5 deletions(-)
> >
> > Please drop this patch from the queue for now (and for 4.4 as well). It
> > introduced a regression for some devices. The fix is available, but not
> > accepted yet.
>
> I've dropped it from the older branches, but note that it's already in
> newer released stable kernels. Should it be reverted or should we wait
> for the fix?
I was going to wait for the fix.
On Tue, 30 Jun 2020 20:33:28 +0200,
Greg KH wrote:
>
> On Tue, Jun 30, 2020 at 12:54:07PM -0400, Sasha Levin wrote:
> > On Tue, Jun 30, 2020 at 01:49:50PM +0300, Alexander Tsoy wrote:
> > > В Пн, 29/06/2020 в 11:37 -0400, Sasha Levin пишет:
> > > > From: Alexander Tsoy <[email protected]>
> > > >
> > > > [ Upstream commit f0bd62b64016508938df9babe47f65c2c727d25c ]
> > > >
> > > > For computation of the the next frame size current value of fs/fps
> > > > and
> > > > accumulated fractional parts of fs/fps are used, where values are
> > > > stored
> > > > in Q16.16 format. This is quite natural for computing frame size for
> > > > asynchronous endpoints driven by explicit feedback, since in this
> > > > case
> > > > fs/fps is a value provided by the feedback endpoint and it's already
> > > > in
> > > > the Q format. If an error is accumulated over time, the device can
> > > > adjust fs/fps value to prevent buffer overruns/underruns.
> > > >
> > > > But for synchronous endpoints the accuracy provided by these
> > > > computations
> > > > is not enough. Due to accumulated error the driver periodically
> > > > produces
> > > > frames with incorrect size (+/- 1 audio sample).
> > > >
> > > > This patch fixes this issue by implementing a different algorithm for
> > > > frame size computation. It is based on accumulating of the remainders
> > > > from division fs/fps and it doesn't accumulate errors over time. This
> > > > new method is enabled for synchronous and adaptive playback
> > > > endpoints.
> > > >
> > > > Signed-off-by: Alexander Tsoy <[email protected]>
> > > > Link:
> > > > https://lore.kernel.org/r/[email protected]
> > > > Signed-off-by: Takashi Iwai <[email protected]>
> > > > Signed-off-by: Sasha Levin <[email protected]>
> > > > ---
> > > > sound/usb/card.h | 4 ++++
> > > > sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++--
> > > > ---
> > > > sound/usb/endpoint.h | 1 +
> > > > sound/usb/pcm.c | 2 ++
> > > > 4 files changed, 45 insertions(+), 5 deletions(-)
> > >
> > > Please drop this patch from the queue for now (and for 4.4 as well). It
> > > introduced a regression for some devices. The fix is available, but not
> > > accepted yet.
> >
> > I've dropped it from the older branches, but note that it's already in
> > newer released stable kernels. Should it be reverted or should we wait
> > for the fix?
>
> I was going to wait for the fix.
The corresponding fix is now in sound git tree.
But since I'm traveling in this week, the pull request to Linus will
be delayed likely to the next week.
thanks,
Takashi