diff mbox

drm: Explicitly compute the last cacheline for clflush on range

Message ID 1445025355-19348-1-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson Oct. 16, 2015, 7:55 p.m. UTC
Fixes regression from

commit afcd950cafea6e27b739fe7772cbbeed37d05b8b
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jun 10 15:58:01 2015 +0100

    drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range()

I'm stumped. Looking at the loop we should be iterating over every cache
line until we reach the start of the cacheline after the end of the
virtual range. Evidence says otherwise.

More bizarely, I stored the last address to be clflushed and found it to
be equal to the start of the cacheline containing the last byte. Doubly
purplexed.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92501
Testcase: gem_tiled_partial_pwrite_pread/reads
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_cache.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Imre Deak Oct. 17, 2015, 8:03 p.m. UTC | #1
On Fri, 2015-10-16 at 20:55 +0100, Chris Wilson wrote:
> Fixes regression from
> 
> commit afcd950cafea6e27b739fe7772cbbeed37d05b8b
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Jun 10 15:58:01 2015 +0100
> 
>     drm: Avoid the double clflush on the last cache line in drm_clflush_virt_range()
> 
> I'm stumped. Looking at the loop we should be iterating over every cache
> line until we reach the start of the cacheline after the end of the
> virtual range. Evidence says otherwise.
> 
> More bizarely, I stored the last address to be clflushed and found it to
> be equal to the start of the cacheline containing the last byte. Doubly
> purplexed.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92501
> Testcase: gem_tiled_partial_pwrite_pread/reads
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_cache.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
> index 6743ff7dccfa..7c909bc8b68a 100644
> --- a/drivers/gpu/drm/drm_cache.c
> +++ b/drivers/gpu/drm/drm_cache.c
> @@ -131,10 +131,13 @@ 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);
> +		void *end;
> +
> +		end = (void *)(((unsigned long)addr + length - 1) & -size);
> +		addr = (void *)((unsigned long)addr & -size);
> +
>  		mb();
> -		for (; addr < end; addr += size)
> +		for (; addr <= end; addr += size)

Hm, I can't see how could this make any difference. The old way still
looks ok to me and the new version would flush the exact same cache
lines as the old one using the same addresses (beginning of each cache
line).

>  			clflushopt(addr);
>  		mb();
>  		return;
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index 6743ff7dccfa..7c909bc8b68a 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -131,10 +131,13 @@  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);
+		void *end;
+
+		end = (void *)(((unsigned long)addr + length - 1) & -size);
+		addr = (void *)((unsigned long)addr & -size);
+
 		mb();
-		for (; addr < end; addr += size)
+		for (; addr <= end; addr += size)
 			clflushopt(addr);
 		mb();
 		return;