Re: [RFC v4 1/2] drm/i915: Engine discovery uAPI

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2017-06-26 16:47:41)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine discovery uAPI allows userspace to probe for engine
> configuration and features without needing to maintain the
> internal PCI id based database.
> 
> This enables removal of code duplications across userspace
> components.
> 
> Probing is done via the new DRM_IOCTL_I915_GEM_ENGINE_INFO ioctl
> which returns the number and information on the specified engine
> class.
> 
> Currently only general engine configuration and HEVC feature of
> the VCS engine can be probed but the uAPI is designed to be
> generic and extensible.
> 
> Code is based almost exactly on the earlier proposal on the
> topic by Jon Bloomfield. Engine class and instance refactoring
> made recently by Daniele Ceraolo Spurio enabled this to be
> implemented in an elegant fashion.
> 
> To probe configuration userspace sets the engine class it wants
> to query (struct drm_i915_gem_engine_info) and provides an array
> of drm_i915_engine_info structs which will be filled in by the
> driver. Userspace also has to tell i915 how many elements are in
> the array, and the driver will report back the total number of
> engine instances in any case.
> 
> v2:
>  * Add a version field and consolidate to one engine count.
>    (Chris Wilson)
>  * Rename uAPI flags for VCS engines to DRM_I915_ENGINE_CLASS_VIDEO.
>    (Gong Zhipeng)
> 
> v3:
>  * Drop the DRM_ prefix from uAPI enums. (Chris Wilson)
>  * Only reserve 8-bits for the engine info for time being.
> 
> v4:
>  * Made user_class_map static.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c        |  1 +
>  drivers/gpu/drm/i915/i915_drv.h        |  3 ++
>  drivers/gpu/drm/i915/intel_engine_cs.c | 64 ++++++++++++++++++++++++++++++++++
>  include/uapi/drm/i915_drm.h            | 41 ++++++++++++++++++++++
>  4 files changed, 109 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e9d8e9ee51b2..44dd2f37937f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2724,6 +2724,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>         DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
>         DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(I915_GEM_ENGINE_INFO, i915_gem_engine_info_ioctl, DRM_RENDER_ALLOW),
>  };
>  
>  static struct drm_driver driver = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 427d10c7eca5..496565e1753f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3598,6 +3598,9 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>                             struct i915_gem_context *ctx,
>                             uint32_t *reg_state);
>  
> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
> +                              struct drm_file *file);
> +
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct i915_address_space *vm,
>                                           u64 min_size, u64 alignment,
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 3b46c1f7b88b..a98669f6ad85 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -25,6 +25,7 @@
>  #include "i915_drv.h"
>  #include "intel_ringbuffer.h"
>  #include "intel_lrc.h"
> +#include <uapi/drm/i915_drm.h>
>  
>  /* Haswell does have the CXT_SIZE register however it does not appear to be
>   * valid. Now, docs explain in dwords what is in the context object. The full
> @@ -1332,6 +1333,69 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
>         }
>  }
>  
> +static u8 user_class_map[I915_ENGINE_CLASS_MAX] = {
> +       [I915_ENGINE_CLASS_OTHER] = OTHER_CLASS,
> +       [I915_ENGINE_CLASS_RENDER] = RENDER_CLASS,
> +       [I915_ENGINE_CLASS_COPY] = COPY_ENGINE_CLASS,
> +       [I915_ENGINE_CLASS_VIDEO] = VIDEO_DECODE_CLASS,
> +       [I915_ENGINE_CLASS_VIDEO_ENHANCE] = VIDEO_ENHANCEMENT_CLASS,
> +};
> +
> +int i915_gem_engine_info_ioctl(struct drm_device *dev, void *data,
> +                              struct drm_file *file)
> +{
> +       struct drm_i915_private *i915 = to_i915(dev);
> +       struct drm_i915_gem_engine_info *args = data;
> +       struct drm_i915_engine_info __user *user_info =
> +                                       u64_to_user_ptr(args->info_ptr);
> +       unsigned int info_size = args->num_engines;
> +       struct drm_i915_engine_info info;
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
> +       u8 class;
> +
> +       if (args->rsvd)
> +               return -EINVAL;
> +
> +       switch (args->engine_class) {
> +       case I915_ENGINE_CLASS_OTHER:
> +       case I915_ENGINE_CLASS_RENDER:
> +       case I915_ENGINE_CLASS_COPY:
> +       case I915_ENGINE_CLASS_VIDEO:
> +       case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> +               class = user_class_map[args->engine_class];
> +               break;
> +       case I915_ENGINE_CLASS_MAX:
> +       default:
> +               return -EINVAL;
> +       };
> +
> +       args->num_engines = 0;
> +
> +       if (args->version != 1) {
> +               args->version = 1;
> +               return 0;

My gut feeling says we want to report both the version and count in the
first pass. Otherwise we have to do a version check, followed by a count
query, followed by the actual retrieval. If we can combine the version +
count check into one pass, it may be a little less hassle?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx