Quoting Lionel Landwerlin (2019-07-09 13:33:51)
> Listing configurations at the moment is supported only through sysfs.
> This might cause issues for applications wanting to list
> configurations from a container where sysfs isn't available.
>
> This change adds a way to query the number of configurations and their
> content through the i915 query uAPI.
>
> v2: Fix sparse warnings (Lionel)
> Add support to query configuration using uuid (Lionel)
>
> v3: Fix some inconsistency in uapi header (Lionel)
> Fix unlocking when not locked issue (Lionel)
> Add debug messages (Lionel)
>
> v4: Fix missing unlock (Dan)
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 +
> drivers/gpu/drm/i915/i915_perf.c | 3 +
> drivers/gpu/drm/i915/i915_query.c | 277 ++++++++++++++++++++++++++++++
> include/uapi/drm/i915_drm.h | 65 ++++++-
> 4 files changed, 348 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c72e7c746b57..28f0f490b7b1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1729,6 +1729,12 @@ struct drm_i915_private {
> */
> struct list_head metrics_buffers;
>
> + /*
> + * Number of dynamic configurations, you need to hold
> + * dev_priv->perf.metrics_lock to access it.
> + */
> + u32 n_metrics;
> +
> /*
> * Lock associated with anything below within this structure
> * except exclusive_stream.
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index dfd9ed4f321e..9cd1362d6922 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -3722,6 +3722,8 @@ int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
> goto sysfs_err;
> }
>
> + dev_priv->perf.n_metrics++;
> +
> mutex_unlock(&dev_priv->perf.metrics_lock);
>
> DRM_DEBUG("Added config %s id=%i\n", oa_config->uuid, oa_config->id);
> @@ -3782,6 +3784,7 @@ int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
> &oa_config->sysfs_metric);
>
> idr_remove(&dev_priv->perf.metrics_idr, *arg);
> + dev_priv->perf.n_metrics--;
>
> mutex_unlock(&dev_priv->perf.metrics_lock);
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 7b7016171057..74b632998d0e 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -143,10 +143,287 @@ query_engine_info(struct drm_i915_private *i915,
> return len;
> }
>
> +static int can_copy_perf_config_registers_or_number(u32 user_n_regs,
> + u64 user_regs_ptr,
> + u32 kernel_n_regs)
> +{
> + /*
> + * We'll just put the number of registers, and won't copy the
> + * register.
> + */
> + if (user_n_regs == 0)
> + return 0;
> +
> + if (user_n_regs < kernel_n_regs)
> + return -EINVAL;
> +
> + if (!access_ok(u64_to_user_ptr(user_regs_ptr),
> + 2 * sizeof(u32) * kernel_n_regs))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int copy_perf_config_registers_or_number(const struct i915_oa_reg *kernel_regs,
> + u32 kernel_n_regs,
> + u64 user_regs_ptr,
> + u32 *user_n_regs)
> +{
> + u32 r;
> +
> + if (*user_n_regs == 0) {
> + *user_n_regs = kernel_n_regs;
> + return 0;
> + }
> +
> + *user_n_regs = kernel_n_regs;
> +
> + for (r = 0; r < kernel_n_regs; r++) {
> + u32 __user *user_reg_ptr =
> + u64_to_user_ptr(user_regs_ptr + sizeof(u32) * r * 2);
> + u32 __user *user_val_ptr =
> + u64_to_user_ptr(user_regs_ptr + sizeof(u32) * r * 2 +
> + sizeof(u32));
> + int ret;
> +
> + ret = __put_user(i915_mmio_reg_offset(kernel_regs[r].addr),
> + user_reg_ptr);
> + if (ret)
> + return -EFAULT;
> +
> + ret = __put_user(kernel_regs[r].value, user_val_ptr);
> + if (ret)
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +static int query_perf_config_data(struct drm_i915_private *i915,
> + struct drm_i915_query_item *query_item,
> + bool use_uuid)
> +{
> + struct drm_i915_query_perf_config __user *user_query_config_ptr =
> + u64_to_user_ptr(query_item->data_ptr);
> + struct drm_i915_perf_oa_config __user *user_config_ptr =
> + u64_to_user_ptr(query_item->data_ptr +
> + sizeof(struct drm_i915_query_perf_config));
> + struct drm_i915_perf_oa_config user_config;
> + struct i915_oa_config *oa_config = NULL;
> + char uuid[UUID_STRING_LEN + 1];
> + u64 config_id;
> + u32 flags, total_size;
> + int ret;
> +
> + if (!i915->perf.initialized)
> + return -ENODEV;
> +
> + total_size = sizeof(struct drm_i915_query_perf_config) +
> + sizeof(struct drm_i915_perf_oa_config);
> +
> + if (query_item->length == 0)
> + return total_size;
> +
> + if (query_item->length < total_size) {
> + DRM_DEBUG("Invalid query config data item size=%u expected=%u\n",
> + query_item->length, total_size);
> + return -EINVAL;
> + }
> +
> + if (!access_ok(user_query_config_ptr, total_size))
> + return -EFAULT;
> +
> + if (__get_user(flags, &user_query_config_ptr->flags))
> + return -EFAULT;
> +
> + if (flags != 0)
> + return -EINVAL;
> +
> + if (use_uuid) {
> + BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid));
> +
> + memset(&uuid, 0, sizeof(uuid));
> + if (__copy_from_user(uuid, user_query_config_ptr->uuid,
> + sizeof(user_query_config_ptr->uuid)))
> + return -EFAULT;
> + } else {
> + if (__get_user(config_id, &user_query_config_ptr->config)) {
> + return -EFAULT;
> + }
> + }
> +
> + ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
> + if (ret)
> + return ret;
> +
> + if (use_uuid) {
> + struct i915_oa_config *tmp;
> + int id;
> +
> + idr_for_each_entry(&i915->perf.metrics_idr, tmp, id) {
> + if (!strcmp(tmp->uuid, uuid)) {
> + kref_get(&tmp->ref);
> + oa_config = tmp;
Bah, was expecting oa_config = oa_config_get(tmp);
> + break;
> + }
Pray be small. A secondary hashtable would be trivial to add if
required.
> + }
> + } else {
> + ret = i915_perf_get_oa_config(i915, config_id, &oa_config, NULL);
> + }
> +
> + mutex_unlock(&i915->perf.metrics_lock);
> +
> + if (ret || !oa_config)
> + return -ENOENT;
> +
> + if (__copy_from_user(&user_config, user_config_ptr,
> + sizeof(user_config))) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = can_copy_perf_config_registers_or_number(user_config.n_boolean_regs,
> + user_config.boolean_regs_ptr,
> + oa_config->b_counter_regs_len);
> + if (ret)
> + goto out;
> +
> + ret = can_copy_perf_config_registers_or_number(user_config.n_flex_regs,
> + user_config.flex_regs_ptr,
> + oa_config->flex_regs_len);
> + if (ret)
> + goto out;
> +
> + ret = can_copy_perf_config_registers_or_number(user_config.n_mux_regs,
> + user_config.mux_regs_ptr,
> + oa_config->mux_regs_len);
> + if (ret)
> + goto out;
> +
> + ret = copy_perf_config_registers_or_number(oa_config->b_counter_regs,
> + oa_config->b_counter_regs_len,
> + user_config.boolean_regs_ptr,
> + &user_config.n_boolean_regs);
> + if (ret)
> + goto out;
> +
> + ret = copy_perf_config_registers_or_number(oa_config->flex_regs,
> + oa_config->flex_regs_len,
> + user_config.flex_regs_ptr,
> + &user_config.n_flex_regs);
> + if (ret)
> + goto out;
> +
> + ret = copy_perf_config_registers_or_number(oa_config->mux_regs,
> + oa_config->mux_regs_len,
> + user_config.mux_regs_ptr,
> + &user_config.n_mux_regs);
> + if (ret)
> + goto out;
> +
> + memcpy(user_config.uuid, oa_config->uuid, sizeof(user_config.uuid));
> +
> + if (__copy_to_user(user_config_ptr, &user_config,
> + sizeof(user_config))) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = total_size;
> +
> +out:
> + i915_oa_config_put(oa_config);
Much more comfortable with all the user access out from under the locks.
> +
> + return ret;
> +}
> +
> +static int query_perf_config_list(struct drm_i915_private *i915,
> + struct drm_i915_query_item *query_item)
> +{
> + struct drm_i915_query_perf_config __user *user_query_config_ptr =
> + u64_to_user_ptr(query_item->data_ptr);
> + struct i915_oa_config *oa_config;
> + u32 flags, total_size;
> + u64 n_configs;
> + int ret, id;
> +
> + if (!i915->perf.initialized)
> + return -ENODEV;
> +
> + /* Count the default test configuration */
> + n_configs = i915->perf.n_metrics + 1;
> + total_size = sizeof(struct drm_i915_query_perf_config) +
> + sizeof(u64) * n_configs;
> +
> + if (query_item->length == 0)
> + return total_size;
> +
> + if (query_item->length < total_size) {
> + DRM_DEBUG("Invalid query config list item size=%u expected=%u\n",
> + query_item->length, total_size);
> + return -EINVAL;
> + }
> +
> + if (!access_ok(user_query_config_ptr, total_size))
> + return -EFAULT;
> +
> + if (__get_user(flags, &user_query_config_ptr->flags))
> + return -EFAULT;
> +
> + if (flags != 0)
> + return -EINVAL;
> +
> + if (__put_user(n_configs, &user_query_config_ptr->config))
> + return -EFAULT;
> +
> + if (__put_user((u64)1ULL, &user_query_config_ptr->data[0]))
> + return -EFAULT;
> +
> + ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
> + if (ret)
> + return ret;
> +
> + n_configs = 1;
> + idr_for_each_entry(&i915->perf.metrics_idr, oa_config, id) {
> + u64 __user *item =
> + u64_to_user_ptr(query_item->data_ptr +
> + sizeof(struct drm_i915_query_perf_config) +
> + n_configs * sizeof(u64));
> +
> + if (__put_user((u64)id, item)) {
Oh, that is a nuisance.
It would be easy to avoid the lock here by using a temporary kmalloc,
and this should not be a hot path...
(A stitch in time is worth nine :)
With a temporary copy,
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Probably best to grab Tvrtko to see if he can spot potential issues with
i915_query usage.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
