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

[Zhi Wang <zhi.a.wang@intel.com>]

On 09/11/17 16:59, Joonas Lahtinen wrote:
> 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?
>
Aiha. cache attribute of src == cache attribute of dst is mandatory 
since the mismatch of other attribute only causes performance drop, but 
mismatch of cache attribute causes problem of correctness.
>> +
>> +	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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx