Re: [Intel-gfx] [PATCH 13/43] drm/i915: Abstract the legacy workload submission mechanism away
On Thu, Jul 24, 2014 at 05:04:21PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
As suggested by Daniel Vetter. The idea, in subsequent patches, is to
provide an alternative to these vfuncs for the Execlists submission
mechanism.
v2: Splitted into two and reordered to illustrate our intentions, instead
of showing it off. Also, remove the add_request vfunc and added the
stop_ring one.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h| 24
drivers/gpu/drm/i915/i915_gem.c| 15 +++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++--
3 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff2c373..1caed52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1617,6 +1617,21 @@ struct drm_i915_private {
/* Old ums support infrastructure, same warning applies. */
struct i915_ums_state ums;
+ /* Abstract the submission mechanism (legacy ringbuffer or execlists)
away */
+ struct {
+ int (*do_execbuf) (struct drm_device *dev, struct drm_file
*file,
+struct intel_engine_cs *ring,
+struct intel_context *ctx,
+struct drm_i915_gem_execbuffer2 *args,
+struct list_head *vmas,
+struct drm_i915_gem_object *batch_obj,
+u64 exec_start, u32 flags);
+ int (*init_rings) (struct drm_device *dev);
+ void (*cleanup_ring) (struct intel_engine_cs *ring);
+ void (*stop_ring) (struct intel_engine_cs *ring);
My rule of thumb is that with just one caller it's probably better to not
have a vtable - just makes it harder to follow the code flow.
Usually (with non-borked code design at least) init/teardown functions
have only one caller, so there's a good chance I'll ask you to remove them
again.
+ bool (*is_ring_initialized) (struct intel_engine_cs *ring);
This one is unused and I didn't really like the taste of it. So I killed
it.
-Daniel
+ } gt;
+
/*
* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
* will be rejected. Instead look for a better place.
@@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev,
void *data,
struct drm_file *file_priv);
int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+struct drm_file *file,
+struct intel_engine_cs *ring,
+struct intel_context *ctx,
+struct drm_i915_gem_execbuffer2 *args,
+struct list_head *vmas,
+struct drm_i915_gem_object *batch_obj,
+u64 exec_start, u32 flags);
int i915_gem_execbuffer(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int i915_gem_execbuffer2(struct drm_device *dev, void *data,
@@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
int __must_check i915_gem_init_hw(struct drm_device *dev);
int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
void i915_gem_init_swizzling(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d8bf4fa..6544286 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
int i;
for_each_ring(ring, dev_priv, i)
- intel_stop_ring_buffer(ring);
+ dev_priv-gt.stop_ring(ring);
}
int
@@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
return true;
}
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
@@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
i915_gem_init_swizzling(dev);
- ret = i915_gem_init_rings(dev);
+ ret = dev_priv-gt.init_rings(dev);
if (ret)
return ret;
@@ -4759,6 +4759,13 @@ int i915_gem_init(struct drm_device *dev)
Re: [Intel-gfx] [PATCH 13/43] drm/i915: Abstract the legacy workload submission mechanism away
On Mon, Aug 11, 2014 at 04:36:53PM +0200, Daniel Vetter wrote:
On Thu, Jul 24, 2014 at 05:04:21PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
As suggested by Daniel Vetter. The idea, in subsequent patches, is to
provide an alternative to these vfuncs for the Execlists submission
mechanism.
v2: Splitted into two and reordered to illustrate our intentions, instead
of showing it off. Also, remove the add_request vfunc and added the
stop_ring one.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h| 24
drivers/gpu/drm/i915/i915_gem.c| 15 +++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++--
3 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index ff2c373..1caed52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1617,6 +1617,21 @@ struct drm_i915_private {
/* Old ums support infrastructure, same warning applies. */
struct i915_ums_state ums;
+ /* Abstract the submission mechanism (legacy ringbuffer or execlists)
away */
+ struct {
+ int (*do_execbuf) (struct drm_device *dev, struct drm_file
*file,
+ struct intel_engine_cs *ring,
+ struct intel_context *ctx,
+ struct drm_i915_gem_execbuffer2 *args,
+ struct list_head *vmas,
+ struct drm_i915_gem_object *batch_obj,
+ u64 exec_start, u32 flags);
+ int (*init_rings) (struct drm_device *dev);
+ void (*cleanup_ring) (struct intel_engine_cs *ring);
+ void (*stop_ring) (struct intel_engine_cs *ring);
My rule of thumb is that with just one caller it's probably better to not
have a vtable - just makes it harder to follow the code flow.
Usually (with non-borked code design at least) init/teardown functions
have only one caller, so there's a good chance I'll ask you to remove them
again.
Also checkpatch (and my eyes!) where unhappy about the space you've put
between ) and (. Fixed that too.
-Daniel
+ bool (*is_ring_initialized) (struct intel_engine_cs *ring);
This one is unused and I didn't really like the taste of it. So I killed
it.
-Daniel
+ } gt;
+
/*
* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
* will be rejected. Instead look for a better place.
@@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device
*dev, void *data,
struct drm_file *file_priv);
int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+ struct drm_file *file,
+ struct intel_engine_cs *ring,
+ struct intel_context *ctx,
+ struct drm_i915_gem_execbuffer2 *args,
+ struct list_head *vmas,
+ struct drm_i915_gem_object *batch_obj,
+ u64 exec_start, u32 flags);
int i915_gem_execbuffer(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int i915_gem_execbuffer2(struct drm_device *dev, void *data,
@@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object
*obj);
int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
int __must_check i915_gem_init_hw(struct drm_device *dev);
int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
void i915_gem_init_swizzling(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index d8bf4fa..6544286 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
int i;
for_each_ring(ring, dev_priv, i)
- intel_stop_ring_buffer(ring);
+ dev_priv-gt.stop_ring(ring);
}
int
@@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
return true;
}
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
@@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
i915_gem_init_swizzling(dev);
- ret =
Re: [Intel-gfx] [PATCH 13/43] drm/i915: Abstract the legacy workload submission mechanism away
On Thu, Jul 24, 2014 at 05:04:21PM +0100, Thomas Daniel wrote: @@ -1408,8 +1408,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, else exec_start += i915_gem_obj_offset(batch_obj, vm); - ret = legacy_ringbuffer_submission(dev, file, ring, ctx, - args, eb-vmas, batch_obj, exec_start, flags); + ret = dev_priv-gt.do_execbuf(dev, file, ring, ctx, args, + eb-vmas, batch_obj, exec_start, flags); Also misaligned and too long line here. Fixed, too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/43] drm/i915: Abstract the legacy workload submission mechanism away
On Thu, Jul 24, 2014 at 05:04:21PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
As suggested by Daniel Vetter. The idea, in subsequent patches, is to
provide an alternative to these vfuncs for the Execlists submission
mechanism.
v2: Splitted into two and reordered to illustrate our intentions, instead
of showing it off. Also, remove the add_request vfunc and added the
stop_ring one.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h| 24
drivers/gpu/drm/i915/i915_gem.c| 15 +++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++--
3 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff2c373..1caed52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1617,6 +1617,21 @@ struct drm_i915_private {
/* Old ums support infrastructure, same warning applies. */
struct i915_ums_state ums;
+ /* Abstract the submission mechanism (legacy ringbuffer or execlists)
away */
+ struct {
+ int (*do_execbuf) (struct drm_device *dev, struct drm_file
*file,
+struct intel_engine_cs *ring,
+struct intel_context *ctx,
+struct drm_i915_gem_execbuffer2 *args,
+struct list_head *vmas,
+struct drm_i915_gem_object *batch_obj,
+u64 exec_start, u32 flags);
+ int (*init_rings) (struct drm_device *dev);
+ void (*cleanup_ring) (struct intel_engine_cs *ring);
+ void (*stop_ring) (struct intel_engine_cs *ring);
+ bool (*is_ring_initialized) (struct intel_engine_cs *ring);
Aside: ring here is a bit a misnomer, we talk about the engines. I guess
at the end of all this we should throw a few patches on top to name things
correctly with ring/engine where appropriate.
-Daniel
+ } gt;
+
/*
* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
* will be rejected. Instead look for a better place.
@@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev,
void *data,
struct drm_file *file_priv);
int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+struct drm_file *file,
+struct intel_engine_cs *ring,
+struct intel_context *ctx,
+struct drm_i915_gem_execbuffer2 *args,
+struct list_head *vmas,
+struct drm_i915_gem_object *batch_obj,
+u64 exec_start, u32 flags);
int i915_gem_execbuffer(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int i915_gem_execbuffer2(struct drm_device *dev, void *data,
@@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
int __must_check i915_gem_init_hw(struct drm_device *dev);
int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
void i915_gem_init_swizzling(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d8bf4fa..6544286 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
int i;
for_each_ring(ring, dev_priv, i)
- intel_stop_ring_buffer(ring);
+ dev_priv-gt.stop_ring(ring);
}
int
@@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
return true;
}
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
@@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
i915_gem_init_swizzling(dev);
- ret = i915_gem_init_rings(dev);
+ ret = dev_priv-gt.init_rings(dev);
if (ret)
return ret;
@@ -4759,6 +4759,13 @@ int i915_gem_init(struct drm_device *dev)
DRM_DEBUG_DRIVER(allow wake ack timed out\n);
}
+ if (!i915.enable_execlists) {
+ dev_priv-gt.do_execbuf = i915_gem_ringbuffer_submission;
+
[Intel-gfx] [PATCH 13/43] drm/i915: Abstract the legacy workload submission mechanism away
From: Oscar Mateo oscar.ma...@intel.com
As suggested by Daniel Vetter. The idea, in subsequent patches, is to
provide an alternative to these vfuncs for the Execlists submission
mechanism.
v2: Splitted into two and reordered to illustrate our intentions, instead
of showing it off. Also, remove the add_request vfunc and added the
stop_ring one.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h| 24
drivers/gpu/drm/i915/i915_gem.c| 15 +++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++--
3 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff2c373..1caed52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1617,6 +1617,21 @@ struct drm_i915_private {
/* Old ums support infrastructure, same warning applies. */
struct i915_ums_state ums;
+ /* Abstract the submission mechanism (legacy ringbuffer or execlists)
away */
+ struct {
+ int (*do_execbuf) (struct drm_device *dev, struct drm_file
*file,
+ struct intel_engine_cs *ring,
+ struct intel_context *ctx,
+ struct drm_i915_gem_execbuffer2 *args,
+ struct list_head *vmas,
+ struct drm_i915_gem_object *batch_obj,
+ u64 exec_start, u32 flags);
+ int (*init_rings) (struct drm_device *dev);
+ void (*cleanup_ring) (struct intel_engine_cs *ring);
+ void (*stop_ring) (struct intel_engine_cs *ring);
+ bool (*is_ring_initialized) (struct intel_engine_cs *ring);
+ } gt;
+
/*
* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
* will be rejected. Instead look for a better place.
@@ -2224,6 +2239,14 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev,
void *data,
struct drm_file *file_priv);
int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+int i915_gem_ringbuffer_submission(struct drm_device *dev,
+ struct drm_file *file,
+ struct intel_engine_cs *ring,
+ struct intel_context *ctx,
+ struct drm_i915_gem_execbuffer2 *args,
+ struct list_head *vmas,
+ struct drm_i915_gem_object *batch_obj,
+ u64 exec_start, u32 flags);
int i915_gem_execbuffer(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int i915_gem_execbuffer2(struct drm_device *dev, void *data,
@@ -2376,6 +2399,7 @@ void i915_gem_reset(struct drm_device *dev);
bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
int __must_check i915_gem_init(struct drm_device *dev);
+int i915_gem_init_rings(struct drm_device *dev);
int __must_check i915_gem_init_hw(struct drm_device *dev);
int i915_gem_l3_remap(struct intel_engine_cs *ring, int slice);
void i915_gem_init_swizzling(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d8bf4fa..6544286 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4518,7 +4518,7 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
int i;
for_each_ring(ring, dev_priv, i)
- intel_stop_ring_buffer(ring);
+ dev_priv-gt.stop_ring(ring);
}
int
@@ -4635,7 +4635,7 @@ intel_enable_blt(struct drm_device *dev)
return true;
}
-static int i915_gem_init_rings(struct drm_device *dev)
+int i915_gem_init_rings(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
@@ -4718,7 +4718,7 @@ i915_gem_init_hw(struct drm_device *dev)
i915_gem_init_swizzling(dev);
- ret = i915_gem_init_rings(dev);
+ ret = dev_priv-gt.init_rings(dev);
if (ret)
return ret;
@@ -4759,6 +4759,13 @@ int i915_gem_init(struct drm_device *dev)
DRM_DEBUG_DRIVER(allow wake ack timed out\n);
}
+ if (!i915.enable_execlists) {
+ dev_priv-gt.do_execbuf = i915_gem_ringbuffer_submission;
+ dev_priv-gt.init_rings = i915_gem_init_rings;
+ dev_priv-gt.cleanup_ring = intel_cleanup_ring_buffer;
+ dev_priv-gt.stop_ring = intel_stop_ring_buffer;
+ }
+
i915_gem_init_userptr(dev);
i915_gem_init_global_gtt(dev);
@@ -4794,7 +4801,7 @@
