[Intel-gfx] [RFC 3/5] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

On 11/03/2014 04:52 PM, Daniel Vetter wrote:
> On Mon, Nov 03, 2014 at 04:34:29PM +0000, Tvrtko Ursulin wrote:
>>
>> On 11/03/2014 03:58 PM, Daniel Vetter wrote:
>>> On Thu, Oct 30, 2014 at 04:39:36PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Things like reliable GGTT mappings and mirrored 3d display will need to be
>>>> to map the same object twice into the GGTT.
>>>>
>>>> Add a ggtt_view field per VMA and select the page view based on the type
>>>> of the view.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>
>>> lgtm overall, some comments for polish in the crucial parts below.
>>>
>>>> @@ -2189,3 +2191,21 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
>>>>
>>>>   	return vma;
>>>>   }
>>>> +
>>>> +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>>>> +			u32 flags)
>>>> +{
>>>> +	struct sg_table *pages;
>>>> +
>>>> +	if (vma->ggtt_view.get_pages)
>>>> +		pages = vma->ggtt_view.get_pages(&vma->ggtt_view, vma->obj);
>>>> +	else
>>>> +		pages = vma->obj->pages;
>>>> +
>>>> +	if (pages && !IS_ERR(pages)) {
>>>> +		vma->bind_vma(vma, pages, cache_level, flags);
>>>> +
>>>> +		if (vma->ggtt_view.put_pages)
>>>> +			vma->ggtt_view.put_pages(&vma->ggtt_view);
>>>> +	}
>>>
>>> tbh this looks a bit like overboarding generalism to me ;-) Imo
>>> - drop the ->put_pages callback and just free the sg table if you have
>>>    one.
>>> - drop teh ->get_pages callbacks and replace it with a new
>>>    i915_vma_shuffle_pages or similar you'll call for non-standard ggtt
>>>    views. Two reasons: shuffle is a more accurate name than get since this
>>>    version doesn't grab it's own page references any more, and with
>>>    currently just one internal user for this a vtable is serious overkill.
>>
>> I actually thought I will need semi-persistent view for this in the future
>> patch which get_pages()/put_pages() caters for.
>>
>> More precisely it would be for holding onto created pages during view
>> creation across the i915_gem_gtt_prepare_object and i915_vma_bind in
>> i915_gem_object_bind_to_vm.
>>
>> Also, ->put_pages looks much neater to me from i915_gem_vma_destroy rather
>> than leaking the knowledge of internal implementation. That is of course if
>> you will allow the above justification for making the alternative view at
>> least semi-persistent.
>
> I don't see why the view needs to be semi-persistent. And it certainly
> doesn't work by attaching the sg table to the vma, since then the pages
> can't survive the vma. And the vma is already cached (i.e. we only ever
> throw them away lazily).
 >
> And I don't hink caching it longer than the vma is useful since when we
> throw away the vma that's usually a much bigger performance disaster,
> often involving stalls and chasing down backing storage again. Allocating
> a puny sg table and filling it again isn't a lot of work. Especially since
> this is only for uncommon cases like scanout buffers or ggtt mmaps thus
> far.

Ah alright, think I see what you mean. Hm.. to explain once more why I 
put that in...

At the moment it is just the means of allow the alternative GGTT view to 
exists (as in sg_table) for the duration of i915_gem_object_bind_to_vm.

Because it does a two stage process there; the gtt_prepare_object and 
insert entries.

I did not like your idea, well I did not think it is feasible - as in 
easily doable, of stealing the DMA addresses since the SG tables between 
view don't have a 1:1 relationship in number of chunk/pages.

So maybe I am missing something, but to me it looked like a handy way of 
achieving that goal.

You don't like the fact that my put_pages is done only in 
i915_gem_vma_destroy in this patch, which only happens lazily, correct?

If the new get/put_pages calls on the view were done explicitly in 
i915_gem_object_bind_to_vm would that be any better? That would show 
explicitly that the SG table is a throw away.

>> So you suggest to imply VMA id to be equal to GGTT view type and
>> VMA_ID_DEFAULT to be fixed to I915_GGTT_VIEW_NORMAL? Is it not a
>> logistics/maintenance problem to sync the two then?
>
> Well if we kill one of them completely there's no syncing required, which
> is what I think we want. Having two will be a nightmare, and I don't see
> any use a few years out even for non-ggtt special views.
>
> We can always add more complexity later on if needed.

Ok I don't see it at the moment since they are two different concepts to 
me but I'll think about it.

>>> Removing vma_id also removes the int, which I'd ask you to replace with an
>>> explicit enum anyway ;-)
>>
>> Yes I left that for later. I mean, when at stage to be talking about such
>> detail it is a happy place.
>
> I'm not a fan of preemptive generalization - it tends to be the wrong one
> and hurt doubly in the future since you have to remove the wrong one first
> before you can add the right stuff.

In general :), as always it is the question of getting the balance 
right. Because the opposite can also happen and maybe even has in some 
parts of the driver.

Anyway, that's why I didn't bother with polishing the enums etc. :)

Regards,

Tvrtko