Re: [alsa-devel] A bug about cache inconsistency report
- noise relate test:
When I annotate the return after set_pages_array_wc, then set_memory_wc works, noise disappear static void __mark_pages_wc(struct azx *chip, struct snd_dma_buffer *dmab, bool on) { ... set_pages_array_wb(sgbuf->page_table, sgbuf->pages); //return; } #endif
set_memory_uc() performs __pa() for the given address, and in this case, it's vmapped address. Something bogus in your test, I'm afraid.
[Hans:] I know __pa() handle the vmapped address is wrong, that why I don't know how to fix the problem, I try this because I see kernel older than 3.7.6. [bogus? pretending to be real or genuine?] no no no, but the noise problem machine(8086:1c20) in another city, I will make sure it again.
I meant about the reliability of the test result. Or whether we're heading to a wrong fish. In anyway, which kernel version are you testing? And exactly what is your problem? Which machine, which hardware setup (codec, etc) and which output, and how is the sound backend (PulseAudio or direct ALSA write)?
[Hans:] OS: Ubuntu 17.10 + 4.13.0-21-generic (or any one after 3.7.6). HW: mother board is Dell 042P49, HDA controller is 8086:1c20, codec is cx20641. Output and sound backend: Except the command below, anything other is default setting.
Problem(what I have done and found): When system setup, I add "modprobe snd-had-intel.snoop=0" in grub, after setup, execute command "aplay -D plughw:0,0 44.1k_16b_2c.wav", then I hear music with noise. The reason cause this is the buffer address should been mark as WC type and the __mark_pages_wc was patched for this, but if define CONFIG_SND_DMA_SGBUF, the actual marked address is the address before vmapped(0xffff88*...*), however the address used in snd_pcm_lib_write_transfer() is the address after vmapped(0xffffc9*...*), so the mark didn't work and cache inconsistency occur(noise).
Well, basically the vmapped address isn't used *at all* on the HD-audio controller hardware. The SG buffer addresses are passed on BDL of the chip, but the vmapped address is used only in PCM core for some data manipulation. And this is purely software stuff, hence it shouldn't matter there whether it's in snoop mode or not. The snoop mode difference appears mainly when accessed via mmap from user-space directly to kernel-space. That said, I guess your suspect heading to a wrong direction.
[Hans:] So, am I misunderstand? Or snoop has two different meanings? As I know, In the hardware layer HDAC’s stream have two data transport path : non-snoop & snoop; In the software layer ALSA-Driver have two data transport path : mmap & not mmap(test shows, it is dependent on wav's format or mmap_flag in aplay.c). When hardware at non-snoop mode, without hardware module's help, software must mark the mem to WC type: when mmap used, the mark action happened at pcm_mmap_prepare(), the not-mmap mode's mark action happened at __mark_pages_wc(). And when at not-mmap mode, the vmapped address directly used in snd_pcm_lib_write_transfer() -> copy_from_user(). Yes, of course, this path combinations(non-snoop + not-mmap) unlikely be used in most music players.
And moreover, all Intel chipsets do work with snooping well, AFAIK.
[Hans:] yes, all Intel chipsets do work with snooping well here too. And the problem case here is "non-snoop"+"aplay -D plughw:*.*"+"44.1k 16bit"+"8086:1c20", I will see if I can find more machine do the test. Ah, so you explicitly test with non-snoop mode for this chipset? For what purpose...?
[Hans:] Yes, I've been testing a HDA controller recently(predecessor is VIA), it has the same problem. I want to figure out if it is hardware's problem or not.
[Hans:] I did the same test on another platform(8086:A170), it do work with non-snoop well. In addition, I did such a change, oneliner, the purpose is to mark nothing, and it still do work with non-snoop well, this supposed to be right?
--- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c #ifdef CONFIG_X86 static void __mark_pages_wc(struct azx *chip, struct snd_dma_buffer *dmab, bool on) { int pages; +return;
This is essentially other way round; it skips the non-cache setup.
- change *_SG relate test:
Just followed your proposal -snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV_SG, +snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
OK, you need one more oneliner:
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index a12e594d4e3b..2eaed4956e7f 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -707,7 +707,7 @@ static const struct snd_pcm_ops azx_pcm_ops = { .pointer = azx_pcm_pointer, .get_time_info = azx_get_time_info, >.mmap = azx_pcm_mmap, -.page = snd_pcm_sgbuf_ops_page, +// .page = snd_pcm_sgbuf_ops_page, };
static void azx_pcm_free(struct snd_pcm *pcm)
[Hans :] I did this, and the Oops report as the same.
My bad, there are other SG-buffer codes in sound/hda/*, too.
So scratch these two oneliners. The only thing you'd need is the big-hammer change (again oneliner :)
--- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -181,7 +181,7 @@ config SND_VMASTER bool
config SND_DMA_SGBUF -def_bool y +def_bool n depends on X86
source "sound/core/seq/Kconfig"
[Hans :] If this, All the platform are all right.
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
---a/sound/core/sgbuf.c +++b/sound/core/sgbuf.c void *snd_malloc_sgbuf_pages(struct device *device, size_t size, struct snd_dma_buffer *dmab, size_t *res_size) { struct snd_sg_buf *sgbuf; unsigned int i, pages, chunk, maxpages; struct snd_dma_buffer tmpb; struct snd_sg_page *table; struct page **pgtable;
+size *= 2; /* expand the buffer two times */
dmab->area = NULL; dmab->addr = 0;
... ...
if (chunk < maxpages) maxpages = chunk; }
+for(i = 0; i < sgbuf->pages / 2; i ++) /* restore size to the original it wanted */ +sgbuf->page_table[i] = sgbuf->page_table[i * 2]; /* take a page every other page, just for test, ignore page release */ +size /= 2; +sgbuf->pages /= 2;
sgbuf->size = size; dmab->area = vmap(sgbuf->page_table, sgbuf->pages, VM_MAP, PAGE_KERNEL);
Thanks,
Hans
保密声明： 本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。 CONFIDENTIAL NOTE: This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited.
On Tue, 07 Aug 2018 11:00:39 +0200, Hans Hu(SH-RD) wrote:
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
Not really what I meant. Rather something like below (totally untested).
Takashi
---
diff --git a/sound/core/sgbuf.c b/sound/core/sgbuf.c index 84fffabdd129..643f6cb2048c 100644 --- a/sound/core/sgbuf.c +++ b/sound/core/sgbuf.c @@ -68,7 +68,7 @@ void *snd_malloc_sgbuf_pages(struct device *device, size_t *res_size) { struct snd_sg_buf *sgbuf; - unsigned int i, pages, chunk, maxpages; + unsigned int i, pages; struct snd_dma_buffer tmpb; struct snd_sg_page *table; struct page **pgtable; @@ -90,38 +90,18 @@ void *snd_malloc_sgbuf_pages(struct device *device, goto _failed; sgbuf->page_table = pgtable;
- /* allocate pages */ - maxpages = MAX_ALLOC_PAGES; - while (pages > 0) { - chunk = pages; - /* don't be too eager to take a huge chunk */ - if (chunk > maxpages) - chunk = maxpages; - chunk <<= PAGE_SHIFT; - if (snd_dma_alloc_pages_fallback(SNDRV_DMA_TYPE_DEV, device, - chunk, &tmpb) < 0) { - if (!sgbuf->pages) - goto _failed; - if (!res_size) - goto _failed; - size = sgbuf->pages * PAGE_SIZE; - break; - } - chunk = tmpb.bytes >> PAGE_SHIFT; - for (i = 0; i < chunk; i++) { - table->buf = tmpb.area; - table->addr = tmpb.addr; - if (!i) - table->addr |= chunk; /* mark head */ - table++; - *pgtable++ = virt_to_page(tmpb.area); - tmpb.area += PAGE_SIZE; - tmpb.addr += PAGE_SIZE; - } - sgbuf->pages += chunk; - pages -= chunk; - if (chunk < maxpages) - maxpages = chunk; + if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, device, size, &tmpb) < 0) + goto _failed; + + for (i = 0; i < pages; i++) { + table->buf = tmpb.area; + table->addr = tmpb.addr; + if (!i) + table->addr |= pages; /* mark head */ + table++; + *pgtable++ = virt_to_page(tmpb.area); + tmpb.area += PAGE_SIZE; + tmpb.addr += PAGE_SIZE; }
sgbuf->size = size;
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
Not really what I meant. Rather something like below (totally untested).
[Hans :] I know what you mean now and complete code like below, but the result is still noise.
Thanks,
Hans ---
diff --git a/sound/core/sgbuf.c b/sound/core/sgbuf.c index 84fffabdd129..643f6cb2048c 100644 --- a/sound/core/sgbuf.c +++ b/sound/core/sgbuf.c @@ -68,7 +68,7 @@ void *snd_malloc_sgbuf_pages(struct device *device, size_t *res_size) { struct snd_sg_buf *sgbuf; -unsigned int i, pages, chunk, maxpages; +unsigned int i, pages, chunk; struct snd_dma_buffer tmpb; struct snd_sg_page *table; struct page **pgtable; @@ -90,38 +90,18 @@ void *snd_malloc_sgbuf_pages(struct device *device, goto _failed; sgbuf->page_table = pgtable;
-/* allocate pages */ -maxpages = MAX_ALLOC_PAGES; -while (pages > 0) { -chunk = pages; -/* don't be too eager to take a huge chunk */ -if (chunk > maxpages) -chunk = maxpages; -chunk <<= PAGE_SHIFT; -if (snd_dma_alloc_pages_fallback(SNDRV_DMA_TYPE_DEV, device, - chunk, &tmpb) < 0) { -if (!sgbuf->pages) -goto _failed; -if (!res_size) -goto _failed; -size = sgbuf->pages * PAGE_SIZE; -break; -} -chunk = tmpb.bytes >> PAGE_SHIFT; -for (i = 0; i < chunk; i++) { -table->buf = tmpb.area; -table->addr = tmpb.addr; -if (!i) -table->addr |= chunk; /* mark head */ -table++; -*pgtable++ = virt_to_page(tmpb.area); -tmpb.area += PAGE_SIZE; -tmpb.addr += PAGE_SIZE; -} -sgbuf->pages += chunk; -pages -= chunk; -if (chunk < maxpages) -maxpages = chunk; +if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, device, size, &tmpb) < 0) +goto _failed; +chunk = tmpb.bytes >> PAGE_SHIFT; +for (i = 0; i < pages; i++) { +table->buf = tmpb.area; +table->addr = tmpb.addr; +if (!i) +table->addr |= pages; /* mark head */ +table++; +*pgtable++ = virt_to_page(tmpb.area); +tmpb.area += PAGE_SIZE; +tmpb.addr += PAGE_SIZE; +sgbuf->pages += chunk; }
sgbuf->size = size;
保密声明： 本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。 CONFIDENTIAL NOTE: This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited.
On Tue, 07 Aug 2018 12:59:04 +0200, Hans Hu(SH-RD) wrote:
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
Not really what I meant. Rather something like below (totally untested).
[Hans :] I know what you mean now and complete code like below, but the result is still noise.
OK, so indeed the vmapped address does seem matter. Interesting. What kind of user access does produce the noise? Does it via aplay mmap mode, too?
In anyway, if the vmap is a problem, it might be worked around a patch like below (again totally untested and not sure whether it's correct).
Takashi
--- diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -410,12 +410,23 @@ static void __mark_pages_wc(struct azx *chip, struct snd_dma_buffer *dmab, bool #ifdef CONFIG_SND_DMA_SGBUF if (dmab->dev.type == SNDRV_DMA_TYPE_DEV_SG) { struct snd_sg_buf *sgbuf = dmab->private_data; + pgprot_t prot; + void *new_area; + if (chip->driver_type == AZX_DRIVER_CMEDIA) return; /* deal with only CORB/RIRB buffers */ - if (on) + if (on) { set_pages_array_wc(sgbuf->page_table, sgbuf->pages); - else + prot = pgprot_noncached(PAGE_KERNEL); + } else { set_pages_array_wb(sgbuf->page_table, sgbuf->pages); + prot = PAGE_KERNEL; + } + new_area = vmap(sgbuf->page_table, sgbuf->pages, VM_MAP, prot); + if (WARN_ON(!new_area)) + return; + vunmap(dmab->area); + dmab->area = new_area; return; } #endif
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
Not really what I meant. Rather something like below (totally untested).
[Hans :] I know what you mean now and complete code like below, but the result is still noise.
OK, so indeed the vmapped address does seem matter. Interesting. What kind of user access does produce the noise? Does it via aplay mmap mode, too?
In anyway, if the vmap is a problem, it might be worked around a patch like below (again totally untested and not sure whether it's correct).
[Hans :] As I know, In the hardware layer HDAC’s stream have two data transport path : non-snoop & snoop; In the software layer ALSA-Driver have two data transport path : mmap & not mmap(test shows, it is dependent on wav's format or mmap_flag in aplay.c). When hardware at non-snoop mode, without hardware module's help, software must mark the mem to uncacheable type: when mmap used, the mark action happened at pcm_mmap_prepare(), the not-mmap mode's mark action happened at __mark_pages_wc(). And when at not-mmap mode, the vmapped address directly used in snd_pcm_lib_write_transfer() -> copy_from_user(). Yes, of course, this path combinations(non-snoop + not-mmap) unlikely be used by most music players.
[Hans :] I complete the patch in your last mail like below, not used the vunmap & re-vmap way(it has a little problem). Result is OK now(no noise anymore).
--- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c #ifdef CONFIG_X86 +#ifdef CONFIG_SND_DMA_SGBUF +#include <linux/vmalloc.h> +#include <linux/sched.h> +#include <asm/paravirt.h> + +static int bad_address(void *p) +{ +unsigned long dummy; + +return probe_kernel_address((unsigned long *)p, dummy); +} + +static pte_t * dump_pte(unsigned long address) +{ +pgd_t *base = __va(read_cr3() & PHYSICAL_PAGE_MASK); +pgd_t *pgd = base + pgd_index(address); +pud_t *pud; +pmd_t *pmd; +pte_t *pte; + +if (bad_address(pgd)) +goto bad; + +if (!pgd_present(*pgd)) +goto bad; + +pud = pud_offset(pgd, address); +if (bad_address(pud)) +goto bad; + +if (!pud_present(*pud) || pud_large(*pud)) +goto bad; + +pmd = pmd_offset(pud, address); +if (bad_address(pmd)) +goto bad; + +if (!pmd_present(*pmd) || pmd_large(*pmd)) +goto bad; + +pte = pte_offset_kernel(pmd, address); +if (bad_address(pte)) +goto bad; + +return pte; +bad: +return NULL; +} +#endif static void __mark_pages_wc(struct azx *chip, struct snd_dma_buffer *dmab, bool on) { int pages; @@ -410,12 +459,28 @@ static void __mark_pages_wc(struct azx *chip, struct snd_dma_buffer *dmab, bool #ifdef CONFIG_SND_DMA_SGBUF if (dmab->dev.type == SNDRV_DMA_TYPE_DEV_SG) { struct snd_sg_buf *sgbuf = dmab->private_data; +pgprot_t prot; +int i; +unsigned long addr; +pte_t * pte; if (chip->driver_type == AZX_DRIVER_CMEDIA) return; /* deal with only CORB/RIRB buffers */ -if (on) +if (on){ set_pages_array_wc(sgbuf->page_table, sgbuf->pages); -else +prot = pgprot_writecombine(PAGE_KERNEL); +} +else{ set_pages_array_wb(sgbuf->page_table, sgbuf->pages); +prot = PAGE_KERNEL; +} +for (i = 0; i < sgbuf->pages; i ++) +{ +addr = (unsigned long)dmab->area + PAGE_SIZE * i; +pte = dump_pte(addr); +if (pte) +set_pte_at(&init_mm, addr, pte, mk_pte(sgbuf->page_table[i], prot)); +else WARN_ON(1); +} return; } #endif
保密声明： 本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。 CONFIDENTIAL NOTE: This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited.
On Wed, 08 Aug 2018 11:54:57 +0200, Hans Hu(SH-RD) wrote:
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
Not really what I meant. Rather something like below (totally untested).
[Hans :] I know what you mean now and complete code like below, but the result is still noise.
OK, so indeed the vmapped address does seem matter. Interesting. What kind of user access does produce the noise? Does it via aplay mmap mode, too?
In anyway, if the vmap is a problem, it might be worked around a patch like below (again totally untested and not sure whether it's correct).
[Hans :] As I know, In the hardware layer HDAC’s stream have two data transport path : non-snoop & snoop; In the software layer ALSA-Driver have two data transport path : mmap & not mmap(test shows, it is dependent on wav's format or mmap_flag in aplay.c). When hardware at non-snoop mode, without hardware module's help, software must mark the mem to uncacheable type: when mmap used, the mark action happened at pcm_mmap_prepare(), the not-mmap mode's mark action happened at __mark_pages_wc(). And when at not-mmap mode, the vmapped address directly used in snd_pcm_lib_write_transfer() -> copy_from_user().
Well, that's usually no problem regarding that cache coherency. At least it hasn't been any issue with Intel and AMD CPUs. Does the problem happen with Intel CPU instead of VIA?
Yes, of course, this path combinations(non-snoop + not-mmap) unlikely be used by most music players.
[Hans :] I complete the patch in your last mail like below, not used the vunmap & re-vmap way(it has a little problem). Result is OK now(no noise anymore).
What problem did you get with vunmap / vmap?
Takashi
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
Not really what I meant. Rather something like below (totally untested).
[Hans :] I know what you mean now and complete code like below, but the result is still noise.
OK, so indeed the vmapped address does seem matter. Interesting. What kind of user access does produce the noise? Does it via aplay mmap mode, too?
In anyway, if the vmap is a problem, it might be worked around a patch like below (again totally untested and not sure whether it's correct).
[Hans :] As I know, In the hardware layer HDAC’s stream have two data transport path : non-snoop & snoop; In the software layer ALSA-Driver have two data transport path : mmap & not mmap(test shows, it is dependent on wav's format or mmap_flag in aplay.c). When hardware at non-snoop mode, without hardware module's help, software must mark the mem to uncacheable type: when mmap used, the mark action happened at pcm_mmap_prepare(), the not-mmap mode's mark action happened at __mark_pages_wc(). And when at not-mmap mode, the vmapped address directly used in snd_pcm_lib_write_transfer() -> copy_from_user().
Well, that's usually no problem regarding that cache coherency. At least it hasn't been any issue with Intel and AMD CPUs. Does the problem happen with Intel CPU instead of VIA?
[Hans :] I don't have enough Intel/AMD machine here, up to now, only find one Intel's : HW: mother board is Dell 042P49, HDA controller is 8086:1c20, codec is cx20641.
Yes, of course, this path combinations(non-snoop + not-mmap) unlikely be used by most music players.
[Hans :] I complete the patch in your last mail like below, not used the vunmap & re-vmap way(it has a little problem). Result is OK now(no noise anymore).
What problem did you get with vunmap / vmap?
[Hans :] if patch like below, even the new vmapped address new_area be assigned to dmab->area, it will not use this time, but next. This time used dmab->area still the last one, but the old one had been vunmap, so Oops reported.
+new_area = vmap(sgbuf->page_table, sgbuf->pages, VM_MAP, prot); +if (WARN_ON(!new_area)) +return; +vunmap(dmab->area); +dmab->area = new_area;
Thanks,
Hans
保密声明： 本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。 CONFIDENTIAL NOTE: This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited.
On Wed, 08 Aug 2018 12:44:00 +0200, Hans Hu(SH-RD) wrote:
Then the next step would be to fake sg-buffer from this straight buffer. Revert the above, and modify sgbuf.c to the following:
- Allocate a large continuous buffer
- Assign each page in this large buffer
If this still works, it's not about vmap, but it just means that the physically ordered pages do matter -- implicitly showing that the snooping behavior isn't properly turned on / off on the controller.
To fake SG-buffer, I did this test: restore all the codes to the original, then added some codes in snd_malloc_sgbuf_pages() like below, the result is badly niose.
Not really what I meant. Rather something like below (totally untested).
[Hans :] I know what you mean now and complete code like below, but the result is still noise.
OK, so indeed the vmapped address does seem matter. Interesting. What kind of user access does produce the noise? Does it via aplay mmap mode, too?
In anyway, if the vmap is a problem, it might be worked around a patch like below (again totally untested and not sure whether it's correct).
[Hans :] As I know, In the hardware layer HDAC’s stream have two data transport path : non-snoop & snoop; In the software layer ALSA-Driver have two data transport path : mmap & not mmap(test shows, it is dependent on wav's format or mmap_flag in aplay.c). When hardware at non-snoop mode, without hardware module's help, software must mark the mem to uncacheable type: when mmap used, the mark action happened at pcm_mmap_prepare(), the not-mmap mode's mark action happened at __mark_pages_wc(). And when at not-mmap mode, the vmapped address directly used in snd_pcm_lib_write_transfer() -> copy_from_user().
Well, that's usually no problem regarding that cache coherency. At least it hasn't been any issue with Intel and AMD CPUs. Does the problem happen with Intel CPU instead of VIA?
[Hans :] I don't have enough Intel/AMD machine here, up to now, only find one Intel's : HW: mother board is Dell 042P49, HDA controller is 8086:1c20, codec is cx20641.
And the same issue is seen on that machine?
Yes, of course, this path combinations(non-snoop + not-mmap) unlikely be used by most music players.
[Hans :] I complete the patch in your last mail like below, not used the vunmap & re-vmap way(it has a little problem). Result is OK now(no noise anymore).
What problem did you get with vunmap / vmap?
[Hans :] if patch like below, even the new vmapped address new_area be assigned to dmab->area, it will not use this time, but next. This time used dmab->area still the last one, but the old one had been vunmap, so Oops reported.
That's bad. But the problem isn't as you guessed, I guess. The allocator re-uses the pre-allocated buffer if available, and this makes things inconsistent. I'll cook later.
Takashi
participants (2)
-
Hans Hu(SH-RD) -
Takashi Iwai