From: John Stultz <john.stultz@linaro.org> To: Brian Starkey <brian.starkey@arm.com> Cc: lkml <linux-kernel@vger.kernel.org>, "Sumit Semwal" <sumit.semwal@linaro.org>, "Liam Mark" <lmark@codeaurora.org>, "Laura Abbott" <labbott@kernel.org>, "Hridya Valsaraju" <hridya@google.com>, "Suren Baghdasaryan" <surenb@google.com>, "Sandeep Patil" <sspatil@google.com>, "Daniel Mentz" <danielmentz@google.com>, "Chris Goldsworthy" <cgoldswo@codeaurora.org>, "Ørjan Eide" <orjan.eide@arm.com>, "Robin Murphy" <robin.murphy@arm.com>, "Ezequiel Garcia" <ezequiel@collabora.com>, "Simon Ser" <contact@emersion.fr>, "James Jones" <jajones@nvidia.com>, linux-media <linux-media@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, nd <nd@arm.com> Subject: Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap Date: Fri, 16 Oct 2020 12:03:52 -0700 [thread overview] Message-ID: <CALAqxLWrbgHoh=BCnuB4US77AOPMYmgGrE85WT6DYnEV-bad-A@mail.gmail.com> (raw) In-Reply-To: <20201008115101.4qi6wh3hhkb6krg5@DESKTOP-E1NTVVP.localdomain> 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
WARNING: multiple messages have this Message-ID (diff)
From: John Stultz <john.stultz@linaro.org> To: Brian Starkey <brian.starkey@arm.com> Cc: nd <nd@arm.com>, "Sandeep Patil" <sspatil@google.com>, dri-devel <dri-devel@lists.freedesktop.org>, "Ezequiel Garcia" <ezequiel@collabora.com>, "Robin Murphy" <robin.murphy@arm.com>, "James Jones" <jajones@nvidia.com>, lkml <linux-kernel@vger.kernel.org>, "Liam Mark" <lmark@codeaurora.org>, "Laura Abbott" <labbott@kernel.org>, "Chris Goldsworthy" <cgoldswo@codeaurora.org>, "Hridya Valsaraju" <hridya@google.com>, "Ørjan Eide" <orjan.eide@arm.com>, linux-media <linux-media@vger.kernel.org>, "Suren Baghdasaryan" <surenb@google.com>, "Daniel Mentz" <danielmentz@google.com> Subject: Re: [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap Date: Fri, 16 Oct 2020 12:03:52 -0700 [thread overview] Message-ID: <CALAqxLWrbgHoh=BCnuB4US77AOPMYmgGrE85WT6DYnEV-bad-A@mail.gmail.com> (raw) In-Reply-To: <20201008115101.4qi6wh3hhkb6krg5@DESKTOP-E1NTVVP.localdomain> 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
next prev parent reply other threads:[~2020-10-16 19:04 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 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 2020-10-03 4:02 ` [PATCH v3 1/7] dma-buf: system_heap: Rework system heap to use sgtables instead of pagelists John Stultz 2020-10-03 4:02 ` John Stultz 2020-10-03 4:02 ` [PATCH v3 2/7] dma-buf: heaps: Move heap-helper logic into the cma_heap implementation John Stultz 2020-10-03 4:02 ` John Stultz 2020-10-03 4:02 ` [PATCH v3 3/7] dma-buf: heaps: Remove heap-helpers code John Stultz 2020-10-03 4:02 ` John Stultz 2020-10-03 4:02 ` [PATCH v3 4/7] dma-buf: heaps: Skip sync if not mapped John Stultz 2020-10-03 4:02 ` John Stultz 2020-10-03 4:02 ` [PATCH v3 5/7] dma-buf: system_heap: Allocate higher order pages if available John Stultz 2020-10-03 4:02 ` John Stultz 2020-10-03 4:02 ` [PATCH v3 6/7] dma-buf: dma-heap: Keep track of the heap device struct John Stultz 2020-10-03 4:02 ` 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-05 21:21 ` [RFC PATCH] dma-buf: system_heap: sys_uncached_heap can be static kernel test robot 2020-10-05 21:21 ` kernel test robot 2020-10-07 7:43 ` [PATCH v3 7/7] dma-buf: system_heap: Add a system-uncached heap re-using the system heap 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 [this message] 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 2020-10-08 11:36 ` [PATCH v3 0/7] dma-buf: Performance improvements for system heap & a system-uncached implementation Brian Starkey 2020-10-08 11:36 ` Brian Starkey 2020-10-16 18:47 ` John Stultz 2020-10-16 18:47 ` John Stultz 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 17:41 kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CALAqxLWrbgHoh=BCnuB4US77AOPMYmgGrE85WT6DYnEV-bad-A@mail.gmail.com' \ --to=john.stultz@linaro.org \ --cc=brian.starkey@arm.com \ --cc=cgoldswo@codeaurora.org \ --cc=contact@emersion.fr \ --cc=danielmentz@google.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=ezequiel@collabora.com \ --cc=hridya@google.com \ --cc=jajones@nvidia.com \ --cc=labbott@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=lmark@codeaurora.org \ --cc=nd@arm.com \ --cc=orjan.eide@arm.com \ --cc=robin.murphy@arm.com \ --cc=sspatil@google.com \ --cc=sumit.semwal@linaro.org \ --cc=surenb@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.