* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-03 8:24 kernel test robot
2020-10-03 8:24 ` [PATCH] dma-buf: system_heap: fix odd_ptr_err.cocci warnings kernel test robot
0 siblings, 1 reply; 23+ messages in thread
From: kernel test robot @ 2020-10-03 8:24 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20201003040257.62768-8-john.stultz@linaro.org>
References: <20201003040257.62768-8-john.stultz@linaro.org>
TO: John Stultz <john.stultz@linaro.org>
TO: lkml <linux-kernel@vger.kernel.org>
CC: John Stultz <john.stultz@linaro.org>
CC: Sumit Semwal <sumit.semwal@linaro.org>
CC: Liam Mark <lmark@codeaurora.org>
CC: Laura Abbott <labbott@kernel.org>
CC: Brian Starkey <Brian.Starkey@arm.com>
CC: Hridya Valsaraju <hridya@google.com>
CC: Suren Baghdasaryan <surenb@google.com>
CC: Sandeep Patil <sspatil@google.com>
CC: Daniel Mentz <danielmentz@google.com>
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on tegra-drm/drm/tegra/for-next linus/master v5.9-rc7]
[cannot apply to next-20201002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
config: i386-randconfig-c001-20201002 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>
echo
echo "coccinelle warnings: (new ones prefixed by >>)"
echo
>> drivers/dma-buf/heaps/system_heap.c:495:5-11: inconsistent IS_ERR and PTR_ERR on line 496.
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29576 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] dma-buf: system_heap: fix odd_ptr_err.cocci warnings
2020-10-03 8:24 [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap kernel test robot
@ 2020-10-03 8:24 ` kernel test robot
0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-10-03 8:24 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20201003040257.62768-8-john.stultz@linaro.org>
References: <20201003040257.62768-8-john.stultz@linaro.org>
TO: John Stultz <john.stultz@linaro.org>
TO: lkml <linux-kernel@vger.kernel.org>
CC: John Stultz <john.stultz@linaro.org>
CC: Sumit Semwal <sumit.semwal@linaro.org>
CC: Liam Mark <lmark@codeaurora.org>
CC: Laura Abbott <labbott@kernel.org>
CC: Brian Starkey <Brian.Starkey@arm.com>
CC: Hridya Valsaraju <hridya@google.com>
CC: Suren Baghdasaryan <surenb@google.com>
CC: Sandeep Patil <sspatil@google.com>
CC: Daniel Mentz <danielmentz@google.com>
From: kernel test robot <lkp@intel.com>
drivers/dma-buf/heaps/system_heap.c:495:5-11: inconsistent IS_ERR and PTR_ERR on line 496.
PTR_ERR should access the value just tested by IS_ERR
Semantic patch information:
There can be false positives in the patch case, where it is the call to
IS_ERR that is wrong.
Generated by: scripts/coccinelle/tests/odd_ptr_err.cocci
CC: John Stultz <john.stultz@linaro.org>
Signed-off-by: kernel test robot <lkp@intel.com>
---
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
Please take the patch only if it's a positive warning. Thanks!
system_heap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -493,7 +493,7 @@ static int system_heap_create(void)
sys_uncached_heap = dma_heap_add(&exp_info);
if (IS_ERR(sys_uncached_heap))
- return PTR_ERR(sys_heap);
+ return PTR_ERR(sys_uncached_heap);
dma_coerce_mask_and_coherent(dma_heap_get_dev(sys_uncached_heap), DMA_BIT_MASK(64));
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-16 19:03 ` John Stultz
@ 2021-01-29 1:23 ` Daniel Mentz
-1 siblings, 0 replies; 23+ messages in thread
From: Daniel Mentz @ 2021-01-29 1:23 UTC (permalink / raw)
To: John Stultz
Cc: Brian Starkey, lkml, Sumit Semwal, Liam Mark, Laura Abbott,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Chris Goldsworthy, Ørjan Eide, Robin Murphy,
Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel,
nd
On Fri, Oct 16, 2020 at 12:04 PM John Stultz <john.stultz@linaro.org> wrote:
>
> On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@arm.com> wrote:
> > On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> > > @@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
> > > /* just return, as put will call release and that will free */
> > > return ret;
> > > }
> > > +
> > > + /*
> > > + * For uncached buffers, we need to initially flush cpu cache, since
> > > + * the __GFP_ZERO on the allocation means the zeroing was done by the
> > > + * cpu and thus it is likely cached. Map (and implicitly flush) it out
> > > + * now so we don't get corruption later on.
> > > + */
> > > + if (buffer->uncached)
> > > + dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
> >
> > Do we have to keep this mapping around for the entire lifetime of the
> > buffer?
>
> Yea, I guess we can just map and unmap it right there. It will look a
> little absurd, but that sort of aligns with your next point.
>
> > Also, this problem (and solution) keeps lingering around. It really
> > feels like there should be a better way to solve "clean the linear
> > mapping all the way to DRAM", but I don't know what that should be.
>
> Yea, something better here would be nice...
In ION, we had a little helper function named
ion_buffer_prep_noncached that called arch_dma_prep_coherent() on all
sg entries like so
for_each_sg(table->sgl, sg, table->orig_nents, i)
arch_dma_prep_coherent(sg_page(sg), sg->length);
Would that help?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2021-01-29 1:23 ` Daniel Mentz
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Mentz @ 2021-01-29 1:23 UTC (permalink / raw)
To: John Stultz
Cc: Sandeep Patil, dri-devel, Ezequiel Garcia, Robin Murphy,
James Jones, lkml, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, nd, Suren Baghdasaryan,
linux-media
On Fri, Oct 16, 2020 at 12:04 PM John Stultz <john.stultz@linaro.org> wrote:
>
> On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@arm.com> wrote:
> > On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> > > @@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
> > > /* just return, as put will call release and that will free */
> > > return ret;
> > > }
> > > +
> > > + /*
> > > + * For uncached buffers, we need to initially flush cpu cache, since
> > > + * the __GFP_ZERO on the allocation means the zeroing was done by the
> > > + * cpu and thus it is likely cached. Map (and implicitly flush) it out
> > > + * now so we don't get corruption later on.
> > > + */
> > > + if (buffer->uncached)
> > > + dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
> >
> > Do we have to keep this mapping around for the entire lifetime of the
> > buffer?
>
> Yea, I guess we can just map and unmap it right there. It will look a
> little absurd, but that sort of aligns with your next point.
>
> > Also, this problem (and solution) keeps lingering around. It really
> > feels like there should be a better way to solve "clean the linear
> > mapping all the way to DRAM", but I don't know what that should be.
>
> Yea, something better here would be nice...
In ION, we had a little helper function named
ion_buffer_prep_noncached that called arch_dma_prep_coherent() on all
sg entries like so
for_each_sg(table->sgl, sg, table->orig_nents, i)
arch_dma_prep_coherent(sg_page(sg), sg->length);
Would that help?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-16 19:03 ` John Stultz
@ 2020-10-16 23:15 ` John Stultz
-1 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-16 23:15 UTC (permalink / raw)
To: Brian Starkey
Cc: lkml, Sumit Semwal, Liam Mark, Laura Abbott, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz,
Chris Goldsworthy, Ørjan Eide, Robin Murphy,
Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel,
nd
On Fri, Oct 16, 2020 at 12:03 PM John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@arm.com> wrote:
> > On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> > > @@ -426,6 +487,16 @@ static int system_heap_create(void)
> > > if (IS_ERR(sys_heap))
> > > return PTR_ERR(sys_heap);
> > >
> > > + exp_info.name = "system-uncached";
> > > + exp_info.ops = &system_uncached_heap_ops;
> > > + exp_info.priv = NULL;
> > > +
> > > + sys_uncached_heap = dma_heap_add(&exp_info);
> > > + if (IS_ERR(sys_uncached_heap))
> > > + return PTR_ERR(sys_heap);
> > > +
> >
> > In principle, there's a race here between the heap getting registered
> > to sysfs and the dma_mask getting updated.
> >
> > I don't think it would cause a problem in practice, but with the API
> > as it is, there's no way to avoid it - so I wonder if the
> > dma_heap_get_dev() accessor isn't the right API design.
>
> Hrm. I guess to address your concern we would need split
> dma_heap_add() into something like:
> dma_heap_create()
> dma_heap_add()
>
> Which breaks the creation of the heap with the registering it to the
> subsystem, so some attributes can be tweaked inbetween?
Looking at this some more, this approach isn't going to work. We
create the device and then we call dma_coerce_mask_and_coherent() on
it, but as soon as the device is created it seems possible for
userland to directly access it. Again, though, as you mentioned this
isn't terribly likely in practice.
The best thing I can think of for now is to have the uncached heap's
allocate pointer initially point to a dummy function that returns
EBUSY and then after we update the dma mask then we can set it to the
real alloc. I'll go with that for now, but let me know if you have
other suggestions.
thanks
-john
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-16 23:15 ` John Stultz
0 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-16 23:15 UTC (permalink / raw)
To: Brian Starkey
Cc: nd, Sandeep Patil, dri-devel, Ezequiel Garcia, Robin Murphy,
James Jones, lkml, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Daniel Mentz
On Fri, Oct 16, 2020 at 12:03 PM John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@arm.com> wrote:
> > On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> > > @@ -426,6 +487,16 @@ static int system_heap_create(void)
> > > if (IS_ERR(sys_heap))
> > > return PTR_ERR(sys_heap);
> > >
> > > + exp_info.name = "system-uncached";
> > > + exp_info.ops = &system_uncached_heap_ops;
> > > + exp_info.priv = NULL;
> > > +
> > > + sys_uncached_heap = dma_heap_add(&exp_info);
> > > + if (IS_ERR(sys_uncached_heap))
> > > + return PTR_ERR(sys_heap);
> > > +
> >
> > In principle, there's a race here between the heap getting registered
> > to sysfs and the dma_mask getting updated.
> >
> > I don't think it would cause a problem in practice, but with the API
> > as it is, there's no way to avoid it - so I wonder if the
> > dma_heap_get_dev() accessor isn't the right API design.
>
> Hrm. I guess to address your concern we would need split
> dma_heap_add() into something like:
> dma_heap_create()
> dma_heap_add()
>
> Which breaks the creation of the heap with the registering it to the
> subsystem, so some attributes can be tweaked inbetween?
Looking at this some more, this approach isn't going to work. We
create the device and then we call dma_coerce_mask_and_coherent() on
it, but as soon as the device is created it seems possible for
userland to directly access it. Again, though, as you mentioned this
isn't terribly likely in practice.
The best thing I can think of for now is to have the uncached heap's
allocate pointer initially point to a dummy function that returns
EBUSY and then after we update the dma mask then we can set it to the
real alloc. I'll go with that for now, but let me know if you have
other suggestions.
thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-08 11:51 ` Brian Starkey
@ 2020-10-16 19:03 ` John Stultz
-1 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-16 19:03 UTC (permalink / raw)
To: Brian Starkey
Cc: lkml, Sumit Semwal, Liam Mark, Laura Abbott, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz,
Chris Goldsworthy, Ørjan Eide, Robin Murphy,
Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel,
nd
On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@arm.com> wrote:
> On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> > @@ -215,8 +236,12 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
> > struct page **pages = vmalloc(sizeof(struct page *) * npages);
> > struct page **tmp = pages;
> > struct sg_page_iter piter;
> > + pgprot_t pgprot = PAGE_KERNEL;
> > void *vaddr;
> >
> > + if (buffer->uncached)
> > + pgprot = pgprot_writecombine(PAGE_KERNEL);
>
> I think this should go after the 'if (!pages)' check. Best to get the
> allocation failure check as close to the allocation as possible IMO.
Sounds good. Changed in my tree.
> > @@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
> > /* just return, as put will call release and that will free */
> > return ret;
> > }
> > +
> > + /*
> > + * For uncached buffers, we need to initially flush cpu cache, since
> > + * the __GFP_ZERO on the allocation means the zeroing was done by the
> > + * cpu and thus it is likely cached. Map (and implicitly flush) it out
> > + * now so we don't get corruption later on.
> > + */
> > + if (buffer->uncached)
> > + dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
>
> Do we have to keep this mapping around for the entire lifetime of the
> buffer?
Yea, I guess we can just map and unmap it right there. It will look a
little absurd, but that sort of aligns with your next point.
> Also, this problem (and solution) keeps lingering around. It really
> feels like there should be a better way to solve "clean the linear
> mapping all the way to DRAM", but I don't know what that should be.
Yea, something better here would be nice...
> > @@ -426,6 +487,16 @@ static int system_heap_create(void)
> > if (IS_ERR(sys_heap))
> > return PTR_ERR(sys_heap);
> >
> > + exp_info.name = "system-uncached";
> > + exp_info.ops = &system_uncached_heap_ops;
> > + exp_info.priv = NULL;
> > +
> > + sys_uncached_heap = dma_heap_add(&exp_info);
> > + if (IS_ERR(sys_uncached_heap))
> > + return PTR_ERR(sys_heap);
> > +
>
> In principle, there's a race here between the heap getting registered
> to sysfs and the dma_mask getting updated.
>
> I don't think it would cause a problem in practice, but with the API
> as it is, there's no way to avoid it - so I wonder if the
> dma_heap_get_dev() accessor isn't the right API design.
Hrm. I guess to address your concern we would need split
dma_heap_add() into something like:
dma_heap_create()
dma_heap_add()
Which breaks the creation of the heap with the registering it to the
subsystem, so some attributes can be tweaked inbetween?
I'll see about taking a stab at this, but I'll probably submit my
updated series sooner with this un-addressed so I can get some further
review.
thanks
-john
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-16 19:03 ` John Stultz
0 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-16 19:03 UTC (permalink / raw)
To: Brian Starkey
Cc: nd, Sandeep Patil, dri-devel, Ezequiel Garcia, Robin Murphy,
James Jones, lkml, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Daniel Mentz
On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@arm.com> wrote:
> On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> > @@ -215,8 +236,12 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
> > struct page **pages = vmalloc(sizeof(struct page *) * npages);
> > struct page **tmp = pages;
> > struct sg_page_iter piter;
> > + pgprot_t pgprot = PAGE_KERNEL;
> > void *vaddr;
> >
> > + if (buffer->uncached)
> > + pgprot = pgprot_writecombine(PAGE_KERNEL);
>
> I think this should go after the 'if (!pages)' check. Best to get the
> allocation failure check as close to the allocation as possible IMO.
Sounds good. Changed in my tree.
> > @@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
> > /* just return, as put will call release and that will free */
> > return ret;
> > }
> > +
> > + /*
> > + * For uncached buffers, we need to initially flush cpu cache, since
> > + * the __GFP_ZERO on the allocation means the zeroing was done by the
> > + * cpu and thus it is likely cached. Map (and implicitly flush) it out
> > + * now so we don't get corruption later on.
> > + */
> > + if (buffer->uncached)
> > + dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
>
> Do we have to keep this mapping around for the entire lifetime of the
> buffer?
Yea, I guess we can just map and unmap it right there. It will look a
little absurd, but that sort of aligns with your next point.
> Also, this problem (and solution) keeps lingering around. It really
> feels like there should be a better way to solve "clean the linear
> mapping all the way to DRAM", but I don't know what that should be.
Yea, something better here would be nice...
> > @@ -426,6 +487,16 @@ static int system_heap_create(void)
> > if (IS_ERR(sys_heap))
> > return PTR_ERR(sys_heap);
> >
> > + exp_info.name = "system-uncached";
> > + exp_info.ops = &system_uncached_heap_ops;
> > + exp_info.priv = NULL;
> > +
> > + sys_uncached_heap = dma_heap_add(&exp_info);
> > + if (IS_ERR(sys_uncached_heap))
> > + return PTR_ERR(sys_heap);
> > +
>
> In principle, there's a race here between the heap getting registered
> to sysfs and the dma_mask getting updated.
>
> I don't think it would cause a problem in practice, but with the API
> as it is, there's no way to avoid it - so I wonder if the
> dma_heap_get_dev() accessor isn't the right API design.
Hrm. I guess to address your concern we would need split
dma_heap_add() into something like:
dma_heap_create()
dma_heap_add()
Which breaks the creation of the heap with the registering it to the
subsystem, so some attributes can be tweaked inbetween?
I'll see about taking a stab at this, but I'll probably submit my
updated series sooner with this un-addressed so I can get some further
review.
thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-03 4:02 ` John Stultz
@ 2020-10-08 11:51 ` Brian Starkey
-1 siblings, 0 replies; 23+ messages in thread
From: Brian Starkey @ 2020-10-08 11:51 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Sumit Semwal, Liam Mark, Laura Abbott, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz,
Chris Goldsworthy, Ørjan Eide, Robin Murphy,
Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel,
nd
On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> This adds a heap that allocates non-contiguous buffers that are
> marked as writecombined, so they are not cached by the CPU.
>
> This is useful, as most graphics buffers are usually not touched
> by the CPU or only written into once by the CPU. So when mapping
> the buffer over and over between devices, we can skip the CPU
> syncing, which saves a lot of cache management overhead, greatly
> improving performance.
>
> For folk using ION, there was a ION_FLAG_CACHED flag, which
> signaled if the returned buffer should be CPU cacheable or not.
> With DMA-BUF heaps, we do not yet have such a flag, and by default
> the current heaps (system and cma) produce CPU cachable buffers.
> So for folks transitioning from ION to DMA-BUF Heaps, this fills
> in some of that missing functionality.
>
> There has been a suggestion to make this functionality a flag
> (DMAHEAP_FLAG_UNCACHED?) on the system heap, similar to how
> ION used the ION_FLAG_CACHED. But I want to make sure an
> _UNCACHED flag would truely be a generic attribute across all
> heaps. So far that has been unclear, so having it as a separate
> heap seemes better for now. (But I'm open to discussion on this
> point!)
>
> This is a rework of earlier efforts to add a uncached system heap,
> done utilizing the exisitng system heap, adding just a bit of
> logic to handle the uncached case.
>
> Feedback would be very welcome!
>
> Many thanks to Liam Mark for his help to get this working.
>
> Pending opensource users of this code include:
> * AOSP HiKey960 gralloc:
> - https://android-review.googlesource.com/c/device/linaro/hikey/+/1399519
> - Visibly improves performance over the system heap
> * AOSP Codec2 (possibly, needs more review):
> - https://android-review.googlesource.com/c/platform/frameworks/av/+/1360640/17/media/codec2/vndk/C2DmaBufAllocator.cpp#325
>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Laura Abbott <labbott@kernel.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Sandeep Patil <sspatil@google.com>
> Cc: Daniel Mentz <danielmentz@google.com>
> Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> Cc: �rjan Eide <orjan.eide@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Ezequiel Garcia <ezequiel@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: James Jones <jajones@nvidia.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> drivers/dma-buf/heaps/system_heap.c | 87 ++++++++++++++++++++++++++---
> 1 file changed, 79 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 2b8d4b6abacb..952f1fd9dacf 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -22,6 +22,7 @@
> #include <linux/vmalloc.h>
>
> struct dma_heap *sys_heap;
> +struct dma_heap *sys_uncached_heap;
>
> struct system_heap_buffer {
> struct dma_heap *heap;
> @@ -31,6 +32,8 @@ struct system_heap_buffer {
> struct sg_table sg_table;
> int vmap_cnt;
> void *vaddr;
> +
> + bool uncached;
> };
>
> struct dma_heap_attachment {
> @@ -38,6 +41,8 @@ struct dma_heap_attachment {
> struct sg_table *table;
> struct list_head list;
> bool mapped;
> +
> + bool uncached;
> };
>
> #define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \
> @@ -94,7 +99,7 @@ static int system_heap_attach(struct dma_buf *dmabuf,
> a->dev = attachment->dev;
> INIT_LIST_HEAD(&a->list);
> a->mapped = false;
> -
> + a->uncached = buffer->uncached;
> attachment->priv = a;
>
> mutex_lock(&buffer->lock);
> @@ -124,9 +129,13 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac
> {
> struct dma_heap_attachment *a = attachment->priv;
> struct sg_table *table = a->table;
> + int attr = 0;
> int ret;
>
> - ret = dma_map_sgtable(attachment->dev, table, direction, 0);
> + if (a->uncached)
> + attr = DMA_ATTR_SKIP_CPU_SYNC;
> +
> + ret = dma_map_sgtable(attachment->dev, table, direction, attr);
> if (ret)
> return ERR_PTR(ret);
>
> @@ -139,9 +148,12 @@ static void system_heap_unmap_dma_buf(struct dma_buf_attachment *attachment,
> enum dma_data_direction direction)
> {
> struct dma_heap_attachment *a = attachment->priv;
> + int attr = 0;
>
> + if (a->uncached)
> + attr = DMA_ATTR_SKIP_CPU_SYNC;
> a->mapped = false;
> - dma_unmap_sgtable(attachment->dev, table, direction, 0);
> + dma_unmap_sgtable(attachment->dev, table, direction, attr);
> }
>
> static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
> @@ -150,6 +162,9 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
> struct system_heap_buffer *buffer = dmabuf->priv;
> struct dma_heap_attachment *a;
>
> + if (buffer->uncached)
> + return 0;
> +
> mutex_lock(&buffer->lock);
>
> if (buffer->vmap_cnt)
> @@ -171,6 +186,9 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
> struct system_heap_buffer *buffer = dmabuf->priv;
> struct dma_heap_attachment *a;
>
> + if (buffer->uncached)
> + return 0;
> +
> mutex_lock(&buffer->lock);
>
> if (buffer->vmap_cnt)
> @@ -194,6 +212,9 @@ static int system_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> struct sg_page_iter piter;
> int ret;
>
> + if (buffer->uncached)
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +
> for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
> struct page *page = sg_page_iter_page(&piter);
>
> @@ -215,8 +236,12 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
> struct page **pages = vmalloc(sizeof(struct page *) * npages);
> struct page **tmp = pages;
> struct sg_page_iter piter;
> + pgprot_t pgprot = PAGE_KERNEL;
> void *vaddr;
>
> + if (buffer->uncached)
> + pgprot = pgprot_writecombine(PAGE_KERNEL);
I think this should go after the 'if (!pages)' check. Best to get the
allocation failure check as close to the allocation as possible IMO.
> +
> if (!pages)
> return ERR_PTR(-ENOMEM);
>
> @@ -225,7 +250,7 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
> *tmp++ = sg_page_iter_page(&piter);
> }
>
> - vaddr = vmap(pages, npages, VM_MAP, PAGE_KERNEL);
> + vaddr = vmap(pages, npages, VM_MAP, pgprot);
> vfree(pages);
>
> if (!vaddr)
> @@ -278,6 +303,10 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
> int i;
>
> table = &buffer->sg_table;
> + /* Unmap the uncached buffers from the heap device (pairs with the map on allocate) */
> + if (buffer->uncached)
> + dma_unmap_sgtable(dma_heap_get_dev(buffer->heap), table, DMA_BIDIRECTIONAL, 0);
> +
> for_each_sg(table->sgl, sg, table->nents, i) {
> struct page *page = sg_page(sg);
>
> @@ -320,10 +349,11 @@ static struct page *alloc_largest_available(unsigned long size,
> return NULL;
> }
>
> -static int system_heap_allocate(struct dma_heap *heap,
> - unsigned long len,
> - unsigned long fd_flags,
> - unsigned long heap_flags)
> +static int system_heap_do_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags,
> + bool uncached)
> {
> struct system_heap_buffer *buffer;
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> @@ -344,6 +374,7 @@ static int system_heap_allocate(struct dma_heap *heap,
> mutex_init(&buffer->lock);
> buffer->heap = heap;
> buffer->len = len;
> + buffer->uncached = uncached;
>
> INIT_LIST_HEAD(&pages);
> i = 0;
> @@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
> /* just return, as put will call release and that will free */
> return ret;
> }
> +
> + /*
> + * For uncached buffers, we need to initially flush cpu cache, since
> + * the __GFP_ZERO on the allocation means the zeroing was done by the
> + * cpu and thus it is likely cached. Map (and implicitly flush) it out
> + * now so we don't get corruption later on.
> + */
> + if (buffer->uncached)
> + dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
Do we have to keep this mapping around for the entire lifetime of the
buffer?
Also, this problem (and solution) keeps lingering around. It really
feels like there should be a better way to solve "clean the linear
mapping all the way to DRAM", but I don't know what that should be.
> +
> return ret;
>
> free_pages:
> @@ -410,10 +451,30 @@ static int system_heap_allocate(struct dma_heap *heap,
> return ret;
> }
>
> +static int system_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> +{
> + return system_heap_do_allocate(heap, len, fd_flags, heap_flags, false);
> +}
> +
> static const struct dma_heap_ops system_heap_ops = {
> .allocate = system_heap_allocate,
> };
>
> +static int system_uncached_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> +{
> + return system_heap_do_allocate(heap, len, fd_flags, heap_flags, true);
> +}
> +
> +static const struct dma_heap_ops system_uncached_heap_ops = {
> + .allocate = system_uncached_heap_allocate,
> +};
> +
> static int system_heap_create(void)
> {
> struct dma_heap_export_info exp_info;
> @@ -426,6 +487,16 @@ static int system_heap_create(void)
> if (IS_ERR(sys_heap))
> return PTR_ERR(sys_heap);
>
> + exp_info.name = "system-uncached";
> + exp_info.ops = &system_uncached_heap_ops;
> + exp_info.priv = NULL;
> +
> + sys_uncached_heap = dma_heap_add(&exp_info);
> + if (IS_ERR(sys_uncached_heap))
> + return PTR_ERR(sys_heap);
> +
In principle, there's a race here between the heap getting registered
to sysfs and the dma_mask getting updated.
I don't think it would cause a problem in practice, but with the API
as it is, there's no way to avoid it - so I wonder if the
dma_heap_get_dev() accessor isn't the right API design.
Cheers,
-Brian
> + dma_coerce_mask_and_coherent(dma_heap_get_dev(sys_uncached_heap), DMA_BIT_MASK(64));
> +
> return 0;
> }
> module_init(system_heap_create);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-08 11:51 ` Brian Starkey
0 siblings, 0 replies; 23+ messages in thread
From: Brian Starkey @ 2020-10-08 11:51 UTC (permalink / raw)
To: John Stultz
Cc: nd, Sandeep Patil, dri-devel, Ezequiel Garcia, Robin Murphy,
James Jones, lkml, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Daniel Mentz
On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
> This adds a heap that allocates non-contiguous buffers that are
> marked as writecombined, so they are not cached by the CPU.
>
> This is useful, as most graphics buffers are usually not touched
> by the CPU or only written into once by the CPU. So when mapping
> the buffer over and over between devices, we can skip the CPU
> syncing, which saves a lot of cache management overhead, greatly
> improving performance.
>
> For folk using ION, there was a ION_FLAG_CACHED flag, which
> signaled if the returned buffer should be CPU cacheable or not.
> With DMA-BUF heaps, we do not yet have such a flag, and by default
> the current heaps (system and cma) produce CPU cachable buffers.
> So for folks transitioning from ION to DMA-BUF Heaps, this fills
> in some of that missing functionality.
>
> There has been a suggestion to make this functionality a flag
> (DMAHEAP_FLAG_UNCACHED?) on the system heap, similar to how
> ION used the ION_FLAG_CACHED. But I want to make sure an
> _UNCACHED flag would truely be a generic attribute across all
> heaps. So far that has been unclear, so having it as a separate
> heap seemes better for now. (But I'm open to discussion on this
> point!)
>
> This is a rework of earlier efforts to add a uncached system heap,
> done utilizing the exisitng system heap, adding just a bit of
> logic to handle the uncached case.
>
> Feedback would be very welcome!
>
> Many thanks to Liam Mark for his help to get this working.
>
> Pending opensource users of this code include:
> * AOSP HiKey960 gralloc:
> - https://android-review.googlesource.com/c/device/linaro/hikey/+/1399519
> - Visibly improves performance over the system heap
> * AOSP Codec2 (possibly, needs more review):
> - https://android-review.googlesource.com/c/platform/frameworks/av/+/1360640/17/media/codec2/vndk/C2DmaBufAllocator.cpp#325
>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Laura Abbott <labbott@kernel.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Sandeep Patil <sspatil@google.com>
> Cc: Daniel Mentz <danielmentz@google.com>
> Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> Cc: �rjan Eide <orjan.eide@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Ezequiel Garcia <ezequiel@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: James Jones <jajones@nvidia.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> drivers/dma-buf/heaps/system_heap.c | 87 ++++++++++++++++++++++++++---
> 1 file changed, 79 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 2b8d4b6abacb..952f1fd9dacf 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -22,6 +22,7 @@
> #include <linux/vmalloc.h>
>
> struct dma_heap *sys_heap;
> +struct dma_heap *sys_uncached_heap;
>
> struct system_heap_buffer {
> struct dma_heap *heap;
> @@ -31,6 +32,8 @@ struct system_heap_buffer {
> struct sg_table sg_table;
> int vmap_cnt;
> void *vaddr;
> +
> + bool uncached;
> };
>
> struct dma_heap_attachment {
> @@ -38,6 +41,8 @@ struct dma_heap_attachment {
> struct sg_table *table;
> struct list_head list;
> bool mapped;
> +
> + bool uncached;
> };
>
> #define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \
> @@ -94,7 +99,7 @@ static int system_heap_attach(struct dma_buf *dmabuf,
> a->dev = attachment->dev;
> INIT_LIST_HEAD(&a->list);
> a->mapped = false;
> -
> + a->uncached = buffer->uncached;
> attachment->priv = a;
>
> mutex_lock(&buffer->lock);
> @@ -124,9 +129,13 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac
> {
> struct dma_heap_attachment *a = attachment->priv;
> struct sg_table *table = a->table;
> + int attr = 0;
> int ret;
>
> - ret = dma_map_sgtable(attachment->dev, table, direction, 0);
> + if (a->uncached)
> + attr = DMA_ATTR_SKIP_CPU_SYNC;
> +
> + ret = dma_map_sgtable(attachment->dev, table, direction, attr);
> if (ret)
> return ERR_PTR(ret);
>
> @@ -139,9 +148,12 @@ static void system_heap_unmap_dma_buf(struct dma_buf_attachment *attachment,
> enum dma_data_direction direction)
> {
> struct dma_heap_attachment *a = attachment->priv;
> + int attr = 0;
>
> + if (a->uncached)
> + attr = DMA_ATTR_SKIP_CPU_SYNC;
> a->mapped = false;
> - dma_unmap_sgtable(attachment->dev, table, direction, 0);
> + dma_unmap_sgtable(attachment->dev, table, direction, attr);
> }
>
> static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
> @@ -150,6 +162,9 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
> struct system_heap_buffer *buffer = dmabuf->priv;
> struct dma_heap_attachment *a;
>
> + if (buffer->uncached)
> + return 0;
> +
> mutex_lock(&buffer->lock);
>
> if (buffer->vmap_cnt)
> @@ -171,6 +186,9 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
> struct system_heap_buffer *buffer = dmabuf->priv;
> struct dma_heap_attachment *a;
>
> + if (buffer->uncached)
> + return 0;
> +
> mutex_lock(&buffer->lock);
>
> if (buffer->vmap_cnt)
> @@ -194,6 +212,9 @@ static int system_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> struct sg_page_iter piter;
> int ret;
>
> + if (buffer->uncached)
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +
> for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
> struct page *page = sg_page_iter_page(&piter);
>
> @@ -215,8 +236,12 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
> struct page **pages = vmalloc(sizeof(struct page *) * npages);
> struct page **tmp = pages;
> struct sg_page_iter piter;
> + pgprot_t pgprot = PAGE_KERNEL;
> void *vaddr;
>
> + if (buffer->uncached)
> + pgprot = pgprot_writecombine(PAGE_KERNEL);
I think this should go after the 'if (!pages)' check. Best to get the
allocation failure check as close to the allocation as possible IMO.
> +
> if (!pages)
> return ERR_PTR(-ENOMEM);
>
> @@ -225,7 +250,7 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
> *tmp++ = sg_page_iter_page(&piter);
> }
>
> - vaddr = vmap(pages, npages, VM_MAP, PAGE_KERNEL);
> + vaddr = vmap(pages, npages, VM_MAP, pgprot);
> vfree(pages);
>
> if (!vaddr)
> @@ -278,6 +303,10 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
> int i;
>
> table = &buffer->sg_table;
> + /* Unmap the uncached buffers from the heap device (pairs with the map on allocate) */
> + if (buffer->uncached)
> + dma_unmap_sgtable(dma_heap_get_dev(buffer->heap), table, DMA_BIDIRECTIONAL, 0);
> +
> for_each_sg(table->sgl, sg, table->nents, i) {
> struct page *page = sg_page(sg);
>
> @@ -320,10 +349,11 @@ static struct page *alloc_largest_available(unsigned long size,
> return NULL;
> }
>
> -static int system_heap_allocate(struct dma_heap *heap,
> - unsigned long len,
> - unsigned long fd_flags,
> - unsigned long heap_flags)
> +static int system_heap_do_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags,
> + bool uncached)
> {
> struct system_heap_buffer *buffer;
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> @@ -344,6 +374,7 @@ static int system_heap_allocate(struct dma_heap *heap,
> mutex_init(&buffer->lock);
> buffer->heap = heap;
> buffer->len = len;
> + buffer->uncached = uncached;
>
> INIT_LIST_HEAD(&pages);
> i = 0;
> @@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
> /* just return, as put will call release and that will free */
> return ret;
> }
> +
> + /*
> + * For uncached buffers, we need to initially flush cpu cache, since
> + * the __GFP_ZERO on the allocation means the zeroing was done by the
> + * cpu and thus it is likely cached. Map (and implicitly flush) it out
> + * now so we don't get corruption later on.
> + */
> + if (buffer->uncached)
> + dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
Do we have to keep this mapping around for the entire lifetime of the
buffer?
Also, this problem (and solution) keeps lingering around. It really
feels like there should be a better way to solve "clean the linear
mapping all the way to DRAM", but I don't know what that should be.
> +
> return ret;
>
> free_pages:
> @@ -410,10 +451,30 @@ static int system_heap_allocate(struct dma_heap *heap,
> return ret;
> }
>
> +static int system_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> +{
> + return system_heap_do_allocate(heap, len, fd_flags, heap_flags, false);
> +}
> +
> static const struct dma_heap_ops system_heap_ops = {
> .allocate = system_heap_allocate,
> };
>
> +static int system_uncached_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> +{
> + return system_heap_do_allocate(heap, len, fd_flags, heap_flags, true);
> +}
> +
> +static const struct dma_heap_ops system_uncached_heap_ops = {
> + .allocate = system_uncached_heap_allocate,
> +};
> +
> static int system_heap_create(void)
> {
> struct dma_heap_export_info exp_info;
> @@ -426,6 +487,16 @@ static int system_heap_create(void)
> if (IS_ERR(sys_heap))
> return PTR_ERR(sys_heap);
>
> + exp_info.name = "system-uncached";
> + exp_info.ops = &system_uncached_heap_ops;
> + exp_info.priv = NULL;
> +
> + sys_uncached_heap = dma_heap_add(&exp_info);
> + if (IS_ERR(sys_uncached_heap))
> + return PTR_ERR(sys_heap);
> +
In principle, there's a race here between the heap getting registered
to sysfs and the dma_mask getting updated.
I don't think it would cause a problem in practice, but with the API
as it is, there's no way to avoid it - so I wonder if the
dma_heap_get_dev() accessor isn't the right API design.
Cheers,
-Brian
> + dma_coerce_mask_and_coherent(dma_heap_get_dev(sys_uncached_heap), DMA_BIT_MASK(64));
> +
> return 0;
> }
> module_init(system_heap_create);
> --
> 2.17.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-07 7:43 ` Dan Carpenter
@ 2020-10-08 5:38 ` John Stultz
-1 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-08 5:38 UTC (permalink / raw)
To: Dan Carpenter
Cc: kbuild, lkml, kbuild test robot, kbuild-all, Sumit Semwal,
Liam Mark, Laura Abbott, Brian Starkey, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz
On Wed, Oct 7, 2020 at 12:44 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hi John,
>
> url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
> config: i386-randconfig-m021-20201002 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/dma-buf/heaps/system_heap.c:496 system_heap_create() warn: passing zero to 'PTR_ERR'
>
> vim +/PTR_ERR +496 drivers/dma-buf/heaps/system_heap.c
>
> efa04fefebbd724 John Stultz 2019-12-03 478 static int system_heap_create(void)
> efa04fefebbd724 John Stultz 2019-12-03 479 {
> efa04fefebbd724 John Stultz 2019-12-03 480 struct dma_heap_export_info exp_info;
> efa04fefebbd724 John Stultz 2019-12-03 481
> 263e38f82cbb35b Andrew F. Davis 2019-12-16 482 exp_info.name = "system";
> efa04fefebbd724 John Stultz 2019-12-03 483 exp_info.ops = &system_heap_ops;
> efa04fefebbd724 John Stultz 2019-12-03 484 exp_info.priv = NULL;
> efa04fefebbd724 John Stultz 2019-12-03 485
> efa04fefebbd724 John Stultz 2019-12-03 486 sys_heap = dma_heap_add(&exp_info);
> efa04fefebbd724 John Stultz 2019-12-03 487 if (IS_ERR(sys_heap))
> a2e10cdd2e4d12a John Stultz 2020-10-03 488 return PTR_ERR(sys_heap);
> efa04fefebbd724 John Stultz 2019-12-03 489
> 553f4e0fafc5b3b John Stultz 2020-10-03 490 exp_info.name = "system-uncached";
> 553f4e0fafc5b3b John Stultz 2020-10-03 491 exp_info.ops = &system_uncached_heap_ops;
> 553f4e0fafc5b3b John Stultz 2020-10-03 492 exp_info.priv = NULL;
> 553f4e0fafc5b3b John Stultz 2020-10-03 493
> 553f4e0fafc5b3b John Stultz 2020-10-03 494 sys_uncached_heap = dma_heap_add(&exp_info);
> 553f4e0fafc5b3b John Stultz 2020-10-03 495 if (IS_ERR(sys_uncached_heap))
> 553f4e0fafc5b3b John Stultz 2020-10-03 @496 return PTR_ERR(sys_heap);
> ^^^^^^^^^^^^^^^^^
> This should be return PTR_ERR(sys_uncached_heap);
Oh nice! Very impressed that the tool caught that!
Thanks so much for the report! I'll fix it up here shortly.
-john
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-08 5:38 ` John Stultz
0 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-08 5:38 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]
On Wed, Oct 7, 2020 at 12:44 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hi John,
>
> url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
> config: i386-randconfig-m021-20201002 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/dma-buf/heaps/system_heap.c:496 system_heap_create() warn: passing zero to 'PTR_ERR'
>
> vim +/PTR_ERR +496 drivers/dma-buf/heaps/system_heap.c
>
> efa04fefebbd724 John Stultz 2019-12-03 478 static int system_heap_create(void)
> efa04fefebbd724 John Stultz 2019-12-03 479 {
> efa04fefebbd724 John Stultz 2019-12-03 480 struct dma_heap_export_info exp_info;
> efa04fefebbd724 John Stultz 2019-12-03 481
> 263e38f82cbb35b Andrew F. Davis 2019-12-16 482 exp_info.name = "system";
> efa04fefebbd724 John Stultz 2019-12-03 483 exp_info.ops = &system_heap_ops;
> efa04fefebbd724 John Stultz 2019-12-03 484 exp_info.priv = NULL;
> efa04fefebbd724 John Stultz 2019-12-03 485
> efa04fefebbd724 John Stultz 2019-12-03 486 sys_heap = dma_heap_add(&exp_info);
> efa04fefebbd724 John Stultz 2019-12-03 487 if (IS_ERR(sys_heap))
> a2e10cdd2e4d12a John Stultz 2020-10-03 488 return PTR_ERR(sys_heap);
> efa04fefebbd724 John Stultz 2019-12-03 489
> 553f4e0fafc5b3b John Stultz 2020-10-03 490 exp_info.name = "system-uncached";
> 553f4e0fafc5b3b John Stultz 2020-10-03 491 exp_info.ops = &system_uncached_heap_ops;
> 553f4e0fafc5b3b John Stultz 2020-10-03 492 exp_info.priv = NULL;
> 553f4e0fafc5b3b John Stultz 2020-10-03 493
> 553f4e0fafc5b3b John Stultz 2020-10-03 494 sys_uncached_heap = dma_heap_add(&exp_info);
> 553f4e0fafc5b3b John Stultz 2020-10-03 495 if (IS_ERR(sys_uncached_heap))
> 553f4e0fafc5b3b John Stultz 2020-10-03 @496 return PTR_ERR(sys_heap);
> ^^^^^^^^^^^^^^^^^
> This should be return PTR_ERR(sys_uncached_heap);
Oh nice! Very impressed that the tool caught that!
Thanks so much for the report! I'll fix it up here shortly.
-john
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-05 13:45 ` Christoph Hellwig
@ 2020-10-08 5:03 ` John Stultz
0 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-08 5:03 UTC (permalink / raw)
To: Christoph Hellwig
Cc: lkml, Sumit Semwal, Liam Mark, Laura Abbott, Brian Starkey,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Daniel Mentz, Chris Goldsworthy, ??rjan Eide, Robin Murphy,
Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel
On Mon, Oct 5, 2020 at 6:45 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> How is this going to deal with VIVT caches?
Hrm. That's a good question. I'm not sure I totally have my head
around it but, I guess we could make sure to call
invalidate_kernel_vmap_range() in begin_cpu_access() and
flush_kernel_vmap_range() in end_cpu_access() rather then exiting out
early as we do now?
Unless you have better guidance?
Worse case we could check CONFIG_CPU_CACHE_VIVT and not register the
system-uncached heap.
thanks
-john
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-08 5:03 ` John Stultz
0 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-08 5:03 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sandeep Patil, dri-devel, Ezequiel Garcia, Robin Murphy,
James Jones, lkml, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, ??rjan Eide, linux-media, Suren Baghdasaryan,
Daniel Mentz
On Mon, Oct 5, 2020 at 6:45 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> How is this going to deal with VIVT caches?
Hrm. That's a good question. I'm not sure I totally have my head
around it but, I guess we could make sure to call
invalidate_kernel_vmap_range() in begin_cpu_access() and
flush_kernel_vmap_range() in end_cpu_access() rather then exiting out
early as we do now?
Unless you have better guidance?
Worse case we could check CONFIG_CPU_CACHE_VIVT and not register the
system-uncached heap.
thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-03 4:02 ` John Stultz
(?)
@ 2020-10-07 7:43 ` Dan Carpenter
-1 siblings, 0 replies; 23+ messages in thread
From: Dan Carpenter @ 2020-10-07 7:43 UTC (permalink / raw)
To: kbuild, John Stultz, lkml
Cc: lkp, kbuild-all, John Stultz, Sumit Semwal, Liam Mark,
Laura Abbott, Brian Starkey, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz
[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]
Hi John,
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
config: i386-randconfig-m021-20201002 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/dma-buf/heaps/system_heap.c:496 system_heap_create() warn: passing zero to 'PTR_ERR'
vim +/PTR_ERR +496 drivers/dma-buf/heaps/system_heap.c
efa04fefebbd724 John Stultz 2019-12-03 478 static int system_heap_create(void)
efa04fefebbd724 John Stultz 2019-12-03 479 {
efa04fefebbd724 John Stultz 2019-12-03 480 struct dma_heap_export_info exp_info;
efa04fefebbd724 John Stultz 2019-12-03 481
263e38f82cbb35b Andrew F. Davis 2019-12-16 482 exp_info.name = "system";
efa04fefebbd724 John Stultz 2019-12-03 483 exp_info.ops = &system_heap_ops;
efa04fefebbd724 John Stultz 2019-12-03 484 exp_info.priv = NULL;
efa04fefebbd724 John Stultz 2019-12-03 485
efa04fefebbd724 John Stultz 2019-12-03 486 sys_heap = dma_heap_add(&exp_info);
efa04fefebbd724 John Stultz 2019-12-03 487 if (IS_ERR(sys_heap))
a2e10cdd2e4d12a John Stultz 2020-10-03 488 return PTR_ERR(sys_heap);
efa04fefebbd724 John Stultz 2019-12-03 489
553f4e0fafc5b3b John Stultz 2020-10-03 490 exp_info.name = "system-uncached";
553f4e0fafc5b3b John Stultz 2020-10-03 491 exp_info.ops = &system_uncached_heap_ops;
553f4e0fafc5b3b John Stultz 2020-10-03 492 exp_info.priv = NULL;
553f4e0fafc5b3b John Stultz 2020-10-03 493
553f4e0fafc5b3b John Stultz 2020-10-03 494 sys_uncached_heap = dma_heap_add(&exp_info);
553f4e0fafc5b3b John Stultz 2020-10-03 495 if (IS_ERR(sys_uncached_heap))
553f4e0fafc5b3b John Stultz 2020-10-03 @496 return PTR_ERR(sys_heap);
^^^^^^^^^^^^^^^^^
This should be return PTR_ERR(sys_uncached_heap);
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32558 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-07 7:43 ` Dan Carpenter
0 siblings, 0 replies; 23+ messages in thread
From: Dan Carpenter @ 2020-10-07 7:43 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]
Hi John,
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
config: i386-randconfig-m021-20201002 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/dma-buf/heaps/system_heap.c:496 system_heap_create() warn: passing zero to 'PTR_ERR'
vim +/PTR_ERR +496 drivers/dma-buf/heaps/system_heap.c
efa04fefebbd724 John Stultz 2019-12-03 478 static int system_heap_create(void)
efa04fefebbd724 John Stultz 2019-12-03 479 {
efa04fefebbd724 John Stultz 2019-12-03 480 struct dma_heap_export_info exp_info;
efa04fefebbd724 John Stultz 2019-12-03 481
263e38f82cbb35b Andrew F. Davis 2019-12-16 482 exp_info.name = "system";
efa04fefebbd724 John Stultz 2019-12-03 483 exp_info.ops = &system_heap_ops;
efa04fefebbd724 John Stultz 2019-12-03 484 exp_info.priv = NULL;
efa04fefebbd724 John Stultz 2019-12-03 485
efa04fefebbd724 John Stultz 2019-12-03 486 sys_heap = dma_heap_add(&exp_info);
efa04fefebbd724 John Stultz 2019-12-03 487 if (IS_ERR(sys_heap))
a2e10cdd2e4d12a John Stultz 2020-10-03 488 return PTR_ERR(sys_heap);
efa04fefebbd724 John Stultz 2019-12-03 489
553f4e0fafc5b3b John Stultz 2020-10-03 490 exp_info.name = "system-uncached";
553f4e0fafc5b3b John Stultz 2020-10-03 491 exp_info.ops = &system_uncached_heap_ops;
553f4e0fafc5b3b John Stultz 2020-10-03 492 exp_info.priv = NULL;
553f4e0fafc5b3b John Stultz 2020-10-03 493
553f4e0fafc5b3b John Stultz 2020-10-03 494 sys_uncached_heap = dma_heap_add(&exp_info);
553f4e0fafc5b3b John Stultz 2020-10-03 495 if (IS_ERR(sys_uncached_heap))
553f4e0fafc5b3b John Stultz 2020-10-03 @496 return PTR_ERR(sys_heap);
^^^^^^^^^^^^^^^^^
This should be return PTR_ERR(sys_uncached_heap);
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32558 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-07 7:43 ` Dan Carpenter
0 siblings, 0 replies; 23+ messages in thread
From: Dan Carpenter @ 2020-10-07 7:43 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]
Hi John,
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
config: i386-randconfig-m021-20201002 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/dma-buf/heaps/system_heap.c:496 system_heap_create() warn: passing zero to 'PTR_ERR'
vim +/PTR_ERR +496 drivers/dma-buf/heaps/system_heap.c
efa04fefebbd724 John Stultz 2019-12-03 478 static int system_heap_create(void)
efa04fefebbd724 John Stultz 2019-12-03 479 {
efa04fefebbd724 John Stultz 2019-12-03 480 struct dma_heap_export_info exp_info;
efa04fefebbd724 John Stultz 2019-12-03 481
263e38f82cbb35b Andrew F. Davis 2019-12-16 482 exp_info.name = "system";
efa04fefebbd724 John Stultz 2019-12-03 483 exp_info.ops = &system_heap_ops;
efa04fefebbd724 John Stultz 2019-12-03 484 exp_info.priv = NULL;
efa04fefebbd724 John Stultz 2019-12-03 485
efa04fefebbd724 John Stultz 2019-12-03 486 sys_heap = dma_heap_add(&exp_info);
efa04fefebbd724 John Stultz 2019-12-03 487 if (IS_ERR(sys_heap))
a2e10cdd2e4d12a John Stultz 2020-10-03 488 return PTR_ERR(sys_heap);
efa04fefebbd724 John Stultz 2019-12-03 489
553f4e0fafc5b3b John Stultz 2020-10-03 490 exp_info.name = "system-uncached";
553f4e0fafc5b3b John Stultz 2020-10-03 491 exp_info.ops = &system_uncached_heap_ops;
553f4e0fafc5b3b John Stultz 2020-10-03 492 exp_info.priv = NULL;
553f4e0fafc5b3b John Stultz 2020-10-03 493
553f4e0fafc5b3b John Stultz 2020-10-03 494 sys_uncached_heap = dma_heap_add(&exp_info);
553f4e0fafc5b3b John Stultz 2020-10-03 495 if (IS_ERR(sys_uncached_heap))
553f4e0fafc5b3b John Stultz 2020-10-03 @496 return PTR_ERR(sys_heap);
^^^^^^^^^^^^^^^^^
This should be return PTR_ERR(sys_uncached_heap);
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32558 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-03 4:02 ` John Stultz
@ 2020-10-05 21:21 ` kernel test robot
-1 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-10-05 21:21 UTC (permalink / raw)
To: John Stultz, lkml
Cc: kbuild-all, John Stultz, Sumit Semwal, Liam Mark, Laura Abbott,
Brian Starkey, Hridya Valsaraju, Suren Baghdasaryan,
Sandeep Patil, Daniel Mentz
[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on tegra-drm/drm/tegra/for-next linus/master v5.9-rc8]
[cannot apply to next-20201002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
config: i386-randconfig-s032-20201005 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-201-g24bdaac6-dirty
# https://github.com/0day-ci/linux/commit/553f4e0fafc5b3b4542ea0c7bc21a70fce504c7f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
git checkout 553f4e0fafc5b3b4542ea0c7bc21a70fce504c7f
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
echo
echo "sparse warnings: (new ones prefixed by >>)"
echo
>> drivers/dma-buf/heaps/system_heap.c:25:17: sparse: sparse: symbol 'sys_uncached_heap' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36182 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-05 21:21 ` kernel test robot
0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-10-05 21:21 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on tegra-drm/drm/tegra/for-next linus/master v5.9-rc8]
[cannot apply to next-20201002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
config: i386-randconfig-s032-20201005 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-201-g24bdaac6-dirty
# https://github.com/0day-ci/linux/commit/553f4e0fafc5b3b4542ea0c7bc21a70fce504c7f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
git checkout 553f4e0fafc5b3b4542ea0c7bc21a70fce504c7f
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
echo
echo "sparse warnings: (new ones prefixed by >>)"
echo
>> drivers/dma-buf/heaps/system_heap.c:25:17: sparse: sparse: symbol 'sys_uncached_heap' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36182 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-03 4:02 ` John Stultz
(?)
@ 2020-10-05 13:45 ` Christoph Hellwig
2020-10-08 5:03 ` John Stultz
-1 siblings, 1 reply; 23+ messages in thread
From: Christoph Hellwig @ 2020-10-05 13:45 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Sumit Semwal, Liam Mark, Laura Abbott, Brian Starkey,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Daniel Mentz, Chris Goldsworthy, ??rjan Eide, Robin Murphy,
Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel
How is this going to deal with VIVT caches?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-03 17:41 kernel test robot
0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-10-03 17:41 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 3397 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20201003040257.62768-8-john.stultz@linaro.org>
References: <20201003040257.62768-8-john.stultz@linaro.org>
TO: John Stultz <john.stultz@linaro.org>
TO: lkml <linux-kernel@vger.kernel.org>
CC: John Stultz <john.stultz@linaro.org>
CC: Sumit Semwal <sumit.semwal@linaro.org>
CC: Liam Mark <lmark@codeaurora.org>
CC: Laura Abbott <labbott@kernel.org>
CC: Brian Starkey <Brian.Starkey@arm.com>
CC: Hridya Valsaraju <hridya@google.com>
CC: Suren Baghdasaryan <surenb@google.com>
CC: Sandeep Patil <sspatil@google.com>
CC: Daniel Mentz <danielmentz@google.com>
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on tegra-drm/drm/tegra/for-next linus/master v5.9-rc7]
[cannot apply to next-20201002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/dma-buf-Performance-improvements-for-system-heap-a-system-uncached-implementation/20201003-120520
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git bcf876870b95592b52519ed4aafcf9d95999bc9c
:::::: branch date: 14 hours ago
:::::: commit date: 14 hours ago
config: i386-randconfig-m021-20201002 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/dma-buf/heaps/system_heap.c:496 system_heap_create() warn: passing zero to 'PTR_ERR'
vim +/PTR_ERR +496 drivers/dma-buf/heaps/system_heap.c
553f4e0fafc5b3b John Stultz 2020-10-03 477
efa04fefebbd724 John Stultz 2019-12-03 478 static int system_heap_create(void)
efa04fefebbd724 John Stultz 2019-12-03 479 {
efa04fefebbd724 John Stultz 2019-12-03 480 struct dma_heap_export_info exp_info;
efa04fefebbd724 John Stultz 2019-12-03 481
263e38f82cbb35b Andrew F. Davis 2019-12-16 482 exp_info.name = "system";
efa04fefebbd724 John Stultz 2019-12-03 483 exp_info.ops = &system_heap_ops;
efa04fefebbd724 John Stultz 2019-12-03 484 exp_info.priv = NULL;
efa04fefebbd724 John Stultz 2019-12-03 485
efa04fefebbd724 John Stultz 2019-12-03 486 sys_heap = dma_heap_add(&exp_info);
efa04fefebbd724 John Stultz 2019-12-03 487 if (IS_ERR(sys_heap))
a2e10cdd2e4d12a John Stultz 2020-10-03 488 return PTR_ERR(sys_heap);
efa04fefebbd724 John Stultz 2019-12-03 489
553f4e0fafc5b3b John Stultz 2020-10-03 490 exp_info.name = "system-uncached";
553f4e0fafc5b3b John Stultz 2020-10-03 491 exp_info.ops = &system_uncached_heap_ops;
553f4e0fafc5b3b John Stultz 2020-10-03 492 exp_info.priv = NULL;
553f4e0fafc5b3b John Stultz 2020-10-03 493
553f4e0fafc5b3b John Stultz 2020-10-03 494 sys_uncached_heap = dma_heap_add(&exp_info);
553f4e0fafc5b3b John Stultz 2020-10-03 495 if (IS_ERR(sys_uncached_heap))
553f4e0fafc5b3b John Stultz 2020-10-03 @496 return PTR_ERR(sys_heap);
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32558 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
2020-10-03 4:02 [PATCH v3 0/7] dma-buf: Performance improvements for system heap & a system-uncached implementation John Stultz
@ 2020-10-03 4:02 ` John Stultz
0 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-03 4:02 UTC (permalink / raw)
To: lkml
Cc: John Stultz, Sumit Semwal, Liam Mark, Laura Abbott,
Brian Starkey, Hridya Valsaraju, Suren Baghdasaryan,
Sandeep Patil, Daniel Mentz, Chris Goldsworthy, Ørjan Eide,
Robin Murphy, Ezequiel Garcia, Simon Ser, James Jones,
linux-media, dri-devel
This adds a heap that allocates non-contiguous buffers that are
marked as writecombined, so they are not cached by the CPU.
This is useful, as most graphics buffers are usually not touched
by the CPU or only written into once by the CPU. So when mapping
the buffer over and over between devices, we can skip the CPU
syncing, which saves a lot of cache management overhead, greatly
improving performance.
For folk using ION, there was a ION_FLAG_CACHED flag, which
signaled if the returned buffer should be CPU cacheable or not.
With DMA-BUF heaps, we do not yet have such a flag, and by default
the current heaps (system and cma) produce CPU cachable buffers.
So for folks transitioning from ION to DMA-BUF Heaps, this fills
in some of that missing functionality.
There has been a suggestion to make this functionality a flag
(DMAHEAP_FLAG_UNCACHED?) on the system heap, similar to how
ION used the ION_FLAG_CACHED. But I want to make sure an
_UNCACHED flag would truely be a generic attribute across all
heaps. So far that has been unclear, so having it as a separate
heap seemes better for now. (But I'm open to discussion on this
point!)
This is a rework of earlier efforts to add a uncached system heap,
done utilizing the exisitng system heap, adding just a bit of
logic to handle the uncached case.
Feedback would be very welcome!
Many thanks to Liam Mark for his help to get this working.
Pending opensource users of this code include:
* AOSP HiKey960 gralloc:
- https://android-review.googlesource.com/c/device/linaro/hikey/+/1399519
- Visibly improves performance over the system heap
* AOSP Codec2 (possibly, needs more review):
- https://android-review.googlesource.com/c/platform/frameworks/av/+/1360640/17/media/codec2/vndk/C2DmaBufAllocator.cpp#325
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Laura Abbott <labbott@kernel.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Daniel Mentz <danielmentz@google.com>
Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc: Ørjan Eide <orjan.eide@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: James Jones <jajones@nvidia.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
drivers/dma-buf/heaps/system_heap.c | 87 ++++++++++++++++++++++++++---
1 file changed, 79 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 2b8d4b6abacb..952f1fd9dacf 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -22,6 +22,7 @@
#include <linux/vmalloc.h>
struct dma_heap *sys_heap;
+struct dma_heap *sys_uncached_heap;
struct system_heap_buffer {
struct dma_heap *heap;
@@ -31,6 +32,8 @@ struct system_heap_buffer {
struct sg_table sg_table;
int vmap_cnt;
void *vaddr;
+
+ bool uncached;
};
struct dma_heap_attachment {
@@ -38,6 +41,8 @@ struct dma_heap_attachment {
struct sg_table *table;
struct list_head list;
bool mapped;
+
+ bool uncached;
};
#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \
@@ -94,7 +99,7 @@ static int system_heap_attach(struct dma_buf *dmabuf,
a->dev = attachment->dev;
INIT_LIST_HEAD(&a->list);
a->mapped = false;
-
+ a->uncached = buffer->uncached;
attachment->priv = a;
mutex_lock(&buffer->lock);
@@ -124,9 +129,13 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac
{
struct dma_heap_attachment *a = attachment->priv;
struct sg_table *table = a->table;
+ int attr = 0;
int ret;
- ret = dma_map_sgtable(attachment->dev, table, direction, 0);
+ if (a->uncached)
+ attr = DMA_ATTR_SKIP_CPU_SYNC;
+
+ ret = dma_map_sgtable(attachment->dev, table, direction, attr);
if (ret)
return ERR_PTR(ret);
@@ -139,9 +148,12 @@ static void system_heap_unmap_dma_buf(struct dma_buf_attachment *attachment,
enum dma_data_direction direction)
{
struct dma_heap_attachment *a = attachment->priv;
+ int attr = 0;
+ if (a->uncached)
+ attr = DMA_ATTR_SKIP_CPU_SYNC;
a->mapped = false;
- dma_unmap_sgtable(attachment->dev, table, direction, 0);
+ dma_unmap_sgtable(attachment->dev, table, direction, attr);
}
static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
@@ -150,6 +162,9 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
struct system_heap_buffer *buffer = dmabuf->priv;
struct dma_heap_attachment *a;
+ if (buffer->uncached)
+ return 0;
+
mutex_lock(&buffer->lock);
if (buffer->vmap_cnt)
@@ -171,6 +186,9 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
struct system_heap_buffer *buffer = dmabuf->priv;
struct dma_heap_attachment *a;
+ if (buffer->uncached)
+ return 0;
+
mutex_lock(&buffer->lock);
if (buffer->vmap_cnt)
@@ -194,6 +212,9 @@ static int system_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
struct sg_page_iter piter;
int ret;
+ if (buffer->uncached)
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
struct page *page = sg_page_iter_page(&piter);
@@ -215,8 +236,12 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
struct page **pages = vmalloc(sizeof(struct page *) * npages);
struct page **tmp = pages;
struct sg_page_iter piter;
+ pgprot_t pgprot = PAGE_KERNEL;
void *vaddr;
+ if (buffer->uncached)
+ pgprot = pgprot_writecombine(PAGE_KERNEL);
+
if (!pages)
return ERR_PTR(-ENOMEM);
@@ -225,7 +250,7 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
*tmp++ = sg_page_iter_page(&piter);
}
- vaddr = vmap(pages, npages, VM_MAP, PAGE_KERNEL);
+ vaddr = vmap(pages, npages, VM_MAP, pgprot);
vfree(pages);
if (!vaddr)
@@ -278,6 +303,10 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
int i;
table = &buffer->sg_table;
+ /* Unmap the uncached buffers from the heap device (pairs with the map on allocate) */
+ if (buffer->uncached)
+ dma_unmap_sgtable(dma_heap_get_dev(buffer->heap), table, DMA_BIDIRECTIONAL, 0);
+
for_each_sg(table->sgl, sg, table->nents, i) {
struct page *page = sg_page(sg);
@@ -320,10 +349,11 @@ static struct page *alloc_largest_available(unsigned long size,
return NULL;
}
-static int system_heap_allocate(struct dma_heap *heap,
- unsigned long len,
- unsigned long fd_flags,
- unsigned long heap_flags)
+static int system_heap_do_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags,
+ bool uncached)
{
struct system_heap_buffer *buffer;
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
@@ -344,6 +374,7 @@ static int system_heap_allocate(struct dma_heap *heap,
mutex_init(&buffer->lock);
buffer->heap = heap;
buffer->len = len;
+ buffer->uncached = uncached;
INIT_LIST_HEAD(&pages);
i = 0;
@@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
/* just return, as put will call release and that will free */
return ret;
}
+
+ /*
+ * For uncached buffers, we need to initially flush cpu cache, since
+ * the __GFP_ZERO on the allocation means the zeroing was done by the
+ * cpu and thus it is likely cached. Map (and implicitly flush) it out
+ * now so we don't get corruption later on.
+ */
+ if (buffer->uncached)
+ dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
+
return ret;
free_pages:
@@ -410,10 +451,30 @@ static int system_heap_allocate(struct dma_heap *heap,
return ret;
}
+static int system_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
+{
+ return system_heap_do_allocate(heap, len, fd_flags, heap_flags, false);
+}
+
static const struct dma_heap_ops system_heap_ops = {
.allocate = system_heap_allocate,
};
+static int system_uncached_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
+{
+ return system_heap_do_allocate(heap, len, fd_flags, heap_flags, true);
+}
+
+static const struct dma_heap_ops system_uncached_heap_ops = {
+ .allocate = system_uncached_heap_allocate,
+};
+
static int system_heap_create(void)
{
struct dma_heap_export_info exp_info;
@@ -426,6 +487,16 @@ static int system_heap_create(void)
if (IS_ERR(sys_heap))
return PTR_ERR(sys_heap);
+ exp_info.name = "system-uncached";
+ exp_info.ops = &system_uncached_heap_ops;
+ exp_info.priv = NULL;
+
+ sys_uncached_heap = dma_heap_add(&exp_info);
+ if (IS_ERR(sys_uncached_heap))
+ return PTR_ERR(sys_heap);
+
+ dma_coerce_mask_and_coherent(dma_heap_get_dev(sys_uncached_heap), DMA_BIT_MASK(64));
+
return 0;
}
module_init(system_heap_create);
--
2.17.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap
@ 2020-10-03 4:02 ` John Stultz
0 siblings, 0 replies; 23+ messages in thread
From: John Stultz @ 2020-10-03 4:02 UTC (permalink / raw)
To: lkml
Cc: Sandeep Patil, dri-devel, Ezequiel Garcia, Robin Murphy,
James Jones, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Daniel Mentz
This adds a heap that allocates non-contiguous buffers that are
marked as writecombined, so they are not cached by the CPU.
This is useful, as most graphics buffers are usually not touched
by the CPU or only written into once by the CPU. So when mapping
the buffer over and over between devices, we can skip the CPU
syncing, which saves a lot of cache management overhead, greatly
improving performance.
For folk using ION, there was a ION_FLAG_CACHED flag, which
signaled if the returned buffer should be CPU cacheable or not.
With DMA-BUF heaps, we do not yet have such a flag, and by default
the current heaps (system and cma) produce CPU cachable buffers.
So for folks transitioning from ION to DMA-BUF Heaps, this fills
in some of that missing functionality.
There has been a suggestion to make this functionality a flag
(DMAHEAP_FLAG_UNCACHED?) on the system heap, similar to how
ION used the ION_FLAG_CACHED. But I want to make sure an
_UNCACHED flag would truely be a generic attribute across all
heaps. So far that has been unclear, so having it as a separate
heap seemes better for now. (But I'm open to discussion on this
point!)
This is a rework of earlier efforts to add a uncached system heap,
done utilizing the exisitng system heap, adding just a bit of
logic to handle the uncached case.
Feedback would be very welcome!
Many thanks to Liam Mark for his help to get this working.
Pending opensource users of this code include:
* AOSP HiKey960 gralloc:
- https://android-review.googlesource.com/c/device/linaro/hikey/+/1399519
- Visibly improves performance over the system heap
* AOSP Codec2 (possibly, needs more review):
- https://android-review.googlesource.com/c/platform/frameworks/av/+/1360640/17/media/codec2/vndk/C2DmaBufAllocator.cpp#325
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Laura Abbott <labbott@kernel.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Daniel Mentz <danielmentz@google.com>
Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc: Ørjan Eide <orjan.eide@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: James Jones <jajones@nvidia.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
drivers/dma-buf/heaps/system_heap.c | 87 ++++++++++++++++++++++++++---
1 file changed, 79 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 2b8d4b6abacb..952f1fd9dacf 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -22,6 +22,7 @@
#include <linux/vmalloc.h>
struct dma_heap *sys_heap;
+struct dma_heap *sys_uncached_heap;
struct system_heap_buffer {
struct dma_heap *heap;
@@ -31,6 +32,8 @@ struct system_heap_buffer {
struct sg_table sg_table;
int vmap_cnt;
void *vaddr;
+
+ bool uncached;
};
struct dma_heap_attachment {
@@ -38,6 +41,8 @@ struct dma_heap_attachment {
struct sg_table *table;
struct list_head list;
bool mapped;
+
+ bool uncached;
};
#define HIGH_ORDER_GFP (((GFP_HIGHUSER | __GFP_ZERO | __GFP_NOWARN \
@@ -94,7 +99,7 @@ static int system_heap_attach(struct dma_buf *dmabuf,
a->dev = attachment->dev;
INIT_LIST_HEAD(&a->list);
a->mapped = false;
-
+ a->uncached = buffer->uncached;
attachment->priv = a;
mutex_lock(&buffer->lock);
@@ -124,9 +129,13 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac
{
struct dma_heap_attachment *a = attachment->priv;
struct sg_table *table = a->table;
+ int attr = 0;
int ret;
- ret = dma_map_sgtable(attachment->dev, table, direction, 0);
+ if (a->uncached)
+ attr = DMA_ATTR_SKIP_CPU_SYNC;
+
+ ret = dma_map_sgtable(attachment->dev, table, direction, attr);
if (ret)
return ERR_PTR(ret);
@@ -139,9 +148,12 @@ static void system_heap_unmap_dma_buf(struct dma_buf_attachment *attachment,
enum dma_data_direction direction)
{
struct dma_heap_attachment *a = attachment->priv;
+ int attr = 0;
+ if (a->uncached)
+ attr = DMA_ATTR_SKIP_CPU_SYNC;
a->mapped = false;
- dma_unmap_sgtable(attachment->dev, table, direction, 0);
+ dma_unmap_sgtable(attachment->dev, table, direction, attr);
}
static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
@@ -150,6 +162,9 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
struct system_heap_buffer *buffer = dmabuf->priv;
struct dma_heap_attachment *a;
+ if (buffer->uncached)
+ return 0;
+
mutex_lock(&buffer->lock);
if (buffer->vmap_cnt)
@@ -171,6 +186,9 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
struct system_heap_buffer *buffer = dmabuf->priv;
struct dma_heap_attachment *a;
+ if (buffer->uncached)
+ return 0;
+
mutex_lock(&buffer->lock);
if (buffer->vmap_cnt)
@@ -194,6 +212,9 @@ static int system_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
struct sg_page_iter piter;
int ret;
+ if (buffer->uncached)
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
struct page *page = sg_page_iter_page(&piter);
@@ -215,8 +236,12 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
struct page **pages = vmalloc(sizeof(struct page *) * npages);
struct page **tmp = pages;
struct sg_page_iter piter;
+ pgprot_t pgprot = PAGE_KERNEL;
void *vaddr;
+ if (buffer->uncached)
+ pgprot = pgprot_writecombine(PAGE_KERNEL);
+
if (!pages)
return ERR_PTR(-ENOMEM);
@@ -225,7 +250,7 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer)
*tmp++ = sg_page_iter_page(&piter);
}
- vaddr = vmap(pages, npages, VM_MAP, PAGE_KERNEL);
+ vaddr = vmap(pages, npages, VM_MAP, pgprot);
vfree(pages);
if (!vaddr)
@@ -278,6 +303,10 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
int i;
table = &buffer->sg_table;
+ /* Unmap the uncached buffers from the heap device (pairs with the map on allocate) */
+ if (buffer->uncached)
+ dma_unmap_sgtable(dma_heap_get_dev(buffer->heap), table, DMA_BIDIRECTIONAL, 0);
+
for_each_sg(table->sgl, sg, table->nents, i) {
struct page *page = sg_page(sg);
@@ -320,10 +349,11 @@ static struct page *alloc_largest_available(unsigned long size,
return NULL;
}
-static int system_heap_allocate(struct dma_heap *heap,
- unsigned long len,
- unsigned long fd_flags,
- unsigned long heap_flags)
+static int system_heap_do_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags,
+ bool uncached)
{
struct system_heap_buffer *buffer;
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
@@ -344,6 +374,7 @@ static int system_heap_allocate(struct dma_heap *heap,
mutex_init(&buffer->lock);
buffer->heap = heap;
buffer->len = len;
+ buffer->uncached = uncached;
INIT_LIST_HEAD(&pages);
i = 0;
@@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap,
/* just return, as put will call release and that will free */
return ret;
}
+
+ /*
+ * For uncached buffers, we need to initially flush cpu cache, since
+ * the __GFP_ZERO on the allocation means the zeroing was done by the
+ * cpu and thus it is likely cached. Map (and implicitly flush) it out
+ * now so we don't get corruption later on.
+ */
+ if (buffer->uncached)
+ dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);
+
return ret;
free_pages:
@@ -410,10 +451,30 @@ static int system_heap_allocate(struct dma_heap *heap,
return ret;
}
+static int system_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
+{
+ return system_heap_do_allocate(heap, len, fd_flags, heap_flags, false);
+}
+
static const struct dma_heap_ops system_heap_ops = {
.allocate = system_heap_allocate,
};
+static int system_uncached_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
+{
+ return system_heap_do_allocate(heap, len, fd_flags, heap_flags, true);
+}
+
+static const struct dma_heap_ops system_uncached_heap_ops = {
+ .allocate = system_uncached_heap_allocate,
+};
+
static int system_heap_create(void)
{
struct dma_heap_export_info exp_info;
@@ -426,6 +487,16 @@ static int system_heap_create(void)
if (IS_ERR(sys_heap))
return PTR_ERR(sys_heap);
+ exp_info.name = "system-uncached";
+ exp_info.ops = &system_uncached_heap_ops;
+ exp_info.priv = NULL;
+
+ sys_uncached_heap = dma_heap_add(&exp_info);
+ if (IS_ERR(sys_uncached_heap))
+ return PTR_ERR(sys_heap);
+
+ dma_coerce_mask_and_coherent(dma_heap_get_dev(sys_uncached_heap), DMA_BIT_MASK(64));
+
return 0;
}
module_init(system_heap_create);
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
end of thread, other threads:[~2021-01-29 8:12 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-03 8:24 [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap kernel test robot
2020-10-03 8:24 ` [PATCH] dma-buf: system_heap: fix odd_ptr_err.cocci warnings kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2020-10-03 17:41 [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap kernel test robot
2020-10-03 4:02 [PATCH v3 0/7] dma-buf: Performance improvements for system heap & a system-uncached implementation John Stultz
2020-10-03 4:02 ` [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap John Stultz
2020-10-03 4:02 ` John Stultz
2020-10-05 13:45 ` Christoph Hellwig
2020-10-08 5:03 ` John Stultz
2020-10-08 5:03 ` John Stultz
2020-10-05 21:21 ` kernel test robot
2020-10-05 21:21 ` kernel test robot
2020-10-07 7:43 ` Dan Carpenter
2020-10-07 7:43 ` Dan Carpenter
2020-10-07 7:43 ` Dan Carpenter
2020-10-08 5:38 ` John Stultz
2020-10-08 5:38 ` John Stultz
2020-10-08 11:51 ` Brian Starkey
2020-10-08 11:51 ` Brian Starkey
2020-10-16 19:03 ` John Stultz
2020-10-16 19:03 ` John Stultz
2020-10-16 23:15 ` John Stultz
2020-10-16 23:15 ` John Stultz
2021-01-29 1:23 ` Daniel Mentz
2021-01-29 1:23 ` Daniel Mentz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.