[Intel-gfx] [RFC v3] drm/i915: Select engines via class and instance in execbuffer2
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu May 18 13:30:56 UTC 2017
On 17/05/2017 17:44, Chris Wilson wrote:
> On Wed, May 17, 2017 at 04:40:57PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> 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 16-bits of flags, to pass in
>> the engine class and instance (8 bits each), and a new flag
>> named I915_EXEC_CLASS_INSTACE 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(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
>>
>> Or to send a command to VCS1:
>>
>> eb.flags = i915_execbuffer2_engine(DRM_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)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Ben Widawsky <ben at bwidawsk.net>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
>> Cc: Daniel Charles <daniel.charles at intel.com>
>> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: "Gong, Zhipeng" <zhipeng.gong at intel.com>
>> Cc: intel-vaapi-media at lists.01.org
>> Cc: mesa-dev at lists.freedesktop.org
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 30 ++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> drivers/gpu/drm/i915/intel_engine_cs.c | 7 +++++++
>> include/uapi/drm/i915_drm.h | 11 ++++++++++-
>> 5 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 5dfa4a12e647..7bf4fd42480c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2066,6 +2066,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..c1ad49ab64cd 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1492,6 +1492,33 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>> return file_priv->bsd_engine;
>> }
>>
>> +extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
>> +
>> +static struct intel_engine_cs *get_engine_class(struct drm_i915_private *i915,
>> + u8 class, u8 instance)
>> +{
>> + if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
>> + return NULL;
>> +
>> + return i915->engine_class[class][instance];
>> +}
>
> Be bold make this this an intel_engine_lookup(), I forsee some other
> users appearing very shortly.
Still static or you want to export it straight away? Preference for
where to put it? Here or intel_engine_cs.c?
>> +static struct intel_engine_cs *
>> +eb_select_engine_class_instance(struct drm_i915_private *i915,
>> + struct drm_i915_gem_execbuffer2 *args)
>> +{
>> + u8 class, instance;
>> +
>> + class = args->flags & I915_EXEC_RING_MASK;
>> + if (class >= DRM_I915_ENGINE_CLASS_MAX)
>> + return NULL;
>> +
>> + instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
>> + I915_EXEC_INSTANCE_MASK;
>> +
>> + return get_engine_class(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 +1537,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 ee144ec57935..a3b59043b991 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_CLASS 2
>> #define COPY_ENGINE_CLASS 3
>> #define OTHER_CLASS 4
>> +#define MAX_ENGINE_CLASS 4
>> +
>> +#define MAX_ENGINE_INSTANCE 1
>
> We also need the names in the uapi.h as well. Do we want to mention
> OTHER_CLASS before it is defined? I trust your crystal ball.
I have mentioned it just by the virtue of it existing in i915_reg.h
Crystal ball is otherwise quiet.
> Amusingly I notice that you do claim that you have these names in the
> changelog. :) We don't need DRM_I915 all the time. I915_ENGINE_CLASS is
> going to be unique, at least for those users of i915_drm.h
They are all there courtesy of the previous patch.
I will drop the DRM_ prefix throughout.
>
>> /* PCI config space */
>>
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index 7566cf48012f..c5ad51c43d23 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -225,7 +225,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>>
>> ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
>>
>> + if (WARN_ON(info->class > MAX_ENGINE_CLASS ||
>> + info->instance > MAX_ENGINE_INSTANCE ||
>> + dev_priv->engine_class[info->class][info->instance]))
>
> Spread these out or add a message telling what was wrong.
Can do, but I considered these to be such a basic programming errors
which should be caught during development. Only thing which prevented me
from putting in under the GEM_BUG_ON is the fact someone could not have
it enabled and that this function can already handle failures.
>> + return -EINVAL;
>> +
>> + dev_priv->engine_class[info->class][info->instance] = engine;
>> dev_priv->engine[id] = engine;
>> +
>> return 0;
>> }
>>
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 2ac6667e57ea..6a26bdf5e684 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
>> */
>> #define I915_EXEC_FENCE_OUT (1<<17)
>>
>> -#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
>> +#define I915_EXEC_CLASS_INSTANCE (1<<18)
>> +
>> +#define I915_EXEC_INSTANCE_SHIFT (19)
>> +#define I915_EXEC_INSTANCE_MASK (0xff << I915_EXEC_INSTANCE_SHIFT)
>> +
>> +#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
>>
>> #define I915_EXEC_CONTEXT_ID_MASK (0xffffffff)
>> #define i915_execbuffer2_set_context_id(eb2, context) \
>> @@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
>> #define i915_execbuffer2_get_context_id(eb2) \
>> ((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
>>
>> +#define i915_execbuffer2_engine(class, instance) \
>> + (I915_EXEC_CLASS_INSTANCE | (class) | \
>> + ((instance) << I915_EXEC_INSTANCE_SHIFT))
>
> (class |
> (instance) << I915_EXEC_INSTANCE_SHIFT |
> I915_EXEC_CLASS_INSTANCE)
>
> Just suggesting to spread it out over another line for better
> readability.
Okay but I'd like for the flag to come first, followed by class and then
instance. Okay with that?
Regards,
Tvrtko
More information about the Intel-gfx
mailing list