From: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> Cc: David Airlie <airlied-cv59FeDIM0c@public.gmane.org>, nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Ben Skeggs <bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH 4/4] drm/nouveau: introduce CPU cache flushing macro Date: Mon, 19 May 2014 11:02:03 +0200 [thread overview] Message-ID: <20140519090202.GC7138@ulmo> (raw) In-Reply-To: <1400483458-9648-5-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> [-- Attachment #1.1: Type: text/plain, Size: 2755 bytes --] On Mon, May 19, 2014 at 04:10:58PM +0900, Alexandre Courbot wrote: > Some architectures (e.g. ARM) need the CPU buffers to be explicitely > flushed for a memory write to take effect. Not doing so results in > synchronization issues, especially after writing to BOs. It seems to me that the above is generally true for all architectures, not just ARM. Also: s/explicitely/explicitly/ > This patch introduces a macro that flushes the caches on ARM and > translates to a no-op on other architectures, and uses it when > writing to in-memory BOs. It will also be useful for implementations of > instmem that access shared memory directly instead of going through > PRAMIN. Presumably instmem can access shared memory on all architectures, so this doesn't seem like a property of the architecture but rather of the memory pool backing the instmem. In that case I wonder if this shouldn't be moved into an operation that is implemented by the backing memory pool and be a noop where the cache doesn't need explicit flushing. > diff --git a/drivers/gpu/drm/nouveau/core/os.h b/drivers/gpu/drm/nouveau/core/os.h > index d0ced94ca54c..274b4460bb03 100644 > --- a/drivers/gpu/drm/nouveau/core/os.h > +++ b/drivers/gpu/drm/nouveau/core/os.h > @@ -38,4 +38,21 @@ > #endif /* def __BIG_ENDIAN else */ > #endif /* !ioread32_native */ > > +#if defined(__arm__) > + > +#define nv_cpu_cache_flush_area(va, size) \ > +do { \ > + phys_addr_t pa = virt_to_phys(va); \ > + __cpuc_flush_dcache_area(va, size); \ > + outer_flush_range(pa, pa + size); \ > +} while (0) Couldn't this be a static inline function? > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c [...] > index 0886f47e5244..b9c9729c5733 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -437,8 +437,10 @@ nouveau_bo_wr16(struct nouveau_bo *nvbo, unsigned index, u16 val) > mem = &mem[index]; > if (is_iomem) > iowrite16_native(val, (void __force __iomem *)mem); > - else > + else { > *mem = val; > + nv_cpu_cache_flush_area(mem, 2); > + } > } > > u32 > @@ -461,8 +463,10 @@ nouveau_bo_wr32(struct nouveau_bo *nvbo, unsigned index, u32 val) > mem = &mem[index]; > if (is_iomem) > iowrite32_native(val, (void __force __iomem *)mem); > - else > + else { > *mem = val; > + nv_cpu_cache_flush_area(mem, 4); > + } This looks rather like a sledgehammer to me. Effectively this turns nvbo into an uncached buffer. With additional overhead of constantly flushing caches. Wouldn't it make more sense to locate the places where these are called and flush the cache after all the writes have completed? Thierry [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 181 bytes --] _______________________________________________ Nouveau mailing list Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org http://lists.freedesktop.org/mailman/listinfo/nouveau
WARNING: multiple messages have this Message-ID (diff)
From: Thierry Reding <thierry.reding@gmail.com> To: Alexandre Courbot <acourbot@nvidia.com> Cc: David Airlie <airlied@linux.ie>, Ben Skeggs <bskeggs@redhat.com>, Lucas Stach <dev@lynxeye.de>, nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, gnurou@gmail.com Subject: Re: [PATCH 4/4] drm/nouveau: introduce CPU cache flushing macro Date: Mon, 19 May 2014 11:02:03 +0200 [thread overview] Message-ID: <20140519090202.GC7138@ulmo> (raw) In-Reply-To: <1400483458-9648-5-git-send-email-acourbot@nvidia.com> [-- Attachment #1: Type: text/plain, Size: 2755 bytes --] On Mon, May 19, 2014 at 04:10:58PM +0900, Alexandre Courbot wrote: > Some architectures (e.g. ARM) need the CPU buffers to be explicitely > flushed for a memory write to take effect. Not doing so results in > synchronization issues, especially after writing to BOs. It seems to me that the above is generally true for all architectures, not just ARM. Also: s/explicitely/explicitly/ > This patch introduces a macro that flushes the caches on ARM and > translates to a no-op on other architectures, and uses it when > writing to in-memory BOs. It will also be useful for implementations of > instmem that access shared memory directly instead of going through > PRAMIN. Presumably instmem can access shared memory on all architectures, so this doesn't seem like a property of the architecture but rather of the memory pool backing the instmem. In that case I wonder if this shouldn't be moved into an operation that is implemented by the backing memory pool and be a noop where the cache doesn't need explicit flushing. > diff --git a/drivers/gpu/drm/nouveau/core/os.h b/drivers/gpu/drm/nouveau/core/os.h > index d0ced94ca54c..274b4460bb03 100644 > --- a/drivers/gpu/drm/nouveau/core/os.h > +++ b/drivers/gpu/drm/nouveau/core/os.h > @@ -38,4 +38,21 @@ > #endif /* def __BIG_ENDIAN else */ > #endif /* !ioread32_native */ > > +#if defined(__arm__) > + > +#define nv_cpu_cache_flush_area(va, size) \ > +do { \ > + phys_addr_t pa = virt_to_phys(va); \ > + __cpuc_flush_dcache_area(va, size); \ > + outer_flush_range(pa, pa + size); \ > +} while (0) Couldn't this be a static inline function? > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c [...] > index 0886f47e5244..b9c9729c5733 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -437,8 +437,10 @@ nouveau_bo_wr16(struct nouveau_bo *nvbo, unsigned index, u16 val) > mem = &mem[index]; > if (is_iomem) > iowrite16_native(val, (void __force __iomem *)mem); > - else > + else { > *mem = val; > + nv_cpu_cache_flush_area(mem, 2); > + } > } > > u32 > @@ -461,8 +463,10 @@ nouveau_bo_wr32(struct nouveau_bo *nvbo, unsigned index, u32 val) > mem = &mem[index]; > if (is_iomem) > iowrite32_native(val, (void __force __iomem *)mem); > - else > + else { > *mem = val; > + nv_cpu_cache_flush_area(mem, 4); > + } This looks rather like a sledgehammer to me. Effectively this turns nvbo into an uncached buffer. With additional overhead of constantly flushing caches. Wouldn't it make more sense to locate the places where these are called and flush the cache after all the writes have completed? Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
next prev parent reply other threads:[~2014-05-19 9:02 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-05-19 7:10 [PATCH 0/4] drm/ttm: nouveau: memory coherency fixes for ARM Alexandre Courbot 2014-05-19 7:10 ` Alexandre Courbot 2014-05-19 7:10 ` [PATCH 1/4] drm/ttm: recognize ARM arch in ioprot handler Alexandre Courbot 2014-05-19 7:10 ` Alexandre Courbot 2014-05-19 7:10 ` [PATCH 3/4] drm/nouveau: hook up cache sync functions Alexandre Courbot 2014-05-19 7:10 ` Alexandre Courbot [not found] ` <1400483458-9648-4-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2014-05-19 8:46 ` Thierry Reding 2014-05-19 8:46 ` Thierry Reding 2014-05-19 9:44 ` Lucas Stach 2014-05-19 9:44 ` Lucas Stach 2014-05-23 6:00 ` Alexandre Courbot 2014-05-23 6:00 ` Alexandre Courbot 2014-05-19 9:31 ` Lucas Stach 2014-05-19 9:31 ` Lucas Stach [not found] ` <1400491887.8467.15.camel-WzVe3FnzCwFR6QfukMTsflXZhhPuCNm+@public.gmane.org> 2014-05-23 6:01 ` Alexandre Courbot 2014-05-23 6:01 ` Alexandre Courbot [not found] ` <1400483458-9648-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2014-05-19 7:10 ` [PATCH 2/4] drm/ttm: introduce dma cache sync helpers Alexandre Courbot 2014-05-19 7:10 ` Alexandre Courbot 2014-05-19 8:33 ` Thierry Reding 2014-05-19 8:33 ` Thierry Reding 2014-05-23 5:49 ` Alexandre Courbot 2014-05-23 5:49 ` Alexandre Courbot 2014-05-23 7:31 ` Thierry Reding 2014-05-23 7:31 ` Thierry Reding 2014-05-19 7:10 ` [PATCH 4/4] drm/nouveau: introduce CPU cache flushing macro Alexandre Courbot 2014-05-19 7:10 ` Alexandre Courbot [not found] ` <1400483458-9648-5-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2014-05-19 9:02 ` Thierry Reding [this message] 2014-05-19 9:02 ` Thierry Reding 2014-05-19 9:22 ` Lucas Stach 2014-05-19 9:22 ` Lucas Stach [not found] ` <1400491331.8467.8.camel-WzVe3FnzCwFR6QfukMTsflXZhhPuCNm+@public.gmane.org> 2014-05-19 10:03 ` Thierry Reding 2014-05-19 10:03 ` Thierry Reding 2014-05-19 10:27 ` Daniel Vetter 2014-05-19 10:27 ` [Nouveau] " Daniel Vetter 2014-05-23 6:58 ` Alexandre Courbot 2014-05-23 6:58 ` Alexandre Courbot 2014-06-09 10:41 ` Alexandre Courbot 2014-06-09 10:41 ` Alexandre Courbot [not found] ` <CAAVeFu+KZ9AqB5ji5-AA+qzEFDWd7y0=J1eSEPqQ-OyhmXufig-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-06-12 13:50 ` Alexandre Courbot 2014-06-12 13:50 ` Alexandre Courbot [not found] ` <CAAVeFuJYe5wVH_gTok80hT=4GbwhYq4C9c7S5No_V11qjs3brQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-06-12 18:15 ` Roy Spliet
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=20140519090202.GC7138@ulmo \ --to=thierry.reding-re5jqeeqqe8avxtiumwx3w@public.gmane.org \ --cc=acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ --cc=airlied-cv59FeDIM0c@public.gmane.org \ --cc=bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \ --cc=dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \ /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.