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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
