Re: [PATCH v13 4/5] drm/i915: Do not allocate unused PPAT entries

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Zhi Wang <zhi.a.wang@intel.com>,
	intel-gfx@lists.freedesktop.org,
	intel-gvt-dev@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [PATCH v13 4/5] drm/i915: Do not allocate unused PPAT entries
Date: Mon, 11 Sep 2017 12:16:31 +0300	[thread overview]
Message-ID: <1505121391.4107.26.camel@linux.intel.com> (raw)
In-Reply-To: <1505103993-9009-4-git-send-email-zhi.a.wang@intel.com>

On Mon, 2017-09-11 at 12:26 +0800, Zhi Wang wrote:
> Only PPAT entries 0/2/3/4 are using. Remove extra PPAT entry allocation
> during initialization.
> 
> v8:
> 
> - Move ppat_index() into i915_gem_gtt.c. (Chris)
> - Change the name of ppat_bits_to_index to ppat_index.
> 
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>

<SNIP>

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 53 +++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8b388aa..56fdfc6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2979,6 +2979,13 @@ static unsigned int chv_private_pat_match(u8 src, u8 dst)
>  		INTEL_PPAT_PERFECT_MATCH : 0;
>  }
>  
> +/* PPAT index = 4 * PAT + 2 * PCD + PWT */
> +static inline unsigned int ppat_index(unsigned int bits)
> +{
> +	return (4 * !!(bits & _PAGE_PAT) + 2 * !!(bits & _PAGE_PCD)
> +		+ !!(bits & _PAGE_PWT));

Would this be more readable as

	return 4 * !!(bits & _PAGE_PAT) +
	       2 * !!(bits & _PAGE_PCD) +
	       1 * !!(bits & _PAGE_PWT);

>  /* The GGTT and PPGTT need a private PPAT setup in order to handle cacheability
> @@ -3026,18 +3030,18 @@ static void bdw_setup_private_ppat(struct intel_ppat *ppat)
>  		 * So we can still hold onto all our assumptions wrt cpu
>  		 * clflushing on LLC machines.
>  		 */
> -		__alloc_ppat_entry(ppat, 0, GEN8_PPAT_UC);
> +		__alloc_ppat_entry(ppat, ppat_index(PPAT_CACHED_PDE), GEN8_PPAT_UC);
>  		return;
>  	}
>  
> -	__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));
> +	/* See gen8_pte_encode() for the mapping from cache-level to PPAT */

Maybe lift these three comments below to the gen8_pte_encode? The
usages are driver wide, really.

Anyway, this is;

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

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