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

[Wang, Zhi A]

Sure. No problem. :)

-----Original Message-----
From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On Behalf Of Joonas Lahtinen
Sent: Tuesday, September 12, 2017 4:42 PM
To: Wang, Zhi A <zhi.a.wang at intel.com>; intel-gvt-dev at lists.freedesktop.org
Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>; Chris Wilson <chris at chris-wilson.co.uk>; Widawsky, Benjamin <benjamin.widawsky at intel.com>
Subject: Re: [PATCH v15 2/5] drm/i915: Introduce private PAT management

On Tue, 2017-09-12 at 15:42 +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.
> 
> v15:
> 
> - Refine some code flow. (Joonas)
> 
> v12:
> 
> - Fix a problem "not returning the entry of best score". (Zhenyu)
> 
> 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)
> 
> Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
> Cc: Ben Widawsky <benjamin.widawsky at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk> #v7
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Acquiring this R-b still needs the other fix :)

<SNIP>

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

This needs to be;

	if (GEN8_PPAT_GET_CA(src) != GEN8_PPAT_GET_CA(dst))
		return 0;
	score += 4;

Otherwise we're returning 0 score for a matching cache attribute pair, which is an equal score to non-matching cache-attribute pair and gets skipped when finding the best entry. So a fully functional combination (although not necessarily very performant) gets omitted as it is scored zero. I think that's exactly what we don't want to happen.

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

And end of function should then be along the lines of;

	return score >= 7 ? INTEL_PPAT_PERFECT_MATCH : score;

> +
> +	return score;
> +}

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