[alsa-devel] [PATCH RFC 00/26] Kill set_fs() in ALSA codes

[Takashi Sakamoto]

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