Hi,
On May 12 2017 06:08, Takashi Iwai wrote:
this is a slightly lengthy patchset, basically targeted for 4.13, thus
it's still in RFC. The main purpose of this patchset is to get rid of
get_fs() / set_fs() usages in ALSA codes.
The patchset starts from a patch that looks fairly irrelevant with the
goal (the conversion of HD-audio bind-mute code), but it's a step for
the long trail. The biggest change in the patchset is the conversion
of copy & silence PCM ops into the new copy_silence ops. It helped
for reducing the code size and for handling the in-kernel buffer
copies.
The current patches are found in topic/kill-set_fs branch of my
sound.git tree.
thanks,
Takashi
===
Takashi Iwai (26):
ALSA: hda - Simplify bound-beep mute control for ALC268
ALSA: hda - Move bind-mixer switch codes to generic parser
ALSA: hda - Remove the generic bind ctl helpers
ALSA: hda - Remove the use of set_fs()
ALSA: hda - Fix a typo in comment
ALSA: hda - Remove superfluous header inclusions
Patch 01-06 look good to me, and they can be applied regardless of the
rest as a refactoring of PCI HDA driver. I have had concern about usage
of TLV feature with the set_fs() for a bit time. I note that patch 06
includes a fixup to a commit 59ed1eade1d6 ("ALSA: hda - Move codec
suspend/resume to codec driver").
ALSA: opl3: Kill unused set_fs()
This is also good to me. These lines were forgot to remove at a commit
224a033252bb ("[ALSA] opl3 - Use hwdep for patch loading").
ALSA: emu10k1: Get rid of set_fs() usage
Looks good to me, but I'm not so good for emu10k1 driver and its hwdep
functionality. I think existent code is basically designed to run on
process contexts of the hwdep ioctl and not expected on device probing.
ALSA: pcm: Remove set_fs() in PCM core code
ALSA: pcm: Introduce copy_silence PCM ops
ALSA: Update document about copy_silence PCM ops
ALSA: dummy: Convert to copy_silence ops
ALSA: es1938: Convert to copy_silence ops
ALSA: korg1212: Convert to copy_silence ops
ALSA: nm256: Convert to copy_silence ops
ALSA: rme32: Convert to copy_silence ops
ALSA: rme96: Convert to copy_silence ops
ALSA: rme9652: Convert to copy_silence ops
ALSA: hdsp: Convert to copy_silence ops
ALSA: gus: Convert to copy_silence ops
ALSA: sb: Convert to copy_silence ops
ALSA: sh: Convert to copy_silence ops
ASoC: blackfin: Convert to copy_silence ops
[media] solo6x10: Convert to copy_silence ops
ALSA: pcm: Drop the old copy and silence ops
ALSA: pcm: Kill set_fs() usage in OSS layer and USB gadget
I prefer the idea to unify 'struct snd_pcm_ops.copy' and 'struct
snd_pcm_ops.silence', but we should consider better name for it so that
developers can easily get its intension; e.g. just naming 'copy_frames'.
About the way to copy PCM frames between several regions, I have a
concern about disadvantage due to adding extra branching because the
write/read operations repeatedly during runtime of PCM substream for
devices which doesn't support memory mapping of DMA-dedicated buffer.
When letting me assume 'proxy' driver such ad OSS compatibility layer
and UAC1 gadget driver (as a peripheral), below diagram show copy
operation of PCM frames between memory regions. I also illustrate an use
case for applications.
applications
(__user *) <-----+
v
usual driver
(__kernel */__iomem *) <-----> hardware
proxy drivers ^
(__kernel *) <-----+
The case to handle data for the proxy drivers is not general use cases
in ALSA PCM core and drivers. On the other hand, this patchset adds
extra branching in runtime of PCM substream. Additionally, increase of
codes in each function is not good in an aspect of hit rate of i-cache.
Even if practically these codes are used for minor drivers which doesn't
support memory mapping of DMA-dedicated buffer to user processes, it's
better to care of such efficiencies somehow in a point of applications.
Furthermore, such efficiency is good to the proxy drivers because
currently they don't utilize 'struct snd_pcm_runtime.dma_area' directly.
(If allowing the proxy drivers to have full PCM functionality, the other
ioctl commands should be available via snd_pcm_kernel_ioctl().)
This is a callgraph for relevant functions:
(sound/core/pcm_lib.c)
snd_pcm_lib_read()
snd_pcm_lib_readv()
->snd_pcm_lib_read1()
->snd_pcm_lib_read_transfer()
->struct snd_pcm_ops.copy()
->copy_to_user()
->snd_pcm_lib_readv_transfer()
for(**bufs)
->struct snd_pcm_ops.copy()
->copy_to_user()
(sound/core/pcm_lib.c)
snd_pcm_lib_write()
snd_pcm_lib_writev()
->snd_pcm_lib_write1()
->snd_pcm_lib_write_transfer()
->struct snd_pcm_ops.copy()
->copy_from_user()
->snd_pcm_lib_writev_transfer()
for(**bufs)
->struct snd_pcm_ops.copy()
->copy_from_user()
Some helper functions can be assumed to perform copy operation between
below three cases:
- struct snd_pcm_ops.copy()
- between __kernel * and __user *
- between __kernel * and __kernel *
When adding a member for a callback function into 'struct
snd_pcm_runtime' and assigning one of the helper functions in a call of
HW_PARAMS or PREPARE, then call it from each call of the exported
functions, needless branching can be removed in runtime of PCM substream.
In the above reasons, I think it better to apply refactoring at first,
then get rid of set_fs() fro the 'proxy' drivers. All of ALSA drivers
can potentially get some merit from the refactoring because the drivers
support simple READ_FRAME/WRITE_FRAME ioctl commands as a default.
Regards
Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
