[Intel-gfx] [PATCH 1/4] drm/i915: introduce query info uAPI

[Tvrtko Ursulin]

On 09/11/2017 16:15, Lionel Landwerlin wrote:
> On 09/11/17 15:57, Joonas Lahtinen wrote:
>> On Wed, 2017-11-08 at 16:22 +0000, Lionel Landwerlin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>
>>> Query info uAPI allows userspace to probe for a number of properties
>>> of the GPU. This partially removes the need for userspace to maintain
>>> the internal PCI id based database (as well as code duplication across
>>> userspace components).
>>>
>>> This first version enables engine configuration and features to be
>>> queried.
>>>
>>> Probing is done via the new DRM_IOCTL_I915_QUERY_INFO ioctl which
>>> takes a query ID as well as query arguments.
>>>
>>> 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.
>>>
>>> Pseudo-code example of userspace using the uAPI:
>>>
>>>    struct drm_i915_query_info info = {};
>>>    struct drm_i915_engine_info *engines;
>>>    int i, ret;
>>>
>>>    info.version = 1;
>>>    info.query = I915_QUERY_INFO_ENGINE;
>>>    info.query_params[0] = I915_ENGINE_CLASS_VIDEO;
>>>    info.info_ptr_len = 0;
>>>    ret = ioctl(..., &info);
>>>    assert(ret == 0);
>>>
>>>    engines = malloc(info.info_ptr_len);
>>>    info.info_ptr = to_user_pointer(engines);
>>>    ret = ioctl(..., &info);
>>>    assert(ret == 0);
>>>
>>>    for (i = 0; i < info.info_ptr / sizeof(*engines); i++)
>>>        printf("VCS%u: flags=%x\n",
>>>               engines[i].instance,
>>>               engines[i].info);
>>>
>>> First time with info.info_ptr_len set to zero, so the kernel reports
>>> back the size of the data to be written into info.info_ptr. Then a
>>> second time with the info.info_ptr_len field set to the correct
>>> length.
>>>
>>> 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.
>>>
>>> v5:
>>>   * Example usage added to commit msg. (Chris Wilson)
>>>   * Report engine count in case of version mismatch. (Chris Wilson)
>>>
>>> v6:
>>>   * Return API to query_info to make it more generic (Lionel)
>>>   * Add query ID & query params (Lionel)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin 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: "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>
>> <SNIP>
>>
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -2776,6 +2776,7 @@ static const struct drm_ioctl_desc 
>>> i915_ioctls[] = {
>>>       DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, 
>>> DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, 
>>> i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, 
>>> i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>> +    DRM_IOCTL_DEF_DRV(I915_QUERY_INFO, i915_query_info_ioctl, 
>>> DRM_RENDER_ALLOW),
>> I'm not a fan. This is bit much to the direction of I915_META_IOCTL.
> 
> So you would prefer a different ioctl for every bit of information we 
> would want to query from the kernel?
> 
>>
>> So can we keep this as engine info query without versioning, and if you
>> need to query something else, lets have another ioctl for that.
>>
>> (I heard a rumour of this being about RCS topology, which could be in
>> the engine info.).
> 
> I'm not sure to understand what you mean by "in the engine info". Would 
> you mind to give an example?

We chatted about this internally a bit and no one seems to like a 
multiplexer within a multiplexer approach.

So one option is a completely separate ioctl, but before that the 
question is if everything you need to export is called RCS topology, so 
logically belongs to the render engine, could it be exported under the 
engine info name after all?

Like maybe adding drm_i915_rcs_engine_info, containing all the extra 
fields you need on top of the existing info, to be returned when the 
render class is queried.

One problem I spotted is that your new data is only valid on gen8+. So 
that would mean having a flag saying what data is valid.

Regards,

Tvrtko