[alsa-devel] [PATCH] ALSA: sb: remove needless evaluation in implementation for copy callback
In design of ALSA pcm core, 'struct snd_pcm_ops.copy' is expected to copy PCM frames, according to frame alignment on intermediate buffer for userspace and dedicated buffer for data transmission. In this callback, value of 'channel' argument depends on the frame alignment, which drivers registers to runtime of PCM substream. When target devices can handle non-interleaved buffer, this value has positive value, otherwise negative.
ALSA driver for PCM component of EMU8000 chip is programmed with local macro to switch the frame alignment. The 'copy' operation in non-interleaved side has evaluation of the 'channel' argument (actually it's 'voice' argument). This is useless.
This commit remove the evaluation.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/isa/sb/emu8000_pcm.c | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/sound/isa/sb/emu8000_pcm.c b/sound/isa/sb/emu8000_pcm.c index 32f234f494e5..bab796d75dbd 100644 --- a/sound/isa/sb/emu8000_pcm.c +++ b/sound/isa/sb/emu8000_pcm.c @@ -450,20 +450,8 @@ static int emu8k_pcm_copy(struct snd_pcm_substream *subs, struct snd_emu8000 *emu = rec->emu;
snd_emu8000_write_wait(emu, 1); - if (voice == -1) { - unsigned short *buf = src; - int i, err; - count /= rec->voices; - for (i = 0; i < rec->voices; i++) { - err = emu8k_transfer_block(emu, pos + rec->loop_start[i], buf, count); - if (err < 0) - return err; - buf += count; - } - return 0; - } else { - return emu8k_transfer_block(emu, pos + rec->loop_start[voice], src, count); - } + return emu8k_transfer_block(emu, pos + rec->loop_start[voice], src, + count); }
/* make a channel block silence */ @@ -487,17 +475,9 @@ static int emu8k_pcm_silence(struct snd_pcm_substream *subs, struct snd_emu8000 *emu = rec->emu;
snd_emu8000_write_wait(emu, 1); - if (voice == -1 && rec->voices == 1) + if (rec->voices == 1) voice = 0; - if (voice == -1) { - int err; - err = emu8k_silence_block(emu, pos + rec->loop_start[0], count / 2); - if (err < 0) - return err; - return emu8k_silence_block(emu, pos + rec->loop_start[1], count / 2); - } else { - return emu8k_silence_block(emu, pos + rec->loop_start[voice], count); - } + return emu8k_silence_block(emu, pos + rec->loop_start[voice], count); }
#else /* interleave */
On Tue, 23 May 2017 15:40:42 +0200, Takashi Sakamoto wrote:
In design of ALSA pcm core, 'struct snd_pcm_ops.copy' is expected to copy PCM frames, according to frame alignment on intermediate buffer for userspace and dedicated buffer for data transmission. In this callback, value of 'channel' argument depends on the frame alignment, which drivers registers to runtime of PCM substream. When target devices can handle non-interleaved buffer, this value has positive value, otherwise negative.
ALSA driver for PCM component of EMU8000 chip is programmed with local macro to switch the frame alignment. The 'copy' operation in non-interleaved side has evaluation of the 'channel' argument (actually it's 'voice' argument). This is useless.
This commit remove the evaluation.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Passing -1 to the channel was valid even for non-interleaved access. It was meant to apply to all channels.
The call with channel = -1 itself might have been dropped meanwhile, so removing it may be OK now. But the description is confusing as if it were incorrectly implemented.
Takashi
sound/isa/sb/emu8000_pcm.c | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/sound/isa/sb/emu8000_pcm.c b/sound/isa/sb/emu8000_pcm.c index 32f234f494e5..bab796d75dbd 100644 --- a/sound/isa/sb/emu8000_pcm.c +++ b/sound/isa/sb/emu8000_pcm.c @@ -450,20 +450,8 @@ static int emu8k_pcm_copy(struct snd_pcm_substream *subs, struct snd_emu8000 *emu = rec->emu;
snd_emu8000_write_wait(emu, 1);
- if (voice == -1) {
unsigned short *buf = src;int i, err;count /= rec->voices;for (i = 0; i < rec->voices; i++) {err = emu8k_transfer_block(emu, pos + rec->loop_start[i], buf, count);if (err < 0)return err;buf += count;}return 0;- } else {
return emu8k_transfer_block(emu, pos + rec->loop_start[voice], src, count);- }
- return emu8k_transfer_block(emu, pos + rec->loop_start[voice], src,
count);}
/* make a channel block silence */ @@ -487,17 +475,9 @@ static int emu8k_pcm_silence(struct snd_pcm_substream *subs, struct snd_emu8000 *emu = rec->emu;
snd_emu8000_write_wait(emu, 1);
- if (voice == -1 && rec->voices == 1)
- if (rec->voices == 1) voice = 0;
- if (voice == -1) {
int err;err = emu8k_silence_block(emu, pos + rec->loop_start[0], count / 2);if (err < 0)return err;return emu8k_silence_block(emu, pos + rec->loop_start[1], count / 2);- } else {
return emu8k_silence_block(emu, pos + rec->loop_start[voice], count);- }
- return emu8k_silence_block(emu, pos + rec->loop_start[voice], count);
}
#else /* interleave */
2.11.0
Hi,
On May 23 2017 22:49, Takashi Iwai wrote:
On Tue, 23 May 2017 15:40:42 +0200, Takashi Sakamoto wrote:
In design of ALSA pcm core, 'struct snd_pcm_ops.copy' is expected to copy PCM frames, according to frame alignment on intermediate buffer for userspace and dedicated buffer for data transmission. In this callback, value of 'channel' argument depends on the frame alignment, which drivers registers to runtime of PCM substream. When target devices can handle non-interleaved buffer, this value has positive value, otherwise negative.
ALSA driver for PCM component of EMU8000 chip is programmed with local macro to switch the frame alignment. The 'copy' operation in non-interleaved side has evaluation of the 'channel' argument (actually it's 'voice' argument). This is useless.
This commit remove the evaluation.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Passing -1 to the channel was valid even for non-interleaved access. It was meant to apply to all channels.
The call with channel = -1 itself might have been dropped meanwhile, so removing it may be OK now. But the description is confusing as if it were incorrectly implemented.
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/Documen...
You need to check the channel argument, and if it's -1, copy the whole channels. Otherwise, you have to copy only the specified channel.
Hm. We need to correct this API documentation, because there's no codes to pass negative value to the argument for vector buffer operation. I'll post my proposal later.
Thanks
Takashi Sakamoto
On Wed, 24 May 2017 17:01:07 +0200, Takashi Sakamoto wrote:
Hi,
On May 23 2017 22:49, Takashi Iwai wrote:
On Tue, 23 May 2017 15:40:42 +0200, Takashi Sakamoto wrote:
In design of ALSA pcm core, 'struct snd_pcm_ops.copy' is expected to copy PCM frames, according to frame alignment on intermediate buffer for userspace and dedicated buffer for data transmission. In this callback, value of 'channel' argument depends on the frame alignment, which drivers registers to runtime of PCM substream. When target devices can handle non-interleaved buffer, this value has positive value, otherwise negative.
ALSA driver for PCM component of EMU8000 chip is programmed with local macro to switch the frame alignment. The 'copy' operation in non-interleaved side has evaluation of the 'channel' argument (actually it's 'voice' argument). This is useless.
This commit remove the evaluation.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Passing -1 to the channel was valid even for non-interleaved access. It was meant to apply to all channels.
The call with channel = -1 itself might have been dropped meanwhile, so removing it may be OK now. But the description is confusing as if it were incorrectly implemented.
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/Documen...
You need to check the channel argument, and if it's -1, copy the whole channels. Otherwise, you have to copy only the specified channel.
Hm. We need to correct this API documentation, because there's no codes to pass negative value to the argument for vector buffer operation. I'll post my proposal later.
Just leave them as is. We're working on these codes in anyway, and touching them just confuse the development.
If it were a fix, I'd take it quickly. But it's nothing but a very minor optimization, so it's really not worth to spend your time.
thanks,
Takashi
On Wed, 24 May 2017 18:28:50 +0200, Takashi Iwai wrote:
On Wed, 24 May 2017 17:01:07 +0200, Takashi Sakamoto wrote:
Hi,
On May 23 2017 22:49, Takashi Iwai wrote:
On Tue, 23 May 2017 15:40:42 +0200, Takashi Sakamoto wrote:
In design of ALSA pcm core, 'struct snd_pcm_ops.copy' is expected to copy PCM frames, according to frame alignment on intermediate buffer for userspace and dedicated buffer for data transmission. In this callback, value of 'channel' argument depends on the frame alignment, which drivers registers to runtime of PCM substream. When target devices can handle non-interleaved buffer, this value has positive value, otherwise negative.
ALSA driver for PCM component of EMU8000 chip is programmed with local macro to switch the frame alignment. The 'copy' operation in non-interleaved side has evaluation of the 'channel' argument (actually it's 'voice' argument). This is useless.
This commit remove the evaluation.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Passing -1 to the channel was valid even for non-interleaved access. It was meant to apply to all channels.
The call with channel = -1 itself might have been dropped meanwhile, so removing it may be OK now. But the description is confusing as if it were incorrectly implemented.
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/Documen...
You need to check the channel argument, and if it's -1, copy the whole channels. Otherwise, you have to copy only the specified channel.
Hm. We need to correct this API documentation, because there's no codes to pass negative value to the argument for vector buffer operation. I'll post my proposal later.
Just leave them as is. We're working on these codes in anyway, and touching them just confuse the development.
If it were a fix, I'd take it quickly. But it's nothing but a very minor optimization, so it's really not worth to spend your time.
Despite the situation, I decided to take these two patches. Also, the API correction isn't needed -- the behavior I described was about the pre-historic one.
thanks,
Takashi
participants (2)
-
Takashi Iwai -
Takashi Sakamoto