Re: [Intel-gfx] [PATCH 6/9] drm/i915: Wrap VMA binding
On Tue, May 06, 2014 at 10:21:35PM -0700, Ben Widawsky wrote:
This will be useful for some upcoming patches which do more platform
specific work. Having it in one central place just makes things a bit
cleaner and easier.
There is a small functional change here. There are more calls to the
tracepoints.
NOTE: I didn't actually end up using this patch for the intended purpose, but
I
thought it was a nice patch to keep around.
v2: s/i915_gem_bind_vma/i915_gem_vma_bind/
s/i915_gem_unbind_vma/i915_gem_vma_unbind/
(Chris)
v3: Missed one spot
Signed-off-by: Ben Widawsky b...@bwidawsk.net
You change the semantics of the vma bind/unbind tracepoints - previously
they've meant we reserve/unreserve a ppgtt address for an object, not
they're called every time we touch the ptes (which is what
vma_bind|unbind actually do). That doesn't look too terribly useful for
tracing any more.
What's the use case for these added tracepoints?
-Daniel
---
drivers/gpu/drm/i915/i915_drv.h| 3 +++
drivers/gpu/drm/i915/i915_gem.c| 13 ++---
drivers/gpu/drm/i915/i915_gem_context.c| 2 +-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +++--
drivers/gpu/drm/i915/i915_gem_gtt.c| 16 ++--
5 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c1d2bea..30cde3d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2264,6 +2264,9 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
struct i915_address_space *vm);
unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
struct i915_address_space *vm);
+void i915_gem_vma_bind(struct i915_vma *vma, enum i915_cache_level,
+unsigned flags);
+void i915_gem_vma_unbind(struct i915_vma *vma);
struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
struct i915_address_space *vm);
struct i915_vma *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e9599e9..1567911 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2771,7 +2771,7 @@ int i915_vma_unbind(struct i915_vma *vma)
trace_i915_vma_unbind(vma);
- vma-unbind_vma(vma);
+ i915_gem_vma_unbind(vma);
i915_gem_gtt_finish_object(obj);
@@ -3342,9 +3342,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object
*obj,
WARN_ON(flags PIN_MAPPABLE !obj-map_and_fenceable);
- trace_i915_vma_bind(vma, flags);
- vma-bind_vma(vma, obj-cache_level,
- flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
+ i915_gem_vma_bind(vma, obj-cache_level,
+ flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND :
0);
i915_gem_verify_gtt(dev);
return vma;
@@ -3548,8 +3547,8 @@ int i915_gem_object_set_cache_level(struct
drm_i915_gem_object *obj,
list_for_each_entry(vma, obj-vma_list, vma_link)
if (drm_mm_node_allocated(vma-node))
- vma-bind_vma(vma, cache_level,
- obj-has_global_gtt_mapping ?
GLOBAL_BIND : 0);
+ i915_gem_vma_bind(vma, cache_level,
+ obj-has_global_gtt_mapping ?
GLOBAL_BIND : 0);
}
list_for_each_entry(vma, obj-vma_list, vma_link)
@@ -3914,7 +3913,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
}
if (flags PIN_GLOBAL !obj-has_global_gtt_mapping)
- vma-bind_vma(vma, obj-cache_level, GLOBAL_BIND);
+ i915_gem_vma_bind(vma, obj-cache_level, GLOBAL_BIND);
vma-pin_count++;
if (flags PIN_MAPPABLE)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index f77b4c1..00c7d88 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -655,7 +655,7 @@ static int do_switch(struct intel_ring_buffer *ring,
if (!to-obj-has_global_gtt_mapping) {
struct i915_vma *vma = i915_gem_obj_to_vma(to-obj,
dev_priv-gtt.base);
- vma-bind_vma(vma, to-obj-cache_level, GLOBAL_BIND);
+ i915_gem_vma_bind(vma, to-obj-cache_level, GLOBAL_BIND);
}
if (!to-is_initialized || i915_gem_context_is_default(to))
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6cc004f..211bbbd 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -369,7 +369,8 @@ i915_gem_execbuffer_relocate_entry(struct
drm_i915_gem_object *obj,
struct i915_vma *vma =
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Wrap VMA binding
On Wed, May 07, 2014 at 09:55:49AM +0200, Daniel Vetter wrote:
On Tue, May 06, 2014 at 10:21:35PM -0700, Ben Widawsky wrote:
This will be useful for some upcoming patches which do more platform
specific work. Having it in one central place just makes things a bit
cleaner and easier.
There is a small functional change here. There are more calls to the
tracepoints.
NOTE: I didn't actually end up using this patch for the intended purpose,
but I
thought it was a nice patch to keep around.
v2: s/i915_gem_bind_vma/i915_gem_vma_bind/
s/i915_gem_unbind_vma/i915_gem_vma_unbind/
(Chris)
v3: Missed one spot
Signed-off-by: Ben Widawsky b...@bwidawsk.net
You change the semantics of the vma bind/unbind tracepoints - previously
they've meant we reserve/unreserve a ppgtt address for an object, not
they're called every time we touch the ptes (which is what
vma_bind|unbind actually do). That doesn't look too terribly useful for
tracing any more.
What's the use case for these added tracepoints?
-Daniel
You're right. The tracepoint is incredibly useful for debug purposes,
but not as an actual tracepoint. The current tracepoint is badly named,
IMO, but that's fine. I am not sure it's worth fixing though, since
nobody has really spoken up for the patch.
---
drivers/gpu/drm/i915/i915_drv.h| 3 +++
drivers/gpu/drm/i915/i915_gem.c| 13 ++---
drivers/gpu/drm/i915/i915_gem_context.c| 2 +-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +++--
drivers/gpu/drm/i915/i915_gem_gtt.c| 16 ++--
5 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index c1d2bea..30cde3d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2264,6 +2264,9 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
struct i915_address_space *vm);
unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
struct i915_address_space *vm);
+void i915_gem_vma_bind(struct i915_vma *vma, enum i915_cache_level,
+ unsigned flags);
+void i915_gem_vma_unbind(struct i915_vma *vma);
struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
struct i915_address_space *vm);
struct i915_vma *
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index e9599e9..1567911 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2771,7 +2771,7 @@ int i915_vma_unbind(struct i915_vma *vma)
trace_i915_vma_unbind(vma);
- vma-unbind_vma(vma);
+ i915_gem_vma_unbind(vma);
i915_gem_gtt_finish_object(obj);
@@ -3342,9 +3342,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object
*obj,
WARN_ON(flags PIN_MAPPABLE !obj-map_and_fenceable);
- trace_i915_vma_bind(vma, flags);
- vma-bind_vma(vma, obj-cache_level,
- flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
+ i915_gem_vma_bind(vma, obj-cache_level,
+ flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND :
0);
i915_gem_verify_gtt(dev);
return vma;
@@ -3548,8 +3547,8 @@ int i915_gem_object_set_cache_level(struct
drm_i915_gem_object *obj,
list_for_each_entry(vma, obj-vma_list, vma_link)
if (drm_mm_node_allocated(vma-node))
- vma-bind_vma(vma, cache_level,
- obj-has_global_gtt_mapping ?
GLOBAL_BIND : 0);
+ i915_gem_vma_bind(vma, cache_level,
+ obj-has_global_gtt_mapping ?
GLOBAL_BIND : 0);
}
list_for_each_entry(vma, obj-vma_list, vma_link)
@@ -3914,7 +3913,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
}
if (flags PIN_GLOBAL !obj-has_global_gtt_mapping)
- vma-bind_vma(vma, obj-cache_level, GLOBAL_BIND);
+ i915_gem_vma_bind(vma, obj-cache_level, GLOBAL_BIND);
vma-pin_count++;
if (flags PIN_MAPPABLE)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index f77b4c1..00c7d88 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -655,7 +655,7 @@ static int do_switch(struct intel_ring_buffer *ring,
if (!to-obj-has_global_gtt_mapping) {
struct i915_vma *vma = i915_gem_obj_to_vma(to-obj,
dev_priv-gtt.base);
- vma-bind_vma(vma, to-obj-cache_level, GLOBAL_BIND);
+ i915_gem_vma_bind(vma, to-obj-cache_level, GLOBAL_BIND);
}
if (!to-is_initialized || i915_gem_context_is_default(to))
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Wrap VMA binding
On Wed, May 07, 2014 at 08:54:56AM -0700, Ben Widawsky wrote: On Wed, May 07, 2014 at 09:55:49AM +0200, Daniel Vetter wrote: On Tue, May 06, 2014 at 10:21:35PM -0700, Ben Widawsky wrote: This will be useful for some upcoming patches which do more platform specific work. Having it in one central place just makes things a bit cleaner and easier. There is a small functional change here. There are more calls to the tracepoints. NOTE: I didn't actually end up using this patch for the intended purpose, but I thought it was a nice patch to keep around. v2: s/i915_gem_bind_vma/i915_gem_vma_bind/ s/i915_gem_unbind_vma/i915_gem_vma_unbind/ (Chris) v3: Missed one spot Signed-off-by: Ben Widawsky b...@bwidawsk.net You change the semantics of the vma bind/unbind tracepoints - previously they've meant we reserve/unreserve a ppgtt address for an object, not they're called every time we touch the ptes (which is what vma_bind|unbind actually do). That doesn't look too terribly useful for tracing any more. What's the use case for these added tracepoints? -Daniel You're right. The tracepoint is incredibly useful for debug purposes, but not as an actual tracepoint. The current tracepoint is badly named, IMO, but that's fine. I am not sure it's worth fixing though, since nobody has really spoken up for the patch. Yeah I think a new tracepoint we use every time we frob ptes could be useful indeed. But conflating that operation with the vm operation isn't good, especially since the vm operation lacks crucial information like the actual caching bits we're using with the ptes. -Daniel --- drivers/gpu/drm/i915/i915_drv.h| 3 +++ drivers/gpu/drm/i915/i915_gem.c| 13 ++--- drivers/gpu/drm/i915/i915_gem_context.c| 2 +- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +++-- drivers/gpu/drm/i915/i915_gem_gtt.c| 16 ++-- 5 files changed, 27 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c1d2bea..30cde3d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2264,6 +2264,9 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o, struct i915_address_space *vm); unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, struct i915_address_space *vm); +void i915_gem_vma_bind(struct i915_vma *vma, enum i915_cache_level, +unsigned flags); +void i915_gem_vma_unbind(struct i915_vma *vma); struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, struct i915_address_space *vm); struct i915_vma * diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e9599e9..1567911 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2771,7 +2771,7 @@ int i915_vma_unbind(struct i915_vma *vma) trace_i915_vma_unbind(vma); - vma-unbind_vma(vma); + i915_gem_vma_unbind(vma); i915_gem_gtt_finish_object(obj); @@ -3342,9 +3342,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, WARN_ON(flags PIN_MAPPABLE !obj-map_and_fenceable); - trace_i915_vma_bind(vma, flags); - vma-bind_vma(vma, obj-cache_level, - flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0); + i915_gem_vma_bind(vma, obj-cache_level, + flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0); i915_gem_verify_gtt(dev); return vma; @@ -3548,8 +3547,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, list_for_each_entry(vma, obj-vma_list, vma_link) if (drm_mm_node_allocated(vma-node)) - vma-bind_vma(vma, cache_level, - obj-has_global_gtt_mapping ? GLOBAL_BIND : 0); + i915_gem_vma_bind(vma, cache_level, + obj-has_global_gtt_mapping ? GLOBAL_BIND : 0); } list_for_each_entry(vma, obj-vma_list, vma_link) @@ -3914,7 +3913,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, } if (flags PIN_GLOBAL !obj-has_global_gtt_mapping) - vma-bind_vma(vma, obj-cache_level, GLOBAL_BIND); + i915_gem_vma_bind(vma, obj-cache_level, GLOBAL_BIND); vma-pin_count++; if (flags PIN_MAPPABLE) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index f77b4c1..00c7d88 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -655,7 +655,7 @@ static int do_switch(struct intel_ring_buffer *ring,
[Intel-gfx] [PATCH 6/9] drm/i915: Wrap VMA binding
This will be useful for some upcoming patches which do more platform
specific work. Having it in one central place just makes things a bit
cleaner and easier.
There is a small functional change here. There are more calls to the
tracepoints.
NOTE: I didn't actually end up using this patch for the intended purpose, but I
thought it was a nice patch to keep around.
v2: s/i915_gem_bind_vma/i915_gem_vma_bind/
s/i915_gem_unbind_vma/i915_gem_vma_unbind/
(Chris)
v3: Missed one spot
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_drv.h| 3 +++
drivers/gpu/drm/i915/i915_gem.c| 13 ++---
drivers/gpu/drm/i915/i915_gem_context.c| 2 +-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +++--
drivers/gpu/drm/i915/i915_gem_gtt.c| 16 ++--
5 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c1d2bea..30cde3d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2264,6 +2264,9 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
struct i915_address_space *vm);
unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
struct i915_address_space *vm);
+void i915_gem_vma_bind(struct i915_vma *vma, enum i915_cache_level,
+ unsigned flags);
+void i915_gem_vma_unbind(struct i915_vma *vma);
struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
struct i915_address_space *vm);
struct i915_vma *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e9599e9..1567911 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2771,7 +2771,7 @@ int i915_vma_unbind(struct i915_vma *vma)
trace_i915_vma_unbind(vma);
- vma-unbind_vma(vma);
+ i915_gem_vma_unbind(vma);
i915_gem_gtt_finish_object(obj);
@@ -3342,9 +3342,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object
*obj,
WARN_ON(flags PIN_MAPPABLE !obj-map_and_fenceable);
- trace_i915_vma_bind(vma, flags);
- vma-bind_vma(vma, obj-cache_level,
- flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
+ i915_gem_vma_bind(vma, obj-cache_level,
+ flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND :
0);
i915_gem_verify_gtt(dev);
return vma;
@@ -3548,8 +3547,8 @@ int i915_gem_object_set_cache_level(struct
drm_i915_gem_object *obj,
list_for_each_entry(vma, obj-vma_list, vma_link)
if (drm_mm_node_allocated(vma-node))
- vma-bind_vma(vma, cache_level,
- obj-has_global_gtt_mapping ?
GLOBAL_BIND : 0);
+ i915_gem_vma_bind(vma, cache_level,
+ obj-has_global_gtt_mapping ?
GLOBAL_BIND : 0);
}
list_for_each_entry(vma, obj-vma_list, vma_link)
@@ -3914,7 +3913,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
}
if (flags PIN_GLOBAL !obj-has_global_gtt_mapping)
- vma-bind_vma(vma, obj-cache_level, GLOBAL_BIND);
+ i915_gem_vma_bind(vma, obj-cache_level, GLOBAL_BIND);
vma-pin_count++;
if (flags PIN_MAPPABLE)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index f77b4c1..00c7d88 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -655,7 +655,7 @@ static int do_switch(struct intel_ring_buffer *ring,
if (!to-obj-has_global_gtt_mapping) {
struct i915_vma *vma = i915_gem_obj_to_vma(to-obj,
dev_priv-gtt.base);
- vma-bind_vma(vma, to-obj-cache_level, GLOBAL_BIND);
+ i915_gem_vma_bind(vma, to-obj-cache_level, GLOBAL_BIND);
}
if (!to-is_initialized || i915_gem_context_is_default(to))
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6cc004f..211bbbd 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -369,7 +369,8 @@ i915_gem_execbuffer_relocate_entry(struct
drm_i915_gem_object *obj,
struct i915_vma *vma =
list_first_entry(target_i915_obj-vma_list,
typeof(*vma), vma_link);
- vma-bind_vma(vma, target_i915_obj-cache_level, GLOBAL_BIND);
+ i915_gem_vma_bind(vma, target_i915_obj-cache_level,
+ GLOBAL_BIND);
}
/* Validate that the target is in a valid r/w GPU domain */
@@ -1266,7 +1267,7 @@
