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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On Mon, 2017-09-11 at 12:26 +0800, Zhi Wang wrote:
> The private PAT management is to support PPAT entry manipulation. Two
> APIs are introduced for dynamically managing PPAT entries: intel_ppat_get
> and intel_ppat_put.
> 
> intel_ppat_get will search for an existing PPAT entry which perfectly
> matches the required PPAT value. If not, it will try to allocate a new
> entry if there is any available PPAT indexs, or return a partially
> matched PPAT entry if there is no available PPAT indexes.
> 
> intel_ppat_put will put back the PPAT entry which comes from
> intel_ppat_get. If it's dynamically allocated, the reference count will
> be decreased. If the reference count turns into zero, the PPAT index is
> freed again.
> 
> Besides, another two callbacks are introduced to support the private PAT
> management framework. One is ppat->update_hw(), which writes the PPAT
> configurations in ppat->entries into HW. Another one is ppat->match, which
> will return a score to show how two PPAT values match with each other.
> 
> v12:
> 
> - Fix a problem "not returning the entry of best score". (Zhenyu)

This change should have resulted in adding an indication that Chris
reviewed only a previous version of the patch.

> 
> v7:
> 
> - Keep all the register writes unchanged in this patch. (Joonas)
> 
> v6:
> 
> - Address all comments from Chris:
> http://www.spinics.net/lists/intel-gfx/msg136850.html
> 
> - Address all comments from Joonas:
> http://www.spinics.net/lists/intel-gfx/msg136845.html
> 
> v5:
> 
> - Add check and warnnings for those platforms which don't have PPAT.
> 
> v3:
> 
> - Introduce dirty bitmap for PPAT registers. (Chris)
> - Change the name of the pointer "dev_priv" to "i915". (Chris)
> - intel_ppat_{get, put} returns/takes a const intel_ppat_entry *. (Chris)
> 
> v2:
> 
> - API re-design. (Chris)
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>

<SNIP>

> +/**
> + * 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;

You can drop this extra newline.

> +	for_each_set_bit(i, ppat->used, ppat->max_entries) {
> +		unsigned int score;
> +
> +		score = ppat->match(ppat->entries[i].value, value);
> +		if (score > best_score) {

If you set "entry = &ppat->entries[i];" here already.

> +			if (score == INTEL_PPAT_PERFECT_MATCH) {
> +				kref_get(&ppat->entries[i].ref);
> +				return &ppat->entries[i];

These become "kref_get(&entry->ref);" and "return entry;"

> +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;

We're not giving any points for when only cache attribute matches? Does
not this result in ENOSPC when we would have an entry with matching
"cache attribute", but no other matching entries while PPAT is full.

	so maybe score += 4 here?

> +
> +	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)

	(score == 7) respectively.

> +		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;

This handles the situation correctly, when snooping is the only
attribute looked for.

With the BDW attribute fix scoring, this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Remember to add to the end of the tag list ;)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx