Re: [PATCH v9 10/19] drm/i915/gen8: Add 4 level support in insert_entries and clear_range

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

On 8/5/2015 4:46 PM, Daniel Vetter wrote:
> On Mon, Aug 03, 2015 at 09:53:27AM +0100, Michel Thierry wrote:
>> @@ -735,12 +736,21 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>>   {
>>   	struct i915_hw_ppgtt *ppgtt =
>>   		container_of(vm, struct i915_hw_ppgtt, base);
>> -	struct i915_page_directory_pointer *pdp = &ppgtt->pdp; /* FIXME: 48b */
>> -
>>   	gen8_pte_t scratch_pte = gen8_pte_encode(px_dma(vm->scratch_page),
>>   						 I915_CACHE_LLC, use_scratch);
>>
>> -	gen8_ppgtt_clear_pte_range(vm, pdp, start, length, scratch_pte);
>> +	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
>
> Hm, this isn't pretty, and looking through earlier patches you have a lot
> of if (48BIT) functions right at the topmost level where we have vfuncs,
> e.g. gen8_ppgtt_cleanup. Imo much better to just do a
> gen8_legacy_ppgtt_cleanup and gen8_4lvl_ppgtt_cleanup. Yeah means we
> duplicate the call to free_scracth but really that's meh - we committed to
> that abstraction so let's use it.
>
> But reworking all the patches to get rid of all the 48bit ifs and
> exploiting the vfunc abstraction we have will be a bit of work, so I'll
> keep merging and sign you up for that follow-up task. The usual design
> when we have vfuncs should be:
> - do per-platform vfuncs everywhere you need a split
> - for functionality shared between different vfuncs extract common helper
>    functions and call them from both places.
>
> E.g. for this case here I think we need a new 4lvl insert_entries
> function which calls the existing one in a loop, and a 3lvl inser_entries
> function which calls the existing one for the single legacy pdp. Make 2
> copies of this and pull out the if to the top-level of each, then
> simplify.
>
> If we have abstraction in the form of vfuncs _and_ pile in lots of ifs at
> low level then we pay both the price for the abstraction and the price for
> tightly nit code, i.e. both downsides without an upside.
>
> Anway, I expect follow-up work here ;-)
>
> Thanks, Daniel
>

Yes, all the main functions (alloc, clear, cleanup, dump, insert) have

if (USES_FULL_48BIT_PPGTT)
	pml4 function
else
	pdp function

I'll make a patch to set the correct ones as vfuncs in gen8_ppgtt_init.

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