[Intel-gfx] [PATCH 2/2] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface

Lionel Landwerlin lionel.g.landwerlin at intel.com
Fri Jul 14 10:25:33 UTC 2017


On 13/07/17 15:42, Matthew Auld wrote:
> On 13 July 2017 at 12:22, Lionel Landwerlin
> <lionel.g.landwerlin at intel.com> wrote:
>> From: Matthew Auld <matthew.auld at intel.com>
>>
>> The motivation behind this new interface is expose at runtime the
>> creation of new OA configs which can be used as part of the i915 perf
>> open interface. This will enable the kernel to learn new configs which
>> may be experimental, or otherwise not part of the core set currently
>> available through the i915 perf interface.
>>
>> This will relieve the kernel from holding all the possible configs, so
>> we can leave only the test configs here.
>>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> Signed-off-by: Andrzej Datczuk <andrzej.datczuk at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c  |   2 +
>>   drivers/gpu/drm/i915/i915_drv.h  |  25 +++
>>   drivers/gpu/drm/i915/i915_perf.c | 351 ++++++++++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/i915_reg.h  |   2 +
>>   include/uapi/drm/i915_drm.h      |  24 +++
>>   5 files changed, 403 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index d310d8245dca..a3d339670ec1 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -2730,6 +2730,8 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>>          DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
>>          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),
>>   };
>>
>>   static struct drm_driver driver = {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 0c45a89d165e..3d62276d9fad 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2037,6 +2037,25 @@ struct i915_perf_stream {
>>    */
>>   struct i915_oa_ops {
>>          /**
>> +        * @is_valid_b_counter_reg: Validates register's address for
>> +        * programming boolean counters for a particular platform.
>> +        */
>> +       bool (*is_valid_b_counter_reg)(struct drm_i915_private *dev_priv,
>> +                                      u32 addr);
>> +
>> +       /**
>> +        * @is_valid_mux_reg: Validates register's address for programming mux
>> +        * for a particular platform.
>> +        */
>> +       bool (*is_valid_mux_reg)(struct drm_i915_private *dev_priv, u32 addr);
>> +
>> +       /**
>> +        * @is_valid_flex_reg: Validates register's address for programming
>> +        * flex EU filtering for a particular platform.
>> +        */
>> +       bool (*is_valid_flex_reg)(struct drm_i915_private *dev_priv, u32 addr);
>> +
>> +       /**
>>           * @init_oa_buffer: Resets the head and tail pointers of the
>>           * circular buffer for periodic OA reports.
>>           *
>> @@ -2427,6 +2446,8 @@ struct drm_i915_private {
>>                  struct mutex lock;
>>                  struct list_head streams;
>>
>> +               struct idr metrics_idr;
>> +
>>                  struct {
>>                          struct i915_perf_stream *exclusive_stream;
>>
>> @@ -3595,6 +3616,10 @@ i915_gem_context_lookup_timeline(struct i915_gem_context *ctx,
>>
>>   int i915_perf_open_ioctl(struct drm_device *dev, void *data,
>>                           struct drm_file *file);
>> +int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
>> +                              struct drm_file *file);
>> +int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
>> +                                 struct drm_file *file);
>>   void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>>                              struct i915_gem_context *ctx,
>>                              uint32_t *reg_state);
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index d4647b3d8119..5f3d544a93cf 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -2652,7 +2652,7 @@ static struct i915_oa_config *get_oa_config(struct drm_i915_private *dev_priv,
>>          if (metrics_set == 1)
>>                  return &dev_priv->perf.oa.test_config;
>>
>> -       return NULL;
>> +       return idr_find(&dev_priv->perf.metrics_idr, metrics_set);
>>   }
>>
>>   /**
>> @@ -2913,6 +2913,7 @@ void i915_perf_register(struct drm_i915_private *dev_priv)
>>                                   &dev_priv->perf.oa.test_config.sysfs_metric);
>>          if (ret)
>>                  goto sysfs_error;
>> +
>>          goto exit;
>>
>>   sysfs_error:
>> @@ -2944,6 +2945,333 @@ void i915_perf_unregister(struct drm_i915_private *dev_priv)
>>          dev_priv->perf.metrics_kobj = NULL;
>>   }
>>
>> +static bool gen8_is_valid_flex_addr(struct drm_i915_private *dev_priv, u32 addr)
>> +{
>> +       static const i915_reg_t flex_eu_regs[] = {
>> +               EU_PERF_CNTL0,
>> +               EU_PERF_CNTL1,
>> +               EU_PERF_CNTL2,
>> +               EU_PERF_CNTL3,
>> +               EU_PERF_CNTL4,
>> +               EU_PERF_CNTL5,
>> +               EU_PERF_CNTL6,
>> +       };
>> +       int i;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(flex_eu_regs); i++) {
>> +               if (flex_eu_regs[i].reg == addr)
>> +                       return true;
>> +       }
>> +       return false;
>> +}
>> +
>> +static bool gen7_is_valid_b_counter_addr(struct drm_i915_private *dev_priv, u32 addr)
>> +{
>> +       return (addr >= 0x2380 && addr <= 0x27ac);
>> +}
>> +
>> +static bool gen7_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
>> +{
>> +       return addr == NOA_WRITE.reg ||
>> +               (addr >= 0xd0c && addr <= 0xd3c) ||
>> +               (addr >= 0x25100 && addr <= 0x2FB9C);
>> +}
>> +
>> +static bool hsw_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
>> +{
>> +       return (addr >= 0x25100 && addr <= 0x2FF90) ||
>> +               gen7_is_valid_mux_addr(dev_priv, addr);
>> +}
>> +
>> +static bool chv_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
>> +{
>> +       return (addr >= 0x182300 && addr <= 0x1823A4) ||
>> +               gen7_is_valid_mux_addr(dev_priv, addr);
>> +}
>> +
>> +static struct i915_oa_reg *alloc_oa_regs(struct drm_i915_private *dev_priv,
>> +                                        bool (*is_valid)(struct drm_i915_private *dev_priv, u32 addr),
>> +                                        u32 __user *regs,
>> +                                        u32 n_regs)
>> +{
>> +       struct i915_oa_reg *oa_regs;
>> +       int err, i;
>> +
>> +       if (!n_regs)
>> +               return NULL;
>> +
>> +       /* No is_valid function means we're not allowing any register to be programmed. */
>> +       GEM_BUG_ON(!is_valid);
>> +       if (!is_valid)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       oa_regs = kmalloc(sizeof(*oa_regs) * n_regs, GFP_KERNEL);
>> +       if (!oa_regs)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       for (i = 0; i < n_regs; i++) {
> I think in a bunch of places we mix u32/int for n_regs, make it
> consistent now that user-space can set this?

Done.

>
>> +               u32 addr, value;
>> +
>> +               err = get_user(addr, regs);
>> +               if (err)
>> +                       goto addr_err;
>> +
>> +               if (!is_valid(dev_priv, addr)) {
>> +                       DRM_DEBUG("Invalid oa_reg address: %X\n", addr);
>> +                       err = -EINVAL;
>> +                       goto addr_err;
>> +               }
>> +
>> +               err = get_user(value, regs + 1);
>> +               if (err)
>> +                       goto addr_err;
>> +
>> +               oa_regs[i].addr = _MMIO(addr);
>> +               oa_regs[i].value = value;
>> +
>> +               regs += 2;
>> +       }
>> +
>> +       return oa_regs;
>> +
>> +addr_err:
>> +       kfree(oa_regs);
>> +       return ERR_PTR(err);
>> +}
>> +
>> +static ssize_t show_dynamic_id(struct device *dev,
>> +                              struct device_attribute *attr,
>> +                              char *buf)
>> +{
>> +       struct i915_oa_config *oa_config =
>> +               container_of(attr, typeof(*oa_config), sysfs_metric_id);
>> +
>> +       return sprintf(buf, "%d\n", oa_config->id);
>> +}
>> +
>> +static int create_dynamic_oa_sysfs_entry(struct drm_i915_private *dev_priv,
>> +                                        struct i915_oa_config *oa_config)
>> +{
>> +       oa_config->sysfs_metric_id.attr.name = "id";
>> +       oa_config->sysfs_metric_id.attr.mode = S_IRUGO;
>> +       oa_config->sysfs_metric_id.show = show_dynamic_id;
>> +       oa_config->sysfs_metric_id.store = NULL;
>> +
>> +       oa_config->attrs[0] = &oa_config->sysfs_metric_id.attr;
>> +       oa_config->attrs[1] = NULL;
>> +
>> +       oa_config->sysfs_metric.name = oa_config->uuid;
>> +       oa_config->sysfs_metric.attrs = oa_config->attrs;
>> +
>> +       return sysfs_create_group(dev_priv->perf.metrics_kobj,
>> +                                 &oa_config->sysfs_metric);
>> +}
>> +
>> +static int destroy_config(int id, void *p, void *data)
>> +{
>> +       struct drm_i915_private *dev_priv = data;
>> +       struct i915_oa_config *oa_config = p;
>> +
>> +       sysfs_remove_group(dev_priv->perf.metrics_kobj,
>> +                          &oa_config->sysfs_metric);
>> +       kfree(oa_config->flex_regs);
>> +       kfree(oa_config->b_counter_regs);
>> +       kfree(oa_config->mux_regs);
>> +       kfree(oa_config);
>> +
>> +       return 0;
>> +}
>> +
>> +int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
>> +                              struct drm_file *file)
>> +{
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct drm_i915_perf_oa_config *args = data;
>> +       struct i915_oa_config *oa_config, *tmp;
>> +       unsigned int x1, x2, x3, x4, x5;
>> +       int err, id;
>> +
>> +       if (!dev_priv->perf.initialized) {
>> +               DRM_DEBUG("i915 perf interface not available for this system\n");
>> +               return -ENOTSUPP;
>> +       }
>> +
>> +       if (!dev_priv->perf.metrics_kobj) {
>> +               DRM_DEBUG("OA metrics weren't advertised via sysfs\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!capable(CAP_SYS_ADMIN)) {
>> +               DRM_DEBUG("Insufficient privileges to add i915 OA config\n");
>> +               return -EACCES;
>> +       }
>> +
>> +       if ((!args->mux_regs || !args->n_mux_regs) &&
>> +           (!args->boolean_regs || !args->n_boolean_regs) &&
>> +           (!args->flex_regs || !args->n_flex_regs)) {
>> +               DRM_DEBUG("No OA registers given\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       oa_config = kzalloc(sizeof(*oa_config), GFP_KERNEL);
>> +       if (!oa_config) {
>> +               DRM_DEBUG("Failed to allocate memory for the OA config\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       err = strncpy_from_user(oa_config->uuid, u64_to_user_ptr(args->uuid),
>> +                               sizeof(oa_config->uuid));
> I think we'll need to do:
>
> struct i915_oa_config {
>      char uuid[UUID_STRING_LEN+1];
> };
>
> strncpy_from_user(oa_config->uuid, u64_to_user_ptr(args->uuid),
> UUID_STRING_LEN);

Done.

> Hmm, alternatively we might want to consider using uuid_t, maybe just
> for the user-space portion, once that lands in tip, not sure how
> worthwhile that would be though...
>
No uuid_t for now.



More information about the Intel-gfx mailing list