* [PATCH] drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range() @ 2015-06-10 14:58 Chris Wilson 2015-06-11 8:25 ` Dave Gordon 0 siblings, 1 reply; 5+ messages in thread From: Chris Wilson @ 2015-06-10 14:58 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx As the clflush operates on cache lines, and we can flush any byte address, in order to flush all bytes given in the range we issue an extra clflush on the last byte to ensure the last cacheline is flushed. We can can the iteration to be over the actual cache lines to avoid this double clflush on the last byte. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/drm_cache.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c index 9a62d7a53553..6743ff7dccfa 100644 --- a/drivers/gpu/drm/drm_cache.c +++ b/drivers/gpu/drm/drm_cache.c @@ -130,11 +130,12 @@ drm_clflush_virt_range(void *addr, unsigned long length) { #if defined(CONFIG_X86) if (cpu_has_clflush) { + const int size = boot_cpu_data.x86_clflush_size; void *end = addr + length; + addr = (void *)(((unsigned long)addr) & -size); mb(); - for (; addr < end; addr += boot_cpu_data.x86_clflush_size) + for (; addr < end; addr += size) clflushopt(addr); - clflushopt(end - 1); mb(); return; } -- 2.1.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range() 2015-06-10 14:58 [PATCH] drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range() Chris Wilson @ 2015-06-11 8:25 ` Dave Gordon 2015-06-11 8:33 ` Chris Wilson 0 siblings, 1 reply; 5+ messages in thread From: Dave Gordon @ 2015-06-11 8:25 UTC (permalink / raw) To: Chris Wilson, dri-devel; +Cc: intel-gfx On 10/06/15 15:58, Chris Wilson wrote: > As the clflush operates on cache lines, and we can flush any byte > address, in order to flush all bytes given in the range we issue an > extra clflush on the last byte to ensure the last cacheline is flushed. > We can can the iteration to be over the actual cache lines to avoid this > double clflush on the last byte. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/drm_cache.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c > index 9a62d7a53553..6743ff7dccfa 100644 > --- a/drivers/gpu/drm/drm_cache.c > +++ b/drivers/gpu/drm/drm_cache.c > @@ -130,11 +130,12 @@ drm_clflush_virt_range(void *addr, unsigned long length) > { > #if defined(CONFIG_X86) > if (cpu_has_clflush) { > + const int size = boot_cpu_data.x86_clflush_size; > void *end = addr + length; > + addr = (void *)(((unsigned long)addr) & -size); Should this cast be to uintptr_t? Or intptr_t, as size has somewhat strangely been defined as signed? To complete the mix, x86_clflush_size is 'u16'! So maybe we should write + const size_t size = boot_cpu_data.x86_clflush_size; + const size_t mask = ~(size - 1); void *end = addr + length; + addr = (void *)(((uintptr_t)addr) & mask); > mb(); > - for (; addr < end; addr += boot_cpu_data.x86_clflush_size) > + for (; addr < end; addr += size) > clflushopt(addr); > - clflushopt(end - 1); > mb(); > return; > } > .Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range() 2015-06-11 8:25 ` Dave Gordon @ 2015-06-11 8:33 ` Chris Wilson 2015-06-18 15:31 ` [Intel-gfx] " Imre Deak 0 siblings, 1 reply; 5+ messages in thread From: Chris Wilson @ 2015-06-11 8:33 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx, dri-devel On Thu, Jun 11, 2015 at 09:25:16AM +0100, Dave Gordon wrote: > On 10/06/15 15:58, Chris Wilson wrote: > > As the clflush operates on cache lines, and we can flush any byte > > address, in order to flush all bytes given in the range we issue an > > extra clflush on the last byte to ensure the last cacheline is flushed. > > We can can the iteration to be over the actual cache lines to avoid this > > double clflush on the last byte. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/drm_cache.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c > > index 9a62d7a53553..6743ff7dccfa 100644 > > --- a/drivers/gpu/drm/drm_cache.c > > +++ b/drivers/gpu/drm/drm_cache.c > > @@ -130,11 +130,12 @@ drm_clflush_virt_range(void *addr, unsigned long length) > > { > > #if defined(CONFIG_X86) > > if (cpu_has_clflush) { > > + const int size = boot_cpu_data.x86_clflush_size; > > void *end = addr + length; > > + addr = (void *)(((unsigned long)addr) & -size); > > Should this cast be to uintptr_t? The kernel has a strict equivalence between sizeof(unsigned long) and sizeof(pointer). You will see unsigned long used universally to pass along pointers to functions and as closures. > Or intptr_t, as size has somewhat > strangely been defined as signed? To complete the mix, x86_clflush_size > is 'u16'! So maybe we should write > > + const size_t size = boot_cpu_data.x86_clflush_size; > + const size_t mask = ~(size - 1); > void *end = addr + length; > + addr = (void *)(((uintptr_t)addr) & mask); No. size_t has very poor definition inside the kernel - what does the maximum size of a userspace allocation have to do with kernel internals? Let's keep userspace types in userspace, or else we end up with i915_gem_gtt.c. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range() 2015-06-11 8:33 ` Chris Wilson @ 2015-06-18 15:31 ` Imre Deak 2015-06-18 15:37 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Imre Deak @ 2015-06-18 15:31 UTC (permalink / raw) To: Chris Wilson; +Cc: Dave Gordon, intel-gfx, dri-devel On to, 2015-06-11 at 09:33 +0100, Chris Wilson wrote: > On Thu, Jun 11, 2015 at 09:25:16AM +0100, Dave Gordon wrote: > > On 10/06/15 15:58, Chris Wilson wrote: > > > As the clflush operates on cache lines, and we can flush any byte > > > address, in order to flush all bytes given in the range we issue an > > > extra clflush on the last byte to ensure the last cacheline is flushed. > > > We can can the iteration to be over the actual cache lines to avoid this > > > double clflush on the last byte. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/drm_cache.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c > > > index 9a62d7a53553..6743ff7dccfa 100644 > > > --- a/drivers/gpu/drm/drm_cache.c > > > +++ b/drivers/gpu/drm/drm_cache.c > > > @@ -130,11 +130,12 @@ drm_clflush_virt_range(void *addr, unsigned long length) > > > { > > > #if defined(CONFIG_X86) > > > if (cpu_has_clflush) { > > > + const int size = boot_cpu_data.x86_clflush_size; > > > void *end = addr + length; > > > + addr = (void *)(((unsigned long)addr) & -size); > > > > Should this cast be to uintptr_t? > > The kernel has a strict equivalence between sizeof(unsigned long) and > sizeof(pointer). You will see unsigned long used universally to pass > along pointers to functions and as closures. > > > Or intptr_t, as size has somewhat > > strangely been defined as signed? To complete the mix, x86_clflush_size > > is 'u16'! So maybe we should write > > > > + const size_t size = boot_cpu_data.x86_clflush_size; > > + const size_t mask = ~(size - 1); > > void *end = addr + length; > > + addr = (void *)(((uintptr_t)addr) & mask); > > No. size_t has very poor definition inside the kernel - what does the > maximum size of a userspace allocation have to do with kernel internals? > > Let's keep userspace types in userspace, or else we end up with > i915_gem_gtt.c. I also think using unsigned long for virtual addresses is standard in the kernel and I can't see how using int would lead to problems given the expected range of x86_clflush_size, so this looks ok to me: Reviewed-by: Imre Deak <imre.deak@intel.com> > -Chris > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range() 2015-06-18 15:31 ` [Intel-gfx] " Imre Deak @ 2015-06-18 15:37 ` Daniel Vetter 0 siblings, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2015-06-18 15:37 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx, dri-devel On Thu, Jun 18, 2015 at 06:31:18PM +0300, Imre Deak wrote: > On to, 2015-06-11 at 09:33 +0100, Chris Wilson wrote: > > On Thu, Jun 11, 2015 at 09:25:16AM +0100, Dave Gordon wrote: > > > On 10/06/15 15:58, Chris Wilson wrote: > > > > As the clflush operates on cache lines, and we can flush any byte > > > > address, in order to flush all bytes given in the range we issue an > > > > extra clflush on the last byte to ensure the last cacheline is flushed. > > > > We can can the iteration to be over the actual cache lines to avoid this > > > > double clflush on the last byte. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/drm_cache.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c > > > > index 9a62d7a53553..6743ff7dccfa 100644 > > > > --- a/drivers/gpu/drm/drm_cache.c > > > > +++ b/drivers/gpu/drm/drm_cache.c > > > > @@ -130,11 +130,12 @@ drm_clflush_virt_range(void *addr, unsigned long length) > > > > { > > > > #if defined(CONFIG_X86) > > > > if (cpu_has_clflush) { > > > > + const int size = boot_cpu_data.x86_clflush_size; > > > > void *end = addr + length; > > > > + addr = (void *)(((unsigned long)addr) & -size); > > > > > > Should this cast be to uintptr_t? > > > > The kernel has a strict equivalence between sizeof(unsigned long) and > > sizeof(pointer). You will see unsigned long used universally to pass > > along pointers to functions and as closures. > > > > > Or intptr_t, as size has somewhat > > > strangely been defined as signed? To complete the mix, x86_clflush_size > > > is 'u16'! So maybe we should write > > > > > > + const size_t size = boot_cpu_data.x86_clflush_size; > > > + const size_t mask = ~(size - 1); > > > void *end = addr + length; > > > + addr = (void *)(((uintptr_t)addr) & mask); > > > > No. size_t has very poor definition inside the kernel - what does the > > maximum size of a userspace allocation have to do with kernel internals? > > > > Let's keep userspace types in userspace, or else we end up with > > i915_gem_gtt.c. > > I also think using unsigned long for virtual addresses is standard in > the kernel and I can't see how using int would lead to problems given > the expected range of x86_clflush_size, so this looks ok to me: > Reviewed-by: Imre Deak <imre.deak@intel.com> Applied to drm-misc, thanks. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-18 15:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-10 14:58 [PATCH] drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range() Chris Wilson 2015-06-11 8:25 ` Dave Gordon 2015-06-11 8:33 ` Chris Wilson 2015-06-18 15:31 ` [Intel-gfx] " Imre Deak 2015-06-18 15:37 ` Daniel Vetter
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.