Re: [PATCH v8 13/13] drm/i915: add support for perf configuration queries

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

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@intel.com>
> ---
>  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@chris-wilson.co.uk>

Probably best to grab Tvrtko to see if he can spot potential issues with
i915_query usage.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx