[Intel-gfx] [PATCH v7 5/6] drm/i915: add query uAPI
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Wed Jan 17 12:45:29 UTC 2018
On 17/01/18 10:10, Tvrtko Ursulin wrote:
>
> On 16/01/2018 19:18, Lionel Landwerlin wrote:
>> There are a number of information that are readable from hardware
>> registers and that we would like to make accessible to userspace. One
>> particular example is the topology of the execution units (how are
>> execution units grouped in subslices and slices and also which ones
>> have been fused off for die recovery).
>>
>> At the moment the GET_PARAM ioctl covers some basic needs, but
>> generally is only able to return a single value for each defined
>> parameter. This is a bit problematic with topology descriptions which
>> are array/maps of available units.
>>
>> This change introduces a new ioctl that can deal with requests to fill
>> structures of potentially variable lengths. The user is expected fill
>> a query with length fields set at 0 on the first call, the kernel then
>> sets the length fields to the their expected values. A second call to
>> the kernel with length fields at their expected values will trigger a
>> copy of the data to the pointed memory locations.
>>
>> The scope of this uAPI is only to provide information to userspace,
>> not to allow configuration of the device.
>>
>> v2: Simplify dispatcher code iteration (Tvrtko)
>> Tweak uapi drm_i915_query_item structure (Tvrtko)
>>
>> v3: Rename pad fields into flags (Chris)
>> Return error on flags field != 0 (Chris)
>> Only copy length back to userspace in drm_i915_query_item (Chris)
>>
>> v4: Use array of functions instead of switch (Chris)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> I had not read v3 and v4 yet, so lets quickly see if I still approve...
>
>> ---
>> drivers/gpu/drm/i915/Makefile | 1 +
>> drivers/gpu/drm/i915/i915_drv.c | 1 +
>> drivers/gpu/drm/i915/i915_drv.h | 3 ++
>> drivers/gpu/drm/i915/i915_query.c | 69
>> +++++++++++++++++++++++++++++++++++++++
>> include/uapi/drm/i915_drm.h | 32 ++++++++++++++++++
>> 5 files changed, 106 insertions(+)
>> create mode 100644 drivers/gpu/drm/i915/i915_query.c
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile
>> b/drivers/gpu/drm/i915/Makefile
>> index 3bddd8a06806..b0415a3e2d59 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -69,6 +69,7 @@ i915-y += i915_cmd_parser.o \
>> i915_gem_timeline.o \
>> i915_gem_userptr.o \
>> i915_gemfs.o \
>> + i915_query.o \
>> i915_trace_points.o \
>> i915_vma.o \
>> intel_breadcrumbs.o \
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 969835d3cbcd..d92e1b7236fc 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -2824,6 +2824,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, i915_query_ioctl,
>> DRM_UNLOCKED|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 c42015b05b47..b2615a88936e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3622,6 +3622,9 @@ extern void i915_perf_fini(struct
>> drm_i915_private *dev_priv);
>> extern void i915_perf_register(struct drm_i915_private *dev_priv);
>> extern void i915_perf_unregister(struct drm_i915_private *dev_priv);
>> +/* i915_query.c */
>> +int i915_query_ioctl(struct drm_device *dev, void *data, struct
>> drm_file *file);
>> +
>> /* i915_suspend.c */
>> extern int i915_save_state(struct drm_i915_private *dev_priv);
>> extern int i915_restore_state(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_query.c
>> b/drivers/gpu/drm/i915/i915_query.c
>> new file mode 100644
>> index 000000000000..51736af7f573
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -0,0 +1,69 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> the next
>> + * paragraph) shall be included in all copies or substantial
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "i915_drv.h"
>> +#include <uapi/drm/i915_drm.h>
>> +
>> +static int (* const i915_query_funcs[])(struct drm_i915_private
>> *dev_priv,
>> + struct drm_i915_query_item *query_item) = {
>> +};
>> +
>> +int i915_query_ioctl(struct drm_device *dev, void *data, struct
>> drm_file *file)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct drm_i915_query *args = data;
>> + struct drm_i915_query_item __user *user_item_ptr =
>> + u64_to_user_ptr(args->items_ptr);
>> + u32 i;
>> +
>> + if (args->flags != 0)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < args->num_items; i++, user_item_ptr++) {
>> + struct drm_i915_query_item item;
>> + u64 func_idx;
>> + int ret;
>> +
>> + if (copy_from_user(&item, user_item_ptr, sizeof(item)))
>> + return -EFAULT;
>> +
>> + if (item.query_id == 0 || item.flags != 0)
>> + return -EINVAL;
>
> How do we define item.flags? I am thinking if it should be handled
> here, or perhaps in the item handler? Some items may have flags, some
> may not, at some point and then this becomes at the wrong level I think.
Moving this into the vfuncs.
>
>> +
>> + func_idx = item.query_id - 1;
>> +
>> + if (func_idx >= ARRAY_SIZE(i915_query_funcs))
>> + return -EINVAL;
>> +
>> + ret = i915_query_funcs[func_idx](dev_priv, &item);
>> + if (ret < 0)
>> + return ret;
>
> One downside I can think of, of the vfunc approach, is if we decide to
> allocated bit-groups of item ids to different categories of stuff at
> some point. But I guess it's too hypothetical for now.
>
>> +
>> + /* Only write the length back to userspace if they differ. */
>> + if (ret != item.length && put_user(ret,
>> &user_item_ptr->length))
>> + return -EFAULT;
>
> Would it make sense to return the error code in user_item_ptr->length
> (both from put_user, and from the vfunc), so userspace can detect
> which item in the list of queries was the problematic one? It would
> probably mean re-defining the length field as s32, but 2 GiB of item
> blobs should be enough for everyone? :)
Yeah, Chris raised a similar point with unsupported queries (like this
series doesn't enable slice/subslice info query on gen < 8).
I'm not that experienced with uapi stuff... If that sounds good to
people, then sure.
>
>> + }
>> +
>> + return 0;
>> +}
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 536ee4febd74..07a2a35a4277 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -318,6 +318,7 @@ typedef struct _drm_i915_sarea {
>> #define DRM_I915_PERF_OPEN 0x36
>> #define DRM_I915_PERF_ADD_CONFIG 0x37
>> #define DRM_I915_PERF_REMOVE_CONFIG 0x38
>> +#define DRM_I915_QUERY 0x39
>> #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE +
>> DRM_I915_INIT, drm_i915_init_t)
>> #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE +
>> DRM_I915_FLUSH)
>> @@ -375,6 +376,7 @@ typedef struct _drm_i915_sarea {
>> #define DRM_IOCTL_I915_PERF_OPEN DRM_IOW(DRM_COMMAND_BASE +
>> DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
>> #define DRM_IOCTL_I915_PERF_ADD_CONFIG DRM_IOW(DRM_COMMAND_BASE +
>> DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
>> #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG DRM_IOW(DRM_COMMAND_BASE
>> + DRM_I915_PERF_REMOVE_CONFIG, __u64)
>> +#define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE +
>> DRM_I915_QUERY, struct drm_i915_query)
>> /* Allow drivers to submit batchbuffers directly to hardware,
>> relying
>> * on the security mechanisms provided by hardware.
>> @@ -1613,6 +1615,36 @@ struct drm_i915_perf_oa_config {
>> __u64 flex_regs_ptr;
>> };
>> +
>> +struct drm_i915_query_item {
>> + __u64 query_id;
>> +
>> + /*
>> + * When set to zero by userspace, this is filled with the size
>> of the
>> + * data to be written at the data_ptr pointer.
>> + */
>> + __u32 length;
>> +
>> + __u32 flags;
>
> Say in the comment it's unused for now, here and below, so it looks
> less weird in a sandwich between two commented fields?
Done.
>
>> +
>> + /*
>> + * Data will be written at the location pointed by data_ptr when
>> the
>> + * value of length matches the length of the data to be written
>> by the
>> + * kernel.
>> + */
>> + __u64 data_ptr;
>> +};
>> +
>> +struct drm_i915_query {
>> + __u32 num_items;
>> + __u32 flags;
>> +
>> + /*
>> + * This point to an array of num_items drm_i915_query_item
>> structures.
>> + */
>> + __u64 items_ptr;
>> +};
>> +
>> #if defined(__cplusplus)
>> }
>> #endif
>>
>
> Regards,
>
> Tvrtko
>
More information about the Intel-gfx
mailing list