From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751449AbeBMFAx (ORCPT ); Tue, 13 Feb 2018 00:00:53 -0500 Received: from mx2.suse.de ([195.135.220.15]:56727 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750823AbeBMFAw (ORCPT ); Tue, 13 Feb 2018 00:00:52 -0500 Date: Tue, 13 Feb 2018 06:00:49 +0100 Message-ID: From: Takashi Iwai To: "Maciej S. Szmigiero" Cc: Jaroslav Kysela , alsa-devel@alsa-project.org, linux-kernel Subject: Re: [PATCH v2 5/5] ALSA: emu10k1: add a IOMMU workaround In-Reply-To: References: <13361c4f-6203-8d83-d6c3-1e150224be49@maciej.szmigiero.name> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 12 Feb 2018 23:13:13 +0100, Maciej S. Szmigiero wrote: > > On 12.02.2018 13:56, Takashi Iwai wrote: > > On Sat, 27 Jan 2018 15:42:59 +0100, > > Maciej S. Szmigiero wrote: > >> > >> The Audigy 2 CA0102 chip (but most likely others from the emu10k1 family, > >> too) has a problem that from time to time it likes to do few DMA reads a > >> bit beyond its normal allocation and gets very confused if these reads get > >> blocked by a IOMMU. > >> > >> For the first (reserved) page this happens multiple times at every > >> playback, for various synth pages it happens randomly, rarely for PCM > >> playback buffers and the page table memory itself. > >> All these reads seem to follow a similar pattern, observed read offsets > >> beyond the allocation end were 0x00, 0x40, 0x80 and 0xc0 (PCI cache line > >> multiples), so it looks like the device tries to accesses up to 256 extra > >> bytes. > >> > >> As a workaround let's widen these DMA allocations by an extra page if we > >> detect that the device is behind a non-passthrough IOMMU (the DMA memory > >> should be relatively plenty on IOMMU systems). > >> > >> Signed-off-by: Maciej S. Szmigiero > >> --- > >> Changes from v1: Apply this workaround also to PCM playback buffers since > >> it seems they are affected, too. > > > > Instead of adjusting the allocation size in the caller side, how about > > adding a new helper to wrap around the call of snd_dma_alloc_pages()? > > > > We may need a counterpart to free pages in synth, but it's a single > > place in __synth_free_pages(), so it can be open-coded with some > > proper comments, too. > > I guess you mean adding a new wrapper to the ALSA core somewhere near > snd_dma_alloc_pages() (something named like > snd_dma_dev_alloc_pages_maybe_wider() ?). Well, not a common code but emu10k1 specific, something like int snd_emu10k1_alloc_pages_maybe_wider(emu, size, res) { if (emu->iommu_workaround) size += PAGE_SIZE; return snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(emu->pci), size, res); } Also, I wonder what if PAGE_SIZE is over 4k. In that case, we don't necessarily need to increase the size, if the allocated size is larger than the requested one? But iommu_workaround is likely only about x86, so we don't need to care about it much, I guess. Takashi > Since snd_dma_alloc_pages() currently takes only a "struct device" > per-device parameter we wouldn't have a place to store a flag indicating > whether a device needs this workaround (or not) so we would need to > detect it every time this wrapper function gets called - in contrast, > in the current implementation this is done just once at the device > initialization time in snd_emu10k1_detect_iommu(). > > There are only 3 allocations that use snd_dma_alloc_pages() in this > driver that would use this new wrapper function, and each time the > overhead is just a two-line "if" block. > If one excludes synth, since it already uses a helper function to > compute these allocations lengths, that count lowers to only 2 places. > > That's why I think a driver-local change here is enough. > > Also, there is always a possibility to refactor the code into a common > helper if it turns out that there are other sound card with the same > problem. > > > > > thanks, > > > > Takashi > > Thanks, > Maciej >