Re: [Intel-gfx] [RFC v4 2/2] drm/i915: Select engines via class and instance in execbuffer2
On Thu, May 18, 2017 at 03:58:26PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Building on top of the previous patch which exported the concept
> of engine classes and instances, we can also use this instead of
> the current awkward engine selection uAPI.
>
> This is primarily interesting for the VCS engine selection which
> is a) currently done via disjoint set of flags, and b) the
> current I915_EXEC_BSD flags has different semantics depending on
> the underlying hardware which is bad.
>
> Proposed idea here is to reserve 8-bits of flags, to pass in the
> engine instance, re-use the existing engine selection bits for
> the class selection, and a new flag named
> I915_EXEC_CLASS_INSTANCE to tell the kernel this new engine
> selection API is in use.
>
> The new uAPI also removes access to the weak VCS engine
> balancing as currently existing in the driver.
>
> Example usage to send a command to VCS0:
>
> eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 0);
>
> Or to send a command to VCS1:
>
> eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 1);
>
> v2:
> * Fix unknown flags mask.
> * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
>
> v3:
> * Add a map for fast class-instance engine lookup. (Chris Wilson)
>
> v4:
> * Update commit to reflect v3.
> * Export intel_engine_lookup for other users. (Chris Wilson)
> * Split out some warns. (Chris Wilson)
>
> Signed-off-by: Tvrtko Ursulin
> Cc: Ben Widawsky
> Cc: Chris Wilson
> Cc: Daniel Vetter
> Cc: Joonas Lahtinen
> Cc: Jon Bloomfield
> Cc: Daniel Charles
> Cc: "Rogozhkin, Dmitry V"
> Cc: Oscar Mateo
> Cc: "Gong, Zhipeng"
> Cc: intel-vaapi-me...@lists.01.org
> Cc: mesa-...@lists.freedesktop.org
> ---
> drivers/gpu/drm/i915/i915_drv.h| 1 +
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20
> drivers/gpu/drm/i915/i915_reg.h| 3 +++
> drivers/gpu/drm/i915/intel_engine_cs.c | 20
> drivers/gpu/drm/i915/intel_ringbuffer.h| 3 +++
> include/uapi/drm/i915_drm.h| 12 +++-
> 6 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e08b82c4823..53b41963f672 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2115,6 +2115,7 @@ struct drm_i915_private {
> struct pci_dev *bridge_dev;
> struct i915_gem_context *kernel_context;
> struct intel_engine_cs *engine[I915_NUM_ENGINES];
> + struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS +
> 1][MAX_ENGINE_INSTANCE + 1];
> struct i915_vma *semaphore;
>
> struct drm_dma_handle *status_page_dmah;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index af1965774e7b..006c8046af5f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1492,6 +1492,23 @@ gen8_dispatch_bsd_engine(struct drm_i915_private
> *dev_priv,
> return file_priv->bsd_engine;
> }
>
> +static struct intel_engine_cs *
> +eb_select_engine_class_instance(struct drm_i915_private *i915,
> + struct drm_i915_gem_execbuffer2 *args)
> +{
> + u8 class = args->flags & I915_EXEC_RING_MASK;
> + extern u8 user_class_map[I915_ENGINE_CLASS_MAX];
> + u8 instance;
> +
> + if (class >= ARRAY_SIZE(user_class_map))
> + return NULL;
> +
> + instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
> +I915_EXEC_INSTANCE_MASK;
> +
> + return intel_engine_lookup(i915, user_class_map[class], instance);
The class mapping is going to be the same for all users of
intel_engine_lookup() presumably? (At least I hope we plan on using the
same id everywhere!)
intel_engine_lookup_from_user(), just to reinforce where those ids are
coming from?
So other than the conversation of what class means, looks good to me.
To give some context for other users, one example is a context param for
setting per-engine settings (e.g. sseu, watchdog thresholds):
struct drm_i915_gem_context_param_sseu {
__u32 engine;
__u32 instance;
__u32 slice_mask;
__u32 subslice_mask;
__u32 min_eu_per_subslice;
__u32 max_eu_per_subslice;
};
static int
i915_gem_context_setparam__sseu(struct i915_gem_context *ctx,
const struct drm_i915_gem_context_param *args)
{
const struct drm_i915_gem_context_param_sseu *user =
u64_to_user_ptr(args->value);
const unsigned int size = args->size;
unsigned int total;
total = 0;
while (total + sizeof(*user) <= size) {
struct drm_i915_gem_context_param_sseu sseu;
int ret;
if (copy_from_user(&sseu, user++, sizeof(sseu)))
return -
[Intel-gfx] [RFC v4 2/2] drm/i915: Select engines via class and instance in execbuffer2
From: Tvrtko Ursulin
Building on top of the previous patch which exported the concept
of engine classes and instances, we can also use this instead of
the current awkward engine selection uAPI.
This is primarily interesting for the VCS engine selection which
is a) currently done via disjoint set of flags, and b) the
current I915_EXEC_BSD flags has different semantics depending on
the underlying hardware which is bad.
Proposed idea here is to reserve 8-bits of flags, to pass in the
engine instance, re-use the existing engine selection bits for
the class selection, and a new flag named
I915_EXEC_CLASS_INSTANCE to tell the kernel this new engine
selection API is in use.
The new uAPI also removes access to the weak VCS engine
balancing as currently existing in the driver.
Example usage to send a command to VCS0:
eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 0);
Or to send a command to VCS1:
eb.flags = i915_execbuffer2_engine(I915_ENGINE_CLASS_VIDEO_DECODE, 1);
v2:
* Fix unknown flags mask.
* Use I915_EXEC_RING_MASK for class. (Chris Wilson)
v3:
* Add a map for fast class-instance engine lookup. (Chris Wilson)
v4:
* Update commit to reflect v3.
* Export intel_engine_lookup for other users. (Chris Wilson)
* Split out some warns. (Chris Wilson)
Signed-off-by: Tvrtko Ursulin
Cc: Ben Widawsky
Cc: Chris Wilson
Cc: Daniel Vetter
Cc: Joonas Lahtinen
Cc: Jon Bloomfield
Cc: Daniel Charles
Cc: "Rogozhkin, Dmitry V"
Cc: Oscar Mateo
Cc: "Gong, Zhipeng"
Cc: intel-vaapi-me...@lists.01.org
Cc: mesa-...@lists.freedesktop.org
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20
drivers/gpu/drm/i915/i915_reg.h| 3 +++
drivers/gpu/drm/i915/intel_engine_cs.c | 20
drivers/gpu/drm/i915/intel_ringbuffer.h| 3 +++
include/uapi/drm/i915_drm.h| 12 +++-
6 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e08b82c4823..53b41963f672 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2115,6 +2115,7 @@ struct drm_i915_private {
struct pci_dev *bridge_dev;
struct i915_gem_context *kernel_context;
struct intel_engine_cs *engine[I915_NUM_ENGINES];
+ struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS +
1][MAX_ENGINE_INSTANCE + 1];
struct i915_vma *semaphore;
struct drm_dma_handle *status_page_dmah;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index af1965774e7b..006c8046af5f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1492,6 +1492,23 @@ gen8_dispatch_bsd_engine(struct drm_i915_private
*dev_priv,
return file_priv->bsd_engine;
}
+static struct intel_engine_cs *
+eb_select_engine_class_instance(struct drm_i915_private *i915,
+ struct drm_i915_gem_execbuffer2 *args)
+{
+ u8 class = args->flags & I915_EXEC_RING_MASK;
+ extern u8 user_class_map[I915_ENGINE_CLASS_MAX];
+ u8 instance;
+
+ if (class >= ARRAY_SIZE(user_class_map))
+ return NULL;
+
+ instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
+ I915_EXEC_INSTANCE_MASK;
+
+ return intel_engine_lookup(i915, user_class_map[class], instance);
+}
+
#define I915_USER_RINGS (4)
static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
@@ -1510,6 +1527,9 @@ eb_select_engine(struct drm_i915_private *dev_priv,
unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
struct intel_engine_cs *engine;
+ if (args->flags & I915_EXEC_CLASS_INSTANCE)
+ return eb_select_engine_class_instance(dev_priv, args);
+
if (user_ring_id > I915_USER_RINGS) {
DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
return NULL;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 89888adb9af1..fa4a3841c4af 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
#define VIDEO_ENHANCEMENT_CLASS2
#define COPY_ENGINE_CLASS 3
#define OTHER_CLASS4
+#define MAX_ENGINE_CLASS 4
+
+#define MAX_ENGINE_INSTANCE1
/* PCI config space */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 8d370c5d07bc..8f31514b01d5 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -198,6 +198,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
GEM_BUG_ON(info->class >= ARRAY_SIZE(intel_engine_classes));
class_info = &intel_engine_classes[info->class];
+ if (GEM_WARN
