Re: [PATCH 2/9] drm/i915/bdw: Reorganize PPGTT init

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9829 bytes --]

On Wed, 2014-02-12 at 14:28 -0800, Ben Widawsky wrote:
> Create 3 clear stages in PPGTT init. This will help with upcoming
> changes be more readable. The 3 stages are, allocation, dma mapping, and
> writing the P[DT]Es
> 
> One nice benefit to the patches is that it makes 2 very clear error
> points, allocation, and mapping, and avoids having to do any handling
> after writing PTEs (something which was likely buggy before). This
> simplified error handling I suspect will be helpful when we move to
> deferred/dynamic page table allocation and mapping.
> 
> The patches also attempts to break up some of the steps into more
> logical reviewable chunks, particularly when we free.
> 
> v2: Don't call cleanup on the error path since that takes down the
> drm_mm and list entry, which aren't setup at this point.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |   2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 124 +++++++++++++++++++++---------------
>  2 files changed, 73 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2572a95..cecbb9a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -709,7 +709,7 @@ struct i915_hw_ppgtt {
>  	};
>  	union {
>  		dma_addr_t *pt_dma_addr;
> -		dma_addr_t *gen8_pt_dma_addr[4];
> +		dma_addr_t **gen8_pt_dma_addr;

If there isn't any reason to allocate this dynamically I'd just leave
the static array. This would make the error path a bit simpler and be
more symmetric wrt. pd_dma_addr which is also a static array.

>  	};
>  
>  	int (*enable)(struct i915_hw_ppgtt *ppgtt);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ee38faf..c6c221c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -326,12 +326,14 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>  	for (i = 0; i < ppgtt->num_pd_pages ; i++)
>  		kfree(ppgtt->gen8_pt_dma_addr[i]);
>  
> +	kfree(ppgtt->gen8_pt_dma_addr);
>  	__free_pages(ppgtt->gen8_pt_pages, get_order(ppgtt->num_pt_pages << PAGE_SHIFT));
>  	__free_pages(ppgtt->pd_pages, get_order(ppgtt->num_pd_pages << PAGE_SHIFT));
>  }
>  
>  static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
>  {
> +	struct pci_dev *hwdev = ppgtt->base.dev->pdev;
>  	int i, j;
>  
>  	for (i = 0; i < ppgtt->num_pd_pages; i++) {
> @@ -340,18 +342,14 @@ static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
>  		if (!ppgtt->pd_dma_addr[i])
>  			continue;
>  
> -		pci_unmap_page(ppgtt->base.dev->pdev,
> -			       ppgtt->pd_dma_addr[i],
> -			       PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
> +		pci_unmap_page(hwdev, ppgtt->pd_dma_addr[i], PAGE_SIZE,
> +			       PCI_DMA_BIDIRECTIONAL);
>  
>  		for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>  			dma_addr_t addr = ppgtt->gen8_pt_dma_addr[i][j];
>  			if (addr)
> -				pci_unmap_page(ppgtt->base.dev->pdev,
> -				       addr,
> -				       PAGE_SIZE,
> -				       PCI_DMA_BIDIRECTIONAL);
> -
> +				pci_unmap_page(hwdev, addr, PAGE_SIZE,
> +					       PCI_DMA_BIDIRECTIONAL);
>  		}
>  	}
>  }
> @@ -369,27 +367,26 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>  }
>  
>  /**
> - * GEN8 legacy ppgtt programming is accomplished through 4 PDP registers with a
> - * net effect resembling a 2-level page table in normal x86 terms. Each PDP
> - * represents 1GB of memory
> - * 4 * 512 * 512 * 4096 = 4GB legacy 32b address space.
> + * GEN8 legacy ppgtt programming is accomplished through a max 4 PDP registers
> + * with a net effect resembling a 2-level page table in normal x86 terms. Each
> + * PDP represents 1GB of memory 4 * 512 * 512 * 4096 = 4GB legacy 32b address
> + * space.
>   *
> + * FIXME: split allocation into smaller pieces. For now we only ever do this
> + * once, but with full PPGTT, the multiple contiguous allocations will be bad.
>   * TODO: Do something with the size parameter
> - **/
> + */
>  static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  {
>  	struct page *pt_pages;
> -	int i, j, ret = -ENOMEM;
>  	const int max_pdp = DIV_ROUND_UP(size, 1 << 30);
>  	const int num_pt_pages = GEN8_PDES_PER_PAGE * max_pdp;
> +	int i, j, ret;
>  
>  	if (size % (1<<30))
>  		DRM_INFO("Pages will be wasted unless GTT size (%llu) is divisible by 1GB\n", size);
>  
> -	/* FIXME: split allocation into smaller pieces. For now we only ever do
> -	 * this once, but with full PPGTT, the multiple contiguous allocations
> -	 * will be bad.
> -	 */
> +	/* 1. Do all our allocations for page directories and page tables */
>  	ppgtt->pd_pages = alloc_pages(GFP_KERNEL, get_order(max_pdp << PAGE_SHIFT));
>  	if (!ppgtt->pd_pages)
>  		return -ENOMEM;
> @@ -404,52 +401,66 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	ppgtt->num_pd_pages = 1 << get_order(max_pdp << PAGE_SHIFT);
>  	ppgtt->num_pt_pages = 1 << get_order(num_pt_pages << PAGE_SHIFT);
>  	ppgtt->num_pd_entries = max_pdp * GEN8_PDES_PER_PAGE;
> -	ppgtt->enable = gen8_ppgtt_enable;
> -	ppgtt->switch_mm = gen8_mm_switch;
> -	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
> -	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
> -	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
> -	ppgtt->base.start = 0;
> -	ppgtt->base.total = ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE;
> -
>  	BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS);
>  
> +	ppgtt->gen8_pt_dma_addr = kcalloc(max_pdp,
> +					  sizeof(*ppgtt->gen8_pt_dma_addr),
> +					  GFP_KERNEL);
> +	if (!ppgtt->gen8_pt_dma_addr) {
> +		ret = -ENOMEM;
> +		goto bail;

On the error path, in gen8_ppgtt_free() we'd dereference the above NULL
ptr.

> +	}
> +
> +	for (i = 0; i < max_pdp; i++) {
> +		ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
> +						     sizeof(dma_addr_t),
> +						     GFP_KERNEL);
> +		if (!ppgtt->gen8_pt_dma_addr[i]) {
> +			ret = -ENOMEM;
> +			goto bail;
> +		}
> +	}
> +
>  	/*
> -	 * - Create a mapping for the page directories.
> -	 * - For each page directory:
> -	 *      allocate space for page table mappings.
> -	 *      map each page table
> +	 * 2. Create all the DMA mappings for the page directories and page
> +	 * tables
>  	 */
>  	for (i = 0; i < max_pdp; i++) {
> -		dma_addr_t temp;
> -		temp = pci_map_page(ppgtt->base.dev->pdev,
> -				    &ppgtt->pd_pages[i], 0,
> -				    PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
> -		if (pci_dma_mapping_error(ppgtt->base.dev->pdev, temp))
> -			goto err_out;
> -
> -		ppgtt->pd_dma_addr[i] = temp;
> -
> -		ppgtt->gen8_pt_dma_addr[i] = kmalloc(sizeof(dma_addr_t) * GEN8_PDES_PER_PAGE, GFP_KERNEL);
> -		if (!ppgtt->gen8_pt_dma_addr[i])
> -			goto err_out;
> +		dma_addr_t pd_addr, pt_addr;
>  
> +		/* Get the page table mappings per page directory */
>  		for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>  			struct page *p = &pt_pages[i * GEN8_PDES_PER_PAGE + j];
> -			temp = pci_map_page(ppgtt->base.dev->pdev,
> -					    p, 0, PAGE_SIZE,
> -					    PCI_DMA_BIDIRECTIONAL);
>  
> -			if (pci_dma_mapping_error(ppgtt->base.dev->pdev, temp))
> -				goto err_out;
> +			pt_addr = pci_map_page(ppgtt->base.dev->pdev,
> +					       p, 0, PAGE_SIZE,
> +					       PCI_DMA_BIDIRECTIONAL);
> +			ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pt_addr);
> +			if (ret)
> +				goto bail;
>  
> -			ppgtt->gen8_pt_dma_addr[i][j] = temp;
> +			ppgtt->gen8_pt_dma_addr[i][j] = pt_addr;
>  		}
> +
> +		/* And the page directory mappings */
> +		pd_addr = pci_map_page(ppgtt->base.dev->pdev,
> +				       &ppgtt->pd_pages[i], 0,
> +				       PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
> +		ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr);
> +		if (ret)
> +			goto bail;

The error path here would leak the above page table mappings, since
ppgtt->pd_dma_addr[i] is still zero, but in gen8_ppgtt_unmap_pages() we
do a if (!ppgtt->pd_dma_addr[i]) continue; skipping the page table unmap
part. This is reworked in your later patches, but the issue is still
there in the final version. 

> +
> +		ppgtt->pd_dma_addr[i] = pd_addr;
>  	}
>  
> -	/* For now, the PPGTT helper functions all require that the PDEs are
> +	/*
> +	 * 3. Map all the page directory entires to point to the page tables
> +	 * we've allocated.
> +	 *
> +	 * For now, the PPGTT helper functions all require that the PDEs are
>  	 * plugged in correctly. So we do that now/here. For aliasing PPGTT, we
> -	 * will never need to touch the PDEs again */
> +	 * will never need to touch the PDEs again.
> +	 */
>  	for (i = 0; i < max_pdp; i++) {
>  		gen8_ppgtt_pde_t *pd_vaddr;
>  		pd_vaddr = kmap_atomic(&ppgtt->pd_pages[i]);
> @@ -461,6 +472,14 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  		kunmap_atomic(pd_vaddr);
>  	}
>  
> +	ppgtt->enable = gen8_ppgtt_enable;
> +	ppgtt->switch_mm = gen8_mm_switch;
> +	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
> +	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
> +	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
> +	ppgtt->base.start = 0;
> +	ppgtt->base.total = ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE;
> +
>  	ppgtt->base.clear_range(&ppgtt->base, 0,
>  				ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE,
>  				true);
> @@ -473,8 +492,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  			 size % (1<<30));
>  	return 0;
>  
> -err_out:
> -	ppgtt->base.cleanup(&ppgtt->base);
> +bail:
> +	gen8_ppgtt_unmap_pages(ppgtt);
> +	gen8_ppgtt_free(ppgtt);
>  	return ret;
>  }
>  


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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