Re: [PATCH] drm/i915: Add ppgtt->kunmap_page_dma vfunc

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 17/05/16 14:34, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Rather than asking itself "am I a Broadwell, am I a Cherryview,
> or am I neither of the two" on on low level page table operations,
> like inserting and clearing PTEs; add a new vfunc kunmap_page_dma
> and set it to appropriate flavour at ppgtt init time.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 51 +++++++++++++++++++++++--------------
>   drivers/gpu/drm/i915/i915_gem_gtt.h |  1 +
>   2 files changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7eab619a3eb2..dc7e128d7483 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -361,29 +361,30 @@ static void *kmap_page_dma(struct i915_page_dma *p)
>   	return kmap_atomic(p->page);
>   }
>   
> +static void kunmap_page_dma(void *vaddr)
> +{
> +	kunmap_atomic(vaddr);
> +}
> +
>   /* We use the flushing unmap only with ppgtt structures:
>    * page directories, page tables and scratch pages.
>    */
> -static void kunmap_page_dma(struct drm_device *dev, void *vaddr)
> +static void kunmap_page_dma_flush(void *vaddr)
>   {
> -	/* There are only few exceptions for gen >=6. chv and bxt.
> -	 * And we are not sure about the latter so play safe for now.
> -	 */
> -	if (IS_CHERRYVIEW(dev) || IS_BROXTON(dev))
> -		drm_clflush_virt_range(vaddr, PAGE_SIZE);
> +	drm_clflush_virt_range(vaddr, PAGE_SIZE);
>   
>   	kunmap_atomic(vaddr);
>   }
>   
>   #define kmap_px(px) kmap_page_dma(px_base(px))
> -#define kunmap_px(ppgtt, vaddr) kunmap_page_dma((ppgtt)->base.dev, (vaddr))
> +#define kunmap_px(ppgtt, vaddr) (ppgtt)->kunmap_page_dma((vaddr))
>   
>   #define setup_px(dev, px) setup_page_dma((dev), px_base(px))
>   #define cleanup_px(dev, px) cleanup_page_dma((dev), px_base(px))
> -#define fill_px(dev, px, v) fill_page_dma((dev), px_base(px), (v))
> -#define fill32_px(dev, px, v) fill_page_dma_32((dev), px_base(px), (v))
> +#define fill_px(ppgtt, px, v) fill_page_dma((ppgtt), px_base(px), (v))
> +#define fill32_px(ppgtt, px, v) fill_page_dma_32((ppgtt), px_base(px), (v))
>   

This feels a bit asymmetric, because some of the functions expect the 
ppgtt pointer while others expect the dev pointer. Nothing clean comes 
to mind to solve this immediately without adding extra pointer 
indirections, but I guess we could come back to standardize it in the 
future if we start passing the ppgtt pointer around more (e.g. like in 
https://patchwork.freedesktop.org/patch/83777/). Not a blocker for this 
patch anyway.

> -static void fill_page_dma(struct drm_device *dev, struct i915_page_dma *p,
> +static void fill_page_dma(struct i915_hw_ppgtt *ppgtt, struct i915_page_dma *p,
>   			  const uint64_t val)
>   {
>   	int i;
> @@ -392,17 +393,17 @@ static void fill_page_dma(struct drm_device *dev, struct i915_page_dma *p,
>   	for (i = 0; i < 512; i++)
>   		vaddr[i] = val;
>   
> -	kunmap_page_dma(dev, vaddr);
> +	ppgtt->kunmap_page_dma(vaddr);
>   }
>   
> -static void fill_page_dma_32(struct drm_device *dev, struct i915_page_dma *p,
> -			     const uint32_t val32)
> +static void fill_page_dma_32(struct i915_hw_ppgtt *ppgtt,
> +			     struct i915_page_dma *p, const uint32_t val32)
>   {
>   	uint64_t v = val32;
>   
>   	v = v << 32 | val32;
>   
> -	fill_page_dma(dev, p, v);
> +	fill_page_dma(ppgtt, p, v);
>   }
>   
>   static struct i915_page_scratch *alloc_scratch_page(struct drm_device *dev)
> @@ -480,7 +481,7 @@ static void gen8_initialize_pt(struct i915_address_space *vm,
>   	scratch_pte = gen8_pte_encode(px_dma(vm->scratch_page),
>   				      I915_CACHE_LLC, true);
>   
> -	fill_px(vm->dev, pt, scratch_pte);
> +	fill_px(i915_vm_to_ppgtt(vm), pt, scratch_pte);
>   }
>   
>   static void gen6_initialize_pt(struct i915_address_space *vm,
> @@ -493,7 +494,7 @@ static void gen6_initialize_pt(struct i915_address_space *vm,
>   	scratch_pte = vm->pte_encode(px_dma(vm->scratch_page),
>   				     I915_CACHE_LLC, true, 0);
>   
> -	fill32_px(vm->dev, pt, scratch_pte);
> +	fill32_px(i915_vm_to_ppgtt(vm), pt, scratch_pte);
>   }
>   
>   static struct i915_page_directory *alloc_pd(struct drm_device *dev)
> @@ -540,7 +541,7 @@ static void gen8_initialize_pd(struct i915_address_space *vm,
>   
>   	scratch_pde = gen8_pde_encode(px_dma(vm->scratch_pt), I915_CACHE_LLC);
>   
> -	fill_px(vm->dev, pd, scratch_pde);
> +	fill_px(i915_vm_to_ppgtt(vm), pd, scratch_pde);
>   }
>   
>   static int __pdp_init(struct drm_device *dev,
> @@ -621,7 +622,7 @@ static void gen8_initialize_pdp(struct i915_address_space *vm,
>   
>   	scratch_pdpe = gen8_pdpe_encode(px_dma(vm->scratch_pd), I915_CACHE_LLC);
>   
> -	fill_px(vm->dev, pdp, scratch_pdpe);
> +	fill_px(i915_vm_to_ppgtt(vm), pdp, scratch_pdpe);
>   }
>   
>   static void gen8_initialize_pml4(struct i915_address_space *vm,
> @@ -632,7 +633,7 @@ static void gen8_initialize_pml4(struct i915_address_space *vm,
>   	scratch_pml4e = gen8_pml4e_encode(px_dma(vm->scratch_pdp),
>   					  I915_CACHE_LLC);
>   
> -	fill_px(vm->dev, pml4, scratch_pml4e);
> +	fill_px(i915_vm_to_ppgtt(vm), pml4, scratch_pml4e);
>   }
>   
>   static void
> @@ -1512,8 +1513,17 @@ static int gen8_preallocate_top_level_pdps(struct i915_hw_ppgtt *ppgtt)
>    */
>   static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>   {
> +	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
>   	int ret;
>   
> +	/* There are only few exceptions for gen >=6. chv and bxt.
> +	 * And we are not sure about the latter so play safe for now.
> +	 */
> +	if (IS_BROADWELL(dev_priv) || IS_CHERRYVIEW(dev_priv))

This should be IS_BROXTON instead of IS_BROADWELL.

> +		ppgtt->kunmap_page_dma = kunmap_page_dma_flush;
> +	else
> +		ppgtt->kunmap_page_dma = kunmap_page_dma;
> +

This virtual function assignment comes before the gen8_init_scratch call 
while all the others are after that. To keep all of them together we 
could move the call to gen8_init_scratch further down.

Regards,
Daniele

>   	ret = gen8_init_scratch(&ppgtt->base);
>   	if (ret)
>   		return ret;
> @@ -2073,6 +2083,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>   	int ret;
>   
>   	ppgtt->base.pte_encode = ggtt->base.pte_encode;
> +
> +	ppgtt->kunmap_page_dma = kunmap_page_dma;
> +
>   	if (IS_GEN6(dev)) {
>   		ppgtt->switch_mm = gen6_mm_switch;
>   	} else if (IS_HASWELL(dev)) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 62be77cac5cd..b36b997406c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -379,6 +379,7 @@ struct i915_hw_ppgtt {
>   
>   	gen6_pte_t __iomem *pd_addr;
>   
> +	void (*kunmap_page_dma)(void *vaddr);
>   	int (*enable)(struct i915_hw_ppgtt *ppgtt);
>   	int (*switch_mm)(struct i915_hw_ppgtt *ppgtt,
>   			 struct drm_i915_gem_request *req);

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