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@xxxxxxxxx>
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@xxxxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
