Re: [PATCH v4 07/24] drm/i915: Create page table allocators

[Michel Thierry <michel.thierry@intel.com>]

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

On 2/20/2015 4:50 PM, Mika Kuoppala wrote:
> Michel Thierry <michel.thierry@intel.com> writes:
>
>> From: Ben Widawsky <benjamin.widawsky@intel.com>
>>
>> As we move toward dynamic page table allocation, it becomes much easier
>> to manage our data structures if break do things less coarsely by
>> breaking up all of our actions into individual tasks.  This makes the
>> code easier to write, read, and verify.
>>
>> Aside from the dissection of the allocation functions, the patch
>> statically allocates the page table structures without a page directory.
>> This remains the same for all platforms,
>>
>> The patch itself should not have much functional difference. The primary
>> noticeable difference is the fact that page tables are no longer
>> allocated, but rather statically declared as part of the page directory.
>> This has non-zero overhead, but things gain non-trivial complexity as a
>> result.
>>
>> This patch exists for a few reasons:
>> 1. Splitting out the functions allows easily combining GEN6 and GEN8
>> code. Page tables have no difference based on GEN8. As we'll see in a
>> future patch when we add the DMA mappings to the allocations, it
>> requires only one small change to make work, and error handling should
>> just fall into place.
>>
>> 2. Unless we always want to allocate all page tables under a given PDE,
>> we'll have to eventually break this up into an array of pointers (or
>> pointer to pointer).
>>
>> 3. Having the discrete functions is easier to review, and understand.
>> All allocations and frees now take place in just a couple of locations.
>> Reviewing, and catching leaks should be easy.
>>
>> 4. Less important: the GFP flags are confined to one location, which
>> makes playing around with such things trivial.
>>
>> v2: Updated commit message to explain why this patch exists
>>
>> v3: For lrc, s/pdp.page_directory[i].daddr/pdp.page_directory[i]->daddr/
>>
>> v4: Renamed free_pt/pd_single functions to unmap_and_free_pt/pd (Daniel)
>>
>> v5: Added additional safety checks in gen8 clear/free/unmap.
>>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v3, v4, v5)
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 251 ++++++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/i915_gem_gtt.h |   4 +-
>>   drivers/gpu/drm/i915/intel_lrc.c    |  16 +--
>>   3 files changed, 179 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 0fe5c1e..85ea535 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -275,6 +275,99 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
>>   	return pte;
>>   }
>>   
>> +static void unmap_and_free_pt(struct i915_page_table_entry *pt)
>> +{
>> +	if (WARN_ON(!pt->page))
>> +		return;
>> +	__free_page(pt->page);
>> +	kfree(pt);
>> +}
>> +
>> +static struct i915_page_table_entry *alloc_pt_single(void)
>> +{
>> +	struct i915_page_table_entry *pt;
>> +
>> +	pt = kzalloc(sizeof(*pt), GFP_KERNEL);
>> +	if (!pt)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> +	if (!pt->page) {
>> +		kfree(pt);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	return pt;
>> +}
>> +
>> +/**
>> + * alloc_pt_range() - Allocate a multiple page tables
>> + * @pd:		The page directory which will have at least @count entries
>> + *		available to point to the allocated page tables.
>> + * @pde:	First page directory entry for which we are allocating.
>> + * @count:	Number of pages to allocate.
>> + *
>> + * Allocates multiple page table pages and sets the appropriate entries in the
>> + * page table structure within the page directory. Function cleans up after
>> + * itself on any failures.
>> + *
>> + * Return: 0 if allocation succeeded.
>> + */
>> +static int alloc_pt_range(struct i915_page_directory_entry *pd, uint16_t pde, size_t count)
>> +{
>> +	int i, ret;
>> +
>> +	/* 512 is the max page tables per page_directory on any platform.
>> +	 * TODO: make WARN after patch series is done
>> +	 */
>> +	BUG_ON(pde + count > GEN6_PPGTT_PD_ENTRIES);
>> +
> WARN_ON in here and return -EINVAL.
>
> -Mika

I applied the changes in v6.

Thanks for the review.

-Michel


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 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