hiface: Fix sample rate changes

Message ID ee24b066-41d2-ad24-8598-f15a26a743a8@sonarnerd.net
State New
Headers show

Commit Message

Jussi Laako Nov. 28, 2016, 9:27 a.m.
When attempting to change sample rate for the interface after first 
set-play cycle, the change is not reflected to the hardware, while from 
user space everything seems to go fine.

This patch fixes the issue and the driver now behaves the same way as 
for example the USB Audio Class driver.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Comments

Michael Trimarchi Nov. 28, 2016, 9:51 a.m. | #1
Hi

On Mon, Nov 28, 2016 at 10:27 AM, Jussi Laako <jussi@sonarnerd.net> wrote:
> When attempting to change sample rate for the interface after first set-play

> cycle, the change is not reflected to the hardware, while from user space

> everything seems to go fine.

>

> This patch fixes the issue and the driver now behaves the same way as for

> example the USB Audio Class driver.

>


Can you please sent it inline? Not having access at the hardware now
but Antonio should have it

Michael


>




-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Michael Trimarchi Nov. 28, 2016, 10:09 a.m. | #2
Hi

On Mon, Nov 28, 2016 at 10:55 AM, Jussi Laako <jussi@sonarnerd.net> wrote:
> Hi,

>

>> Can you please sent it inline? Not having access at the hardware now

>> but Antonio should have it

>

>

> Sure! I have tested this with my hiFace...

>

>

>         - Jussi

>

>

> From f4eec5602d86b5f938abed48a5725f59141d32cd Mon Sep 17 00:00:00 2001

> From: Jussi Laako <jussi@sonarnerd.net>

> Date: Mon, 28 Nov 2016 01:11:46 +0200

> Subject: [PATCH] Fix M2Tech hiFace driver sampling rate change

>

> Sampling rate changes after first set one are not reflected to the

> hardware, while driver and ALSA think the rate has been changed.

>

> Fix the problem by properly stopping the interface at the beginning of

> prepare call, allowing new rate to be set to the hardware. This keeps

> the hardware in sync with the driver.

>

> Signed-off-by: Jussi Laako <jussi@sonarnerd.net>

> ---

>  sound/usb/hiface/pcm.c | 2 ++

>  1 file changed, 2 insertions(+)

>

> diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c

> index 2c44139..33db205 100644

> --- a/sound/usb/hiface/pcm.c

> +++ b/sound/usb/hiface/pcm.c

> @@ -445,6 +445,8 @@ static int hiface_pcm_prepare(struct snd_pcm_substream

> *alsa_sub)

>

>         mutex_lock(&rt->stream_mutex);

>

> +       hiface_pcm_stream_stop(rt);

> +


Ok, I was expecting stream already paused on STOP or SUSPEND call. I
have the impression that they enqueue 0 urb still and they don't
really
stop it.

Michael

>         sub->dma_off = 0;

>         sub->period_off = 0;

>

> --

> 2.7.4




-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Takashi Iwai Nov. 28, 2016, 2:11 p.m. | #3
On Mon, 28 Nov 2016 11:09:25 +0100,
Michael Trimarchi wrote:
> 

> Hi

> 

> On Mon, Nov 28, 2016 at 10:55 AM, Jussi Laako <jussi@sonarnerd.net> wrote:

> > Hi,

> >

> >> Can you please sent it inline? Not having access at the hardware now

> >> but Antonio should have it

> >

> >

> > Sure! I have tested this with my hiFace...

> >

> >

> >         - Jussi

> >

> >

> > From f4eec5602d86b5f938abed48a5725f59141d32cd Mon Sep 17 00:00:00 2001

> > From: Jussi Laako <jussi@sonarnerd.net>

> > Date: Mon, 28 Nov 2016 01:11:46 +0200

> > Subject: [PATCH] Fix M2Tech hiFace driver sampling rate change

> >

> > Sampling rate changes after first set one are not reflected to the

> > hardware, while driver and ALSA think the rate has been changed.

> >

> > Fix the problem by properly stopping the interface at the beginning of

> > prepare call, allowing new rate to be set to the hardware. This keeps

> > the hardware in sync with the driver.

> >

> > Signed-off-by: Jussi Laako <jussi@sonarnerd.net>

> > ---

> >  sound/usb/hiface/pcm.c | 2 ++

> >  1 file changed, 2 insertions(+)

> >

> > diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c

> > index 2c44139..33db205 100644

> > --- a/sound/usb/hiface/pcm.c

> > +++ b/sound/usb/hiface/pcm.c

> > @@ -445,6 +445,8 @@ static int hiface_pcm_prepare(struct snd_pcm_substream

> > *alsa_sub)

> >

> >         mutex_lock(&rt->stream_mutex);

> >

> > +       hiface_pcm_stream_stop(rt);

> > +

> 

> Ok, I was expecting stream already paused on STOP or SUSPEND call. I

> have the impression that they enqueue 0 urb still and they don't

> really

> stop it.


The trigger callback is atomic, so it can't kill urb by itself, and
hiface_pcm_trigger() just flips the sub->active flag.

So, yes, calling hiface_pcm_stream_stop() should be correct.
One question is whether it should be unconditionally called, or it
should be done only when something got changed.

I guess it's OK to call it always (it's safer), but it may lead to
some glitches or such that wasn't present beforehand, when the player
restarts the stream.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Takashi Iwai Dec. 12, 2016, 9:49 p.m. | #4
On Mon, 28 Nov 2016 15:11:25 +0100,
Takashi Iwai wrote:
> 

> On Mon, 28 Nov 2016 11:09:25 +0100,

> Michael Trimarchi wrote:

> > 

> > Hi

> > 

> > On Mon, Nov 28, 2016 at 10:55 AM, Jussi Laako <jussi@sonarnerd.net> wrote:

> > > Hi,

> > >

> > >> Can you please sent it inline? Not having access at the hardware now

> > >> but Antonio should have it

> > >

> > >

> > > Sure! I have tested this with my hiFace...

> > >

> > >

> > >         - Jussi

> > >

> > >

> > > From f4eec5602d86b5f938abed48a5725f59141d32cd Mon Sep 17 00:00:00 2001

> > > From: Jussi Laako <jussi@sonarnerd.net>

> > > Date: Mon, 28 Nov 2016 01:11:46 +0200

> > > Subject: [PATCH] Fix M2Tech hiFace driver sampling rate change

> > >

> > > Sampling rate changes after first set one are not reflected to the

> > > hardware, while driver and ALSA think the rate has been changed.

> > >

> > > Fix the problem by properly stopping the interface at the beginning of

> > > prepare call, allowing new rate to be set to the hardware. This keeps

> > > the hardware in sync with the driver.

> > >

> > > Signed-off-by: Jussi Laako <jussi@sonarnerd.net>

> > > ---

> > >  sound/usb/hiface/pcm.c | 2 ++

> > >  1 file changed, 2 insertions(+)

> > >

> > > diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c

> > > index 2c44139..33db205 100644

> > > --- a/sound/usb/hiface/pcm.c

> > > +++ b/sound/usb/hiface/pcm.c

> > > @@ -445,6 +445,8 @@ static int hiface_pcm_prepare(struct snd_pcm_substream

> > > *alsa_sub)

> > >

> > >         mutex_lock(&rt->stream_mutex);

> > >

> > > +       hiface_pcm_stream_stop(rt);

> > > +

> > 

> > Ok, I was expecting stream already paused on STOP or SUSPEND call. I

> > have the impression that they enqueue 0 urb still and they don't

> > really

> > stop it.

> 

> The trigger callback is atomic, so it can't kill urb by itself, and

> hiface_pcm_trigger() just flips the sub->active flag.

> 

> So, yes, calling hiface_pcm_stream_stop() should be correct.

> One question is whether it should be unconditionally called, or it

> should be done only when something got changed.

> 

> I guess it's OK to call it always (it's safer), but it may lead to

> some glitches or such that wasn't present beforehand, when the player

> restarts the stream.


And I forgot this patch until now, sorry.  I applied it now for
4.10-rc1.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch

From f4eec5602d86b5f938abed48a5725f59141d32cd Mon Sep 17 00:00:00 2001
From: Jussi Laako <jussi@sonarnerd.net>
Date: Mon, 28 Nov 2016 01:11:46 +0200
Subject: [PATCH] Fix M2Tech hiFace driver sampling rate change

Sampling rate changes after first set one are not reflected to the
hardware, while driver and ALSA think the rate has been changed.

Fix the problem by properly stopping the interface at the beginning of
prepare call, allowing new rate to be set to the hardware. This keeps
the hardware in sync with the driver.

Signed-off-by: Jussi Laako <jussi@sonarnerd.net>
---
 sound/usb/hiface/pcm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c
index 2c44139..33db205 100644
--- a/sound/usb/hiface/pcm.c
+++ b/sound/usb/hiface/pcm.c
@@ -445,6 +445,8 @@  static int hiface_pcm_prepare(struct snd_pcm_substream *alsa_sub)
 
 	mutex_lock(&rt->stream_mutex);
 
+	hiface_pcm_stream_stop(rt);
+
 	sub->dma_off = 0;
 	sub->period_off = 0;
 
-- 
2.7.4