| Message ID | 1548057848-15136-1-git-send-email-rppt@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Refine memblock API | expand |
On Mon, Jan 21, 2019 at 9:06 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > Add check for the return value of memblock_alloc*() functions and call > panic() in case of error. > The panic message repeats the one used by panicing memblock allocators with > adjustment of parameters to include only relevant ones. > > The replacement was mostly automated with semantic patches like the one > below with manual massaging of format strings. > > @@ > expression ptr, size, align; > @@ > ptr = memblock_alloc(size, align); > + if (!ptr) > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > size, align); > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > arch/m68k/atari/stram.c | 4 ++++ > arch/m68k/mm/init.c | 3 +++ > arch/m68k/mm/mcfmmu.c | 6 ++++++ > arch/m68k/mm/motorola.c | 9 +++++++++ > arch/m68k/mm/sun3mmu.c | 6 ++++++ > arch/m68k/sun3/sun3dvma.c | 3 +++ For m68k: Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert
On Mon, Jan 21, 2019 at 10:04:06AM +0200, Mike Rapoport wrote: > Add check for the return value of memblock_alloc*() functions and call > panic() in case of error. > The panic message repeats the one used by panicing memblock allocators with > adjustment of parameters to include only relevant ones. > > The replacement was mostly automated with semantic patches like the one > below with manual massaging of format strings. > > @@ > expression ptr, size, align; > @@ > ptr = memblock_alloc(size, align); > + if (!ptr) > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > size, align); > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > Reviewed-by: Guo Ren <ren_guo@c-sky.com> # c-sky > Acked-by: Paul Burton <paul.burton@mips.com> # MIPS > Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com> # s390 > Reviewed-by: Juergen Gross <jgross@suse.com> # Xen > --- > arch/alpha/kernel/core_cia.c | 3 +++ > arch/alpha/kernel/core_marvel.c | 6 ++++++ > arch/alpha/kernel/pci-noop.c | 13 +++++++++++-- > arch/alpha/kernel/pci.c | 11 ++++++++++- > arch/alpha/kernel/pci_iommu.c | 12 ++++++++++++ > arch/arc/mm/highmem.c | 4 ++++ > arch/arm/kernel/setup.c | 6 ++++++ > arch/arm/mm/mmu.c | 14 +++++++++++++- > arch/arm64/kernel/setup.c | 8 +++++--- > arch/arm64/mm/kasan_init.c | 10 ++++++++++ > arch/c6x/mm/dma-coherent.c | 4 ++++ > arch/c6x/mm/init.c | 3 +++ > arch/csky/mm/highmem.c | 5 +++++ > arch/h8300/mm/init.c | 3 +++ > arch/m68k/atari/stram.c | 4 ++++ > arch/m68k/mm/init.c | 3 +++ > arch/m68k/mm/mcfmmu.c | 6 ++++++ > arch/m68k/mm/motorola.c | 9 +++++++++ > arch/m68k/mm/sun3mmu.c | 6 ++++++ > arch/m68k/sun3/sun3dvma.c | 3 +++ > arch/microblaze/mm/init.c | 8 ++++++-- > arch/mips/cavium-octeon/dma-octeon.c | 3 +++ > arch/mips/kernel/setup.c | 3 +++ > arch/mips/kernel/traps.c | 3 +++ > arch/mips/mm/init.c | 5 +++++ > arch/nds32/mm/init.c | 12 ++++++++++++ > arch/openrisc/mm/ioremap.c | 8 ++++++-- > arch/powerpc/kernel/dt_cpu_ftrs.c | 5 +++++ > arch/powerpc/kernel/pci_32.c | 3 +++ > arch/powerpc/kernel/setup-common.c | 3 +++ > arch/powerpc/kernel/setup_64.c | 4 ++++ > arch/powerpc/lib/alloc.c | 3 +++ > arch/powerpc/mm/hash_utils_64.c | 3 +++ > arch/powerpc/mm/mmu_context_nohash.c | 9 +++++++++ > arch/powerpc/mm/pgtable-book3e.c | 12 ++++++++++-- > arch/powerpc/mm/pgtable-book3s64.c | 3 +++ > arch/powerpc/mm/pgtable-radix.c | 9 ++++++++- > arch/powerpc/mm/ppc_mmu_32.c | 3 +++ > arch/powerpc/platforms/pasemi/iommu.c | 3 +++ > arch/powerpc/platforms/powermac/nvram.c | 3 +++ > arch/powerpc/platforms/powernv/opal.c | 3 +++ > arch/powerpc/platforms/powernv/pci-ioda.c | 8 ++++++++ > arch/powerpc/platforms/ps3/setup.c | 3 +++ > arch/powerpc/sysdev/msi_bitmap.c | 3 +++ > arch/s390/kernel/setup.c | 13 +++++++++++++ > arch/s390/kernel/smp.c | 5 ++++- > arch/s390/kernel/topology.c | 6 ++++++ > arch/s390/numa/mode_emu.c | 3 +++ > arch/s390/numa/numa.c | 6 +++++- > arch/sh/mm/init.c | 6 ++++++ > arch/sh/mm/numa.c | 4 ++++ > arch/um/drivers/net_kern.c | 3 +++ > arch/um/drivers/vector_kern.c | 3 +++ > arch/um/kernel/initrd.c | 2 ++ > arch/um/kernel/mem.c | 16 ++++++++++++++++ > arch/unicore32/kernel/setup.c | 4 ++++ > arch/unicore32/mm/mmu.c | 15 +++++++++++++-- > arch/x86/kernel/acpi/boot.c | 3 +++ > arch/x86/kernel/apic/io_apic.c | 5 +++++ > arch/x86/kernel/e820.c | 3 +++ > arch/x86/platform/olpc/olpc_dt.c | 3 +++ > arch/x86/xen/p2m.c | 11 +++++++++-- > arch/xtensa/mm/kasan_init.c | 4 ++++ > arch/xtensa/mm/mmu.c | 3 +++ > drivers/clk/ti/clk.c | 3 +++ > drivers/macintosh/smu.c | 3 +++ > drivers/of/fdt.c | 8 +++++++- > drivers/of/unittest.c | 8 +++++++- Acked-by: Rob Herring <robh@kernel.org> > drivers/xen/swiotlb-xen.c | 7 +++++-- > kernel/power/snapshot.c | 3 +++ > lib/cpumask.c | 3 +++ > mm/kasan/init.c | 10 ++++++++-- > mm/sparse.c | 19 +++++++++++++++++-- > 73 files changed, 409 insertions(+), 28 deletions(-)
Le 21/01/2019 à 09:04, Mike Rapoport a écrit : > Add check for the return value of memblock_alloc*() functions and call > panic() in case of error. > The panic message repeats the one used by panicing memblock allocators with > adjustment of parameters to include only relevant ones. > > The replacement was mostly automated with semantic patches like the one > below with manual massaging of format strings. > > @@ > expression ptr, size, align; > @@ > ptr = memblock_alloc(size, align); > + if (!ptr) > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > size, align); > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > Reviewed-by: Guo Ren <ren_guo@c-sky.com> # c-sky > Acked-by: Paul Burton <paul.burton@mips.com> # MIPS > Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com> # s390 > Reviewed-by: Juergen Gross <jgross@suse.com> # Xen > --- [...] > diff --git a/mm/sparse.c b/mm/sparse.c > index 7ea5dc6..ad94242 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c [...] > @@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long size, int nid) > memblock_alloc_try_nid_raw(size, PAGE_SIZE, > __pa(MAX_DMA_ADDRESS), > MEMBLOCK_ALLOC_ACCESSIBLE, nid); > + if (!sparsemap_buf) > + panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d from=%lx\n", > + __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS)); > + memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does not zero allocated memory, does not panic if request cannot be satisfied.). Stephen Rothwell reports a boot failure due to this change. Christophe > sparsemap_buf_end = sparsemap_buf + size; > } > >
On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote: > > > Le 21/01/2019 à 09:04, Mike Rapoport a écrit : > >Add check for the return value of memblock_alloc*() functions and call > >panic() in case of error. > >The panic message repeats the one used by panicing memblock allocators with > >adjustment of parameters to include only relevant ones. > > > >The replacement was mostly automated with semantic patches like the one > >below with manual massaging of format strings. > > > >@@ > >expression ptr, size, align; > >@@ > >ptr = memblock_alloc(size, align); > >+ if (!ptr) > >+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > >size, align); > > > >Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > >Reviewed-by: Guo Ren <ren_guo@c-sky.com> # c-sky > >Acked-by: Paul Burton <paul.burton@mips.com> # MIPS > >Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com> # s390 > >Reviewed-by: Juergen Gross <jgross@suse.com> # Xen > >--- > > [...] > > >diff --git a/mm/sparse.c b/mm/sparse.c > >index 7ea5dc6..ad94242 100644 > >--- a/mm/sparse.c > >+++ b/mm/sparse.c > > [...] > > >@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long size, int nid) > > memblock_alloc_try_nid_raw(size, PAGE_SIZE, > > __pa(MAX_DMA_ADDRESS), > > MEMBLOCK_ALLOC_ACCESSIBLE, nid); > >+ if (!sparsemap_buf) > >+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d from=%lx\n", > >+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS)); > >+ > > memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does not > zero allocated memory, does not panic if request cannot be satisfied.). "Does not panic" does not mean it always succeeds. > Stephen Rothwell reports a boot failure due to this change. Please see my reply on that thread. > Christophe > > > sparsemap_buf_end = sparsemap_buf + size; > > } > > >
Le 31/01/2019 à 07:41, Mike Rapoport a écrit : > On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote: >> >> >> Le 21/01/2019 à 09:04, Mike Rapoport a écrit : >>> Add check for the return value of memblock_alloc*() functions and call >>> panic() in case of error. >>> The panic message repeats the one used by panicing memblock allocators with >>> adjustment of parameters to include only relevant ones. >>> >>> The replacement was mostly automated with semantic patches like the one >>> below with manual massaging of format strings. >>> >>> @@ >>> expression ptr, size, align; >>> @@ >>> ptr = memblock_alloc(size, align); >>> + if (!ptr) >>> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, >>> size, align); >>> >>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> >>> Reviewed-by: Guo Ren <ren_guo@c-sky.com> # c-sky >>> Acked-by: Paul Burton <paul.burton@mips.com> # MIPS >>> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com> # s390 >>> Reviewed-by: Juergen Gross <jgross@suse.com> # Xen >>> --- >> >> [...] >> >>> diff --git a/mm/sparse.c b/mm/sparse.c >>> index 7ea5dc6..ad94242 100644 >>> --- a/mm/sparse.c >>> +++ b/mm/sparse.c >> >> [...] >> >>> @@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned long size, int nid) >>> memblock_alloc_try_nid_raw(size, PAGE_SIZE, >>> __pa(MAX_DMA_ADDRESS), >>> MEMBLOCK_ALLOC_ACCESSIBLE, nid); >>> + if (!sparsemap_buf) >>> + panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d from=%lx\n", >>> + __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS)); >>> + >> >> memblock_alloc_try_nid_raw() does not panic (help explicitly says: Does not >> zero allocated memory, does not panic if request cannot be satisfied.). > > "Does not panic" does not mean it always succeeds. I agree, but at least here you are changing the behaviour by making it panic explicitly. Are we sure there are not cases where the system could just continue functionning ? Maybe a WARN_ON() would be enough there ? Christophe > >> Stephen Rothwell reports a boot failure due to this change. > > Please see my reply on that thread. > >> Christophe >> >>> sparsemap_buf_end = sparsemap_buf + size; >>> } >>> >> >
Le 31/01/2019 à 07:44, Christophe Leroy a écrit : > > > Le 31/01/2019 à 07:41, Mike Rapoport a écrit : >> On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote: >>> >>> >>> Le 21/01/2019 à 09:04, Mike Rapoport a écrit : >>>> Add check for the return value of memblock_alloc*() functions and call >>>> panic() in case of error. >>>> The panic message repeats the one used by panicing memblock >>>> allocators with >>>> adjustment of parameters to include only relevant ones. >>>> >>>> The replacement was mostly automated with semantic patches like the one >>>> below with manual massaging of format strings. >>>> >>>> @@ >>>> expression ptr, size, align; >>>> @@ >>>> ptr = memblock_alloc(size, align); >>>> + if (!ptr) >>>> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, >>>> size, align); >>>> >>>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> >>>> Reviewed-by: Guo Ren <ren_guo@c-sky.com> # c-sky >>>> Acked-by: Paul Burton <paul.burton@mips.com> # MIPS >>>> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com> # s390 >>>> Reviewed-by: Juergen Gross <jgross@suse.com> # Xen >>>> --- >>> >>> [...] >>> >>>> diff --git a/mm/sparse.c b/mm/sparse.c >>>> index 7ea5dc6..ad94242 100644 >>>> --- a/mm/sparse.c >>>> +++ b/mm/sparse.c >>> >>> [...] >>> >>>> @@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned >>>> long size, int nid) >>>> memblock_alloc_try_nid_raw(size, PAGE_SIZE, >>>> __pa(MAX_DMA_ADDRESS), >>>> MEMBLOCK_ALLOC_ACCESSIBLE, nid); >>>> + if (!sparsemap_buf) >>>> + panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d >>>> from=%lx\n", >>>> + __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS)); >>>> + >>> >>> memblock_alloc_try_nid_raw() does not panic (help explicitly says: >>> Does not >>> zero allocated memory, does not panic if request cannot be satisfied.). >> >> "Does not panic" does not mean it always succeeds. > > I agree, but at least here you are changing the behaviour by making it > panic explicitly. Are we sure there are not cases where the system could > just continue functionning ? Maybe a WARN_ON() would be enough there ? Looking more in details, it looks like everything is done to live with sparsemap_buf NULL, all functions using it check it so having it NULL shouldn't imply a panic I believe, see code below. static void *sparsemap_buf __meminitdata; static void *sparsemap_buf_end __meminitdata; static void __init sparse_buffer_init(unsigned long size, int nid) { WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */ sparsemap_buf = memblock_alloc_try_nid_raw(size, PAGE_SIZE, __pa(MAX_DMA_ADDRESS), MEMBLOCK_ALLOC_ACCESSIBLE, nid); sparsemap_buf_end = sparsemap_buf + size; } static void __init sparse_buffer_fini(void) { unsigned long size = sparsemap_buf_end - sparsemap_buf; if (sparsemap_buf && size > 0) memblock_free_early(__pa(sparsemap_buf), size); sparsemap_buf = NULL; } void * __meminit sparse_buffer_alloc(unsigned long size) { void *ptr = NULL; if (sparsemap_buf) { ptr = PTR_ALIGN(sparsemap_buf, size); if (ptr + size > sparsemap_buf_end) ptr = NULL; else sparsemap_buf = ptr + size; } return ptr; } Christophe
On Thu, Jan 31, 2019 at 08:07:29AM +0100, Christophe Leroy wrote: > > > Le 31/01/2019 à 07:44, Christophe Leroy a écrit : > > > > > >Le 31/01/2019 à 07:41, Mike Rapoport a écrit : > >>On Thu, Jan 31, 2019 at 07:07:46AM +0100, Christophe Leroy wrote: > >>> > >>> > >>>Le 21/01/2019 à 09:04, Mike Rapoport a écrit : > >>>>Add check for the return value of memblock_alloc*() functions and call > >>>>panic() in case of error. > >>>>The panic message repeats the one used by panicing memblock > >>>>allocators with > >>>>adjustment of parameters to include only relevant ones. > >>>> > >>>>The replacement was mostly automated with semantic patches like the one > >>>>below with manual massaging of format strings. > >>>> > >>>>@@ > >>>>expression ptr, size, align; > >>>>@@ > >>>>ptr = memblock_alloc(size, align); > >>>>+ if (!ptr) > >>>>+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > >>>>size, align); > >>>> > >>>>Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > >>>>Reviewed-by: Guo Ren <ren_guo@c-sky.com> # c-sky > >>>>Acked-by: Paul Burton <paul.burton@mips.com> # MIPS > >>>>Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com> # s390 > >>>>Reviewed-by: Juergen Gross <jgross@suse.com> # Xen > >>>>--- > >>> > >>>[...] > >>> > >>>>diff --git a/mm/sparse.c b/mm/sparse.c > >>>>index 7ea5dc6..ad94242 100644 > >>>>--- a/mm/sparse.c > >>>>+++ b/mm/sparse.c > >>> > >>>[...] > >>> > >>>>@@ -425,6 +436,10 @@ static void __init sparse_buffer_init(unsigned > >>>>long size, int nid) > >>>> memblock_alloc_try_nid_raw(size, PAGE_SIZE, > >>>> __pa(MAX_DMA_ADDRESS), > >>>> MEMBLOCK_ALLOC_ACCESSIBLE, nid); > >>>>+ if (!sparsemap_buf) > >>>>+ panic("%s: Failed to allocate %lu bytes align=0x%lx nid=%d > >>>>from=%lx\n", > >>>>+ __func__, size, PAGE_SIZE, nid, __pa(MAX_DMA_ADDRESS)); > >>>>+ > >>> > >>>memblock_alloc_try_nid_raw() does not panic (help explicitly says: > >>>Does not > >>>zero allocated memory, does not panic if request cannot be satisfied.). > >> > >>"Does not panic" does not mean it always succeeds. > > > >I agree, but at least here you are changing the behaviour by making it > >panic explicitly. Are we sure there are not cases where the system could > >just continue functionning ? Maybe a WARN_ON() would be enough there ? > > Looking more in details, it looks like everything is done to live with > sparsemap_buf NULL, all functions using it check it so having it NULL > shouldn't imply a panic I believe, see code below. You are right, I'm preparing the fix right now. > static void *sparsemap_buf __meminitdata; > static void *sparsemap_buf_end __meminitdata; > > static void __init sparse_buffer_init(unsigned long size, int nid) > { > WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */ > sparsemap_buf = > memblock_alloc_try_nid_raw(size, PAGE_SIZE, > __pa(MAX_DMA_ADDRESS), > MEMBLOCK_ALLOC_ACCESSIBLE, nid); > sparsemap_buf_end = sparsemap_buf + size; > } > > static void __init sparse_buffer_fini(void) > { > unsigned long size = sparsemap_buf_end - sparsemap_buf; > > if (sparsemap_buf && size > 0) > memblock_free_early(__pa(sparsemap_buf), size); > sparsemap_buf = NULL; > } > > void * __meminit sparse_buffer_alloc(unsigned long size) > { > void *ptr = NULL; > > if (sparsemap_buf) { > ptr = PTR_ALIGN(sparsemap_buf, size); > if (ptr + size > sparsemap_buf_end) > ptr = NULL; > else > sparsemap_buf = ptr + size; > } > return ptr; > } > > > Christophe >
On Mon, Jan 21, 2019 at 2:05 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > Hi, > > Current memblock API is quite extensive and, which is more annoying, > duplicated. Except the low-level functions that allow searching for a free > memory region and marking it as reserved, memblock provides three (well, > two and a half) sets of functions to allocate memory. There are several > overlapping functions that return a physical address and there are > functions that return virtual address. Those that return the virtual > address may also clear the allocated memory. And, on top of all that, some > allocators panic and some return NULL in case of error. > > This set tries to reduce the mess, and trim down the amount of memblock > allocation methods. > > Patches 1-10 consolidate the functions that return physical address of > the allocated memory > > Patches 11-13 are some trivial cleanups > > Patches 14-19 add checks for the return value of memblock_alloc*() and > panics in case of errors. The patches 14-18 include some minor refactoring > to have better readability of the resulting code and patch 19 is a > mechanical addition of > > if (!ptr) > panic(); > > after memblock_alloc*() calls. > > And, finally, patches 20 and 21 remove panic() calls memblock and _nopanic > variants from memblock. > > v2 changes: > * replace some more %lu with %zu > * remove panics where they are not needed in s390 and in printk > * collect Acked-by and Reviewed-by. > > > Christophe Leroy (1): > powerpc: use memblock functions returning virtual address > > Mike Rapoport (20): > openrisc: prefer memblock APIs returning virtual address > memblock: replace memblock_alloc_base(ANYWHERE) with memblock_phys_alloc > memblock: drop memblock_alloc_base_nid() > memblock: emphasize that memblock_alloc_range() returns a physical address > memblock: memblock_phys_alloc_try_nid(): don't panic > memblock: memblock_phys_alloc(): don't panic > memblock: drop __memblock_alloc_base() > memblock: drop memblock_alloc_base() > memblock: refactor internal allocation functions > memblock: make memblock_find_in_range_node() and choose_memblock_flags() static > arch: use memblock_alloc() instead of memblock_alloc_from(size, align, 0) > arch: don't memset(0) memory returned by memblock_alloc() > ia64: add checks for the return value of memblock_alloc*() > sparc: add checks for the return value of memblock_alloc*() > mm/percpu: add checks for the return value of memblock_alloc*() > init/main: add checks for the return value of memblock_alloc*() > swiotlb: add checks for the return value of memblock_alloc*() > treewide: add checks for the return value of memblock_alloc*() > memblock: memblock_alloc_try_nid: don't panic > memblock: drop memblock_alloc_*_nopanic() variants > I know it's rather late, but this patch broke the Etnaviv 3D graphics in my i.MX6Q. When I try to use the 3D, it returns some errors and the dmesg log shows some memory allocation errors too: [ 3.682347] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) [ 3.688669] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) [ 3.695099] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) [ 3.700800] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 [ 3.723013] etnaviv-gpu 130000.gpu: command buffer outside valid memory window [ 3.731308] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 [ 3.752437] etnaviv-gpu 134000.gpu: command buffer outside valid memory window [ 3.760583] etnaviv-gpu 2204000.gpu: model: GC355, revision: 1215 [ 3.766766] etnaviv-gpu 2204000.gpu: Ignoring GPU with VG and FE2.0 [ 3.776131] [drm] Initialized etnaviv 1.2.0 20151214 for etnaviv on minor 0 # glmark2-es2-drm Error creating gpu Error: eglCreateWindowSurface failed with error: 0x3009 Error: eglCreateWindowSurface failed with error: 0x3009 Error: CanvasGeneric: Invalid EGL state Error: main: Could not initialize canvas Before this patch: [ 3.691995] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) [ 3.698356] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) [ 3.704792] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) [ 3.710488] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 [ 3.733649] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 [ 3.756115] etnaviv-gpu 2204000.gpu: model: GC355, revision: 1215 [ 3.762250] etnaviv-gpu 2204000.gpu: Ignoring GPU with VG and FE2.0 [ 3.771432] [drm] Initialized etnaviv 1.2.0 20151214 for etnaviv on minor 0 and the 3D gemos work without this. I don't know enough about the i.MX6 nor the 3D accelerator to know how to fix it. I am hoping someone in the know might have some suggestions. > arch/alpha/kernel/core_cia.c | 5 +- > arch/alpha/kernel/core_marvel.c | 6 + > arch/alpha/kernel/pci-noop.c | 13 +- > arch/alpha/kernel/pci.c | 11 +- > arch/alpha/kernel/pci_iommu.c | 16 +- > arch/alpha/kernel/setup.c | 2 +- > arch/arc/kernel/unwind.c | 3 +- > arch/arc/mm/highmem.c | 4 + > arch/arm/kernel/setup.c | 6 + > arch/arm/mm/init.c | 6 +- > arch/arm/mm/mmu.c | 14 +- > arch/arm64/kernel/setup.c | 8 +- > arch/arm64/mm/kasan_init.c | 10 ++ > arch/arm64/mm/mmu.c | 2 + > arch/arm64/mm/numa.c | 4 + > arch/c6x/mm/dma-coherent.c | 4 + > arch/c6x/mm/init.c | 4 +- > arch/csky/mm/highmem.c | 5 + > arch/h8300/mm/init.c | 4 +- > arch/ia64/kernel/mca.c | 25 +-- > arch/ia64/mm/contig.c | 8 +- > arch/ia64/mm/discontig.c | 4 + > arch/ia64/mm/init.c | 38 ++++- > arch/ia64/mm/tlb.c | 6 + > arch/ia64/sn/kernel/io_common.c | 3 + > arch/ia64/sn/kernel/setup.c | 12 +- > arch/m68k/atari/stram.c | 4 + > arch/m68k/mm/init.c | 3 + > arch/m68k/mm/mcfmmu.c | 7 +- > arch/m68k/mm/motorola.c | 9 ++ > arch/m68k/mm/sun3mmu.c | 6 + > arch/m68k/sun3/sun3dvma.c | 3 + > arch/microblaze/mm/init.c | 10 +- > arch/mips/cavium-octeon/dma-octeon.c | 3 + > arch/mips/kernel/setup.c | 3 + > arch/mips/kernel/traps.c | 5 +- > arch/mips/mm/init.c | 5 + > arch/nds32/mm/init.c | 12 ++ > arch/openrisc/mm/init.c | 5 +- > arch/openrisc/mm/ioremap.c | 8 +- > arch/powerpc/kernel/dt_cpu_ftrs.c | 8 +- > arch/powerpc/kernel/irq.c | 5 - > arch/powerpc/kernel/paca.c | 6 +- > arch/powerpc/kernel/pci_32.c | 3 + > arch/powerpc/kernel/prom.c | 5 +- > arch/powerpc/kernel/rtas.c | 6 +- > arch/powerpc/kernel/setup-common.c | 3 + > arch/powerpc/kernel/setup_32.c | 26 ++-- > arch/powerpc/kernel/setup_64.c | 4 + > arch/powerpc/lib/alloc.c | 3 + > arch/powerpc/mm/hash_utils_64.c | 11 +- > arch/powerpc/mm/mmu_context_nohash.c | 9 ++ > arch/powerpc/mm/numa.c | 4 + > arch/powerpc/mm/pgtable-book3e.c | 12 +- > arch/powerpc/mm/pgtable-book3s64.c | 3 + > arch/powerpc/mm/pgtable-radix.c | 9 +- > arch/powerpc/mm/ppc_mmu_32.c | 3 + > arch/powerpc/platforms/pasemi/iommu.c | 3 + > arch/powerpc/platforms/powermac/nvram.c | 3 + > arch/powerpc/platforms/powernv/opal.c | 3 + > arch/powerpc/platforms/powernv/pci-ioda.c | 8 + > arch/powerpc/platforms/ps3/setup.c | 3 + > arch/powerpc/sysdev/dart_iommu.c | 3 + > arch/powerpc/sysdev/msi_bitmap.c | 3 + > arch/s390/kernel/crash_dump.c | 3 + > arch/s390/kernel/setup.c | 16 ++ > arch/s390/kernel/smp.c | 9 +- > arch/s390/kernel/topology.c | 6 + > arch/s390/numa/mode_emu.c | 3 + > arch/s390/numa/numa.c | 6 +- > arch/sh/boards/mach-ap325rxa/setup.c | 5 +- > arch/sh/boards/mach-ecovec24/setup.c | 10 +- > arch/sh/boards/mach-kfr2r09/setup.c | 5 +- > arch/sh/boards/mach-migor/setup.c | 5 +- > arch/sh/boards/mach-se/7724/setup.c | 10 +- > arch/sh/kernel/machine_kexec.c | 3 +- > arch/sh/mm/init.c | 8 +- > arch/sh/mm/numa.c | 4 + > arch/sparc/kernel/prom_32.c | 6 +- > arch/sparc/kernel/setup_64.c | 6 + > arch/sparc/kernel/smp_64.c | 12 ++ > arch/sparc/mm/init_32.c | 2 +- > arch/sparc/mm/init_64.c | 11 ++ > arch/sparc/mm/srmmu.c | 18 ++- > arch/um/drivers/net_kern.c | 3 + > arch/um/drivers/vector_kern.c | 3 + > arch/um/kernel/initrd.c | 2 + > arch/um/kernel/mem.c | 16 ++ > arch/unicore32/kernel/setup.c | 4 + > arch/unicore32/mm/mmu.c | 15 +- > arch/x86/kernel/acpi/boot.c | 3 + > arch/x86/kernel/apic/io_apic.c | 5 + > arch/x86/kernel/e820.c | 5 +- > arch/x86/kernel/setup_percpu.c | 10 +- > arch/x86/mm/kasan_init_64.c | 14 +- > arch/x86/mm/numa.c | 12 +- > arch/x86/platform/olpc/olpc_dt.c | 3 + > arch/x86/xen/p2m.c | 11 +- > arch/xtensa/mm/kasan_init.c | 10 +- > arch/xtensa/mm/mmu.c | 3 + > drivers/clk/ti/clk.c | 3 + > drivers/firmware/memmap.c | 2 +- > drivers/macintosh/smu.c | 5 +- > drivers/of/fdt.c | 8 +- > drivers/of/of_reserved_mem.c | 7 +- > drivers/of/unittest.c | 8 +- > drivers/usb/early/xhci-dbc.c | 2 +- > drivers/xen/swiotlb-xen.c | 7 +- > include/linux/memblock.h | 59 +------ > init/main.c | 26 +++- > kernel/dma/swiotlb.c | 21 ++- > kernel/power/snapshot.c | 3 + > kernel/printk/printk.c | 9 +- > lib/cpumask.c | 3 + > mm/cma.c | 10 +- > mm/kasan/init.c | 10 +- > mm/memblock.c | 249 ++++++++++-------------------- > mm/page_alloc.c | 10 +- > mm/page_ext.c | 2 +- > mm/percpu.c | 84 +++++++--- > mm/sparse.c | 25 ++- > 121 files changed, 860 insertions(+), 412 deletions(-) > > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
(updated CC) Hi, On Tue, Sep 24, 2019 at 12:52:35PM -0500, Adam Ford wrote: > On Mon, Jan 21, 2019 at 2:05 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > Hi, > > > > v2 changes: > > * replace some more %lu with %zu > > * remove panics where they are not needed in s390 and in printk > > * collect Acked-by and Reviewed-by. > > > > > > Christophe Leroy (1): > > powerpc: use memblock functions returning virtual address > > > > Mike Rapoport (20): > > openrisc: prefer memblock APIs returning virtual address > > memblock: replace memblock_alloc_base(ANYWHERE) with memblock_phys_alloc > > memblock: drop memblock_alloc_base_nid() > > memblock: emphasize that memblock_alloc_range() returns a physical address > > memblock: memblock_phys_alloc_try_nid(): don't panic > > memblock: memblock_phys_alloc(): don't panic > > memblock: drop __memblock_alloc_base() > > memblock: drop memblock_alloc_base() > > memblock: refactor internal allocation functions > > memblock: make memblock_find_in_range_node() and choose_memblock_flags() static > > arch: use memblock_alloc() instead of memblock_alloc_from(size, align, 0) > > arch: don't memset(0) memory returned by memblock_alloc() > > ia64: add checks for the return value of memblock_alloc*() > > sparc: add checks for the return value of memblock_alloc*() > > mm/percpu: add checks for the return value of memblock_alloc*() > > init/main: add checks for the return value of memblock_alloc*() > > swiotlb: add checks for the return value of memblock_alloc*() > > treewide: add checks for the return value of memblock_alloc*() > > memblock: memblock_alloc_try_nid: don't panic > > memblock: drop memblock_alloc_*_nopanic() variants > > > I know it's rather late, but this patch broke the Etnaviv 3D graphics > in my i.MX6Q. Can you identify the exact patch from the series that caused the regression? > When I try to use the 3D, it returns some errors and the dmesg log > shows some memory allocation errors too: > [ 3.682347] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) > [ 3.688669] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) > [ 3.695099] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) > [ 3.700800] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 > [ 3.723013] etnaviv-gpu 130000.gpu: command buffer outside valid > memory window > [ 3.731308] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 > [ 3.752437] etnaviv-gpu 134000.gpu: command buffer outside valid > memory window > [ 3.760583] etnaviv-gpu 2204000.gpu: model: GC355, revision: 1215 > [ 3.766766] etnaviv-gpu 2204000.gpu: Ignoring GPU with VG and FE2.0 > [ 3.776131] [drm] Initialized etnaviv 1.2.0 20151214 for etnaviv on minor 0 > > # glmark2-es2-drm > Error creating gpu > Error: eglCreateWindowSurface failed with error: 0x3009 > Error: eglCreateWindowSurface failed with error: 0x3009 > Error: CanvasGeneric: Invalid EGL state > Error: main: Could not initialize canvas > > > Before this patch: > > [ 3.691995] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) > [ 3.698356] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) > [ 3.704792] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) > [ 3.710488] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 > [ 3.733649] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 > [ 3.756115] etnaviv-gpu 2204000.gpu: model: GC355, revision: 1215 > [ 3.762250] etnaviv-gpu 2204000.gpu: Ignoring GPU with VG and FE2.0 > [ 3.771432] [drm] Initialized etnaviv 1.2.0 20151214 for etnaviv on minor 0 > > and the 3D gemos work without this. > > I don't know enough about the i.MX6 nor the 3D accelerator to know how > to fix it. > I am hoping someone in the know might have some suggestions. Can you please add "memblock=debug" to your kernel command line and send kernel logs for both working and failing versions?
Hi Adam, On Wed, Sep 25, 2019 at 6:38 AM Adam Ford <aford173@gmail.com> wrote: > I know it's rather late, but this patch broke the Etnaviv 3D graphics > in my i.MX6Q. > > When I try to use the 3D, it returns some errors and the dmesg log > shows some memory allocation errors too: > [ 3.682347] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) > [ 3.688669] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) > [ 3.695099] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) > [ 3.700800] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 > [ 3.723013] etnaviv-gpu 130000.gpu: command buffer outside valid > memory window > [ 3.731308] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 > [ 3.752437] etnaviv-gpu 134000.gpu: command buffer outside valid > memory window This looks similar to what was reported at: https://bugs.freedesktop.org/show_bug.cgi?id=111789 Does it help if you use the same suggestion and pass cma=256M in your kernel command line?
On Wed, Sep 25, 2019 at 7:12 AM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Adam, > > On Wed, Sep 25, 2019 at 6:38 AM Adam Ford <aford173@gmail.com> wrote: > > > I know it's rather late, but this patch broke the Etnaviv 3D graphics > > in my i.MX6Q. > > > > When I try to use the 3D, it returns some errors and the dmesg log > > shows some memory allocation errors too: > > [ 3.682347] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) > > [ 3.688669] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) > > [ 3.695099] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) > > [ 3.700800] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 > > [ 3.723013] etnaviv-gpu 130000.gpu: command buffer outside valid > > memory window > > [ 3.731308] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 > > [ 3.752437] etnaviv-gpu 134000.gpu: command buffer outside valid > > memory window > > This looks similar to what was reported at: > https://bugs.freedesktop.org/show_bug.cgi?id=111789 > > Does it help if you use the same suggestion and pass cma=256M in your > kernel command line? I tried cma=256M and noticed the cma dump at the beginning didn't change. Do we need to setup a reserved-memory node like imx6ul-ccimx6ulsom.dtsi did? adam
On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > I tried cma=256M and noticed the cma dump at the beginning didn't > change. Do we need to setup a reserved-memory node like > imx6ul-ccimx6ulsom.dtsi did? I don't think so. Were you able to identify what was the exact commit that caused such regression?
On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > change. Do we need to setup a reserved-memory node like > > imx6ul-ccimx6ulsom.dtsi did? > > I don't think so. > > Were you able to identify what was the exact commit that caused such regression? I was able to narrow it down the 92d12f9544b7 ("memblock: refactor internal allocation functions") that caused the regression with Etnaviv. I also noticed that if I create a reserved memory node as was done one imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I was getting errors regardless of the 'cma=256M' or not. I don't have a problem using the reserved memory, but I guess I am not sure what the amount should be. I know for the video decoding 1080p, I have historically used cma=128M, but with the 3D also needing some memory allocation, is that enough or should I use 256M? adam
Hi, On Thu, Sep 26, 2019 at 08:09:52AM -0500, Adam Ford wrote: > On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > > change. Do we need to setup a reserved-memory node like > > > imx6ul-ccimx6ulsom.dtsi did? > > > > I don't think so. > > > > Were you able to identify what was the exact commit that caused such regression? > > I was able to narrow it down the 92d12f9544b7 ("memblock: refactor > internal allocation functions") that caused the regression with > Etnaviv. Can you please test with this change: diff --git a/mm/memblock.c b/mm/memblock.c index 7d4f61a..1f5a0eb 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, align = SMP_CACHE_BYTES; } - if (end > memblock.current_limit) - end = memblock.current_limit; - again: found = memblock_find_in_range_node(size, align, start, end, nid, flags); > I also noticed that if I create a reserved memory node as was done one > imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I > was getting errors regardless of the 'cma=256M' or not. > I don't have a problem using the reserved memory, but I guess I am not > sure what the amount should be. I know for the video decoding 1080p, > I have historically used cma=128M, but with the 3D also needing some > memory allocation, is that enough or should I use 256M? > > adam
On Thu, Sep 26, 2019 at 11:04 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > Hi, > > On Thu, Sep 26, 2019 at 08:09:52AM -0500, Adam Ford wrote: > > On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > > > change. Do we need to setup a reserved-memory node like > > > > imx6ul-ccimx6ulsom.dtsi did? > > > > > > I don't think so. > > > > > > Were you able to identify what was the exact commit that caused such regression? > > > > I was able to narrow it down the 92d12f9544b7 ("memblock: refactor > > internal allocation functions") that caused the regression with > > Etnaviv. > > > Can you please test with this change: > That appears to have fixed my issue. I am not sure what the impact is, but is this a safe option? adam > diff --git a/mm/memblock.c b/mm/memblock.c > index 7d4f61a..1f5a0eb 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > align = SMP_CACHE_BYTES; > } > > - if (end > memblock.current_limit) > - end = memblock.current_limit; > - > again: > found = memblock_find_in_range_node(size, align, start, end, nid, > flags); > > > I also noticed that if I create a reserved memory node as was done one > > imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I > > was getting errors regardless of the 'cma=256M' or not. > > I don't have a problem using the reserved memory, but I guess I am not > > sure what the amount should be. I know for the video decoding 1080p, > > I have historically used cma=128M, but with the 3D also needing some > > memory allocation, is that enough or should I use 256M? > > > > adam > > -- > Sincerely yours, > Mike. >
On Thu, Sep 26, 2019 at 02:35:53PM -0500, Adam Ford wrote: > On Thu, Sep 26, 2019 at 11:04 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > Hi, > > > > On Thu, Sep 26, 2019 at 08:09:52AM -0500, Adam Ford wrote: > > > On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > > > > change. Do we need to setup a reserved-memory node like > > > > > imx6ul-ccimx6ulsom.dtsi did? > > > > > > > > I don't think so. > > > > > > > > Were you able to identify what was the exact commit that caused such regression? > > > > > > I was able to narrow it down the 92d12f9544b7 ("memblock: refactor > > > internal allocation functions") that caused the regression with > > > Etnaviv. > > > > > > Can you please test with this change: > > > > That appears to have fixed my issue. I am not sure what the impact > is, but is this a safe option? It's not really a fix, I just wanted to see how exactly 92d12f9544b7 ("memblock: refactor internal allocation functions") broke your setup. Can you share the dts you are using and the full kernel log? > adam > > > diff --git a/mm/memblock.c b/mm/memblock.c > > index 7d4f61a..1f5a0eb 100644 > > --- a/mm/memblock.c > > +++ b/mm/memblock.c > > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > > align = SMP_CACHE_BYTES; > > } > > > > - if (end > memblock.current_limit) > > - end = memblock.current_limit; > > - > > again: > > found = memblock_find_in_range_node(size, align, start, end, nid, > > flags); > > > > > I also noticed that if I create a reserved memory node as was done one > > > imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I > > > was getting errors regardless of the 'cma=256M' or not. > > > I don't have a problem using the reserved memory, but I guess I am not > > > sure what the amount should be. I know for the video decoding 1080p, > > > I have historically used cma=128M, but with the 3D also needing some > > > memory allocation, is that enough or should I use 256M? > > > > > > adam > > > > -- > > Sincerely yours, > > Mike. > >
I am attaching two logs. I now the mailing lists will be unhappy, but don't want to try and spam a bunch of log through the mailing liast. The two logs show the differences between the working and non-working imx6q 3D accelerator when trying to run a simple glmark2-es2-drm demo. The only change between them is the 2 line code change you suggested. In both cases, I have cma=128M set in my bootargs. Historically this has been sufficient, but cma=256M has not made a difference. adam On Sat, Sep 28, 2019 at 2:33 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > On Thu, Sep 26, 2019 at 02:35:53PM -0500, Adam Ford wrote: > > On Thu, Sep 26, 2019 at 11:04 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > Hi, > > > > > > On Thu, Sep 26, 2019 at 08:09:52AM -0500, Adam Ford wrote: > > > > On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > > > > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > > > > > change. Do we need to setup a reserved-memory node like > > > > > > imx6ul-ccimx6ulsom.dtsi did? > > > > > > > > > > I don't think so. > > > > > > > > > > Were you able to identify what was the exact commit that caused such regression? > > > > > > > > I was able to narrow it down the 92d12f9544b7 ("memblock: refactor > > > > internal allocation functions") that caused the regression with > > > > Etnaviv. > > > > > > > > > Can you please test with this change: > > > > > > > That appears to have fixed my issue. I am not sure what the impact > > is, but is this a safe option? > > It's not really a fix, I just wanted to see how exactly 92d12f9544b7 ("memblock: > refactor internal allocation functions") broke your setup. > > Can you share the dts you are using and the full kernel log? > > > adam > > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > > index 7d4f61a..1f5a0eb 100644 > > > --- a/mm/memblock.c > > > +++ b/mm/memblock.c > > > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > > > align = SMP_CACHE_BYTES; > > > } > > > > > > - if (end > memblock.current_limit) > > > - end = memblock.current_limit; > > > - > > > again: > > > found = memblock_find_in_range_node(size, align, start, end, nid, > > > flags); > > > > > > > I also noticed that if I create a reserved memory node as was done one > > > > imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I > > > > was getting errors regardless of the 'cma=256M' or not. > > > > I don't have a problem using the reserved memory, but I guess I am not > > > > sure what the amount should be. I know for the video decoding 1080p, > > > > I have historically used cma=128M, but with the 3D also needing some > > > > memory allocation, is that enough or should I use 256M? > > > > > > > > adam > > > > > > -- > > > Sincerely yours, > > > Mike. > > > > > -- > Sincerely yours, > Mike. >
On Sun, Sep 29, 2019 at 8:33 AM Adam Ford <aford173@gmail.com> wrote: > > I am attaching two logs. I now the mailing lists will be unhappy, but > don't want to try and spam a bunch of log through the mailing liast. > The two logs show the differences between the working and non-working > imx6q 3D accelerator when trying to run a simple glmark2-es2-drm demo. > > The only change between them is the 2 line code change you suggested. > > In both cases, I have cma=128M set in my bootargs. Historically this > has been sufficient, but cma=256M has not made a difference. > Mike any suggestions on how to move forward? I was hoping to get the fixes tested and pushed before 5.4 is released if at all possible > adam > > On Sat, Sep 28, 2019 at 2:33 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > On Thu, Sep 26, 2019 at 02:35:53PM -0500, Adam Ford wrote: > > > On Thu, Sep 26, 2019 at 11:04 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, Sep 26, 2019 at 08:09:52AM -0500, Adam Ford wrote: > > > > > On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > > > > > > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > > > > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > > > > > > change. Do we need to setup a reserved-memory node like > > > > > > > imx6ul-ccimx6ulsom.dtsi did? > > > > > > > > > > > > I don't think so. > > > > > > > > > > > > Were you able to identify what was the exact commit that caused such regression? > > > > > > > > > > I was able to narrow it down the 92d12f9544b7 ("memblock: refactor > > > > > internal allocation functions") that caused the regression with > > > > > Etnaviv. > > > > > > > > > > > > Can you please test with this change: > > > > > > > > > > That appears to have fixed my issue. I am not sure what the impact > > > is, but is this a safe option? > > > > It's not really a fix, I just wanted to see how exactly 92d12f9544b7 ("memblock: > > refactor internal allocation functions") broke your setup. > > > > Can you share the dts you are using and the full kernel log? > > > > > adam > > > > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > > > index 7d4f61a..1f5a0eb 100644 > > > > --- a/mm/memblock.c > > > > +++ b/mm/memblock.c > > > > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > > > > align = SMP_CACHE_BYTES; > > > > } > > > > > > > > - if (end > memblock.current_limit) > > > > - end = memblock.current_limit; > > > > - > > > > again: > > > > found = memblock_find_in_range_node(size, align, start, end, nid, > > > > flags); > > > > > > > > > I also noticed that if I create a reserved memory node as was done one > > > > > imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I > > > > > was getting errors regardless of the 'cma=256M' or not. > > > > > I don't have a problem using the reserved memory, but I guess I am not > > > > > sure what the amount should be. I know for the video decoding 1080p, > > > > > I have historically used cma=128M, but with the 3D also needing some > > > > > memory allocation, is that enough or should I use 256M? > > > > > > > > > > adam > > > > > > > > -- > > > > Sincerely yours, > > > > Mike. > > > > > > > > -- > > Sincerely yours, > > Mike. > >
Hi Adam, On Tue, Oct 01, 2019 at 07:14:13PM -0500, Adam Ford wrote: > On Sun, Sep 29, 2019 at 8:33 AM Adam Ford <aford173@gmail.com> wrote: > > > > I am attaching two logs. I now the mailing lists will be unhappy, but > > don't want to try and spam a bunch of log through the mailing liast. > > The two logs show the differences between the working and non-working > > imx6q 3D accelerator when trying to run a simple glmark2-es2-drm demo. > > > > The only change between them is the 2 line code change you suggested. > > > > In both cases, I have cma=128M set in my bootargs. Historically this > > has been sufficient, but cma=256M has not made a difference. > > > > Mike any suggestions on how to move forward? > I was hoping to get the fixes tested and pushed before 5.4 is released > if at all possible I have a fix (below) that kinda restores the original behaviour, but I still would like to double check to make sure it's not a band aid and I haven't missed the actual root cause. Can you please send me your device tree definition and the output of cat /sys/kernel/debug/memblock/memory and cat /sys/kernel/debug/memblock/reserved Thanks! From 06529f861772b7dea2912fc2245debe4690139b8 Mon Sep 17 00:00:00 2001 From: Mike Rapoport <rppt@linux.ibm.com> Date: Wed, 2 Oct 2019 10:14:17 +0300 Subject: [PATCH] mm: memblock: do not enforce current limit for memblock_phys* family Until commit 92d12f9544b7 ("memblock: refactor internal allocation functions") the maximal address for memblock allocations was forced to memblock.current_limit only for the allocation functions returning virtual address. The changes introduced by that commit moved the limit enforcement into the allocation core and as a result the allocation functions returning physical address also started to limit allocations to memblock.current_limit. This caused breakage of etnaviv GPU driver: [ 3.682347] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) [ 3.688669] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) [ 3.695099] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) [ 3.700800] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 [ 3.723013] etnaviv-gpu 130000.gpu: command buffer outside valid memory window [ 3.731308] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 [ 3.752437] etnaviv-gpu 134000.gpu: command buffer outside valid memory window [ 3.760583] etnaviv-gpu 2204000.gpu: model: GC355, revision: 1215 [ 3.766766] etnaviv-gpu 2204000.gpu: Ignoring GPU with VG and FE2.0 Restore the behaviour of memblock_phys* family so that these functions will not enforce memblock.current_limit. Fixes: 92d12f9544b7 ("memblock: refactor internal allocation functions") Reported-by: Adam Ford <aford173@gmail.com> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> --- mm/memblock.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mm/memblock.c b/mm/memblock.c index 7d4f61a..c4b16ca 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, align = SMP_CACHE_BYTES; } - if (end > memblock.current_limit) - end = memblock.current_limit; - again: found = memblock_find_in_range_node(size, align, start, end, nid, flags); @@ -1469,6 +1466,9 @@ static void * __init memblock_alloc_internal( if (WARN_ON_ONCE(slab_is_available())) return kzalloc_node(size, GFP_NOWAIT, nid); + if (max_addr > memblock.current_limit) + max_addr = memblock.current_limit; + alloc = memblock_alloc_range_nid(size, align, min_addr, max_addr, nid); /* retry allocation without lower limit */
On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > Hi Adam, > > On Tue, Oct 01, 2019 at 07:14:13PM -0500, Adam Ford wrote: > > On Sun, Sep 29, 2019 at 8:33 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > I am attaching two logs. I now the mailing lists will be unhappy, but > > > don't want to try and spam a bunch of log through the mailing liast. > > > The two logs show the differences between the working and non-working > > > imx6q 3D accelerator when trying to run a simple glmark2-es2-drm demo. > > > > > > The only change between them is the 2 line code change you suggested. > > > > > > In both cases, I have cma=128M set in my bootargs. Historically this > > > has been sufficient, but cma=256M has not made a difference. > > > > > > > Mike any suggestions on how to move forward? > > I was hoping to get the fixes tested and pushed before 5.4 is released > > if at all possible > > I have a fix (below) that kinda restores the original behaviour, but I > still would like to double check to make sure it's not a band aid and I > haven't missed the actual root cause. > > Can you please send me your device tree definition and the output of > > cat /sys/kernel/debug/memblock/memory > > and > > cat /sys/kernel/debug/memblock/reserved > > Thanks! > Before the patch: # cat /sys/kernel/debug/memblock/memory 0: 0x10000000..0x8fffffff # cat /sys/kernel/debug/memblock/reserved 0: 0x10004000..0x10007fff 1: 0x10100000..0x11ab141f 2: 0x1fff1000..0x1fffcfff 3: 0x2ee40000..0x2ef53fff 4: 0x2ef56940..0x2ef56c43 5: 0x2ef56c48..0x2fffefff 6: 0x2ffff0c0..0x2ffff4d8 7: 0x2ffff500..0x2ffff55f 8: 0x2ffff580..0x2ffff703 9: 0x2ffff740..0x2ffff918 10: 0x2ffff940..0x2ffff9cf 11: 0x2ffffa00..0x2ffffa0f 12: 0x2ffffa40..0x2ffffa43 13: 0x2ffffa80..0x2ffffad5 14: 0x2ffffb00..0x2ffffb55 15: 0x2ffffb80..0x2ffffbd5 16: 0x2ffffc00..0x2ffffc4e 17: 0x2ffffc50..0x2ffffc6a 18: 0x2ffffc6c..0x2ffffce6 19: 0x2ffffce8..0x2ffffd02 20: 0x2ffffd04..0x2ffffd1e 21: 0x2ffffd20..0x2ffffd3a 22: 0x2ffffd3c..0x2ffffd56 23: 0x2ffffd58..0x2ffffe30 24: 0x2ffffe34..0x2ffffe4c 25: 0x2ffffe50..0x2ffffe68 26: 0x2ffffe6c..0x2ffffe84 27: 0x2ffffe88..0x2ffffea0 28: 0x2ffffea4..0x2ffffebc 29: 0x2ffffec0..0x2ffffedf 30: 0x2ffffee4..0x2ffffefc 31: 0x2fffff00..0x2fffff13 32: 0x2fffff28..0x2fffff4b 33: 0x2fffff50..0x2fffff84 34: 0x2fffff88..0x3fffffff After the patch: # cat /sys/kernel/debug/memblock/memory 0: 0x10000000..0x8fffffff # cat /sys/kernel/debug/memblock/reserved 0: 0x10004000..0x10007fff 1: 0x10100000..0x11ab141f 2: 0x1fff1000..0x1fffcfff 3: 0x3eec0000..0x3efd3fff 4: 0x3efd6940..0x3efd6c43 5: 0x3efd6c48..0x3fffbfff 6: 0x3fffc0c0..0x3fffc4d8 7: 0x3fffc500..0x3fffc55f 8: 0x3fffc580..0x3fffc703 9: 0x3fffc740..0x3fffc918 10: 0x3fffc940..0x3fffc9cf 11: 0x3fffca00..0x3fffca0f 12: 0x3fffca40..0x3fffca43 13: 0x3fffca80..0x3fffca83 14: 0x3fffcac0..0x3fffcb15 15: 0x3fffcb40..0x3fffcb95 16: 0x3fffcbc0..0x3fffcc15 17: 0x3fffcc28..0x3fffcc72 18: 0x3fffcc74..0x3fffcc8e 19: 0x3fffcc90..0x3fffcd0a 20: 0x3fffcd0c..0x3fffcd26 21: 0x3fffcd28..0x3fffcd42 22: 0x3fffcd44..0x3fffcd5e 23: 0x3fffcd60..0x3fffcd7a 24: 0x3fffcd7c..0x3fffce54 25: 0x3fffce58..0x3fffce70 26: 0x3fffce74..0x3fffce8c 27: 0x3fffce90..0x3fffcea8 28: 0x3fffceac..0x3fffcec4 29: 0x3fffcec8..0x3fffcee0 30: 0x3fffcee4..0x3fffcefc 31: 0x3fffcf00..0x3fffcf1f 32: 0x3fffcf28..0x3fffcf53 33: 0x3fffcf68..0x3fffcf8b 34: 0x3fffcf90..0x3fffcfac 35: 0x3fffcfb0..0x3fffffff 36: 0x80000000..0x8fffffff > From 06529f861772b7dea2912fc2245debe4690139b8 Mon Sep 17 00:00:00 2001 > From: Mike Rapoport <rppt@linux.ibm.com> > Date: Wed, 2 Oct 2019 10:14:17 +0300 > Subject: [PATCH] mm: memblock: do not enforce current limit for memblock_phys* > family > > Until commit 92d12f9544b7 ("memblock: refactor internal allocation > functions") the maximal address for memblock allocations was forced to > memblock.current_limit only for the allocation functions returning virtual > address. The changes introduced by that commit moved the limit enforcement > into the allocation core and as a result the allocation functions returning > physical address also started to limit allocations to > memblock.current_limit. > > This caused breakage of etnaviv GPU driver: > > [ 3.682347] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) > [ 3.688669] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) > [ 3.695099] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) > [ 3.700800] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 > [ 3.723013] etnaviv-gpu 130000.gpu: command buffer outside valid > memory window > [ 3.731308] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 > [ 3.752437] etnaviv-gpu 134000.gpu: command buffer outside valid > memory window > [ 3.760583] etnaviv-gpu 2204000.gpu: model: GC355, revision: 1215 > [ 3.766766] etnaviv-gpu 2204000.gpu: Ignoring GPU with VG and FE2.0 > > Restore the behaviour of memblock_phys* family so that these functions will > not enforce memblock.current_limit. > This fixed the issue. Thank you Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd > Fixes: 92d12f9544b7 ("memblock: refactor internal allocation functions") > Reported-by: Adam Ford <aford173@gmail.com> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > --- > mm/memblock.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/mm/memblock.c b/mm/memblock.c > index 7d4f61a..c4b16ca 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > align = SMP_CACHE_BYTES; > } > > - if (end > memblock.current_limit) > - end = memblock.current_limit; > - > again: > found = memblock_find_in_range_node(size, align, start, end, nid, > flags); > @@ -1469,6 +1466,9 @@ static void * __init memblock_alloc_internal( > if (WARN_ON_ONCE(slab_is_available())) > return kzalloc_node(size, GFP_NOWAIT, nid); > > + if (max_addr > memblock.current_limit) > + max_addr = memblock.current_limit; > + > alloc = memblock_alloc_range_nid(size, align, min_addr, max_addr, nid); > > /* retry allocation without lower limit */ > -- > 2.7.4 > > > > > adam > > > > > > On Sat, Sep 28, 2019 at 2:33 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > On Thu, Sep 26, 2019 at 02:35:53PM -0500, Adam Ford wrote: > > > > > On Thu, Sep 26, 2019 at 11:04 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Thu, Sep 26, 2019 at 08:09:52AM -0500, Adam Ford wrote: > > > > > > > On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > > > > > > > > > > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > > > > > > > > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > > > > > > > > change. Do we need to setup a reserved-memory node like > > > > > > > > > imx6ul-ccimx6ulsom.dtsi did? > > > > > > > > > > > > > > > > I don't think so. > > > > > > > > > > > > > > > > Were you able to identify what was the exact commit that caused such regression? > > > > > > > > > > > > > > I was able to narrow it down the 92d12f9544b7 ("memblock: refactor > > > > > > > internal allocation functions") that caused the regression with > > > > > > > Etnaviv. > > > > > > > > > > > > > > > > > > Can you please test with this change: > > > > > > > > > > > > > > > > That appears to have fixed my issue. I am not sure what the impact > > > > > is, but is this a safe option? > > > > > > > > It's not really a fix, I just wanted to see how exactly 92d12f9544b7 ("memblock: > > > > refactor internal allocation functions") broke your setup. > > > > > > > > Can you share the dts you are using and the full kernel log? > > > > > > > > > adam > > > > > > > > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > > > > > index 7d4f61a..1f5a0eb 100644 > > > > > > --- a/mm/memblock.c > > > > > > +++ b/mm/memblock.c > > > > > > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > > > > > > align = SMP_CACHE_BYTES; > > > > > > } > > > > > > > > > > > > - if (end > memblock.current_limit) > > > > > > - end = memblock.current_limit; > > > > > > - > > > > > > again: > > > > > > found = memblock_find_in_range_node(size, align, start, end, nid, > > > > > > flags); > > > > > > > > > > > > > I also noticed that if I create a reserved memory node as was done one > > > > > > > imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I > > > > > > > was getting errors regardless of the 'cma=256M' or not. > > > > > > > I don't have a problem using the reserved memory, but I guess I am not > > > > > > > sure what the amount should be. I know for the video decoding 1080p, > > > > > > > I have historically used cma=128M, but with the 3D also needing some > > > > > > > memory allocation, is that enough or should I use 256M? > > > > > > > > > > > > > > adam > > > > > > > > > > > > -- > > > > > > Sincerely yours, > > > > > > Mike. > > > > > > > > > > > > > > -- > > > > Sincerely yours, > > > > Mike. > > > > > > -- > Sincerely yours, > Mike. >
(trimmed the CC) On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > Before the patch: > > # cat /sys/kernel/debug/memblock/memory > 0: 0x10000000..0x8fffffff > # cat /sys/kernel/debug/memblock/reserved > 0: 0x10004000..0x10007fff > 34: 0x2fffff88..0x3fffffff > > > After the patch: > # cat /sys/kernel/debug/memblock/memory > 0: 0x10000000..0x8fffffff > # cat /sys/kernel/debug/memblock/reserved > 0: 0x10004000..0x10007fff > 36: 0x80000000..0x8fffffff I'm still not convinced that the memblock refactoring didn't uncovered an issue in etnaviv driver. Why moving the CMA area from 0x80000000 to 0x30000000 makes it fail? BTW, the code that complained about "command buffer outside valid memory window" has been removed by the commit 17e4660ae3d7 ("drm/etnaviv: implement per-process address spaces on MMUv2"). Could be that recent changes to MMU management of etnaviv resolve the issue? > > From 06529f861772b7dea2912fc2245debe4690139b8 Mon Sep 17 00:00:00 2001 > > From: Mike Rapoport <rppt@linux.ibm.com> > > Date: Wed, 2 Oct 2019 10:14:17 +0300 > > Subject: [PATCH] mm: memblock: do not enforce current limit for memblock_phys* > > family > > > > Until commit 92d12f9544b7 ("memblock: refactor internal allocation > > functions") the maximal address for memblock allocations was forced to > > memblock.current_limit only for the allocation functions returning virtual > > address. The changes introduced by that commit moved the limit enforcement > > into the allocation core and as a result the allocation functions returning > > physical address also started to limit allocations to > > memblock.current_limit. > > > > This caused breakage of etnaviv GPU driver: > > > > [ 3.682347] etnaviv etnaviv: bound 130000.gpu (ops gpu_ops) > > [ 3.688669] etnaviv etnaviv: bound 134000.gpu (ops gpu_ops) > > [ 3.695099] etnaviv etnaviv: bound 2204000.gpu (ops gpu_ops) > > [ 3.700800] etnaviv-gpu 130000.gpu: model: GC2000, revision: 5108 > > [ 3.723013] etnaviv-gpu 130000.gpu: command buffer outside valid > > memory window > > [ 3.731308] etnaviv-gpu 134000.gpu: model: GC320, revision: 5007 > > [ 3.752437] etnaviv-gpu 134000.gpu: command buffer outside valid > > memory window > > [ 3.760583] etnaviv-gpu 2204000.gpu: model: GC355, revision: 1215 > > [ 3.766766] etnaviv-gpu 2204000.gpu: Ignoring GPU with VG and FE2.0 > > > > Restore the behaviour of memblock_phys* family so that these functions will > > not enforce memblock.current_limit. > > > > This fixed the issue. Thank you > > Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd > > > Fixes: 92d12f9544b7 ("memblock: refactor internal allocation functions") > > Reported-by: Adam Ford <aford173@gmail.com> > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > > --- > > mm/memblock.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > index 7d4f61a..c4b16ca 100644 > > --- a/mm/memblock.c > > +++ b/mm/memblock.c > > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > > align = SMP_CACHE_BYTES; > > } > > > > - if (end > memblock.current_limit) > > - end = memblock.current_limit; > > - > > again: > > found = memblock_find_in_range_node(size, align, start, end, nid, > > flags); > > @@ -1469,6 +1466,9 @@ static void * __init memblock_alloc_internal( > > if (WARN_ON_ONCE(slab_is_available())) > > return kzalloc_node(size, GFP_NOWAIT, nid); > > > > + if (max_addr > memblock.current_limit) > > + max_addr = memblock.current_limit; > > + > > alloc = memblock_alloc_range_nid(size, align, min_addr, max_addr, nid); > > > > /* retry allocation without lower limit */ > > -- > > 2.7.4 > > > > > > > > adam > > > > > > > > On Sat, Sep 28, 2019 at 2:33 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > > > On Thu, Sep 26, 2019 at 02:35:53PM -0500, Adam Ford wrote: > > > > > > On Thu, Sep 26, 2019 at 11:04 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > On Thu, Sep 26, 2019 at 08:09:52AM -0500, Adam Ford wrote: > > > > > > > > On Wed, Sep 25, 2019 at 10:17 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > > > > > > > > > > > > > On Wed, Sep 25, 2019 at 9:17 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > I tried cma=256M and noticed the cma dump at the beginning didn't > > > > > > > > > > change. Do we need to setup a reserved-memory node like > > > > > > > > > > imx6ul-ccimx6ulsom.dtsi did? > > > > > > > > > > > > > > > > > > I don't think so. > > > > > > > > > > > > > > > > > > Were you able to identify what was the exact commit that caused such regression? > > > > > > > > > > > > > > > > I was able to narrow it down the 92d12f9544b7 ("memblock: refactor > > > > > > > > internal allocation functions") that caused the regression with > > > > > > > > Etnaviv. > > > > > > > > > > > > > > > > > > > > > Can you please test with this change: > > > > > > > > > > > > > > > > > > > That appears to have fixed my issue. I am not sure what the impact > > > > > > is, but is this a safe option? > > > > > > > > > > It's not really a fix, I just wanted to see how exactly 92d12f9544b7 ("memblock: > > > > > refactor internal allocation functions") broke your setup. > > > > > > > > > > Can you share the dts you are using and the full kernel log? > > > > > > > > > > > adam > > > > > > > > > > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > > > > > > index 7d4f61a..1f5a0eb 100644 > > > > > > > --- a/mm/memblock.c > > > > > > > +++ b/mm/memblock.c > > > > > > > @@ -1356,9 +1356,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, > > > > > > > align = SMP_CACHE_BYTES; > > > > > > > } > > > > > > > > > > > > > > - if (end > memblock.current_limit) > > > > > > > - end = memblock.current_limit; > > > > > > > - > > > > > > > again: > > > > > > > found = memblock_find_in_range_node(size, align, start, end, nid, > > > > > > > flags); > > > > > > > > > > > > > > > I also noticed that if I create a reserved memory node as was done one > > > > > > > > imx6ul-ccimx6ulsom.dtsi the 3D seems to work again, but without it, I > > > > > > > > was getting errors regardless of the 'cma=256M' or not. > > > > > > > > I don't have a problem using the reserved memory, but I guess I am not > > > > > > > > sure what the amount should be. I know for the video decoding 1080p, > > > > > > > > I have historically used cma=128M, but with the 3D also needing some > > > > > > > > memory allocation, is that enough or should I use 256M? > > > > > > > > > > > > > > > > adam > > > > > > > > > > > > > > -- > > > > > > > Sincerely yours, > > > > > > > Mike. > > > > > > > > > > > > > > > > > -- > > > > > Sincerely yours, > > > > > Mike. > > > > > > > > > -- > > Sincerely yours, > > Mike. > >
On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > (trimmed the CC) > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > Before the patch: > > > > # cat /sys/kernel/debug/memblock/memory > > 0: 0x10000000..0x8fffffff > > # cat /sys/kernel/debug/memblock/reserved > > 0: 0x10004000..0x10007fff > > 34: 0x2fffff88..0x3fffffff > > > > > > After the patch: > > # cat /sys/kernel/debug/memblock/memory > > 0: 0x10000000..0x8fffffff > > # cat /sys/kernel/debug/memblock/reserved > > 0: 0x10004000..0x10007fff > > 36: 0x80000000..0x8fffffff > > I'm still not convinced that the memblock refactoring didn't uncovered an > issue in etnaviv driver. > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it fail? I think you have that the wrong way round. > BTW, the code that complained about "command buffer outside valid memory > window" has been removed by the commit 17e4660ae3d7 ("drm/etnaviv: > implement per-process address spaces on MMUv2"). > > Could be that recent changes to MMU management of etnaviv resolve the > issue? The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 hardware requires command buffers within the first 2GiB of physical RAM. I've reported the problem previously but there was no resolution, other than pointing the blame at CMA. https://lists.freedesktop.org/archives/dri-devel/2019-June/thread.html#223516
On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux admin wrote: > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > (trimmed the CC) > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > > > Before the patch: > > > > > > # cat /sys/kernel/debug/memblock/memory > > > 0: 0x10000000..0x8fffffff > > > # cat /sys/kernel/debug/memblock/reserved > > > 0: 0x10004000..0x10007fff > > > 34: 0x2fffff88..0x3fffffff > > > > > > > > > After the patch: > > > # cat /sys/kernel/debug/memblock/memory > > > 0: 0x10000000..0x8fffffff > > > # cat /sys/kernel/debug/memblock/reserved > > > 0: 0x10004000..0x10007fff > > > 36: 0x80000000..0x8fffffff > > > > I'm still not convinced that the memblock refactoring didn't uncovered an > > issue in etnaviv driver. > > > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it fail? > > I think you have that the wrong way round. I'm relying on Adam's reports of working and non-working versions. According to that etnaviv works when CMA area is at 0x80000000 and does not work when it is at 0x30000000. He also sent logs a few days ago [1], they also confirm that. [1] https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w@mail.gmail.com/ > > BTW, the code that complained about "command buffer outside valid memory > > window" has been removed by the commit 17e4660ae3d7 ("drm/etnaviv: > > implement per-process address spaces on MMUv2"). > > > > Could be that recent changes to MMU management of etnaviv resolve the > > issue? > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > hardware requires command buffers within the first 2GiB of physical > RAM. I've mentioned that patch because it removed the check for cmdbuf address for MMUv1: @@ -785,15 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) PAGE_SIZE); if (ret) { dev_err(gpu->dev, "could not create command buffer\n"); - goto unmap_suballoc; - } - - if (!(gpu->identity.minor_features1 & chipMinorFeatures1_MMU_VERSION) && - etnaviv_cmdbuf_get_va(&gpu->buffer, &gpu->cmdbuf_mapping) > 0x80000000) { - ret = -EINVAL; - dev_err(gpu->dev, - "command buffer outside valid memory window\n"); - goto free_buffer; + goto fail; } /* Setup event management */ I really don't know how etnaviv works, so I hoped that people who understand it would help. > I've reported the problem previously but there was no resolution, > other than pointing the blame at CMA. > > https://lists.freedesktop.org/archives/dri-devel/2019-June/thread.html#223516 > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up
Am Donnerstag, den 03.10.2019, 14:30 +0300 schrieb Mike Rapoport: > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux admin wrote: > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > > (trimmed the CC) > > > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > Before the patch: > > > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > 0: 0x10000000..0x8fffffff > > > > # cat /sys/kernel/debug/memblock/reserved > > > > 0: 0x10004000..0x10007fff > > > > 34: 0x2fffff88..0x3fffffff > > > > > > > > > > > > After the patch: > > > > # cat /sys/kernel/debug/memblock/memory > > > > 0: 0x10000000..0x8fffffff > > > > # cat /sys/kernel/debug/memblock/reserved > > > > 0: 0x10004000..0x10007fff > > > > 36: 0x80000000..0x8fffffff > > > > > > I'm still not convinced that the memblock refactoring didn't uncovered an > > > issue in etnaviv driver. > > > > > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it fail? > > > > I think you have that the wrong way round. > > I'm relying on Adam's reports of working and non-working versions. > According to that etnaviv works when CMA area is at 0x80000000 and does not > work when it is at 0x30000000. > > He also sent logs a few days ago [1], they also confirm that. > > [1] https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w@mail.gmail.com/ To clarify: Etnaviv needs to know where the CMA area is in order to move a aperture window to cover the CMA area so the command buffers allocated in contig memory can be mapped through this aperture. Now the issue is that there is currently there is no good API for a driver to know where the CMA area is located, so we are trying to infer this from dma_get_required_mask. Unfortunately this can overshoot the real DRAM area by a bit, so combined with the fixed 2GB size of the GPU aperture this means we are no longer able to map the command buffers through the required aperture if the CMA area moves too far down in the physical memory. It's really a bad interaction between etnaviv and CMA area placement, due to insufficient APIs to communicate some crucial information. There is nothing in the etnaviv driver or the hardware which requires the CMA area to be at a certain place, we just need to know where it is located exactly. So my try at fixing this [1] was by adding a API to get the required information, but the first attempt was shot down and I hadn't had time to follow up on this yet. Regards, Lucas [1] https://patchwork.kernel.org/patch/10966767/
> > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > hardware requires command buffers within the first 2GiB of physical > RAM. > I thought that the i.MX6q has the MMUv1 and GC2000 GPU while the i.MX6qp has the MMUv2 and GC3000? Meaning the i.MX6 has both MMUv1 and MMUv2 depending on which i.MX6 part we are talking about.
On Thu, Oct 03, 2019 at 07:46:06AM -0700, Chris Healy wrote: > > > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > > hardware requires command buffers within the first 2GiB of physical > > RAM. > > > I thought that the i.MX6q has the MMUv1 and GC2000 GPU while the > i.MX6qp has the MMUv2 and GC3000? Meaning the i.MX6 has both MMUv1 > and MMUv2 depending on which i.MX6 part we are talking about. The report says iMX6Q with GC2000 - which is what I was referring to here. I'm not aware of what the later SoCs use, since I've never used them. Thanks.
On Thu, Oct 03, 2019 at 02:30:10PM +0300, Mike Rapoport wrote: > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux admin wrote: > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > > (trimmed the CC) > > > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > > > > > > Before the patch: > > > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > 0: 0x10000000..0x8fffffff > > > > # cat /sys/kernel/debug/memblock/reserved > > > > 0: 0x10004000..0x10007fff > > > > 34: 0x2fffff88..0x3fffffff > > > > > > > > > > > > After the patch: > > > > # cat /sys/kernel/debug/memblock/memory > > > > 0: 0x10000000..0x8fffffff > > > > # cat /sys/kernel/debug/memblock/reserved > > > > 0: 0x10004000..0x10007fff > > > > 36: 0x80000000..0x8fffffff > > > > > > I'm still not convinced that the memblock refactoring didn't uncovered an > > > issue in etnaviv driver. > > > > > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it fail? > > > > I think you have that the wrong way round. > > I'm relying on Adam's reports of working and non-working versions. > According to that etnaviv works when CMA area is at 0x80000000 and does not > work when it is at 0x30000000. > > He also sent logs a few days ago [1], they also confirm that. > > [1] https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w@mail.gmail.com/ Sorry, yes, you're right. Still, I've reported this same regression a while back, and it's never gone away. > > > BTW, the code that complained about "command buffer outside valid memory > > > window" has been removed by the commit 17e4660ae3d7 ("drm/etnaviv: > > > implement per-process address spaces on MMUv2"). > > > > > > Could be that recent changes to MMU management of etnaviv resolve the > > > issue? > > > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > > hardware requires command buffers within the first 2GiB of physical > > RAM. > > I've mentioned that patch because it removed the check for cmdbuf address > for MMUv1: > > @@ -785,15 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) > PAGE_SIZE); > if (ret) { > dev_err(gpu->dev, "could not create command buffer\n"); > - goto unmap_suballoc; > - } > - > - if (!(gpu->identity.minor_features1 & chipMinorFeatures1_MMU_VERSION) && > - etnaviv_cmdbuf_get_va(&gpu->buffer, &gpu->cmdbuf_mapping) > 0x80000000) { > - ret = -EINVAL; > - dev_err(gpu->dev, > - "command buffer outside valid memory window\n"); > - goto free_buffer; > + goto fail; > } > > /* Setup event management */ > > > I really don't know how etnaviv works, so I hoped that people who > understand it would help. From what I can see, removing that check is a completely insane thing to do, and I note that these changes are _not_ described in the commit message. The problem was known about _before_ (June 22) the patch was created (July 5). Lucas, please can you explain why removing the above check, which is well known to correctly trigger on various platforms to prevent incorrect GPU behaviour, is safe? Thanks.
Am Freitag, den 04.10.2019, 10:27 +0100 schrieb Russell King - ARM Linux admin: > On Thu, Oct 03, 2019 at 02:30:10PM +0300, Mike Rapoport wrote: > > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux > > admin wrote: > > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > > > (trimmed the CC) > > > > > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport < > > > > > rppt@linux.ibm.com> wrote: > > > > > > > > > > Before the patch: > > > > > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > 0: 0x10000000..0x8fffffff > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > 0: 0x10004000..0x10007fff > > > > > 34: 0x2fffff88..0x3fffffff > > > > > > > > > > > > > > > After the patch: > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > 0: 0x10000000..0x8fffffff > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > 0: 0x10004000..0x10007fff > > > > > 36: 0x80000000..0x8fffffff > > > > > > > > I'm still not convinced that the memblock refactoring didn't > > > > uncovered an > > > > issue in etnaviv driver. > > > > > > > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it > > > > fail? > > > > > > I think you have that the wrong way round. > > > > I'm relying on Adam's reports of working and non-working versions. > > According to that etnaviv works when CMA area is at 0x80000000 and > > does not > > work when it is at 0x30000000. > > > > He also sent logs a few days ago [1], they also confirm that. > > > > [1] > > https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w@mail.gmail.com/ > > Sorry, yes, you're right. Still, I've reported this same regression > a while back, and it's never gone away. > > > > > BTW, the code that complained about "command buffer outside > > > > valid memory > > > > window" has been removed by the commit 17e4660ae3d7 > > > > ("drm/etnaviv: > > > > implement per-process address spaces on MMUv2"). > > > > > > > > Could be that recent changes to MMU management of etnaviv > > > > resolve the > > > > issue? > > > > > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > > > hardware requires command buffers within the first 2GiB of > > > physical > > > RAM. > > > > I've mentioned that patch because it removed the check for cmdbuf > > address > > for MMUv1: > > > > @@ -785,15 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) > > PAGE_SIZE); > > if (ret) { > > dev_err(gpu->dev, "could not create command > > buffer\n"); > > - goto unmap_suballoc; > > - } > > - > > - if (!(gpu->identity.minor_features1 & > > chipMinorFeatures1_MMU_VERSION) && > > - etnaviv_cmdbuf_get_va(&gpu->buffer, &gpu- > > >cmdbuf_mapping) > 0x80000000) { > > - ret = -EINVAL; > > - dev_err(gpu->dev, > > - "command buffer outside valid memory > > window\n"); > > - goto free_buffer; > > + goto fail; > > } > > > > /* Setup event management */ > > > > > > I really don't know how etnaviv works, so I hoped that people who > > understand it would help. > > From what I can see, removing that check is a completely insane thing > to do, and I note that these changes are _not_ described in the > commit > message. The problem was known about _before_ (June 22) the patch > was > created (July 5). > > Lucas, please can you explain why removing the above check, which is > well known to correctly trigger on various platforms to prevent > incorrect GPU behaviour, is safe? It isn't. It's a pretty big oversight in this commit to remove this check. It can't be done at the same spot in the code anymore, as we don't have a mapping context at this time anymore, but it should have moved into etnaviv_iommu_context_init(). I'll send a patch to fix this up. Regards, Lucas
On Fri, Oct 4, 2019 at 8:23 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Freitag, den 04.10.2019, 10:27 +0100 schrieb Russell King - ARM > Linux admin: > > On Thu, Oct 03, 2019 at 02:30:10PM +0300, Mike Rapoport wrote: > > > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux > > > admin wrote: > > > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > > > > (trimmed the CC) > > > > > > > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport < > > > > > > rppt@linux.ibm.com> wrote: > > > > > > > > > > > > Before the patch: > > > > > > > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > > 0: 0x10000000..0x8fffffff > > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > > 0: 0x10004000..0x10007fff > > > > > > 34: 0x2fffff88..0x3fffffff > > > > > > > > > > > > > > > > > > After the patch: > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > > 0: 0x10000000..0x8fffffff > > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > > 0: 0x10004000..0x10007fff > > > > > > 36: 0x80000000..0x8fffffff > > > > > > > > > > I'm still not convinced that the memblock refactoring didn't > > > > > uncovered an > > > > > issue in etnaviv driver. > > > > > > > > > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it > > > > > fail? > > > > > > > > I think you have that the wrong way round. > > > > > > I'm relying on Adam's reports of working and non-working versions. > > > According to that etnaviv works when CMA area is at 0x80000000 and > > > does not > > > work when it is at 0x30000000. > > > > > > He also sent logs a few days ago [1], they also confirm that. > > > > > > [1] > > > https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w@mail.gmail.com/ > > > > Sorry, yes, you're right. Still, I've reported this same regression > > a while back, and it's never gone away. > > > > > > > BTW, the code that complained about "command buffer outside > > > > > valid memory > > > > > window" has been removed by the commit 17e4660ae3d7 > > > > > ("drm/etnaviv: > > > > > implement per-process address spaces on MMUv2"). > > > > > > > > > > Could be that recent changes to MMU management of etnaviv > > > > > resolve the > > > > > issue? > > > > > > > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > > > > hardware requires command buffers within the first 2GiB of > > > > physical > > > > RAM. > > > > > > I've mentioned that patch because it removed the check for cmdbuf > > > address > > > for MMUv1: > > > > > > @@ -785,15 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) > > > PAGE_SIZE); > > > if (ret) { > > > dev_err(gpu->dev, "could not create command > > > buffer\n"); > > > - goto unmap_suballoc; > > > - } > > > - > > > - if (!(gpu->identity.minor_features1 & > > > chipMinorFeatures1_MMU_VERSION) && > > > - etnaviv_cmdbuf_get_va(&gpu->buffer, &gpu- > > > >cmdbuf_mapping) > 0x80000000) { > > > - ret = -EINVAL; > > > - dev_err(gpu->dev, > > > - "command buffer outside valid memory > > > window\n"); > > > - goto free_buffer; > > > + goto fail; > > > } > > > > > > /* Setup event management */ > > > > > > > > > I really don't know how etnaviv works, so I hoped that people who > > > understand it would help. > > > > From what I can see, removing that check is a completely insane thing > > to do, and I note that these changes are _not_ described in the > > commit > > message. The problem was known about _before_ (June 22) the patch > > was > > created (July 5). > > > > Lucas, please can you explain why removing the above check, which is > > well known to correctly trigger on various platforms to prevent > > incorrect GPU behaviour, is safe? > > It isn't. It's a pretty big oversight in this commit to remove this > check. It can't be done at the same spot in the code anymore, as we > don't have a mapping context at this time anymore, but it should have > moved into etnaviv_iommu_context_init(). I'll send a patch to fix this > up. If you CC me, I will test it and report my findings. adam > > Regards, > Lucas >
On Fri, Oct 04, 2019 at 03:21:03PM +0200, Lucas Stach wrote: > Am Freitag, den 04.10.2019, 10:27 +0100 schrieb Russell King - ARM > Linux admin: > > On Thu, Oct 03, 2019 at 02:30:10PM +0300, Mike Rapoport wrote: > > > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux > > > admin wrote: > > > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > > > > (trimmed the CC) > > > > > > > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport < > > > > > > rppt@linux.ibm.com> wrote: > > > > > > > > > > > > Before the patch: > > > > > > > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > > 0: 0x10000000..0x8fffffff > > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > > 0: 0x10004000..0x10007fff > > > > > > 34: 0x2fffff88..0x3fffffff > > > > > > > > > > > > > > > > > > After the patch: > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > > 0: 0x10000000..0x8fffffff > > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > > 0: 0x10004000..0x10007fff > > > > > > 36: 0x80000000..0x8fffffff > > > > > > > > > > I'm still not convinced that the memblock refactoring didn't > > > > > uncovered an > > > > > issue in etnaviv driver. > > > > > > > > > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it > > > > > fail? > > > > > > > > I think you have that the wrong way round. > > > > > > I'm relying on Adam's reports of working and non-working versions. > > > According to that etnaviv works when CMA area is at 0x80000000 and > > > does not > > > work when it is at 0x30000000. > > > > > > He also sent logs a few days ago [1], they also confirm that. > > > > > > [1] > > > https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w@mail.gmail.com/ > > > > Sorry, yes, you're right. Still, I've reported this same regression > > a while back, and it's never gone away. > > > > > > > BTW, the code that complained about "command buffer outside > > > > > valid memory > > > > > window" has been removed by the commit 17e4660ae3d7 > > > > > ("drm/etnaviv: > > > > > implement per-process address spaces on MMUv2"). > > > > > > > > > > Could be that recent changes to MMU management of etnaviv > > > > > resolve the > > > > > issue? > > > > > > > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > > > > hardware requires command buffers within the first 2GiB of > > > > physical > > > > RAM. > > > > > > I've mentioned that patch because it removed the check for cmdbuf > > > address > > > for MMUv1: > > > > > > @@ -785,15 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) > > > PAGE_SIZE); > > > if (ret) { > > > dev_err(gpu->dev, "could not create command > > > buffer\n"); > > > - goto unmap_suballoc; > > > - } > > > - > > > - if (!(gpu->identity.minor_features1 & > > > chipMinorFeatures1_MMU_VERSION) && > > > - etnaviv_cmdbuf_get_va(&gpu->buffer, &gpu- > > > >cmdbuf_mapping) > 0x80000000) { > > > - ret = -EINVAL; > > > - dev_err(gpu->dev, > > > - "command buffer outside valid memory > > > window\n"); > > > - goto free_buffer; > > > + goto fail; > > > } > > > > > > /* Setup event management */ > > > > > > > > > I really don't know how etnaviv works, so I hoped that people who > > > understand it would help. > > > > From what I can see, removing that check is a completely insane thing > > to do, and I note that these changes are _not_ described in the > > commit > > message. The problem was known about _before_ (June 22) the patch > > was > > created (July 5). > > > > Lucas, please can you explain why removing the above check, which is > > well known to correctly trigger on various platforms to prevent > > incorrect GPU behaviour, is safe? > > It isn't. It's a pretty big oversight in this commit to remove this > check. It can't be done at the same spot in the code anymore, as we > don't have a mapping context at this time anymore, but it should have > moved into etnaviv_iommu_context_init(). I'll send a patch to fix this > up. Lucas, can you make the check use SZ_2G instead of 0x80000000 and add a comment about 2G limitation of the aperture window? > Regards, > Lucas >
On Fri, Oct 04, 2019 at 10:27:27AM +0100, Russell King - ARM Linux admin wrote: > On Thu, Oct 03, 2019 at 02:30:10PM +0300, Mike Rapoport wrote: > > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux admin wrote: > > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > > > (trimmed the CC) > > > > > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <rppt@linux.ibm.com> wrote: > > > > > > > > > > > > > > > > Before the patch: > > > > > > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > 0: 0x10000000..0x8fffffff > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > 0: 0x10004000..0x10007fff > > > > > 34: 0x2fffff88..0x3fffffff > > > > > > > > > > > > > > > After the patch: > > > > > # cat /sys/kernel/debug/memblock/memory > > > > > 0: 0x10000000..0x8fffffff > > > > > # cat /sys/kernel/debug/memblock/reserved > > > > > 0: 0x10004000..0x10007fff > > > > > 36: 0x80000000..0x8fffffff > > > > > > > > I'm still not convinced that the memblock refactoring didn't uncovered an > > > > issue in etnaviv driver. > > > > > > > > Why moving the CMA area from 0x80000000 to 0x30000000 makes it fail? > > > > > > I think you have that the wrong way round. > > > > I'm relying on Adam's reports of working and non-working versions. > > According to that etnaviv works when CMA area is at 0x80000000 and does not > > work when it is at 0x30000000. > > > > He also sent logs a few days ago [1], they also confirm that. > > > > [1] https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w@mail.gmail.com/ > > Sorry, yes, you're right. Still, I've reported this same regression > a while back, and it's never gone away. > > > > > BTW, the code that complained about "command buffer outside valid memory > > > > window" has been removed by the commit 17e4660ae3d7 ("drm/etnaviv: > > > > implement per-process address spaces on MMUv2"). > > > > > > > > Could be that recent changes to MMU management of etnaviv resolve the > > > > issue? > > > > > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1 > > > hardware requires command buffers within the first 2GiB of physical > > > RAM. > > > > I've mentioned that patch because it removed the check for cmdbuf address > > for MMUv1: > > > > @@ -785,15 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) > > PAGE_SIZE); > > if (ret) { > > dev_err(gpu->dev, "could not create command buffer\n"); > > - goto unmap_suballoc; > > - } > > - > > - if (!(gpu->identity.minor_features1 & chipMinorFeatures1_MMU_VERSION) && > > - etnaviv_cmdbuf_get_va(&gpu->buffer, &gpu->cmdbuf_mapping) > 0x80000000) { > > - ret = -EINVAL; > > - dev_err(gpu->dev, > > - "command buffer outside valid memory window\n"); > > - goto free_buffer; > > + goto fail; > > } > > > > /* Setup event management */ > > > > > > I really don't know how etnaviv works, so I hoped that people who > > understand it would help. > > From what I can see, removing that check is a completely insane thing > to do, and I note that these changes are _not_ described in the commit > message. The problem was known about _before_ (June 22) the patch was > created (July 5). The memblock refactoring went in in 5.1 which was May 5, and likely it caused the regression. Unless I'm missing something, before the memblock refactoring the CMA reservation could use the entire physical memory because memblock_phys_alloc() didn't enforce memblock.current_limit. Since memblock default is to allocate from top, cma_declare_contiguous() could grab the memory close to the end of DRAM and thus have physical address close enough to the virtual address to fit in the 2G limit. When I've made memblock_phys* limit the memblock allocations to memblock.current_limit the CMA area moved too far away down and the gap became larger than 2G. It does not seem like dealing with this in etnaviv driver and DMA and CMA APIs would happen fast and the "revert" of the memblock changes I've sent earlier in this thread does fix the problem. Andrew, would you like me to resend the patch in a separate e-mail? > Lucas, please can you explain why removing the above check, which is > well known to correctly trigger on various platforms to prevent > incorrect GPU behaviour, is safe? > > Thanks. >