[Intel-gfx] [PATCH 1/3] drm/i915/gtt: Split gen8_ppgtt_clear_pte_range

[Joonas Lahtinen]

I wonder if GCC is doing the right thing, do we have some benchmark for
this, Chris, Mika? The code is much more readable in my eyes after the
changes.

On ti, 2016-10-04 at 15:54 +0200, Michał Winiarski wrote:
> +static void gen8_ppgtt_clear_pt(struct i915_address_space *vm,
> +				struct i915_page_table *pt,
> +				uint64_t start,
> +				uint64_t length,
> +				bool use_scratch)
>  {
>  	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> +
> +	unsigned int pte_start = gen8_pte_index(start);
> +	unsigned int num_entries = min(gen8_pte_count(start, length),
> +				       GEN8_PTES);

i915_pte_count (called by gen8_pte_count) states following;

"the max value would be GEN6_PTES for GEN6, and GEN8_PTES for GEN8."

GEM_BUG_ON(num_entries > GEN8_PTES); would be the standard recipe for
paranoia. But I don't think it's needed here. 

> +	uint64_t pte;
> +

No newlines to the variable block, keep it tight.

> @@ -768,21 +793,13 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>  				   bool use_scratch)
>  {
>  	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> -	gen8_pte_t scratch_pte = gen8_pte_encode(vm->scratch_page.daddr,
> -						 I915_CACHE_LLC, use_scratch);
>  
> -	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
> -		gen8_ppgtt_clear_pte_range(vm, &ppgtt->pdp, start, length,
> -					   scratch_pte);
> -	} else {
> -		uint64_t pml4e;
> -		struct i915_page_directory_pointer *pdp;
> -
> -		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, pml4e) {
> -			gen8_ppgtt_clear_pte_range(vm, pdp, start, length,
> -						   scratch_pte);
> -		}
> -	}
> +	if (!USES_FULL_48BIT_PPGTT(vm->dev))

Now that this is so much simplified, the negation could be removed, and
just just write if (USES_FULL_...)

Other than above, looks good to me.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation