LinuxLists.cc - [PATCH] dma-pool: Do not allocate pool memory from CMA

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Wed, 8 Jul 2020 at 22:43, Nicolas Saenz Julienne
<[email protected]> wrote:
>
> There is no guarantee to CMA's placement, so allocating a zone specific
> atomic pool from CMA might return memory from a completely different
> memory zone. So stop using it.
>
> Fixes: c84dc6e68a1d ("dma-pool: add additional coherent pools to map to gfp mask")

Hi Nicolas,

I see a boot regression with this commit d9765e41d8e9 "dma-pool:
Do not allocate pool memory from CMA" on my Xiaomi Poco F1
(Qcom sdm845) phone running v5.8-rc6. I can't boot past the
bootloader splash screen with this patch.

Phone boots fine if I revert this patch. I carry only one out of tree
dts patch https://lkml.org/lkml/2020/6/25/52. And since this is a stock
phone, I don't have access to serial/dmesg logs until I boot to AOSP
(adb) shell.

Any thoughts as to what might be going wrong here? I'd be happy to
help debug things. For what it's worth, I don't see this regression on
other two sdm845 devices (db845c and Pixel 3) I tested on.

Regards,
Amit Pundir

> Reported-by: Jeremy Linton <[email protected]>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
>
> An more costly alternative would be adding an option to
> dma_alloc_from_contiguous() so it fails when the allocation doesn't fall
> in a specific zone.
>
> kernel/dma/pool.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 8cfa01243ed2..4bc1c46ae6ef 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -6,7 +6,6 @@
> #include <linux/debugfs.h>
> #include <linux/dma-direct.h>
> #include <linux/dma-noncoherent.h>
> -#include <linux/dma-contiguous.h>
> #include <linux/init.h>
> #include <linux/genalloc.h>
> #include <linux/set_memory.h>
> @@ -69,12 +68,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
>
> do {
> pool_size = 1 << (PAGE_SHIFT + order);
> -
> - if (dev_get_cma_area(NULL))
> - page = dma_alloc_from_contiguous(NULL, 1 << order,
> - order, false);
> - else
> - page = alloc_pages(gfp, order);
> + page = alloc_pages(gfp, order);
> } while (!page && order-- > 0);
> if (!page)
> goto out;
> @@ -118,8 +112,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> dma_common_free_remap(addr, pool_size);
> #endif
> free_page: __maybe_unused
> - if (!dma_release_from_contiguous(NULL, page, 1 << order))
> - __free_pages(page, order);
> + __free_pages(page, order);
> out:
> return ret;
> }
> --
> 2.27.0
>

2020-07-21 08:43:24

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

Hi Amit,

On Tue, 2020-07-21 at 12:51 +0530, Amit Pundir wrote:
> On Wed, 8 Jul 2020 at 22:43, Nicolas Saenz Julienne
> <[email protected]> wrote:
> > There is no guarantee to CMA's placement, so allocating a zone
> > specific
> > atomic pool from CMA might return memory from a completely
> > different
> > memory zone. So stop using it.
> >
> > Fixes: c84dc6e68a1d ("dma-pool: add additional coherent pools to
> > map to gfp mask")
>
> Hi Nicolas,
>
> I see a boot regression with this commit d9765e41d8e9 "dma-pool:
> Do not allocate pool memory from CMA" on my Xiaomi Poco F1
> (Qcom sdm845) phone running v5.8-rc6. I can't boot past the
> bootloader splash screen with this patch.
>
> Phone boots fine if I revert this patch. I carry only one out of tree
> dts patch https://lkml.org/lkml/2020/6/25/52. And since this is a
> stock
> phone, I don't have access to serial/dmesg logs until I boot to AOSP
> (adb) shell.
>
> Any thoughts as to what might be going wrong here? I'd be happy to
> help debug things. For what it's worth, I don't see this regression
> on
> other two sdm845 devices (db845c and Pixel 3) I tested on.

Can you provide a boot log (even if without my patch) and the device-
tree files? It'd help a lot figuring things out.

Regards,
Nicolas

> Regards,
> Amit Pundir
>
> > Reported-by: Jeremy Linton <[email protected]>
> > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > ---
> >
> > An more costly alternative would be adding an option to
> > dma_alloc_from_contiguous() so it fails when the allocation doesn't
> > fall
> > in a specific zone.
> >
> > kernel/dma/pool.c | 11 ++---------
> > 1 file changed, 2 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> > index 8cfa01243ed2..4bc1c46ae6ef 100644
> > --- a/kernel/dma/pool.c
> > +++ b/kernel/dma/pool.c
> > @@ -6,7 +6,6 @@
> > #include <linux/debugfs.h>
> > #include <linux/dma-direct.h>
> > #include <linux/dma-noncoherent.h>
> > -#include <linux/dma-contiguous.h>
> > #include <linux/init.h>
> > #include <linux/genalloc.h>
> > #include <linux/set_memory.h>
> > @@ -69,12 +68,7 @@ static int atomic_pool_expand(struct gen_pool
> > *pool, size_t pool_size,
> >
> > do {
> > pool_size = 1 << (PAGE_SHIFT + order);
> > -
> > - if (dev_get_cma_area(NULL))
> > - page = dma_alloc_from_contiguous(NULL, 1 <<
> > order,
> > - order,
> > false);
> > - else
> > - page = alloc_pages(gfp, order);
> > + page = alloc_pages(gfp, order);
> > } while (!page && order-- > 0);
> > if (!page)
> > goto out;
> > @@ -118,8 +112,7 @@ static int atomic_pool_expand(struct gen_pool
> > *pool, size_t pool_size,
> > dma_common_free_remap(addr, pool_size);
> > #endif
> > free_page: __maybe_unused
> > - if (!dma_release_from_contiguous(NULL, page, 1 << order))
> > - __free_pages(page, order);
> > + __free_pages(page, order);
> > out:
> > return ret;
> > }
> > --
> > 2.27.0
> >

2020-07-21 08:56:14

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 21 Jul 2020 at 14:09, Nicolas Saenz Julienne
<[email protected]> wrote:
>
> Hi Amit,
>
> On Tue, 2020-07-21 at 12:51 +0530, Amit Pundir wrote:
> > On Wed, 8 Jul 2020 at 22:43, Nicolas Saenz Julienne
> > <[email protected]> wrote:
> > > There is no guarantee to CMA's placement, so allocating a zone
> > > specific
> > > atomic pool from CMA might return memory from a completely
> > > different
> > > memory zone. So stop using it.
> > >
> > > Fixes: c84dc6e68a1d ("dma-pool: add additional coherent pools to
> > > map to gfp mask")
> >
> > Hi Nicolas,
> >
> > I see a boot regression with this commit d9765e41d8e9 "dma-pool:
> > Do not allocate pool memory from CMA" on my Xiaomi Poco F1
> > (Qcom sdm845) phone running v5.8-rc6. I can't boot past the
> > bootloader splash screen with this patch.
> >
> > Phone boots fine if I revert this patch. I carry only one out of tree
> > dts patch https://lkml.org/lkml/2020/6/25/52. And since this is a
> > stock
> > phone, I don't have access to serial/dmesg logs until I boot to AOSP
> > (adb) shell.
> >
> > Any thoughts as to what might be going wrong here? I'd be happy to
> > help debug things. For what it's worth, I don't see this regression
> > on
> > other two sdm845 devices (db845c and Pixel 3) I tested on.
>
> Can you provide a boot log (even if without my patch) and the device-
> tree files? It'd help a lot figuring things out.

Thank you for the prompt reply Nicolas.

Here is the boot log with the reverted patch
https://pastebin.ubuntu.com/p/BrhPf83nKF/

Here is my phone's dts
https://github.com/pundiramit/linux/commit/2a394c199deeaf4c91e0e008e8fba2a72f494d8c

And here is my kernel tree just in case
https://github.com/pundiramit/linux/commits/beryllium-mainline

Regards,
Amit Pundir

>
> Regards,
> Nicolas
>
> > Regards,
> > Amit Pundir
> >
> > > Reported-by: Jeremy Linton <[email protected]>
> > > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > > ---
> > >
> > > An more costly alternative would be adding an option to
> > > dma_alloc_from_contiguous() so it fails when the allocation doesn't
> > > fall
> > > in a specific zone.
> > >
> > > kernel/dma/pool.c | 11 ++---------
> > > 1 file changed, 2 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> > > index 8cfa01243ed2..4bc1c46ae6ef 100644
> > > --- a/kernel/dma/pool.c
> > > +++ b/kernel/dma/pool.c
> > > @@ -6,7 +6,6 @@
> > > #include <linux/debugfs.h>
> > > #include <linux/dma-direct.h>
> > > #include <linux/dma-noncoherent.h>
> > > -#include <linux/dma-contiguous.h>
> > > #include <linux/init.h>
> > > #include <linux/genalloc.h>
> > > #include <linux/set_memory.h>
> > > @@ -69,12 +68,7 @@ static int atomic_pool_expand(struct gen_pool
> > > *pool, size_t pool_size,
> > >
> > > do {
> > > pool_size = 1 << (PAGE_SHIFT + order);
> > > -
> > > - if (dev_get_cma_area(NULL))
> > > - page = dma_alloc_from_contiguous(NULL, 1 <<
> > > order,
> > > - order,
> > > false);
> > > - else
> > > - page = alloc_pages(gfp, order);
> > > + page = alloc_pages(gfp, order);
> > > } while (!page && order-- > 0);
> > > if (!page)
> > > goto out;
> > > @@ -118,8 +112,7 @@ static int atomic_pool_expand(struct gen_pool
> > > *pool, size_t pool_size,
> > > dma_common_free_remap(addr, pool_size);
> > > #endif
> > > free_page: __maybe_unused
> > > - if (!dma_release_from_contiguous(NULL, page, 1 << order))
> > > - __free_pages(page, order);
> > > + __free_pages(page, order);
> > > out:
> > > return ret;
> > > }
> > > --
> > > 2.27.0
> > >
>

2020-07-21 11:15:59

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 2020-07-21 at 14:24 +0530, Amit Pundir wrote:
> On Tue, 21 Jul 2020 at 14:09, Nicolas Saenz Julienne
> <[email protected]> wrote:
> > Hi Amit,
> > > Hi Nicolas,
> > >
> > > I see a boot regression with this commit d9765e41d8e9 "dma-pool:
> > > Do not allocate pool memory from CMA" on my Xiaomi Poco F1
> > > (Qcom sdm845) phone running v5.8-rc6. I can't boot past the
> > > bootloader splash screen with this patch.
> > >
> > > Phone boots fine if I revert this patch. I carry only one out of
> > > tree
> > > dts patch https://lkml.org/lkml/2020/6/25/52. And since this is a
> > > stock
> > > phone, I don't have access to serial/dmesg logs until I boot to
> > > AOSP
> > > (adb) shell.
> > >
> > > Any thoughts as to what might be going wrong here? I'd be happy
> > > to
> > > help debug things. For what it's worth, I don't see this
> > > regression
> > > on
> > > other two sdm845 devices (db845c and Pixel 3) I tested on.
> >
> > Can you provide a boot log (even if without my patch) and the
> > device-
> > tree files? It'd help a lot figuring things out.
>
> Thank you for the prompt reply Nicolas.
>
> Here is the boot log with the reverted patch
> https://pastebin.ubuntu.com/p/BrhPf83nKF/
>
> Here is my phone's dts
> https://github.com/pundiramit/linux/commit/2a394c199deeaf4c91e0e008e8fba2a72f494d8c

I'm at loss at what could be failing here. Your device should be able
to address the whole 8GB memory space, which AFAIK is the max available
on that smartphone family. But maybe the device-tree is lying, who
knows...

Can you try booting *without* my patch and this in the kernel command
line: "cma=16M@0x100000000-0x200000000".

Regards,
Nicolas

And here is my kernel tree just in case
> https://github.com/pundiramit/linux/commits/beryllium-mainline
>
> Regards,
> Amit Pundir
>
>
> > Regards,
> > Nicolas
> >
> > > Regards,
> > > Amit Pundir
> > >
> > > > Reported-by: Jeremy Linton <[email protected]>
> > > > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > > > ---
> > > >
> > > > An more costly alternative would be adding an option to
> > > > dma_alloc_from_contiguous() so it fails when the allocation
> > > > doesn't
> > > > fall
> > > > in a specific zone.
> > > >
> > > > kernel/dma/pool.c | 11 ++---------

2020-07-21 11:29:35

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, Jul 21, 2020 at 01:15:23PM +0200, Nicolas Saenz Julienne wrote:
> I'm at loss at what could be failing here. Your device should be able
> to address the whole 8GB memory space, which AFAIK is the max available
> on that smartphone family. But maybe the device-tree is lying, who
> knows...

Maybe we should give your patch to allocate from CMA but check the
address a try? (just because we can..)

2020-07-21 11:39:08

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 2020-07-21 at 13:28 +0200, Christoph Hellwig wrote:
> On Tue, Jul 21, 2020 at 01:15:23PM +0200, Nicolas Saenz Julienne
> wrote:
> > I'm at loss at what could be failing here. Your device should be
> > able
> > to address the whole 8GB memory space, which AFAIK is the max
> > available
> > on that smartphone family. But maybe the device-tree is lying, who
> > knows...
>
> Maybe we should give your patch to allocate from CMA but check the
> address a try? (just because we can..)

Yes, good idea!

Amir, could you also test this patch[1] (having reverted the one that
casues trouble) and report on whether it boots or not?

Regards,
Nicolas

[1] https://lore.kernel.org/linux-iomhttps://lore.kernel.org/linux-iom
mu/[email protected]/T/#t

2020-07-21 12:16:35

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 21 Jul 2020 at 16:45, Nicolas Saenz Julienne
<[email protected]> wrote:
>
> On Tue, 2020-07-21 at 14:24 +0530, Amit Pundir wrote:
> > On Tue, 21 Jul 2020 at 14:09, Nicolas Saenz Julienne
> > <[email protected]> wrote:
> > > Hi Amit,
> > > > Hi Nicolas,
> > > >
> > > > I see a boot regression with this commit d9765e41d8e9 "dma-pool:
> > > > Do not allocate pool memory from CMA" on my Xiaomi Poco F1
> > > > (Qcom sdm845) phone running v5.8-rc6. I can't boot past the
> > > > bootloader splash screen with this patch.
> > > >
> > > > Phone boots fine if I revert this patch. I carry only one out of
> > > > tree
> > > > dts patch https://lkml.org/lkml/2020/6/25/52. And since this is a
> > > > stock
> > > > phone, I don't have access to serial/dmesg logs until I boot to
> > > > AOSP
> > > > (adb) shell.
> > > >
> > > > Any thoughts as to what might be going wrong here? I'd be happy
> > > > to
> > > > help debug things. For what it's worth, I don't see this
> > > > regression
> > > > on
> > > > other two sdm845 devices (db845c and Pixel 3) I tested on.
> > >
> > > Can you provide a boot log (even if without my patch) and the
> > > device-
> > > tree files? It'd help a lot figuring things out.
> >
> > Thank you for the prompt reply Nicolas.
> >
> > Here is the boot log with the reverted patch
> > https://pastebin.ubuntu.com/p/BrhPf83nKF/
> >
> > Here is my phone's dts
> > https://github.com/pundiramit/linux/commit/2a394c199deeaf4c91e0e008e8fba2a72f494d8c
>
> I'm at loss at what could be failing here. Your device should be able
> to address the whole 8GB memory space, which AFAIK is the max available
> on that smartphone family. But maybe the device-tree is lying, who
> knows...

If it helps, my phone has 6GB memory space.

>
> Can you try booting *without* my patch and this in the kernel command
> line: "cma=16M@0x100000000-0x200000000".

It doesn't boot with this added kernel command line.

Regards,
Amit Pundir

>
> Regards,
> Nicolas
>
> And here is my kernel tree just in case
> > https://github.com/pundiramit/linux/commits/beryllium-mainline
> >
> > Regards,
> > Amit Pundir
> >
> >
> > > Regards,
> > > Nicolas
> > >
> > > > Regards,
> > > > Amit Pundir
> > > >
> > > > > Reported-by: Jeremy Linton <[email protected]>
> > > > > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > > > > ---
> > > > >
> > > > > An more costly alternative would be adding an option to
> > > > > dma_alloc_from_contiguous() so it fails when the allocation
> > > > > doesn't
> > > > > fall
> > > > > in a specific zone.
> > > > >
> > > > > kernel/dma/pool.c | 11 ++---------
>

2020-07-21 12:19:49

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 21 Jul 2020 at 17:07, Nicolas Saenz Julienne
<[email protected]> wrote:
>
> On Tue, 2020-07-21 at 13:28 +0200, Christoph Hellwig wrote:
> > On Tue, Jul 21, 2020 at 01:15:23PM +0200, Nicolas Saenz Julienne
> > wrote:
> > > I'm at loss at what could be failing here. Your device should be
> > > able
> > > to address the whole 8GB memory space, which AFAIK is the max
> > > available
> > > on that smartphone family. But maybe the device-tree is lying, who
> > > knows...
> >
> > Maybe we should give your patch to allocate from CMA but check the
> > address a try? (just because we can..)
>
> Yes, good idea!
>
> Amir, could you also test this patch[1] (having reverted the one that
> casues trouble) and report on whether it boots or not?

Can't boot with that patch either.

Regards,
Amit Pundir

>
> Regards,
> Nicolas
>
> [1] https://lore.kernel.org/linux-iomhttps://lore.kernel.org/linux-iom
> mu/[email protected]/T/#t
>
>

2020-07-21 12:46:14

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 2020-07-21 at 17:45 +0530, Amit Pundir wrote:
> On Tue, 21 Jul 2020 at 16:45, Nicolas Saenz Julienne
> <[email protected]> wrote:
> > On Tue, 2020-07-21 at 14:24 +0530, Amit Pundir wrote:
> > > On Tue, 21 Jul 2020 at 14:09, Nicolas Saenz Julienne
> > > <[email protected]> wrote:
> > > > Hi Amit,
> > > > > Hi Nicolas,
> > > > >
> > > > > I see a boot regression with this commit d9765e41d8e9 "dma-
> > > > > pool:
> > > > > Do not allocate pool memory from CMA" on my Xiaomi Poco F1
> > > > > (Qcom sdm845) phone running v5.8-rc6. I can't boot past the
> > > > > bootloader splash screen with this patch.
> > > > >
> > > > > Phone boots fine if I revert this patch. I carry only one out
> > > > > of
> > > > > tree
> > > > > dts patch https://lkml.org/lkml/2020/6/25/52. And since this
> > > > > is a
> > > > > stock
> > > > > phone, I don't have access to serial/dmesg logs until I boot
> > > > > to
> > > > > AOSP
> > > > > (adb) shell.
> > > > >
> > > > > Any thoughts as to what might be going wrong here? I'd be
> > > > > happy
> > > > > to
> > > > > help debug things. For what it's worth, I don't see this
> > > > > regression
> > > > > on
> > > > > other two sdm845 devices (db845c and Pixel 3) I tested on.
> > > >
> > > > Can you provide a boot log (even if without my patch) and the
> > > > device-
> > > > tree files? It'd help a lot figuring things out.
> > >
> > > Thank you for the prompt reply Nicolas.
> > >
> > > Here is the boot log with the reverted patch
> > > https://pastebin.ubuntu.com/p/BrhPf83nKF/
> > >
> > > Here is my phone's dts
> > > https://github.com/pundiramit/linux/commit/2a394c199deeaf4c91e0e008e8fba2a72f494d8c
> >
> > I'm at loss at what could be failing here. Your device should be
> > able
> > to address the whole 8GB memory space, which AFAIK is the max
> > available
> > on that smartphone family. But maybe the device-tree is lying, who
> > knows...
>
> If it helps, my phone has 6GB memory space.
>
> > Can you try booting *without* my patch and this in the kernel
> > command
> > line: "cma=16M@0x100000000-0x200000000".
>
> It doesn't boot with this added kernel command line.

For the record, this placed the CMA in the [4GB, 8GB] address space
instead of you setup's default: [3GB, 4GB]. All atomic pools fall in
that memory area without my patch, which makes me think some of the
devices on your board might not like higher addresses.

What happens if you boot with my troublesome patch with this in your
device tree? (insert it at the bottom of sdm845-beryllium.dts)

&soc {
dma-ranges = <0 0 0 0 0x1 0>;
};

Regards,
Nicolas

> Regards,
> Amit Pundir
>
> > Regards,
> > Nicolas
> >
> > And here is my kernel tree just in case
> > > https://github.com/pundiramit/linux/commits/beryllium-mainline
> > >
> > > Regards,
> > > Amit Pundir
> > >
> > >
> > > > Regards,
> > > > Nicolas
> > > >
> > > > > Regards,
> > > > > Amit Pundir
> > > > >
> > > > > > Reported-by: Jeremy Linton <[email protected]>
> > > > > > Signed-off-by: Nicolas Saenz Julienne <
> > > > > > [email protected]>
> > > > > > ---
> > > > > >
> > > > > > An more costly alternative would be adding an option to
> > > > > > dma_alloc_from_contiguous() so it fails when the allocation
> > > > > > doesn't
> > > > > > fall
> > > > > > in a specific zone.
> > > > > >
> > > > > > kernel/dma/pool.c | 11 ++---------

2020-07-21 15:23:33

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 21 Jul 2020 at 18:15, Nicolas Saenz Julienne
<[email protected]> wrote:
>
> On Tue, 2020-07-21 at 17:45 +0530, Amit Pundir wrote:
> > On Tue, 21 Jul 2020 at 16:45, Nicolas Saenz Julienne
> > <[email protected]> wrote:
> > > On Tue, 2020-07-21 at 14:24 +0530, Amit Pundir wrote:
> > > > On Tue, 21 Jul 2020 at 14:09, Nicolas Saenz Julienne
> > > > <[email protected]> wrote:
> > > > > Hi Amit,
> > > > > > Hi Nicolas,
> > > > > >
> > > > > > I see a boot regression with this commit d9765e41d8e9 "dma-
> > > > > > pool:
> > > > > > Do not allocate pool memory from CMA" on my Xiaomi Poco F1
> > > > > > (Qcom sdm845) phone running v5.8-rc6. I can't boot past the
> > > > > > bootloader splash screen with this patch.
> > > > > >
> > > > > > Phone boots fine if I revert this patch. I carry only one out
> > > > > > of
> > > > > > tree
> > > > > > dts patch https://lkml.org/lkml/2020/6/25/52. And since this
> > > > > > is a
> > > > > > stock
> > > > > > phone, I don't have access to serial/dmesg logs until I boot
> > > > > > to
> > > > > > AOSP
> > > > > > (adb) shell.
> > > > > >
> > > > > > Any thoughts as to what might be going wrong here? I'd be
> > > > > > happy
> > > > > > to
> > > > > > help debug things. For what it's worth, I don't see this
> > > > > > regression
> > > > > > on
> > > > > > other two sdm845 devices (db845c and Pixel 3) I tested on.
> > > > >
> > > > > Can you provide a boot log (even if without my patch) and the
> > > > > device-
> > > > > tree files? It'd help a lot figuring things out.
> > > >
> > > > Thank you for the prompt reply Nicolas.
> > > >
> > > > Here is the boot log with the reverted patch
> > > > https://pastebin.ubuntu.com/p/BrhPf83nKF/
> > > >
> > > > Here is my phone's dts
> > > > https://github.com/pundiramit/linux/commit/2a394c199deeaf4c91e0e008e8fba2a72f494d8c
> > >
> > > I'm at loss at what could be failing here. Your device should be
> > > able
> > > to address the whole 8GB memory space, which AFAIK is the max
> > > available
> > > on that smartphone family. But maybe the device-tree is lying, who
> > > knows...
> >
> > If it helps, my phone has 6GB memory space.
> >
> > > Can you try booting *without* my patch and this in the kernel
> > > command
> > > line: "cma=16M@0x100000000-0x200000000".
> >
> > It doesn't boot with this added kernel command line.
>
>
> For the record, this placed the CMA in the [4GB, 8GB] address space
> instead of you setup's default: [3GB, 4GB]. All atomic pools fall in
> that memory area without my patch, which makes me think some of the
> devices on your board might not like higher addresses.
>

Thank you Nicolas for the details. Though we don't set the CMA
alloc-ranges explicitly in upstream sdm845 dts, but I dug around and
found that CMA alloc-ranges in the downstream kernel are indeed in
lower address space.
https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/dipper-q-oss/arch/arm64/boot/dts/qcom/sdm845.dtsi#L662

/* global autoconfigured region for contiguous allocations */
linux,cma {
compatible = "shared-dma-pool";
alloc-ranges = <0 0x00000000 0 0xffffffff>;
reusable;
alignment = <0 0x400000>;
size = <0 0x2000000>;
linux,cma-default;
};

> What happens if you boot with my troublesome patch with this in your
> device tree? (insert it at the bottom of sdm845-beryllium.dts)
>
> &soc {
> dma-ranges = <0 0 0 0 0x1 0>;
> };
>

Device still doesn't boot up to adb shell.

Regards,
Amit Pundir

> Regards,
> Nicolas
>
> > Regards,
> > Amit Pundir
> >
> > > Regards,
> > > Nicolas
> > >
> > > And here is my kernel tree just in case
> > > > https://github.com/pundiramit/linux/commits/beryllium-mainline
> > > >
> > > > Regards,
> > > > Amit Pundir
> > > >
> > > >
> > > > > Regards,
> > > > > Nicolas
> > > > >
> > > > > > Regards,
> > > > > > Amit Pundir
> > > > > >
> > > > > > > Reported-by: Jeremy Linton <[email protected]>
> > > > > > > Signed-off-by: Nicolas Saenz Julienne <
> > > > > > > [email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > An more costly alternative would be adding an option to
> > > > > > > dma_alloc_from_contiguous() so it fails when the allocation
> > > > > > > doesn't
> > > > > > > fall
> > > > > > > in a specific zone.
> > > > > > >
> > > > > > > kernel/dma/pool.c | 11 ++---------
>

2020-07-21 16:29:57

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 2020-07-21 at 20:52 +0530, Amit Pundir wrote:

[...]

> > > > Can you try booting *without* my patch and this in the kernel
> > > > command
> > > > line: "cma=16M@0x100000000-0x200000000".
> > >
> > > It doesn't boot with this added kernel command line.
> >
> > For the record, this placed the CMA in the [4GB, 8GB] address space
> > instead of you setup's default: [3GB, 4GB]. All atomic pools fall
> > in
> > that memory area without my patch, which makes me think some of the
> > devices on your board might not like higher addresses.
> >
>
> Thank you Nicolas for the details. Though we don't set the CMA
> alloc-ranges explicitly in upstream sdm845 dts, but I dug around and
> found that CMA alloc-ranges in the downstream kernel are indeed in
> lower address space.
> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/dipper-q-oss/arch/arm64/boot/dts/qcom/sdm845.dtsi#L662
>
> /* global autoconfigured region for contiguous allocations */
> linux,cma {
> compatible = "shared-dma-pool";
> alloc-ranges = <0 0x00000000 0 0xffffffff>;
> reusable;
> alignment = <0 0x400000>;
> size = <0 0x2000000>;
> linux,cma-default;
> };

Pretty standard, and similar to what it's being used upstream by
default.

>
> > What happens if you boot with my troublesome patch with this in
> > your
> > device tree? (insert it at the bottom of sdm845-beryllium.dts)
> >
> > &soc {
> > dma-ranges = <0 0 0 0 0x1 0>;
> > };
> >
>
> Device still doesn't boot up to adb shell.

Let's get a bigger hammer, I'm just looking for clues here. Can you
apply this and provide the dmesg output.

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 6bc74a2d5127..2160676bf488 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -268,6 +268,8 @@ void *dma_alloc_from_pool(struct device *dev, size_t size,
schedule_work(&atomic_pool_work);
}

+ dev_info(dev, "%s: size %lx, phys addr %llx, flags 0x%x\n", __func__, size, phys, flags);
+
return ptr;
}

Regards,
Nicolas

2020-07-23 05:16:41

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

Hi Nicolas,

Sorry I got stuck on other things yesterday.

On Tue, 21 Jul 2020 at 21:57, Nicolas Saenz Julienne
<[email protected]> wrote:
>
> On Tue, 2020-07-21 at 20:52 +0530, Amit Pundir wrote:
>
> [...]
>
> > > > > Can you try booting *without* my patch and this in the kernel
> > > > > command
> > > > > line: "cma=16M@0x100000000-0x200000000".
> > > >
> > > > It doesn't boot with this added kernel command line.
> > >
> > > For the record, this placed the CMA in the [4GB, 8GB] address space
> > > instead of you setup's default: [3GB, 4GB]. All atomic pools fall
> > > in
> > > that memory area without my patch, which makes me think some of the
> > > devices on your board might not like higher addresses.
> > >
> >
> > Thank you Nicolas for the details. Though we don't set the CMA
> > alloc-ranges explicitly in upstream sdm845 dts, but I dug around and
> > found that CMA alloc-ranges in the downstream kernel are indeed in
> > lower address space.
> > https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/dipper-q-oss/arch/arm64/boot/dts/qcom/sdm845.dtsi#L662
> >
> > /* global autoconfigured region for contiguous allocations */
> > linux,cma {
> > compatible = "shared-dma-pool";
> > alloc-ranges = <0 0x00000000 0 0xffffffff>;
> > reusable;
> > alignment = <0 0x400000>;
> > size = <0 0x2000000>;
> > linux,cma-default;
> > };
>
> Pretty standard, and similar to what it's being used upstream by
> default.
>
> >
> > > What happens if you boot with my troublesome patch with this in
> > > your
> > > device tree? (insert it at the bottom of sdm845-beryllium.dts)
> > >
> > > &soc {
> > > dma-ranges = <0 0 0 0 0x1 0>;
> > > };
> > >
> >
> > Device still doesn't boot up to adb shell.
>
> Let's get a bigger hammer, I'm just looking for clues here. Can you
> apply this and provide the dmesg output.
>
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 6bc74a2d5127..2160676bf488 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -268,6 +268,8 @@ void *dma_alloc_from_pool(struct device *dev, size_t size,
> schedule_work(&atomic_pool_work);
> }
>
> + dev_info(dev, "%s: size %lx, phys addr %llx, flags 0x%x\n", __func__, size, phys, flags);
> +
> return ptr;
> }

I never made it to dma_alloc_from_pool() call from
dma_direct_alloc_pages(), dma_should_alloc_from_pool() returns False
from gfpflags_allow_blocking() block.

Regards,
Amit Pundir

>
>
> Regards,
> Nicolas
>

2020-07-24 09:40:27

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

Hi Amit,

On Thu, 2020-07-23 at 10:44 +0530, Amit Pundir wrote:
> Hi Nicolas,
>
> Sorry I got stuck on other things yesterday.

No worries :)

> On Tue, 21 Jul 2020 at 21:57, Nicolas Saenz Julienne

[...]

> >
> > Let's get a bigger hammer, I'm just looking for clues here. Can you
> > apply this and provide the dmesg output.
> >
> > diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> > index 6bc74a2d5127..2160676bf488 100644
> > --- a/kernel/dma/pool.c
> > +++ b/kernel/dma/pool.c
> > @@ -268,6 +268,8 @@ void *dma_alloc_from_pool(struct device *dev, size_t size,
> > schedule_work(&atomic_pool_work);
> > }
> >
> > + dev_info(dev, "%s: size %lx, phys addr %llx, flags 0x%x\n", __func__, size, phys, flags);
> > +
> > return ptr;
> > }
>
> I never made it to dma_alloc_from_pool() call from
> dma_direct_alloc_pages(), dma_should_alloc_from_pool() returns False
> from gfpflags_allow_blocking() block.

I'm a little sceptical about this. The only way you can get memory from these
pools is through dma_alloc_from_pool(), and given how changes in the pool
initialization affected the phone, it's pretty clear some amount of pool
allocation is happening.

Regards,
Nicolas

2020-07-24 11:10:34

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Fri, 24 Jul 2020 at 15:06, Nicolas Saenz Julienne
<[email protected]> wrote:
>
> Hi Amit,
>
> On Thu, 2020-07-23 at 10:44 +0530, Amit Pundir wrote:
> > Hi Nicolas,
> >
> > Sorry I got stuck on other things yesterday.
>
> No worries :)
>
> > On Tue, 21 Jul 2020 at 21:57, Nicolas Saenz Julienne
>
> [...]
>
> > >
> > > Let's get a bigger hammer, I'm just looking for clues here. Can you
> > > apply this and provide the dmesg output.
> > >
> > > diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> > > index 6bc74a2d5127..2160676bf488 100644
> > > --- a/kernel/dma/pool.c
> > > +++ b/kernel/dma/pool.c
> > > @@ -268,6 +268,8 @@ void *dma_alloc_from_pool(struct device *dev, size_t size,
> > > schedule_work(&atomic_pool_work);
> > > }
> > >
> > > + dev_info(dev, "%s: size %lx, phys addr %llx, flags 0x%x\n", __func__, size, phys, flags);
> > > +
> > > return ptr;
> > > }
> >
> > I never made it to dma_alloc_from_pool() call from
> > dma_direct_alloc_pages(), dma_should_alloc_from_pool() returns False
> > from gfpflags_allow_blocking() block.
>
> I'm a little sceptical about this. The only way you can get memory from these
> pools is through dma_alloc_from_pool(), and given how changes in the pool
> initialization affected the phone, it's pretty clear some amount of pool
> allocation is happening.

Maybe from here iommu_dma_alloc_remap()?

I see two paths to dma_alloc_from_pool(), one from
dma_direct_alloc_pages() which I mentioned above and
2nd one is thru iommu_dma_alloc(), but the flow doesn't
reach up to dma_alloc_from_pool(), and always returns
from iommu_dma_alloc_remap().

Regards,
Amit Pundir

>
> Regards,
> Nicolas
>

2020-07-24 13:41:59

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

Yes, the iommu is an interesting case, and the current code is
wrong for that. Can you try the patch below? It contains a modified
version of Nicolas' patch to try CMA again for the expansion and a new
(for now hackish) way to not apply the addressability check for dma-iommu
allocations.

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 6bc74a2d51273e..ec5e525d2b9309 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -3,7 +3,9 @@
* Copyright (C) 2012 ARM Ltd.
* Copyright (C) 2020 Google LLC
*/
+#include <linux/cma.h>
#include <linux/debugfs.h>
+#include <linux/dma-contiguous.h>
#include <linux/dma-direct.h>
#include <linux/dma-noncoherent.h>
#include <linux/init.h>
@@ -55,6 +57,31 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t size)
pool_size_kernel += size;
}

+static bool cma_in_zone(gfp_t gfp)
+{
+ phys_addr_t end;
+ unsigned long size;
+ struct cma *cma;
+
+ cma = dev_get_cma_area(NULL);
+ if (!cma)
+ return false;
+
+ size = cma_get_size(cma);
+ if (!size)
+ return false;
+ end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
+
+ /* CMA can't cross zone boundaries, see cma_activate_area() */
+ if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA) &&
+ end <= DMA_BIT_MASK(zone_dma_bits))
+ return true;
+ if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32) &&
+ end <= DMA_BIT_MASK(32))
+ return true;
+ return true;
+}
+
static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
gfp_t gfp)
{
@@ -68,7 +95,11 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,

do {
pool_size = 1 << (PAGE_SHIFT + order);
- page = alloc_pages(gfp, order);
+ if (cma_in_zone(gfp))
+ page = dma_alloc_from_contiguous(NULL, 1 << order,
+ order, false);
+ if (!page)
+ page = alloc_pages(gfp, order);
} while (!page && order-- > 0);
if (!page)
goto out;
@@ -251,7 +282,11 @@ void *dma_alloc_from_pool(struct device *dev, size_t size,
continue;

phys = gen_pool_virt_to_phys(pool, val);
- if (dma_coherent_ok(dev, phys, size))
+ /*
+ * Only apply the addressability check for dma-direct.
+ * This is a nasty hack and won't work e.g. for arm.
+ */
+ if (get_dma_ops(dev) || dma_coherent_ok(dev, phys, size))
break;

gen_pool_free(pool, val, size);

2020-07-24 16:20:47

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Fri, 24 Jul 2020 at 19:11, Christoph Hellwig <[email protected]> wrote:
>
> Yes, the iommu is an interesting case, and the current code is
> wrong for that. Can you try the patch below? It contains a modified
> version of Nicolas' patch to try CMA again for the expansion and a new
> (for now hackish) way to not apply the addressability check for dma-iommu
> allocations.

Thank you. The below patch worked on today's linux/master (f37e99aca03f).

Regards,
Amit Pundir

>
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 6bc74a2d51273e..ec5e525d2b9309 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -3,7 +3,9 @@
> * Copyright (C) 2012 ARM Ltd.
> * Copyright (C) 2020 Google LLC
> */
> +#include <linux/cma.h>
> #include <linux/debugfs.h>
> +#include <linux/dma-contiguous.h>
> #include <linux/dma-direct.h>
> #include <linux/dma-noncoherent.h>
> #include <linux/init.h>
> @@ -55,6 +57,31 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t size)
> pool_size_kernel += size;
> }
>
> +static bool cma_in_zone(gfp_t gfp)
> +{
> + phys_addr_t end;
> + unsigned long size;
> + struct cma *cma;
> +
> + cma = dev_get_cma_area(NULL);
> + if (!cma)
> + return false;
> +
> + size = cma_get_size(cma);
> + if (!size)
> + return false;
> + end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
> +
> + /* CMA can't cross zone boundaries, see cma_activate_area() */
> + if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA) &&
> + end <= DMA_BIT_MASK(zone_dma_bits))
> + return true;
> + if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32) &&
> + end <= DMA_BIT_MASK(32))
> + return true;
> + return true;
> +}
> +
> static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> gfp_t gfp)
> {
> @@ -68,7 +95,11 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
>
> do {
> pool_size = 1 << (PAGE_SHIFT + order);
> - page = alloc_pages(gfp, order);
> + if (cma_in_zone(gfp))
> + page = dma_alloc_from_contiguous(NULL, 1 << order,
> + order, false);
> + if (!page)
> + page = alloc_pages(gfp, order);
> } while (!page && order-- > 0);
> if (!page)
> goto out;
> @@ -251,7 +282,11 @@ void *dma_alloc_from_pool(struct device *dev, size_t size,
> continue;
>
> phys = gen_pool_virt_to_phys(pool, val);
> - if (dma_coherent_ok(dev, phys, size))
> + /*
> + * Only apply the addressability check for dma-direct.
> + * This is a nasty hack and won't work e.g. for arm.
> + */
> + if (get_dma_ops(dev) || dma_coherent_ok(dev, phys, size))
> break;
>
> gen_pool_free(pool, val, size);

2020-07-24 16:29:47

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Fri, Jul 24, 2020 at 09:49:17PM +0530, Amit Pundir wrote:
> On Fri, 24 Jul 2020 at 19:11, Christoph Hellwig <[email protected]> wrote:
> >
> > Yes, the iommu is an interesting case, and the current code is
> > wrong for that. Can you try the patch below? It contains a modified
> > version of Nicolas' patch to try CMA again for the expansion and a new
> > (for now hackish) way to not apply the addressability check for dma-iommu
> > allocations.
>
> Thank you. The below patch worked on today's linux/master (f37e99aca03f).

Ok, I'll try to massage it into a few cleaner patches over the weekend
and will send them to you again for retesting.

Thanks a lot for your patience!

2020-07-27 19:04:40

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

Hi Christoph,
thanks for having a look at this!

On Fri, 2020-07-24 at 15:41 +0200, Christoph Hellwig wrote:
> Yes, the iommu is an interesting case, and the current code is
> wrong for that.

Care to expand on this? I do get that checking dma_coherent_ok() on memory
that'll later on be mapped into an iommu is kind of silly, although I think
harmless in Amir's specific case, since devices have wide enough dma-ranges. Is
there more to it?

> Can you try the patch below? It contains a modified version of Nicolas'
> patch to try CMA again for the expansion and a new (for now hackish) way to
> not apply the addressability check for dma-iommu allocations.
>
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 6bc74a2d51273e..ec5e525d2b9309 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -3,7 +3,9 @@
> * Copyright (C) 2012 ARM Ltd.
> * Copyright (C) 2020 Google LLC
> */
> +#include <linux/cma.h>
> #include <linux/debugfs.h>
> +#include <linux/dma-contiguous.h>
> #include <linux/dma-direct.h>
> #include <linux/dma-noncoherent.h>
> #include <linux/init.h>
> @@ -55,6 +57,31 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t
> size)
> pool_size_kernel += size;
> }
>
> +static bool cma_in_zone(gfp_t gfp)
> +{
> + phys_addr_t end;
> + unsigned long size;
> + struct cma *cma;
> +
> + cma = dev_get_cma_area(NULL);
> + if (!cma)
> + return false;
> +
> + size = cma_get_size(cma);
> + if (!size)
> + return false;
> + end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
> +
> + /* CMA can't cross zone boundaries, see cma_activate_area() */
> + if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA) &&
> + end <= DMA_BIT_MASK(zone_dma_bits))
> + return true;
> + if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32) &&
> + end <= DMA_BIT_MASK(32))
> + return true;
> + return true;

IIUC this will always return true given a CMA is present. Which reverts to the
previous behaviour (previous as in breaking some rpi4 setups), isn't it?

Regards,
Nicolas

Attachments:

signature.asc (499.00 B)
This is a digitally signed message part

2020-07-28 09:14:12

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Mon, Jul 27, 2020 at 07:56:56PM +0200, Nicolas Saenz Julienne wrote:
> Hi Christoph,
> thanks for having a look at this!
>
> On Fri, 2020-07-24 at 15:41 +0200, Christoph Hellwig wrote:
> > Yes, the iommu is an interesting case, and the current code is
> > wrong for that.
>
> Care to expand on this? I do get that checking dma_coherent_ok() on memory
> that'll later on be mapped into an iommu is kind of silly, although I think
> harmless in Amir's specific case, since devices have wide enough dma-ranges. Is
> there more to it?

I think the problem is that it can lead to not finding suitable memory.

>
> > Can you try the patch below? It contains a modified version of Nicolas'
> > patch to try CMA again for the expansion and a new (for now hackish) way to
> > not apply the addressability check for dma-iommu allocations.
> >
> > diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> > index 6bc74a2d51273e..ec5e525d2b9309 100644
> > --- a/kernel/dma/pool.c
> > +++ b/kernel/dma/pool.c
> > @@ -3,7 +3,9 @@
> > * Copyright (C) 2012 ARM Ltd.
> > * Copyright (C) 2020 Google LLC
> > */
> > +#include <linux/cma.h>
> > #include <linux/debugfs.h>
> > +#include <linux/dma-contiguous.h>
> > #include <linux/dma-direct.h>
> > #include <linux/dma-noncoherent.h>
> > #include <linux/init.h>
> > @@ -55,6 +57,31 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t
> > size)
> > pool_size_kernel += size;
> > }
> >
> > +static bool cma_in_zone(gfp_t gfp)
> > +{
> > + phys_addr_t end;
> > + unsigned long size;
> > + struct cma *cma;
> > +
> > + cma = dev_get_cma_area(NULL);
> > + if (!cma)
> > + return false;
> > +
> > + size = cma_get_size(cma);
> > + if (!size)
> > + return false;
> > + end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
> > +
> > + /* CMA can't cross zone boundaries, see cma_activate_area() */
> > + if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA) &&
> > + end <= DMA_BIT_MASK(zone_dma_bits))
> > + return true;
> > + if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32) &&
> > + end <= DMA_BIT_MASK(32))
> > + return true;
> > + return true;
>
> IIUC this will always return true given a CMA is present. Which reverts to the
> previous behaviour (previous as in breaking some rpi4 setups), isn't it?

Was that really what broke the PI? I'll try to get the split out series
today, which might have a few more tweaks, and then we'll need to test it
both on these rpi4 setups and Amits phone.

2020-07-28 09:32:05

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, 2020-07-28 at 11:13 +0200, Christoph Hellwig wrote:
> On Mon, Jul 27, 2020 at 07:56:56PM +0200, Nicolas Saenz Julienne wrote:
> > Hi Christoph,
> > thanks for having a look at this!
> >
> > On Fri, 2020-07-24 at 15:41 +0200, Christoph Hellwig wrote:
> > > Yes, the iommu is an interesting case, and the current code is
> > > wrong for that.
> >
> > Care to expand on this? I do get that checking dma_coherent_ok() on memory
> > that'll later on be mapped into an iommu is kind of silly, although I think
> > harmless in Amir's specific case, since devices have wide enough dma-
ranges.
> > Is
> > there more to it?
>
> I think the problem is that it can lead to not finding suitable memory.
>
> > > Can you try the patch below? It contains a modified version of Nicolas'
> > > patch to try CMA again for the expansion and a new (for now hackish) way
> > > to
> > > not apply the addressability check for dma-iommu allocations.
> > >
> > > diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> > > index 6bc74a2d51273e..ec5e525d2b9309 100644
> > > --- a/kernel/dma/pool.c
> > > +++ b/kernel/dma/pool.c
> > > @@ -3,7 +3,9 @@
> > > * Copyright (C) 2012 ARM Ltd.
> > > * Copyright (C) 2020 Google LLC
> > > */
> > > +#include <linux/cma.h>
> > > #include <linux/debugfs.h>
> > > +#include <linux/dma-contiguous.h>
> > > #include <linux/dma-direct.h>
> > > #include <linux/dma-noncoherent.h>
> > > #include <linux/init.h>
> > > @@ -55,6 +57,31 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t
> > > size)
> > > pool_size_kernel += size;
> > > }
> > >
> > > +static bool cma_in_zone(gfp_t gfp)
> > > +{
> > > + phys_addr_t end;
> > > + unsigned long size;
> > > + struct cma *cma;
> > > +
> > > + cma = dev_get_cma_area(NULL);
> > > + if (!cma)
> > > + return false;
> > > +
> > > + size = cma_get_size(cma);
> > > + if (!size)
> > > + return false;
> > > + end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
> > > +
> > > + /* CMA can't cross zone boundaries, see cma_activate_area() */
> > > + if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA) &&
> > > + end <= DMA_BIT_MASK(zone_dma_bits))
> > > + return true;
> > > + if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32) &&
> > > + end <= DMA_BIT_MASK(32))
> > > + return true;
> > > + return true;
> >
> > IIUC this will always return true given a CMA is present. Which reverts to
> > the
> > previous behaviour (previous as in breaking some rpi4 setups), isn't it?
>
> Was that really what broke the PI? I'll try to get the split out series
> today, which might have a few more tweaks, and then we'll need to test it
> both on these rpi4 setups and Amits phone.

There was two issues with RPi:
- Not validating that pool allocated memory was OK for the device
- Locating all atomic pools in CMA, which doesn't work for all RPi4 devices*,
and IMO misses the point of having multiple pools.

* With ACPI RPi4 we have CMA located in ZONE_DMA32, yet have an atomic pool
consumer, PCIe, that only wants memory in the [0 3GB] area, effectively needing
ZONE_DMA memory.

Attachments:

signature.asc (499.00 B)
This is a digitally signed message part

2020-07-28 10:12:55

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, Jul 28, 2020 at 11:30:32AM +0200, Nicolas Saenz Julienne wrote:
> On Tue, 2020-07-28 at 11:13 +0200, Christoph Hellwig wrote:
> > On Mon, Jul 27, 2020 at 07:56:56PM +0200, Nicolas Saenz Julienne wrote:
> > > Hi Christoph,
> > > thanks for having a look at this!
> > >
> > > On Fri, 2020-07-24 at 15:41 +0200, Christoph Hellwig wrote:
> > > > Yes, the iommu is an interesting case, and the current code is
> > > > wrong for that.
> > >
> > > Care to expand on this? I do get that checking dma_coherent_ok() on memory
> > > that'll later on be mapped into an iommu is kind of silly, although I think
> > > harmless in Amir's specific case, since devices have wide enough dma-
> ranges.
> > > Is
> > > there more to it?
> >
> > I think the problem is that it can lead to not finding suitable memory.
> >
> > > > Can you try the patch below? It contains a modified version of Nicolas'
> > > > patch to try CMA again for the expansion and a new (for now hackish) way
> > > > to
> > > > not apply the addressability check for dma-iommu allocations.
> > > >
> > > > diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> > > > index 6bc74a2d51273e..ec5e525d2b9309 100644
> > > > --- a/kernel/dma/pool.c
> > > > +++ b/kernel/dma/pool.c
> > > > @@ -3,7 +3,9 @@
> > > > * Copyright (C) 2012 ARM Ltd.
> > > > * Copyright (C) 2020 Google LLC
> > > > */
> > > > +#include <linux/cma.h>
> > > > #include <linux/debugfs.h>
> > > > +#include <linux/dma-contiguous.h>
> > > > #include <linux/dma-direct.h>
> > > > #include <linux/dma-noncoherent.h>
> > > > #include <linux/init.h>
> > > > @@ -55,6 +57,31 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t
> > > > size)
> > > > pool_size_kernel += size;
> > > > }
> > > >
> > > > +static bool cma_in_zone(gfp_t gfp)
> > > > +{
> > > > + phys_addr_t end;
> > > > + unsigned long size;
> > > > + struct cma *cma;
> > > > +
> > > > + cma = dev_get_cma_area(NULL);
> > > > + if (!cma)
> > > > + return false;
> > > > +
> > > > + size = cma_get_size(cma);
> > > > + if (!size)
> > > > + return false;
> > > > + end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
> > > > +
> > > > + /* CMA can't cross zone boundaries, see cma_activate_area() */
> > > > + if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA) &&
> > > > + end <= DMA_BIT_MASK(zone_dma_bits))
> > > > + return true;
> > > > + if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32) &&
> > > > + end <= DMA_BIT_MASK(32))
> > > > + return true;
> > > > + return true;
> > >
> > > IIUC this will always return true given a CMA is present. Which reverts to
> > > the
> > > previous behaviour (previous as in breaking some rpi4 setups), isn't it?
> >
> > Was that really what broke the PI? I'll try to get the split out series
> > today, which might have a few more tweaks, and then we'll need to test it
> > both on these rpi4 setups and Amits phone.
>
> There was two issues with RPi:
> - Not validating that pool allocated memory was OK for the device
> - Locating all atomic pools in CMA, which doesn't work for all RPi4 devices*,
> and IMO misses the point of having multiple pools.
>
> * With ACPI RPi4 we have CMA located in ZONE_DMA32, yet have an atomic pool
> consumer, PCIe, that only wants memory in the [0 3GB] area, effectively needing
> ZONE_DMA memory.

Ok, I found a slight bug that wasn't intended. I wanted to make sure
we can always fall back to a lower pool, but got that wrong. Should be
fixed in the next version.

2020-07-31 01:11:04

by Nathan Chancellor

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Tue, Jul 28, 2020 at 12:09:18PM +0200, Christoph Hellwig wrote:
> Ok, I found a slight bug that wasn't intended. I wanted to make sure
> we can always fall back to a lower pool, but got that wrong. Should be
> fixed in the next version.

Hi Christoph and Nicolas,

Did a version of that series ever get send out? I am coming into the
conversation late but I am running into an issue with the Raspberry Pi 4
not booting on linux-next, which appears to be due to this patch now in
mainline as commit d9765e41d8e9 ("dma-pool: do not allocate pool memory
from CMA") combined with
https://lore.kernel.org/lkml/[email protected]/
in -next:

[ 1.423163] raspberrypi-firmware soc:firmware: Request 0x00000001 returned status 0x00000000
[ 1.431883] raspberrypi-firmware soc:firmware: Request 0x00030046 returned status 0x00000000
[ 1.443888] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.452527] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 0 config (-22 80)
[ 1.460836] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.469445] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1 config (-22 81)
[ 1.477735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.486350] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2 config (-22 82)
[ 1.494639] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.503246] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 3 config (-22 83)
[ 1.511529] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.520131] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4 config (-22 84)
[ 1.528414] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.537017] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 5 config (-22 85)
[ 1.545299] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.553903] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6 config (-22 86)
[ 1.562184] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.570787] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 7 config (-22 87)
[ 1.579897] raspberrypi-firmware soc:firmware: Request 0x00030030 returned status 0x00000000
[ 1.589419] raspberrypi-firmware soc:firmware: Request 0x00028001 returned status 0x00000000
[ 1.599391] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.608018] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1 config (-22 81)
[ 1.616313] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.624932] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1 config (-22 81)
[ 1.633195] pwrseq_simple: probe of wifi-pwrseq failed with error -22
[ 1.643904] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.652544] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2 config (-22 82)
[ 1.660839] raspberrypi-firmware soc:firmware: Request 0x00030041 returned status 0x00000000
[ 1.669446] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2 state (-22 82)
[ 1.677727] leds-gpio: probe of leds failed with error -22
[ 1.683735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.692346] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6 config (-22 86)
[ 1.700636] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.709240] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6 config (-22 86)
[ 1.717496] reg-fixed-voltage: probe of sd_vcc_reg failed with error -22
[ 1.725546] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.734176] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4 config (-22 84)
[ 1.742465] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
[ 1.751072] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4 config (-22 84)
[ 1.759332] gpio-regulator: probe of sd_io_1v8_reg failed with error -22
[ 1.768042] raspberrypi-firmware soc:firmware: Request 0x00028001 returned status 0x00000000
[ 1.780871] ALSA device list:
[ 1.783960] No soundcards found.
[ 1.787633] Waiting for root device PARTUUID=45a8dd8a-02...

I am unsure if it is related to the issue that Amit is having or
if that makes sense at all but I can reliably reproduce it.

v5.8-rc1: OK
v5.8-rc1 + d9765e41d8e9e: OK
v5.8-rc1 + "of_address: Add bus type match for pci ranges parser": OK
v5.8-rc1 + both: BROKEN

I wanted to test the series to see if this fixes anything. If you would
prefer a different thread for this or further information, please let
me know.

Cheers,
Nathan

2020-07-31 07:21:07

[permalink] [raw]

Subject: Re: [PATCH] dma-pool: Do not allocate pool memory from CMA

On Fri, 31 Jul 2020 at 06:40, Nathan Chancellor
<[email protected]> wrote:
>
> On Tue, Jul 28, 2020 at 12:09:18PM +0200, Christoph Hellwig wrote:
> > Ok, I found a slight bug that wasn't intended. I wanted to make sure
> > we can always fall back to a lower pool, but got that wrong. Should be
> > fixed in the next version.
>
> Hi Christoph and Nicolas,
>
> Did a version of that series ever get send out? I am coming into the
> conversation late but I am running into an issue with the Raspberry Pi 4
> not booting on linux-next, which appears to be due to this patch now in
> mainline as commit d9765e41d8e9 ("dma-pool: do not allocate pool memory
> from CMA") combined with
> https://lore.kernel.org/lkml/[email protected]/
> in -next:
>
> [ 1.423163] raspberrypi-firmware soc:firmware: Request 0x00000001 returned status 0x00000000
> [ 1.431883] raspberrypi-firmware soc:firmware: Request 0x00030046 returned status 0x00000000
> [ 1.443888] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.452527] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 0 config (-22 80)
> [ 1.460836] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.469445] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1 config (-22 81)
> [ 1.477735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.486350] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2 config (-22 82)
> [ 1.494639] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.503246] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 3 config (-22 83)
> [ 1.511529] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.520131] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4 config (-22 84)
> [ 1.528414] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.537017] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 5 config (-22 85)
> [ 1.545299] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.553903] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6 config (-22 86)
> [ 1.562184] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.570787] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 7 config (-22 87)
> [ 1.579897] raspberrypi-firmware soc:firmware: Request 0x00030030 returned status 0x00000000
> [ 1.589419] raspberrypi-firmware soc:firmware: Request 0x00028001 returned status 0x00000000
> [ 1.599391] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.608018] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1 config (-22 81)
> [ 1.616313] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.624932] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1 config (-22 81)
> [ 1.633195] pwrseq_simple: probe of wifi-pwrseq failed with error -22
> [ 1.643904] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.652544] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2 config (-22 82)
> [ 1.660839] raspberrypi-firmware soc:firmware: Request 0x00030041 returned status 0x00000000
> [ 1.669446] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2 state (-22 82)
> [ 1.677727] leds-gpio: probe of leds failed with error -22
> [ 1.683735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.692346] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6 config (-22 86)
> [ 1.700636] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.709240] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6 config (-22 86)
> [ 1.717496] reg-fixed-voltage: probe of sd_vcc_reg failed with error -22
> [ 1.725546] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.734176] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4 config (-22 84)
> [ 1.742465] raspberrypi-firmware soc:firmware: Request 0x00030043 returned status 0x00000000
> [ 1.751072] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4 config (-22 84)
> [ 1.759332] gpio-regulator: probe of sd_io_1v8_reg failed with error -22
> [ 1.768042] raspberrypi-firmware soc:firmware: Request 0x00028001 returned status 0x00000000
> [ 1.780871] ALSA device list:
> [ 1.783960] No soundcards found.
> [ 1.787633] Waiting for root device PARTUUID=45a8dd8a-02...
>
> I am unsure if it is related to the issue that Amit is having or
> if that makes sense at all but I can reliably reproduce it.
>
> v5.8-rc1: OK
> v5.8-rc1 + d9765e41d8e9e: OK
> v5.8-rc1 + "of_address: Add bus type match for pci ranges parser": OK
> v5.8-rc1 + both: BROKEN
>
> I wanted to test the series to see if this fixes anything. If you would
> prefer a different thread for this or further information, please let
> me know.

Hi Nathan,

Here is the patch series:
https://lists.linuxfoundation.org/pipermail/iommu/2020-July/047008.html
https://lists.linuxfoundation.org/pipermail/iommu/2020-July/047010.html

I hope it works for you, it didn't work for me. More details in this
thread here:
https://lists.linuxfoundation.org/pipermail/iommu/2020-July/047009.html

Regards,
Amit Pundir

>
> Cheers,
> Nathan

2020-07-31 10:11:15