Re: [PATCH v11 2/5] drm/i915: Introduce private PAT management

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the review! :)

-----Original Message-----
From: Zhenyu Wang [mailto:zhenyuw@xxxxxxxxxxxxxxx] 
Sent: Monday, September 11, 2017 6:11 AM
To: Wang, Zhi A <zhi.a.wang@xxxxxxxxx>
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx; joonas.lahtinen@xxxxxxxxxxxxxxx; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; chris@xxxxxxxxxxxxxxxxxx; Widawsky, Benjamin <benjamin.widawsky@xxxxxxxxx>
Subject: Re: [PATCH v11 2/5] drm/i915: Introduce private PAT management

On 2017.09.09 17:03:29 +0800, Zhi Wang wrote:
> +/**
> + * intel_ppat_get - get a usable PPAT entry
> + * @i915: i915 device instance
> + * @value: the PPAT value required by the caller
> + *
> + * The function tries to search if there is an existing PPAT entry 
> +which
> + * matches with the required value. If perfectly matched, the 
> +existing PPAT
> + * entry will be used. If only partially matched, it will try to 
> +check if
> + * there is any available PPAT index. If yes, it will allocate a new 
> +PPAT
> + * index for the required entry and update the HW. If not, the 
> +partially
> + * matched entry will be used.
> + */
> +const struct intel_ppat_entry *
> +intel_ppat_get(struct drm_i915_private *i915, u8 value) {
> +	struct intel_ppat *ppat = &i915->ppat;
> +	struct intel_ppat_entry *entry;
> +	unsigned int scanned, best_score;
> +	int i;
> +
> +	GEM_BUG_ON(!ppat->max_entries);
> +
> +	scanned = best_score = 0;
> +
> +	for_each_set_bit(i, ppat->used, ppat->max_entries) {
> +		unsigned int score;
> +
> +		entry = &ppat->entries[i];
> +		score = ppat->match(entry->value, value);
> +		if (score > best_score) {
> +			if (score == INTEL_PPAT_PERFECT_MATCH) {
> +				kref_get(&entry->ref);
> +				return entry;
> +			}
> +			best_score = score;
> +		}
> +		scanned++;
> +	}
> +
> +	if (scanned == ppat->max_entries) {
> +		if (!best_score)
> +			return ERR_PTR(-ENOSPC);
> +
> +		kref_get(&entry->ref);
> +		return entry;
> +	}

This returns last fully scanned entry instead of the real best_score one?

> +
> +	i = find_first_zero_bit(ppat->used, ppat->max_entries);
> +	entry = __alloc_ppat_entry(ppat, i, value);
> +	ppat->update_hw(i915);
> +	return entry;
> +}
> +
> +static void release_ppat(struct kref *kref) {
> +	struct intel_ppat_entry *entry =
> +		container_of(kref, struct intel_ppat_entry, ref);
> +	struct drm_i915_private *i915 = entry->ppat->i915;
> +
> +	__free_ppat_entry(entry);
> +	entry->ppat->update_hw(i915);
> +}
> +
> +/**
> + * intel_ppat_put - put back the PPAT entry got from intel_ppat_get()
> + * @entry: an intel PPAT entry
> + *
> + * Put back the PPAT entry got from intel_ppat_get(). If the PPAT 
> +index of the
> + * entry is dynamically allocated, its reference count will be 
> +decreased. Once
> + * the reference count becomes into zero, the PPAT index becomes free again.
> + */
> +void intel_ppat_put(const struct intel_ppat_entry *entry) {
> +	struct intel_ppat *ppat = entry->ppat;
> +	unsigned int index = entry - ppat->entries;
> +
> +	GEM_BUG_ON(!ppat->max_entries);
> +
> +	kref_put(&ppat->entries[index].ref, release_ppat); }
> +
> +static void cnl_private_pat_update_hw(struct drm_i915_private 
> +*dev_priv) {
> +	struct intel_ppat *ppat = &dev_priv->ppat;
> +	int i;
> +
> +	for_each_set_bit(i, ppat->dirty, ppat->max_entries) {
> +		I915_WRITE(GEN10_PAT_INDEX(i), ppat->entries[i].value);
> +		clear_bit(i, ppat->dirty);
> +	}
> +}
> +
> +static void bdw_private_pat_update_hw(struct drm_i915_private 
> +*dev_priv) {
> +	struct intel_ppat *ppat = &dev_priv->ppat;
> +	u64 pat = 0;
> +	int i;
> +
> +	for (i = 0; i < ppat->max_entries; i++)
> +		pat |= GEN8_PPAT(i, ppat->entries[i].value);
> +
> +	bitmap_clear(ppat->dirty, 0, ppat->max_entries);
> +
> +	I915_WRITE(GEN8_PRIVATE_PAT_LO, lower_32_bits(pat));
> +	I915_WRITE(GEN8_PRIVATE_PAT_HI, upper_32_bits(pat)); }
> +
> +static unsigned int bdw_private_pat_match(u8 src, u8 dst) {
> +	unsigned int score = 0;
> +
> +	/* Cache attribute has to be matched. */
> +	if (GEN8_PPAT_GET_CA(src) != GEN8_PPAT_GET_CA(dst))
> +		return 0;
> +
> +	if (GEN8_PPAT_GET_TC(src) == GEN8_PPAT_GET_TC(dst))
> +		score += 2;
> +
> +	if (GEN8_PPAT_GET_AGE(src) == GEN8_PPAT_GET_AGE(dst))
> +		score += 1;
> +
> +	if (score == 3)
> +		return INTEL_PPAT_PERFECT_MATCH;
> +
> +	return score;
> +}
> +
> +static unsigned int chv_private_pat_match(u8 src, u8 dst) {
> +	return (CHV_PPAT_GET_SNOOP(src) == CHV_PPAT_GET_SNOOP(dst)) ?
> +		INTEL_PPAT_PERFECT_MATCH : 0;
> +}
> +
> +static void cnl_setup_private_ppat(struct intel_ppat *ppat) {
> +	ppat->max_entries = 8;
> +	ppat->update_hw = cnl_private_pat_update_hw;
> +	ppat->match = bdw_private_pat_match;
> +	ppat->clear_value = GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | 
> +GEN8_PPAT_AGE(3);
> +
>  	/* XXX: spec is unclear if this is still needed for CNL+ */
> -	if (!USES_PPGTT(dev_priv)) {
> -		I915_WRITE(GEN10_PAT_INDEX(0), GEN8_PPAT_UC);
> +	if (!USES_PPGTT(ppat->i915)) {
> +		__alloc_ppat_entry(ppat, 0, GEN8_PPAT_UC);
>  		return;
>  	}
>  
> -	I915_WRITE(GEN10_PAT_INDEX(0), GEN8_PPAT_WB | GEN8_PPAT_LLC);
> -	I915_WRITE(GEN10_PAT_INDEX(1), GEN8_PPAT_WC | GEN8_PPAT_LLCELLC);
> -	I915_WRITE(GEN10_PAT_INDEX(2), GEN8_PPAT_WT | GEN8_PPAT_LLCELLC);
> -	I915_WRITE(GEN10_PAT_INDEX(3), GEN8_PPAT_UC);
> -	I915_WRITE(GEN10_PAT_INDEX(4), GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(0));
> -	I915_WRITE(GEN10_PAT_INDEX(5), GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(1));
> -	I915_WRITE(GEN10_PAT_INDEX(6), GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2));
> -	I915_WRITE(GEN10_PAT_INDEX(7), GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(3));
> +	__alloc_ppat_entry(ppat, 0, GEN8_PPAT_WB | GEN8_PPAT_LLC);
> +	__alloc_ppat_entry(ppat, 1, GEN8_PPAT_WC | GEN8_PPAT_LLCELLC);
> +	__alloc_ppat_entry(ppat, 2, GEN8_PPAT_WT | GEN8_PPAT_LLCELLC);
> +	__alloc_ppat_entry(ppat, 3, GEN8_PPAT_UC);
> +	__alloc_ppat_entry(ppat, 4, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(0));
> +	__alloc_ppat_entry(ppat, 5, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(1));
> +	__alloc_ppat_entry(ppat, 6, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2));
> +	__alloc_ppat_entry(ppat, 7, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | 
> +GEN8_PPAT_AGE(3));
>  }
>  
>  /* The GGTT and PPGTT need a private PPAT setup in order to handle cacheability
>   * bits. When using advanced contexts each context stores its own PAT, but
>   * writing this data shouldn't be harmful even in those cases. */ 
> -static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv)
> +static void bdw_setup_private_ppat(struct intel_ppat *ppat)
>  {
> -	u64 pat;
> +	ppat->max_entries = 8;
> +	ppat->update_hw = bdw_private_pat_update_hw;
> +	ppat->match = bdw_private_pat_match;
> +	ppat->clear_value = GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | 
> +GEN8_PPAT_AGE(3);
>  
> -	pat = GEN8_PPAT(0, GEN8_PPAT_WB | GEN8_PPAT_LLC)     | /* for normal objects, no eLLC */
> -	      GEN8_PPAT(1, GEN8_PPAT_WC | GEN8_PPAT_LLCELLC) | /* for something pointing to ptes? */
> -	      GEN8_PPAT(2, GEN8_PPAT_WT | GEN8_PPAT_LLCELLC) | /* for scanout with eLLC */
> -	      GEN8_PPAT(3, GEN8_PPAT_UC)                     | /* Uncached objects, mostly for scanout */
> -	      GEN8_PPAT(4, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(0)) |
> -	      GEN8_PPAT(5, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(1)) |
> -	      GEN8_PPAT(6, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2)) |
> -	      GEN8_PPAT(7, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(3));
> -
> -	if (!USES_PPGTT(dev_priv))
> +	if (!USES_PPGTT(ppat->i915)) {
>  		/* Spec: "For GGTT, there is NO pat_sel[2:0] from the entry,
>  		 * so RTL will always use the value corresponding to
>  		 * pat_sel = 000".
> @@ -2790,17 +2952,26 @@ static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv)
>  		 * So we can still hold onto all our assumptions wrt cpu
>  		 * clflushing on LLC machines.
>  		 */
> -		pat = GEN8_PPAT(0, GEN8_PPAT_UC);
> +		__alloc_ppat_entry(ppat, 0, GEN8_PPAT_UC);
> +		return;
> +	}
>  
> -	/* XXX: spec defines this as 2 distinct registers. It's unclear if a 64b
> -	 * write would work. */
> -	I915_WRITE(GEN8_PRIVATE_PAT_LO, pat);
> -	I915_WRITE(GEN8_PRIVATE_PAT_HI, pat >> 32);
> +	__alloc_ppat_entry(ppat, 0, GEN8_PPAT_WB | GEN8_PPAT_LLC);      /* for normal objects, no eLLC */
> +	__alloc_ppat_entry(ppat, 1, GEN8_PPAT_WC | GEN8_PPAT_LLCELLC);  /* for something pointing to ptes? */
> +	__alloc_ppat_entry(ppat, 2, GEN8_PPAT_WT | GEN8_PPAT_LLCELLC);  /* for scanout with eLLC */
> +	__alloc_ppat_entry(ppat, 3, GEN8_PPAT_UC);                      /* Uncached objects, mostly for scanout */
> +	__alloc_ppat_entry(ppat, 4, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(0));
> +	__alloc_ppat_entry(ppat, 5, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(1));
> +	__alloc_ppat_entry(ppat, 6, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2));
> +	__alloc_ppat_entry(ppat, 7, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | 
> +GEN8_PPAT_AGE(3));
>  }
>  
> -static void chv_setup_private_ppat(struct drm_i915_private *dev_priv)
> +static void chv_setup_private_ppat(struct intel_ppat *ppat)
>  {
> -	u64 pat;
> +	ppat->max_entries = 8;
> +	ppat->update_hw = bdw_private_pat_update_hw;
> +	ppat->match = chv_private_pat_match;
> +	ppat->clear_value = CHV_PPAT_SNOOP;
>  
>  	/*
>  	 * Map WB on BDW to snooped on CHV.
> @@ -2820,17 +2991,15 @@ static void chv_setup_private_ppat(struct drm_i915_private *dev_priv)
>  	 * Which means we must set the snoop bit in PAT entry 0
>  	 * in order to keep the global status page working.
>  	 */
> -	pat = GEN8_PPAT(0, CHV_PPAT_SNOOP) |
> -	      GEN8_PPAT(1, 0) |
> -	      GEN8_PPAT(2, 0) |
> -	      GEN8_PPAT(3, 0) |
> -	      GEN8_PPAT(4, CHV_PPAT_SNOOP) |
> -	      GEN8_PPAT(5, CHV_PPAT_SNOOP) |
> -	      GEN8_PPAT(6, CHV_PPAT_SNOOP) |
> -	      GEN8_PPAT(7, CHV_PPAT_SNOOP);
>  
> -	I915_WRITE(GEN8_PRIVATE_PAT_LO, pat);
> -	I915_WRITE(GEN8_PRIVATE_PAT_HI, pat >> 32);
> +	__alloc_ppat_entry(ppat, 0, CHV_PPAT_SNOOP);
> +	__alloc_ppat_entry(ppat, 1, 0);
> +	__alloc_ppat_entry(ppat, 2, 0);
> +	__alloc_ppat_entry(ppat, 3, 0);
> +	__alloc_ppat_entry(ppat, 4, CHV_PPAT_SNOOP);
> +	__alloc_ppat_entry(ppat, 5, CHV_PPAT_SNOOP);
> +	__alloc_ppat_entry(ppat, 6, CHV_PPAT_SNOOP);
> +	__alloc_ppat_entry(ppat, 7, CHV_PPAT_SNOOP);
>  }
>  
>  static void gen6_gmch_remove(struct i915_address_space *vm) @@ 
> -2843,12 +3012,27 @@ static void gen6_gmch_remove(struct 
> i915_address_space *vm)
>  
>  static void setup_private_pat(struct drm_i915_private *dev_priv)  {
> +	struct intel_ppat *ppat = &dev_priv->ppat;
> +	int i;
> +
> +	ppat->i915 = dev_priv;
> +
>  	if (INTEL_GEN(dev_priv) >= 10)
> -		cnl_setup_private_ppat(dev_priv);
> +		cnl_setup_private_ppat(ppat);
>  	else if (IS_CHERRYVIEW(dev_priv) || IS_GEN9_LP(dev_priv))
> -		chv_setup_private_ppat(dev_priv);
> +		chv_setup_private_ppat(ppat);
>  	else
> -		bdw_setup_private_ppat(dev_priv);
> +		bdw_setup_private_ppat(ppat);
> +
> +	GEM_BUG_ON(ppat->max_entries > INTEL_MAX_PPAT_ENTRIES);
> +
> +	for_each_clear_bit(i, ppat->used, ppat->max_entries) {
> +		ppat->entries[i].value = ppat->clear_value;
> +		ppat->entries[i].ppat = ppat;
> +		set_bit(i, ppat->dirty);
> +	}
> +
> +	ppat->update_hw(dev_priv);
>  }
>  
>  static int gen8_gmch_probe(struct i915_ggtt *ggtt) @@ -3162,13 
> +3346,10 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
>  	ggtt->base.closed = false;
>  
>  	if (INTEL_GEN(dev_priv) >= 8) {
> -		if (INTEL_GEN(dev_priv) >= 10)
> -			cnl_setup_private_ppat(dev_priv);
> -		else if (IS_CHERRYVIEW(dev_priv) || IS_GEN9_LP(dev_priv))
> -			chv_setup_private_ppat(dev_priv);
> -		else
> -			bdw_setup_private_ppat(dev_priv);
> +		struct intel_ppat *ppat = &dev_priv->ppat;
>  
> +		bitmap_set(ppat->dirty, 0, ppat->max_entries);
> +		dev_priv->ppat.update_hw(dev_priv);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index b4e3aa7..e10ca89 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -143,6 +143,11 @@ typedef u64 gen8_ppgtt_pml4e_t;
>  #define GEN8_PPAT_ELLC_OVERRIDE		(0<<2)
>  #define GEN8_PPAT(i, x)			((u64)(x) << ((i) * 8))
>  
> +#define GEN8_PPAT_GET_CA(x) ((x) & 3) #define GEN8_PPAT_GET_TC(x) 
> +((x) & (3 << 2)) #define GEN8_PPAT_GET_AGE(x) ((x) & (3 << 4)) 
> +#define CHV_PPAT_GET_SNOOP(x) ((x) & (1 << 6))
> +
>  struct sg_table;
>  
>  struct intel_rotation_info {
> @@ -536,6 +541,37 @@ i915_vm_to_ggtt(struct i915_address_space *vm)
>  	return container_of(vm, struct i915_ggtt, base);  }
>  
> +#define INTEL_MAX_PPAT_ENTRIES 8
> +#define INTEL_PPAT_PERFECT_MATCH (~0U)
> +
> +struct intel_ppat;
> +
> +struct intel_ppat_entry {
> +	struct intel_ppat *ppat;
> +	struct kref ref;
> +	u8 value;
> +};
> +
> +struct intel_ppat {
> +	struct intel_ppat_entry entries[INTEL_MAX_PPAT_ENTRIES];
> +	DECLARE_BITMAP(used, INTEL_MAX_PPAT_ENTRIES);
> +	DECLARE_BITMAP(dirty, INTEL_MAX_PPAT_ENTRIES);
> +	unsigned int max_entries;
> +	u8 clear_value;
> +	/*
> +	 * Return a score to show how two PPAT values match,
> +	 * a INTEL_PPAT_PERFECT_MATCH indicates a perfect match
> +	 */
> +	unsigned int (*match)(u8 src, u8 dst);
> +	void (*update_hw)(struct drm_i915_private *i915);
> +
> +	struct drm_i915_private *i915;
> +};
> +
> +const struct intel_ppat_entry *
> +intel_ppat_get(struct drm_i915_private *i915, u8 value); void 
> +intel_ppat_put(const struct intel_ppat_entry *entry);
> +
>  int i915_gem_init_aliasing_ppgtt(struct drm_i915_private *i915);  
> void i915_gem_fini_aliasing_ppgtt(struct drm_i915_private *i915);
>  
> --
> 2.7.4
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

--
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux