Re: [PATCH v6] drm/i915: Engine discovery query

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Tvrtko Ursulin (2018-10-04 11:51:18)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> 
> Engine discovery query allows userspace to enumerate engines, probe their
> configuration features, all without needing to maintain the internal PCI
> ID based database.
> 
> A new query for the generic i915 query ioctl is added named
> DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure
> drm_i915_query_engine_info. The address of latter should be passed to the
> kernel in the query.data_ptr field, and should be large enough for the
> kernel to fill out all known engines as struct drm_i915_engine_info
> elements trailing the query.
> 
> As with other queries, setting the item query length to zero allows
> userspace to query minimum required buffer size.
> 
> Enumerated engines have common type mask which can be used to query all
> hardware engines, versus engines userspace can submit to using the execbuf
> uAPI.
> 
> Engines also have capabilities which are per engine class namespace of
> bits describing features not present on all engine instances.
> 
> v2:
>  * Fixed HEVC assignment.
>  * Reorder some fields, rename type to flags, increase width. (Lionel)
>  * No need to allocate temporary storage if we do it engine by engine.
>    (Lionel)
> 
> v3:
>  * Describe engine flags and mark mbz fields. (Lionel)
>  * HEVC only applies to VCS.
> 
> v4:
>  * Squash SFC flag into main patch.
>  * Tidy some comments.
> 
> v5:
>  * Add uabi_ prefix to engine capabilities. (Chris Wilson)
>  * Report exact size of engine info array. (Chris Wilson)
>  * Drop the engine flags. (Joonas Lahtinen)
>  * Added some more reserved fields.
>  * Move flags after class/instance.
> 
> v6:
>  * Do not check engine info array was zeroed by userspace but zero the
>    unused fields for them instead.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@xxxxxxxxx>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Tony Ye <tony.ye@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_query.c       | 56 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 12 ++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
>  include/uapi/drm/i915_drm.h             | 47 +++++++++++++++++++++
>  4 files changed, 118 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 5821002cad42..5ac8ef9f5de4 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -84,9 +84,65 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
>         return total_length;
>  }
>  
> +static int
> +query_engine_info(struct drm_i915_private *i915,
> +                 struct drm_i915_query_item *query_item)
> +{
> +       struct drm_i915_query_engine_info __user *query_ptr =
> +                               u64_to_user_ptr(query_item->data_ptr);
> +       struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0];
> +       struct drm_i915_query_engine_info query;
> +       struct drm_i915_engine_info info = { };
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
> +       int len;
> +
> +       if (query_item->flags)
> +               return -EINVAL;
> +
> +       len = 0;
> +       for_each_engine(engine, i915, id)
> +               len++;

(Isn't this INTEL_INFO()->num_rings?)

> +       len *= sizeof(struct drm_i915_engine_info);
> +       len += sizeof(struct drm_i915_query_engine_info);
> +
> +       if (!query_item->length)
> +               return len;
> +       else if (query_item->length < len)
> +               return -EINVAL;
> +
> +       if (copy_from_user(&query, query_ptr, sizeof(query)))
> +               return -EFAULT;
> +
> +       if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
> +           query.rsvd[2])
> +               return -EINVAL;
> +
> +       if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
> +               return -EFAULT;

Hmm, so length is the sizeof of the whole struct and not the space in
the pointed at block? Ok. I was just a little confused by the lack of
checking for info_ptr, as by this point the connection with query_ptr is
off the page.

May I beg
	info_ptr = &query_ptr->engines[0];
here. That should make it more obvious for feeble readers like myself to
see that info_ptr is contained within the access_ok check.

> +       for_each_engine(engine, i915, id) {
> +               info.class = engine->uabi_class;
> +               info.instance = engine->instance;
> +               info.capabilities = engine->uabi_capabilities;
> +

GEM_BUG_ON((void *)info_ptr > (void *)query_ptr + query_item->length - sizeof(info));

> +               if (__copy_to_user(info_ptr, &info, sizeof(info)))
> +                       return -EFAULT;
> +
> +               query.num_engines++;
> +               info_ptr++;
> +       }
> +
> +       if (__copy_to_user(query_ptr, &query, sizeof(query)))
> +               return -EFAULT;
> +
> +       return len;
> +}
> +
>  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
>                                         struct drm_i915_query_item *query_item) = {
>         query_topology_info,
> +       query_engine_info,
>  };
>  
>  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1c6143bdf5a4..134f0cec724c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>         engine->uabi_id = info->uabi_id;
>         engine->uabi_class = intel_engine_classes[info->class].uabi_class;
>  
> +       if (engine->class == VIDEO_DECODE_CLASS) {
> +               /* HEVC support is present only on vcs0. */
> +               if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
> +                       engine->uabi_capabilities =
> +                               I915_VCS_CLASS_CAPABILITY_HEVC;
> +
> +               /* SFC support is wired only to even VCS instances. */
> +               if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
> +                       engine->uabi_capabilities |=
> +                               I915_VCS_CLASS_CAPABILITY_SFC;

I trust your judgement here.

> +/**
> + * struct drm_i915_query_engine_info
> + *
> + * Engine info query enumerates all engines known to the driver by filling in
> + * an array of struct drm_i915_engine_info structures.
> + */
> +struct drm_i915_query_engine_info {
> +       /** Number of struct drm_i915_engine_info structs following. */
> +       __u32 num_engines;
> +
> +       /** MBZ */
> +       __u32 rsvd[3];
> +
> +       /** Marker for drm_i915_engine_info structures. */
> +       struct drm_i915_engine_info engines[];

Do we need [0] for old-c/c++ compatibility?

Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>

I'd value an ack from Lionel in case I've forgotten a quirk about the
query api, and an ack from a user that this meets their needs. It will
certainly simplify some of our igt code!

Question for overall design, do we want a conjoint capability flag
space, or one per class? One per class gives us more room, so I think
should be preferred, but I wonder if a set of common flags would make
it easier for userspace to filter. (Though not hard to match on both
class and caps!)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux