From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Sakamoto Subject: Re: [PATCH RFC 00/26] Kill set_fs() in ALSA codes Date: Sun, 14 May 2017 17:23:01 +0900 Message-ID: References: <20170511210925.18208-1-tiwai@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-proxy003.phy.lolipop.jp (smtp-proxy003.phy.lolipop.jp [157.7.104.44]) by alsa0.perex.cz (Postfix) with ESMTP id 867742665D5 for ; Sun, 14 May 2017 10:23:08 +0200 (CEST) In-Reply-To: <20170511210925.18208-1-tiwai@suse.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai , alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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