Re: [PATCH v2] agp/intel: Serialise after GTT updates

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 26, 2015 at 10:47:10AM +0000, Chris Wilson wrote:
> An interesting bug occurs on Pineview through which the root cause is
> that the writes of the PTE values into the GTT is not serialised with
> subsequent memory access through the GTT (when using WC updates of the
> PTE values). This is despite there being a posting read after the GTT
> update. However, by changing the address of the posting read, the memory
> access is indeed serialised correctly.
> 
> Whilst we are manipulating the memory barriers, we can remove the
> compiler :memory restraint on the intermediate PTE writes knowing that
> we explicitly perform a posting read afterwards.
> 
> v2: Replace posting reads with explicit write memory barriers - in
> particular this is advantages in case of single page objects. Update
> comments to mention this issue is only with WC writes.
> 
> Testcase: igt/gem_exec_big #pnv
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88191
> Tested-by: huax.lu@intel.com (v1)
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Shouldn't we Cc: stable@vger.kernel.org too?
-Daniel

> ---
>  drivers/char/agp/intel-gtt.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 92aa43fa8d70..0b4188b9af7c 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -225,7 +225,7 @@ static int i810_insert_dcache_entries(struct agp_memory *mem, off_t pg_start,
>  		intel_private.driver->write_entry(addr,
>  						  i, type);
>  	}
> -	readl(intel_private.gtt+i-1);
> +	wmb();
>  
>  	return 0;
>  }
> @@ -329,7 +329,7 @@ static void i810_write_entry(dma_addr_t addr, unsigned int entry,
>  		break;
>  	}
>  
> -	writel(addr | pte_flags, intel_private.gtt + entry);
> +	writel_relaxed(addr | pte_flags, intel_private.gtt + entry);
>  }
>  
>  static const struct aper_size_info_fixed intel_fake_agp_sizes[] = {
> @@ -735,7 +735,7 @@ static void i830_write_entry(dma_addr_t addr, unsigned int entry,
>  	if (flags ==  AGP_USER_CACHED_MEMORY)
>  		pte_flags |= I830_PTE_SYSTEM_CACHED;
>  
> -	writel(addr | pte_flags, intel_private.gtt + entry);
> +	writel_relaxed(addr | pte_flags, intel_private.gtt + entry);
>  }
>  
>  bool intel_enable_gtt(void)
> @@ -858,7 +858,7 @@ void intel_gtt_insert_sg_entries(struct sg_table *st,
>  			j++;
>  		}
>  	}
> -	readl(intel_private.gtt+j-1);
> +	wmb();
>  }
>  EXPORT_SYMBOL(intel_gtt_insert_sg_entries);
>  
> @@ -875,7 +875,7 @@ static void intel_gtt_insert_pages(unsigned int first_entry,
>  		intel_private.driver->write_entry(addr,
>  						  j, flags);
>  	}
> -	readl(intel_private.gtt+j-1);
> +	wmb();
>  }
>  
>  static int intel_fake_agp_insert_entries(struct agp_memory *mem,
> @@ -938,7 +938,7 @@ void intel_gtt_clear_range(unsigned int first_entry, unsigned int num_entries)
>  		intel_private.driver->write_entry(intel_private.scratch_page_dma,
>  						  i, 0);
>  	}
> -	readl(intel_private.gtt+i-1);
> +	wmb();
>  }
>  EXPORT_SYMBOL(intel_gtt_clear_range);
>  
> @@ -1106,7 +1106,7 @@ static void i965_write_entry(dma_addr_t addr,
>  
>  	/* Shift high bits down */
>  	addr |= (addr >> 28) & 0xf0;
> -	writel(addr | pte_flags, intel_private.gtt + entry);
> +	writel_relaxed(addr | pte_flags, intel_private.gtt + entry);
>  }
>  
>  static int i9xx_setup(void)
> -- 
> 2.1.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx