From: Takashi Iwai <tiwai@suse.de>
To: Timo Wischer <twischer@de.adit-jv.com>
Cc: <broonie@kernel.org>, <perex@perex.cz>,
<alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 09/10] ALSA: pcm: Add snd_pcm_ops for snd_pcm_link()
Date: Fri, 05 Apr 2019 17:21:15 +0200 [thread overview]
Message-ID: <s5h8swoxytw.wl-tiwai@suse.de> (raw)
In-Reply-To: <00d97a34-c055-c7ef-dbfd-e17c0f6cfff3@de.adit-jv.com>
On Thu, 04 Apr 2019 12:18:33 +0200,
Timo Wischer wrote:
>
> >>> In principle, the PCM ops is supposed to be safe for operating a
> >>> certain stuff. If a state change may happen during the operation,
> >>> this should be called inside PCM stream lock. So the design of the
> >>> new callback itself is fragile.
> >>>
> >>> then, it comes to a point to re-setup the timer at the link change.
> >>> The idea is interesting, but it's a wrong usage of PCM link feature,
> >>> to be honest.
> >>>
> >>> That said, I find the changes up to patch 8 are acceptable (with some
> >>> fixes expected), but the link feature isn't.
>
> Hi Takashi,
>
> could you provide your feedback to the other patches?
The patch 4 must be superfluous, at least. The trigger callback is
called within the stream lock, so you really don't need to save
flags.
The patch 8 needs more consideration. I'm not fond of the current way
to select the timer source. For example, the ALSA timer source isn't
necessarily bound with cards. It can be a hrtimer, or system timer
backend ALSA global timer, too. This can't be chosen in the suggested
way. Also, the use of tasklet is superfluous. The timer instance can
be called via tasklet by itself if you pass the flag properly.
> I would like to apply these 8 patches first and then I would provide
> an addition patch to get an ALSA control to select the sound timer.
> I fear this would be the best idea (replacing the snd_pcm_link()
> feature) because the ALSA control is accessible by any audio user and
> we could use ALSA hooks to select the right sound timer when opening
> an ALSA device.
It depends on the implementation. Usually you "lock" such a control
that is bound with a PCM stream at opening, so that other process
won't be able to change. Such a trick is used for the dynamic routing
via control element for emu10k1, for example.
But, whether it's really usable, I don't know. Let's see.
thanks,
Takashi
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de>
To: Timo Wischer <twischer@de.adit-jv.com>
Cc: broonie@kernel.org, perex@perex.cz, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/10] ALSA: pcm: Add snd_pcm_ops for snd_pcm_link()
Date: Fri, 05 Apr 2019 17:21:15 +0200 [thread overview]
Message-ID: <s5h8swoxytw.wl-tiwai@suse.de> (raw)
In-Reply-To: <00d97a34-c055-c7ef-dbfd-e17c0f6cfff3@de.adit-jv.com>
On Thu, 04 Apr 2019 12:18:33 +0200,
Timo Wischer wrote:
>
> >>> In principle, the PCM ops is supposed to be safe for operating a
> >>> certain stuff. If a state change may happen during the operation,
> >>> this should be called inside PCM stream lock. So the design of the
> >>> new callback itself is fragile.
> >>>
> >>> then, it comes to a point to re-setup the timer at the link change.
> >>> The idea is interesting, but it's a wrong usage of PCM link feature,
> >>> to be honest.
> >>>
> >>> That said, I find the changes up to patch 8 are acceptable (with some
> >>> fixes expected), but the link feature isn't.
>
> Hi Takashi,
>
> could you provide your feedback to the other patches?
The patch 4 must be superfluous, at least. The trigger callback is
called within the stream lock, so you really don't need to save
flags.
The patch 8 needs more consideration. I'm not fond of the current way
to select the timer source. For example, the ALSA timer source isn't
necessarily bound with cards. It can be a hrtimer, or system timer
backend ALSA global timer, too. This can't be chosen in the suggested
way. Also, the use of tasklet is superfluous. The timer instance can
be called via tasklet by itself if you pass the flag properly.
> I would like to apply these 8 patches first and then I would provide
> an addition patch to get an ALSA control to select the sound timer.
> I fear this would be the best idea (replacing the snd_pcm_link()
> feature) because the ALSA control is accessible by any audio user and
> we could use ALSA hooks to select the right sound timer when opening
> an ALSA device.
It depends on the implementation. Usually you "lock" such a control
that is bound with a PCM stream at opening, so that other process
won't be able to change. Such a trick is used for the dynamic routing
via control element for emu10k1, for example.
But, whether it's really usable, I don't know. Let's see.
thanks,
Takashi
next prev parent reply other threads:[~2019-04-05 15:21 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-25 16:00 [PATCH 00/10] ALSA: aloop: Support selection of snd_timer twischer
2019-03-25 16:00 ` twischer
2019-03-25 16:00 ` [PATCH 01/10] ALSA: aloop: Describe units of variables twischer
2019-03-25 16:00 ` twischer
2019-03-25 16:00 ` [PATCH 02/10] ALSA: aloop: loopback_timer_start: Support return of error code twischer
2019-03-25 16:00 ` twischer
2019-03-25 16:00 ` [PATCH 03/10] ALSA: aloop: loopback_timer_stop: " twischer
2019-03-25 16:00 ` twischer
2019-03-25 16:00 ` [PATCH 04/10] ALSA: aloop: Use always spin_lock_irqsave() for cable->lock twischer
2019-03-25 16:00 ` twischer
2019-03-25 16:07 ` Takashi Iwai
2019-03-25 16:07 ` Takashi Iwai
2019-03-25 16:40 ` Timo Wischer
2019-03-25 16:58 ` Takashi Iwai
2019-03-25 16:58 ` Takashi Iwai
2019-03-26 8:12 ` Timo Wischer
2019-03-26 8:12 ` Timo Wischer
2019-03-26 7:49 ` [PATCH 05/10] ALSA: aloop: Use callback functions for timer specific implementations twischer
2019-03-26 7:49 ` twischer
2019-03-26 7:49 ` [PATCH 06/10] ALSA: aloop: Rename all jiffies timer specific functions twischer
2019-03-26 7:49 ` twischer
2019-03-26 7:49 ` [PATCH 07/10] ALSA: aloop: Move CABLE_VALID_BOTH to the top of file twischer
2019-03-26 7:49 ` twischer
2019-03-26 7:49 ` [PATCH 08/10] ALSA: aloop: Support selection of snd_timer instead of jiffies twischer
2019-03-26 7:49 ` twischer
2019-03-26 7:49 ` [PATCH 09/10] ALSA: pcm: Add snd_pcm_ops for snd_pcm_link() twischer
2019-03-26 7:49 ` twischer
2019-03-26 8:35 ` Takashi Iwai
2019-03-26 8:35 ` Takashi Iwai
2019-03-26 11:25 ` Timo Wischer
2019-03-26 14:23 ` Takashi Iwai
2019-03-26 14:23 ` Takashi Iwai
2019-03-26 15:16 ` Timo Wischer
2019-03-26 15:16 ` Timo Wischer
2019-03-26 16:00 ` Takashi Iwai
2019-03-26 16:00 ` Takashi Iwai
2019-03-27 8:34 ` Timo Wischer
2019-03-27 8:34 ` Timo Wischer
2019-03-27 9:11 ` Takashi Iwai
2019-03-27 9:11 ` Takashi Iwai
2019-03-27 9:26 ` Timo Wischer
2019-03-27 9:26 ` Timo Wischer
2019-03-27 9:38 ` Takashi Iwai
2019-03-27 9:38 ` Takashi Iwai
2019-04-04 10:18 ` Timo Wischer
2019-04-04 10:18 ` Timo Wischer
2019-04-05 15:21 ` Takashi Iwai [this message]
2019-04-05 15:21 ` Takashi Iwai
2019-03-26 7:52 ` [PATCH 10/10] ALSA: aloop: Use timer of linked card if chosen twischer
2019-03-26 7:52 ` twischer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=s5h8swoxytw.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=perex@perex.cz \
--cc=twischer@de.adit-jv.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.