Re: [PATCH RFC 00/26] Kill set_fs() in ALSA codes

[Takashi Iwai <tiwai@suse.de>]

On Sun, 14 May 2017 10:23:01 +0200,
Takashi Sakamoto wrote:
> 
> 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").

Yes, the patch 06 is merely a cleanup.

> >   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'.

Well, the callback doesn't only copy but also silence the buffer.
Your new name misses that.

> 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.

I don't understand yet clearly how your proxy driver is supposed to
work.  But at this moment, the higher priority is to get rid of
set_fs() in the time frame for 4.13 kernel.  For that purpose, my
approach is rather straightforward.  There is no big code flow change,
and all changes can be applied gradually, and less risk of
regressions.

I don't object against the redesign of the PCM transfer if it really
works better.  But please let the code speaks.


thanks,

Takashi