On Tue, Sep 27, 2022 at 10:28:03AM +0100, Tvrtko Ursulin wrote:
On 26/09/2022 18:09, Niranjana Vishwanathapura wrote:
On Mon, Sep 26, 2022 at 05:26:12PM +0100, Tvrtko Ursulin wrote:
On 24/09/2022 05:30, Niranjana Vishwanathapura wrote:
On Fri, Sep 23, 2022 at 09:40:20AM +0100, Tvrtko Ursulin wrote:
On 21/09/2022 08:09, Niranjana Vishwanathapura wrote:
vma_lookup is tied to segment of the object instead of section
Can be, but not only that. It would be more accurate to say it
is based of gtt views.
Yah, but new code is also based on gtt views, the only difference
is that now there can be multiple mappings (at different VAs)
to the same gtt_view of the object.
of VA space. Hence, it do not support aliasing (ie., multiple
bindings to the same section of the object).
Skip vma_lookup for persistent vmas as it supports aliasing.
What's broken without this patch? If something is, should it
go somewhere earlier in the series? If so should be mentioned
in the commit message.
Or is it just a performance optimisation to skip unused
tracking? If so should also be mentioned in the commit
message.
No, it is not a performance optimization.
The vma_lookup is based on the fact that there can be only one mapping
for a given gtt_view of the object.
So, it was looking for gtt_view to find the mapping.
But now, as I mentioned above, there can be multiple mappings for a
given gtt_view of the object. Hence the vma_lookup method won't work
here. Hence, it is being skipped for persistent vmas.
Right, so in that case isn't this patch too late in the series?
Granted you only allow _userspace_ to use vm bind in 14/14, but
the kernel infrastructure is there and if there was a selftest it
would be able to fail without this patch, no?
Yes it is incorrect patch ordering. I am fixing it by moving this patch
to early in the series and adding a new i915_vma_create_persistent()
function and avoid touching i915_vma_instance() everywhere (as you
suggested).
<snip>
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -110,7 +110,8 @@ static void __i915_vma_retire(struct
i915_active *ref)
static struct i915_vma *
vma_create(struct drm_i915_gem_object *obj,
struct i915_address_space *vm,
- const struct i915_gtt_view *view)
+ const struct i915_gtt_view *view,
+ bool persistent)
{
struct i915_vma *pos = ERR_PTR(-E2BIG);
struct i915_vma *vma;
@@ -197,6 +198,9 @@ vma_create(struct drm_i915_gem_object *obj,
__set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(vma));
}
+ if (persistent)
+ goto skip_rb_insert;
Oh so you don't use the gtt_view's fully at all. I now have
reservations whether that was the right approach. Since you
are not using the existing rb tree tracking I mean..
You know if a vma is persistent right? So you could have just
added special case for persistent vmas to __i915_vma_get_pages
and still call intel_partial_pages from there. Maybe union
over struct i915_gtt_view in i915_vma for either the view or
struct intel_partial_info for persistent ones.
We are using the gtt_view fully in this patch for persistent vmas.
I guess yours and mine definition of fully are different. :)
But as mentioned above, now we have support multiple mappings
for the same gtt_view of the object. For this, the current
vma_lookup() falls short. So, we are skipping it.
I get it - but then, having only now noticed how it will be used,
I am less convinced touching the ggtt_view code was the right
approach.
What about what I proposed above? That you just add code to
__i915_vma_get_pages, which in case of a persistent VMA would call
intel_partial_pages from there.
If that works I think it's cleaner and we'd just revert the
ggtt_view to gtt_view rename.
I don't think that is any cleaner. We need to store the partial view
information somewhere for the persistent vmas as well. Why not use
the existing gtt_view for that instead of a new data structure?
In fact long back I had such an implementation and it was looking
odd and was suggested to use the existing infrastructure (gtt_view).
Besides, I think the current i915_vma_lookup method is no longer valid.
(Ever since we had softpinning, lookup should have be based on the VA
and not the vma's view of the object).
As a side note I don't think soft pinning was a problem. That did not establish a partial VMA concept, nor had any interaction view ggtt_views. It was still one obj - one vma per vm relationship.
But okay, it is okay to do it like this. I think when you change to separate create/lookup entry points for persistent it will become much cleaner. I do acknowledge you have to "hide" them from normal lookup to avoid confusing the legacy code paths.
One more note - I think patch 6 should be before or together with patch 4. In general infrastructure to handle vm bind should all be in place before code starts using it.
Thanks. Yah, separating it out is looking lot cleaner. Yah, I have further split the patches including patch 6 and move part it to before patch 4.
Everything is looking much cleaner now. Will be posting updated series soon.
Regards,
Niranjana
Regards,
Tvrtko
