Re: [Intel-gfx] [RFC v4 13/14] drm/i915/vm_bind: Skip vma_lookup for persistent vmas
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).
--- 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
Re: [Intel-gfx] [RFC v4 13/14] drm/i915/vm_bind: Skip vma_lookup for persistent vmas
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).
--- 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.
Regards,
Tvrtko
Re: [Intel-gfx] [RFC v4 13/14] drm/i915/vm_bind: Skip vma_lookup for persistent vmas
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).
--- 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).
Regards,
Niranjana
Regards,
Tvrtko
Regards,
Niranjana
Regards,
Tvrtko
+
rb = NULL;
p = >vma.tree.rb_node;
while (*p) {
@@ -221,6 +225,7 @@ vma_create(struct drm_i915_gem_object *obj,
rb_link_node(>obj_node, rb, p);
rb_insert_color(>obj_node, >vma.tree);
+skip_rb_insert:
if (i915_vma_is_ggtt(vma))
/*
* We put the GGTT vma at the start of the vma-list, followed
@@ -279,6 +284,7 @@ i915_vma_lookup(struct drm_i915_gem_object *obj,
* @obj: parent drm_i915_gem_object to be mapped
* @vm: address space in which the mapping is located
* @view: additional mapping requirements
+ * @persistent: Whether the vma is persistent
*
* i915_vma_instance() looks up an existing VMA of the @obj in
the @vm with
* the same @view
Re: [Intel-gfx] [RFC v4 13/14] drm/i915/vm_bind: Skip vma_lookup for persistent vmas
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? Signed-off-by: Niranjana Vishwanathapura Signed-off-by: Andi Shyti --- drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- .../drm/i915/display/intel_plane_initial.c | 2 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 +- .../drm/i915/gem/i915_gem_vm_bind_object.c | 2 +- .../gpu/drm/i915/gem/selftests/huge_pages.c | 16 +++ .../i915/gem/selftests/i915_gem_client_blt.c | 2 +- .../drm/i915/gem/selftests/i915_gem_context.c | 12 ++--- .../drm/i915/gem/selftests/i915_gem_migrate.c | 2 +- .../drm/i915/gem/selftests/i915_gem_mman.c | 6 ++- .../drm/i915/gem/selftests/igt_gem_utils.c | 2 +- drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 2 +- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +- drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +- drivers/gpu/drm/i915/gt/intel_renderstate.c | 2 +- drivers/gpu/drm/i915/gt/intel_ring.c | 2 +- .../gpu/drm/i915/gt/intel_ring_submission.c | 4 +- drivers/gpu/drm/i915/gt/intel_timeline.c | 2 +- drivers/gpu/drm/i915/gt/mock_engine.c | 2 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 4 +- drivers/gpu/drm/i915/gt/selftest_execlists.c | 16 +++ drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 6 +-- drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +- .../drm/i915/gt/selftest_ring_submission.c | 2 +- drivers/gpu/drm/i915/gt/selftest_rps.c | 2 +- .../gpu/drm/i915/gt/selftest_workarounds.c | 4 +- drivers/gpu/drm/i915/gt/uc/intel_guc.c | 2 +- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_perf.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 26 +++ drivers/gpu/drm/i915/i915_vma.h | 3 +- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 44 +-- drivers/gpu/drm/i915/selftests/i915_request.c | 4 +- drivers/gpu/drm/i915/selftests/i915_vma.c | 2 +- drivers/gpu/drm/i915/selftests/igt_spinner.c | 2 +- .../drm/i915/selftests/intel_memory_region.c | 2 +- 37 files changed, 106 insertions(+), 93 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index c86e5d4ee016..5a718b247bb3 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -47,7 +47,7 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, goto err; } - vma = i915_vma_instance(obj, vm, view); + vma = i915_vma_instance(obj, vm, view, false); Hey why are you touching all the legacy paths? >:P if (IS_ERR(vma)) goto err; diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c index 76be796df255..7667e2faa3fb 100644 --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c @@ -136,7 +136,7 @@ initial_plane_vma(struct drm_i915_private *i915, goto err_obj; } - vma = i915_vma_instance(obj, _gt(i915)->ggtt->vm, NULL); + vma = i915_vma_instance(obj, _gt(i915)->ggtt->vm, NULL, false); if (IS_ERR(vma)) goto err_obj; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index
Re: [Intel-gfx] [RFC v4 13/14] drm/i915/vm_bind: Skip vma_lookup for persistent vmas
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. Signed-off-by: Niranjana Vishwanathapura Signed-off-by: Andi Shyti --- drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- .../drm/i915/display/intel_plane_initial.c| 2 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 4 +- .../drm/i915/gem/i915_gem_vm_bind_object.c| 2 +- .../gpu/drm/i915/gem/selftests/huge_pages.c | 16 +++ .../i915/gem/selftests/i915_gem_client_blt.c | 2 +- .../drm/i915/gem/selftests/i915_gem_context.c | 12 ++--- .../drm/i915/gem/selftests/i915_gem_migrate.c | 2 +- .../drm/i915/gem/selftests/i915_gem_mman.c| 6 ++- .../drm/i915/gem/selftests/igt_gem_utils.c| 2 +- drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 2 +- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt.c| 2 +- drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +- drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +- drivers/gpu/drm/i915/gt/intel_renderstate.c | 2 +- drivers/gpu/drm/i915/gt/intel_ring.c | 2 +- .../gpu/drm/i915/gt/intel_ring_submission.c | 4 +- drivers/gpu/drm/i915/gt/intel_timeline.c | 2 +- drivers/gpu/drm/i915/gt/mock_engine.c | 2 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 4 +- drivers/gpu/drm/i915/gt/selftest_execlists.c | 16 +++ drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 6 +-- drivers/gpu/drm/i915/gt/selftest_lrc.c| 2 +- .../drm/i915/gt/selftest_ring_submission.c| 2 +- drivers/gpu/drm/i915/gt/selftest_rps.c| 2 +- .../gpu/drm/i915/gt/selftest_workarounds.c| 4 +- drivers/gpu/drm/i915/gt/uc/intel_guc.c| 2 +- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_perf.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 26 +++ drivers/gpu/drm/i915/i915_vma.h | 3 +- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 44 +-- drivers/gpu/drm/i915/selftests/i915_request.c | 4 +- drivers/gpu/drm/i915/selftests/i915_vma.c | 2 +- drivers/gpu/drm/i915/selftests/igt_spinner.c | 2 +- .../drm/i915/selftests/intel_memory_region.c | 2 +- 37 files changed, 106 insertions(+), 93 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index c86e5d4ee016..5a718b247bb3 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -47,7 +47,7 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, goto err; } - vma = i915_vma_instance(obj, vm, view); + vma = i915_vma_instance(obj, vm, view, false); Hey why are you touching all the legacy paths? >:P if (IS_ERR(vma)) goto err; diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c index 76be796df255..7667e2faa3fb 100644 --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c @@ -136,7 +136,7 @@ initial_plane_vma(struct drm_i915_private *i915, goto err_obj; } - vma = i915_vma_instance(obj, _gt(i915)->ggtt->vm, NULL); + vma = i915_vma_instance(obj, _gt(i915)->ggtt->vm, NULL, false); if (IS_ERR(vma)) goto err_obj; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 363b2a788cdf..0ee43cb601b5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -876,7 +876,7 @@ static struct i915_vma *eb_lookup_vma(struct i915_execbuffer *eb, u32 handle)
Re: [Intel-gfx] [RFC v4 13/14] drm/i915/vm_bind: Skip vma_lookup for persistent vmas
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.
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.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +-
.../drm/i915/display/intel_plane_initial.c| 2 +-
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 4 +-
.../drm/i915/gem/i915_gem_vm_bind_object.c| 2 +-
.../gpu/drm/i915/gem/selftests/huge_pages.c | 16 +++
.../i915/gem/selftests/i915_gem_client_blt.c | 2 +-
.../drm/i915/gem/selftests/i915_gem_context.c | 12 ++---
.../drm/i915/gem/selftests/i915_gem_migrate.c | 2 +-
.../drm/i915/gem/selftests/i915_gem_mman.c| 6 ++-
.../drm/i915/gem/selftests/igt_gem_utils.c| 2 +-
drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 2 +-
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +-
drivers/gpu/drm/i915/gt/intel_gt.c| 2 +-
drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +-
drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +-
drivers/gpu/drm/i915/gt/intel_renderstate.c | 2 +-
drivers/gpu/drm/i915/gt/intel_ring.c | 2 +-
.../gpu/drm/i915/gt/intel_ring_submission.c | 4 +-
drivers/gpu/drm/i915/gt/intel_timeline.c | 2 +-
drivers/gpu/drm/i915/gt/mock_engine.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 4 +-
drivers/gpu/drm/i915/gt/selftest_execlists.c | 16 +++
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 6 +--
drivers/gpu/drm/i915/gt/selftest_lrc.c| 2 +-
.../drm/i915/gt/selftest_ring_submission.c| 2 +-
drivers/gpu/drm/i915/gt/selftest_rps.c| 2 +-
.../gpu/drm/i915/gt/selftest_workarounds.c| 4 +-
drivers/gpu/drm/i915/gt/uc/intel_guc.c| 2 +-
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_perf.c | 2 +-
drivers/gpu/drm/i915/i915_vma.c | 26 +++
drivers/gpu/drm/i915/i915_vma.h | 3 +-
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 44 +--
drivers/gpu/drm/i915/selftests/i915_request.c | 4 +-
drivers/gpu/drm/i915/selftests/i915_vma.c | 2 +-
drivers/gpu/drm/i915/selftests/igt_spinner.c | 2 +-
.../drm/i915/selftests/intel_memory_region.c | 2 +-
37 files changed, 106 insertions(+), 93 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c
b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index c86e5d4ee016..5a718b247bb3 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -47,7 +47,7 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
goto err;
}
- vma = i915_vma_instance(obj, vm, view);
+ vma = i915_vma_instance(obj, vm, view, false);
Hey why are you touching all the legacy paths? >:P
if (IS_ERR(vma))
goto err;
diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index 76be796df255..7667e2faa3fb 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -136,7 +136,7 @@ initial_plane_vma(struct drm_i915_private *i915,
goto err_obj;
}
- vma = i915_vma_instance(obj, _gt(i915)->ggtt->vm, NULL);
+ vma = i915_vma_instance(obj, _gt(i915)->ggtt->vm, NULL, false);
if (IS_ERR(vma))
goto err_obj;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 363b2a788cdf..0ee43cb601b5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -876,7 +876,7 @@ static struct i915_vma *eb_lookup_vma(struct
i915_execbuffer *eb, u32 handle)
}
}
- vma = i915_vma_instance(obj, vm, NULL);
+ vma = i915_vma_instance(obj, vm, NULL, false);
if (IS_ERR(vma)) {
i915_gem_object_put(obj);
return vma;
@@ -2208,7 +2208,7 @@ shadow_batch_pin(struct i915_execbuffer *eb,
struct i915_vma *vma;
int err;
- vma = i915_vma_instance(obj, vm, NULL);
+ vma = i915_vma_instance(obj, vm, NULL, false);
if (IS_ERR(vma))
return vma;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
