From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lionel Landwerlin Subject: Re: [PATCH 1/2] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface Date: Thu, 13 Jul 2017 12:25:40 +0100 Message-ID: <299190af-5e87-49cb-c6d5-a6ec97f92f28@intel.com> References: <20170707170838.9492-1-lionel.g.landwerlin@intel.com> <20170707170838.9492-2-lionel.g.landwerlin@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0806434591==" Return-path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6AE186E5C6 for ; Thu, 13 Jul 2017 11:25:43 +0000 (UTC) In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Matthew Auld Cc: Intel Graphics Development , Andrzej Datczuk , Matthew Auld List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============0806434591== Content-Type: multipart/alternative; boundary="------------EA01DC6AD336F2D77432B6FA" Content-Language: en-US This is a multi-part message in MIME format. --------------EA01DC6AD336F2D77432B6FA Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 08/07/17 00:53, Matthew Auld wrote: > On 7 July 2017 at 18:08, Lionel Landwerlin > wrote: >> From: Matthew Auld >> >> 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 >> Signed-off-by: Lionel Landwerlin >> Signed-off-by: Andrzej Datczuk >> --- >> drivers/gpu/drm/i915/i915_drv.c | 2 + >> drivers/gpu/drm/i915/i915_drv.h | 30 +++ >> drivers/gpu/drm/i915/i915_perf.c | 391 ++++++++++++++++++++++++++++++++++++++- >> drivers/gpu/drm/i915/i915_reg.h | 2 + >> include/uapi/drm/i915_drm.h | 21 +++ >> 5 files changed, 441 insertions(+), 5 deletions(-) >> >> 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 81cd21ecfa7d..ce733c2db963 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2009,6 +2009,11 @@ struct i915_perf_stream { >> * type of configured stream. >> */ >> const struct i915_perf_stream_ops *ops; >> + >> + /** >> + * @metrics_set: The ID of the config used by the stream. >> + */ >> + int metrics_set; >> }; >> >> /** >> @@ -2016,6 +2021,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. >> * >> @@ -2413,6 +2437,8 @@ struct drm_i915_private { >> struct mutex lock; >> struct list_head streams; >> >> + struct idr metrics_idr; >> + >> struct { >> struct i915_perf_stream *exclusive_stream; >> >> @@ -3589,6 +3615,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 d9f77a4d85db..92cb6a7568e7 100644 >> --- a/drivers/gpu/drm/i915/i915_perf.c >> +++ b/drivers/gpu/drm/i915/i915_perf.c >> @@ -357,6 +357,23 @@ struct perf_open_properties { >> int oa_period_exponent; >> }; >> >> +struct i915_oa_dynamic_config >> +{ >> + char guid[40]; > s/guid/uuid/ to be consistent with elsewhere? done. >> + int id; >> + >> + const struct i915_oa_reg *mux_regs; >> + int mux_regs_len; >> + const struct i915_oa_reg *b_counter_regs; >> + int b_counter_regs_len; >> + const struct i915_oa_reg *flex_regs; >> + int flex_regs_len; >> + >> + struct attribute_group sysfs_metric; >> + struct attribute *attrs[2]; >> + struct device_attribute sysfs_metric_id; >> +}; >> + >> static u32 gen8_oa_hw_tail_read(struct drm_i915_private *dev_priv) >> { >> return I915_READ(GEN8_OATAILPTR) & GEN8_OATAILPTR_MASK; >> @@ -1451,11 +1468,39 @@ static void config_oa_regs(struct drm_i915_private *dev_priv, >> } >> } >> >> +int select_dynamic_metric_set(struct drm_i915_private *dev_priv) > Should be static. Done. >> +{ >> + struct i915_oa_dynamic_config *oa_config; >> + >> + oa_config = idr_find(&dev_priv->perf.metrics_idr, >> + dev_priv->perf.oa.metrics_set); >> + if (!oa_config) { >> + return -EINVAL; >> + } >> + >> + dev_priv->perf.oa.n_mux_configs = 1; >> + dev_priv->perf.oa.mux_regs[0] = oa_config->mux_regs; >> + dev_priv->perf.oa.mux_regs_lens[0] = oa_config->mux_regs_len; >> + >> + dev_priv->perf.oa.b_counter_regs = oa_config->b_counter_regs; >> + dev_priv->perf.oa.b_counter_regs_len = oa_config->b_counter_regs_len; >> + >> + dev_priv->perf.oa.flex_regs = oa_config->flex_regs; >> + dev_priv->perf.oa.flex_regs_len = oa_config->flex_regs_len; >> + >> + return 0; >> +} >> + >> static int hsw_enable_metric_set(struct drm_i915_private *dev_priv) >> { >> - int ret = i915_oa_select_metric_set_hsw(dev_priv); >> + int ret; >> int i; >> >> + if (dev_priv->perf.oa.metrics_set > dev_priv->perf.oa.n_builtin_sets) >> + ret = select_dynamic_metric_set(dev_priv); >> + else >> + ret = i915_oa_select_metric_set_hsw(dev_priv); >> + >> if (ret) >> return ret; >> >> @@ -1776,8 +1821,12 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv, >> >> static int gen8_enable_metric_set(struct drm_i915_private *dev_priv) >> { >> - int ret = dev_priv->perf.oa.ops.select_metric_set(dev_priv); >> - int i; >> + int ret, i; >> + >> + if (dev_priv->perf.oa.metrics_set > dev_priv->perf.oa.n_builtin_sets) >> + ret = select_dynamic_metric_set(dev_priv); >> + else >> + ret = dev_priv->perf.oa.ops.select_metric_set(dev_priv); >> >> if (ret) >> return ret; >> @@ -2055,7 +2104,7 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream, >> dev_priv->perf.oa.oa_buffer.format = >> dev_priv->perf.oa.oa_formats[props->oa_format].format; >> >> - dev_priv->perf.oa.metrics_set = props->metrics_set; >> + dev_priv->perf.oa.metrics_set = stream->metrics_set = props->metrics_set; >> >> dev_priv->perf.oa.periodic = props->oa_periodic; >> if (dev_priv->perf.oa.periodic) >> @@ -2713,7 +2762,9 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv, >> break; >> case DRM_I915_PERF_PROP_OA_METRICS_SET: >> if (value == 0 || >> - value > dev_priv->perf.oa.n_builtin_sets) { >> + (value > dev_priv->perf.oa.n_builtin_sets && >> + idr_find(&dev_priv->perf.metrics_idr, >> + value) == NULL)) { >> DRM_DEBUG("Unknown OA metric set ID\n"); >> return -EINVAL; >> } >> @@ -2957,6 +3008,321 @@ 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) >> +{ >> + int err; >> + int i; >> + struct i915_oa_reg *oa_regs; > Maybe reverse the order of these? Done. >> + >> + if (!n_regs) >> + return NULL; > Caller expects ERR_PTR. It's not an error, there is just no registers to allocate. >> + >> + if (!is_valid) >> + return ERR_PTR(-EINVAL); > GEM_BUG_ON(!is_valid); Indeed! >> + >> + oa_regs = kmalloc(sizeof(*oa_regs) * n_regs, GFP_KERNEL); >> + if (!oa_regs) >> + return ERR_PTR(-ENOMEM); >> + >> + for (i = 0; i < n_regs; i++) { >> + int ret; >> + u32 addr, value; >> + >> + ret = get_user(addr, regs); >> + if (ret) { > Get rid of ret, and use err here and below? Done. >> + err = ret; >> + goto addr_err; >> + } >> + >> + if (!is_valid(dev_priv, addr)) { >> + DRM_ERROR("Invalid oa_reg address: %X\n", addr); >> + err = -EINVAL; >> + goto addr_err; >> + } >> + >> + ret = get_user(value, regs + 1); >> + if (ret) { >> + err = ret; >> + goto value_err; >> + } >> + >> + oa_regs[i].addr = _MMIO(addr); >> + oa_regs[i].value = value; >> + >> + regs += 2; >> + } >> + >> + return oa_regs; >> + >> +addr_err: >> +value_err: > Squash these? Done. >> + 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_dynamic_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_dynamic_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->guid; >> + oa_config->sysfs_metric.attrs = oa_config->attrs; >> + >> + return sysfs_create_group(dev_priv->perf.metrics_kobj, >> + &oa_config->sysfs_metric); >> +} >> + >> +int i915_perf_add_config_ioctl(struct drm_device *dev, void *data, >> + struct drm_file *file) >> +{ >> + int err; > Move this and id to the end of the block? Done. >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct drm_i915_perf_oa_config *args = data; >> + struct i915_oa_dynamic_config *oa_config, *tmp; >> + int id; >> + unsigned int x1, x2, x3, x4, x5; >> + >> + if (!dev_priv->perf.initialized) { >> + DRM_DEBUG("i915 perf interface not available for this system\n"); >> + return -ENOTSUPP; >> + } >> + > if (!dev_priv->perf.metrics_kobj) { > > } Done. >> + if (!capable(CAP_SYS_ADMIN)) { >> + DRM_ERROR("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_ERROR("No OA registers given\n"); >> + return -EINVAL; >> + } >> + >> + oa_config = kzalloc(sizeof(*oa_config), GFP_KERNEL); >> + if (!oa_config) { >> + DRM_ERROR("Failed to allocate memory for the OA config\n"); >> + return -ENOMEM; >> + } >> + >> + err = strncpy_from_user(oa_config->guid, u64_to_user_ptr(args->uuid), >> + sizeof(oa_config->guid)); >> + if (err < 0) { >> + DRM_ERROR("Failed to copy uuid from OA config\n"); >> + goto uuid_err; >> + } >> + >> + if (sscanf(oa_config->guid, "%08x-%04x-%04x-%04x-%012x", &x1, &x2, &x3, >> + &x4, &x5) != 5) { >> + DRM_ERROR("Invalid uuid format for OA config\n"); >> + err = -EINVAL; >> + goto uuid_err; >> + } >> + >> + oa_config->mux_regs_len = args->n_mux_regs; >> + oa_config->mux_regs = >> + alloc_oa_regs(dev_priv, >> + dev_priv->perf.oa.ops.is_valid_mux_reg, >> + u64_to_user_ptr(args->mux_regs), >> + args->n_mux_regs); >> + >> + if (IS_ERR(oa_config->mux_regs)) { >> + DRM_ERROR("Failed to create OA config for mux_regs\n"); >> + err = PTR_ERR(oa_config->mux_regs); >> + goto mux_err; >> + } >> + >> + oa_config->b_counter_regs_len = args->n_boolean_regs; >> + oa_config->b_counter_regs = >> + alloc_oa_regs(dev_priv, >> + dev_priv->perf.oa.ops.is_valid_b_counter_reg, >> + u64_to_user_ptr(args->boolean_regs), >> + args->n_boolean_regs); >> + >> + if (IS_ERR(oa_config->b_counter_regs)) { >> + DRM_ERROR("Failed to create OA config for b_counter_regs\n"); >> + err = PTR_ERR(oa_config->b_counter_regs); >> + goto boolean_err; >> + } >> + >> + if (INTEL_GEN(dev_priv) < 8) { >> + if (args->n_flex_regs != 0) >> + goto flex_err; >> + } else { >> + oa_config->flex_regs_len = args->n_flex_regs; >> + oa_config->flex_regs = >> + alloc_oa_regs(dev_priv, >> + dev_priv->perf.oa.ops.is_valid_flex_reg, >> + u64_to_user_ptr(args->flex_regs), >> + args->n_flex_regs); >> + >> + if (IS_ERR(oa_config->flex_regs)) { >> + DRM_ERROR("Failed to create OA config for flex_regs\n"); >> + err = PTR_ERR(oa_config->flex_regs); >> + goto flex_err; >> + } >> + } >> + >> + err = i915_mutex_lock_interruptible(dev); >> + if (err) >> + goto lock_err; >> + >> + idr_for_each_entry(&dev_priv->perf.metrics_idr, tmp, id) { >> + if (!strcmp(tmp->guid, oa_config->guid)) { >> + DRM_ERROR("OA config already exists with this uuid\n"); >> + err = -EADDRINUSE; >> + goto sysfs_err; >> + } >> + } >> + >> + err = create_dynamic_oa_sysfs_entry(dev_priv, oa_config); >> + if (err) { >> + DRM_ERROR("Failed to create sysfs entry for OA config\n"); >> + goto sysfs_err; >> + } >> + >> + oa_config->id = idr_alloc(&dev_priv->perf.metrics_idr, >> + oa_config, >> + dev_priv->perf.oa.n_builtin_sets + 1, >> + 0, GFP_KERNEL); > Hmm, I think we need to check for the possible error here and unwind > accordingly. Done. >> + >> + mutex_unlock(&dev->struct_mutex); >> + >> + return oa_config->id; >> + >> +sysfs_err: >> + mutex_unlock(&dev->struct_mutex); >> +lock_err: >> + kfree(oa_config->flex_regs); >> +flex_err: >> + kfree(oa_config->b_counter_regs); >> +boolean_err: >> + kfree(oa_config->mux_regs); >> +mux_err: >> +uuid_err: > Squash these? Done. >> + kfree(oa_config); >> + >> + DRM_ERROR("Failed to add new OA config\n"); >> + return err; >> +} >> + >> +int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data, >> + struct drm_file *file) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + u64 *arg = data; > int id = *data; data is void*, I don't think that's going to work. >> + struct i915_oa_dynamic_config *oa_config; >> + int ret; >> + >> + if (!dev_priv->perf.initialized) { >> + DRM_DEBUG("i915 perf interface not available for this system\n"); >> + return -ENOTSUPP; >> + } >> + >> + if (!capable(CAP_SYS_ADMIN)) { >> + DRM_ERROR("Insufficient privileges to remove i915 OA config\n"); >> + return -EACCES; >> + } >> + >> + ret = i915_mutex_lock_interruptible(dev); >> + if (ret) >> + goto lock_err; >> + >> + if (dev_priv->perf.oa.exclusive_stream && >> + dev_priv->perf.oa.exclusive_stream->metrics_set == *arg) { >> + DRM_ERROR("Failed to remove config in use\n"); >> + ret = -EADDRINUSE; >> + goto config_err; >> + } >> + >> + oa_config = idr_find(&dev_priv->perf.metrics_idr, *arg); >> + if (!oa_config) { >> + DRM_ERROR("Failed to remove unknown config\n"); >> + ret = -EINVAL; >> + goto config_err; >> + } > GEM_BUG_ON(id != oa_config->id); Done. >> + >> + idr_remove(&dev_priv->perf.metrics_idr, *arg); >> + >> + sysfs_remove_group(dev_priv->perf.metrics_kobj, >> + &oa_config->sysfs_metric); >> + if (oa_config->flex_regs) >> + kfree(oa_config->flex_regs); >> + if (oa_config->b_counter_regs) >> + kfree(oa_config->b_counter_regs); >> + if (oa_config->mux_regs) >> + kfree(oa_config->mux_regs); > Just let kfree check for NULL? Done. >> + kfree(oa_config); >> + >> +config_err: >> + mutex_unlock(&dev->struct_mutex); >> +lock_err: >> + return ret; >> +} >> + >> static struct ctl_table oa_table[] = { >> { >> .procname = "perf_stream_paranoid", >> @@ -3011,8 +3377,14 @@ static struct ctl_table dev_root[] = { >> void i915_perf_init(struct drm_i915_private *dev_priv) >> { >> dev_priv->perf.oa.n_builtin_sets = 0; >> + idr_init(&dev_priv->perf.metrics_idr); > We should also throw an idr_destroy in perf_fini. Maybe we also need > to cleanup any dynamic configs which haven't been removed also? Indeed! Thanks! >> if (IS_HASWELL(dev_priv)) { >> + dev_priv->perf.oa.ops.is_valid_b_counter_reg = >> + gen7_is_valid_b_counter_addr; >> + dev_priv->perf.oa.ops.is_valid_mux_reg = >> + hsw_is_valid_mux_addr; >> + dev_priv->perf.oa.ops.is_valid_flex_reg = NULL; >> dev_priv->perf.oa.ops.init_oa_buffer = gen7_init_oa_buffer; >> dev_priv->perf.oa.ops.enable_metric_set = hsw_enable_metric_set; >> dev_priv->perf.oa.ops.disable_metric_set = hsw_disable_metric_set; >> @@ -3036,6 +3408,13 @@ void i915_perf_init(struct drm_i915_private *dev_priv) >> * execlist mode by default. >> */ >> >> + dev_priv->perf.oa.ops.is_valid_b_counter_reg = >> + gen7_is_valid_b_counter_addr; >> + dev_priv->perf.oa.ops.is_valid_mux_reg = >> + gen7_is_valid_mux_addr; >> + dev_priv->perf.oa.ops.is_valid_flex_reg = >> + gen8_is_valid_flex_addr; >> + >> if (IS_GEN8(dev_priv)) { >> dev_priv->perf.oa.ctx_oactxctrl_offset = 0x120; >> dev_priv->perf.oa.ctx_flexeu0_offset = 0x2ce; >> @@ -3050,6 +3429,8 @@ void i915_perf_init(struct drm_i915_private *dev_priv) >> dev_priv->perf.oa.ops.select_metric_set = >> i915_oa_select_metric_set_bdw; >> } else if (IS_CHERRYVIEW(dev_priv)) { >> + dev_priv->perf.oa.ops.is_valid_mux_reg = >> + chv_is_valid_mux_addr; >> dev_priv->perf.oa.n_builtin_sets = >> i915_oa_n_builtin_metric_sets_chv; >> dev_priv->perf.oa.ops.select_metric_set = >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 21ab12f4e72a..433fcd7beb72 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -732,6 +732,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) >> #define GDT_CHICKEN_BITS _MMIO(0x9840) >> #define GT_NOA_ENABLE 0x00000080 >> >> +#define NOA_WRITE _MMIO(0x9888) >> + >> /* >> * OA Boolean state >> */ >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >> index 7ccbd6a2bbe0..e2ff47732303 100644 >> --- a/include/uapi/drm/i915_drm.h >> +++ b/include/uapi/drm/i915_drm.h >> @@ -260,6 +260,8 @@ typedef struct _drm_i915_sarea { >> #define DRM_I915_GEM_CONTEXT_GETPARAM 0x34 >> #define DRM_I915_GEM_CONTEXT_SETPARAM 0x35 >> #define DRM_I915_PERF_OPEN 0x36 >> +#define DRM_I915_PERF_ADD_CONFIG 0x37 >> +#define DRM_I915_PERF_REMOVE_CONFIG 0x38 >> >> #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) >> @@ -315,6 +317,8 @@ typedef struct _drm_i915_sarea { >> #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param) >> #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param) >> #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_IOWR(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config) >> +#define DRM_IOCTL_I915_PERF_REMOVE_CONFIG DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64) >> >> /* Allow drivers to submit batchbuffers directly to hardware, relying >> * on the security mechanisms provided by hardware. >> @@ -1467,6 +1471,23 @@ enum drm_i915_perf_record_type { >> DRM_I915_PERF_RECORD_MAX /* non-ABI */ >> }; >> >> +/** >> + * Structure to upload perf dynamic configuration into the kernel. >> + */ >> +struct drm_i915_perf_oa_config { >> + /* string formatted like "%08x-%04x-%04x-%04x-%012x" **/ > /** String formatted like: "%08x-%04x-%04x-%04x-%012x" */ Done. > >> + __u64 __user uuid; >> + >> + __u32 n_mux_regs; >> + __u64 __user mux_regs; >> + >> + __u32 n_boolean_regs; >> + __u64 __user boolean_regs; >> + >> + __u32 n_flex_regs; >> + __u64 __user flex_regs; >> +}; >> + >> #if defined(__cplusplus) >> } >> #endif >> -- >> 2.13.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx --------------EA01DC6AD336F2D77432B6FA Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit
On 08/07/17 00:53, Matthew Auld wrote:
On 7 July 2017 at 18:08, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:

From: Matthew Auld <matthew.auld@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@intel.com>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Andrzej Datczuk <andrzej.datczuk@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  |   2 +
 drivers/gpu/drm/i915/i915_drv.h  |  30 +++
 drivers/gpu/drm/i915/i915_perf.c | 391 ++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h  |   2 +
 include/uapi/drm/i915_drm.h      |  21 +++
 5 files changed, 441 insertions(+), 5 deletions(-)

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 81cd21ecfa7d..ce733c2db963 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2009,6 +2009,11 @@ struct i915_perf_stream {
         * type of configured stream.
         */
        const struct i915_perf_stream_ops *ops;
+
+       /**
+        * @metrics_set: The ID of the config used by the stream.
+        */
+       int metrics_set;
 };

 /**
@@ -2016,6 +2021,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.
         *
@@ -2413,6 +2437,8 @@ struct drm_i915_private {
                struct mutex lock;
                struct list_head streams;

+               struct idr metrics_idr;
+
                struct {
                        struct i915_perf_stream *exclusive_stream;

@@ -3589,6 +3615,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 d9f77a4d85db..92cb6a7568e7 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -357,6 +357,23 @@ struct perf_open_properties {
        int oa_period_exponent;
 };

+struct i915_oa_dynamic_config
+{
+       char guid[40];

s/guid/uuid/ to be consistent with elsewhere?

done.


      

        
+       int id;
+
+       const struct i915_oa_reg *mux_regs;
+       int mux_regs_len;
+       const struct i915_oa_reg *b_counter_regs;
+       int b_counter_regs_len;
+       const struct i915_oa_reg *flex_regs;
+       int flex_regs_len;
+
+       struct attribute_group sysfs_metric;
+       struct attribute *attrs[2];
+       struct device_attribute sysfs_metric_id;
+};
+
 static u32 gen8_oa_hw_tail_read(struct drm_i915_private *dev_priv)
 {
        return I915_READ(GEN8_OATAILPTR) & GEN8_OATAILPTR_MASK;
@@ -1451,11 +1468,39 @@ static void config_oa_regs(struct drm_i915_private *dev_priv,
        }
 }

+int select_dynamic_metric_set(struct drm_i915_private *dev_priv)


      

      
Should be static.

    

    

    Done.

    

    

      

      

        
+{
+       struct i915_oa_dynamic_config *oa_config;
+
+       oa_config = idr_find(&dev_priv->perf.metrics_idr,
+                            dev_priv->perf.oa.metrics_set);
+       if (!oa_config) {
+               return -EINVAL;
+       }
+
+       dev_priv->perf.oa.n_mux_configs = 1;
+       dev_priv->perf.oa.mux_regs[0] = oa_config->mux_regs;
+       dev_priv->perf.oa.mux_regs_lens[0] = oa_config->mux_regs_len;
+
+       dev_priv->perf.oa.b_counter_regs = oa_config->b_counter_regs;
+       dev_priv->perf.oa.b_counter_regs_len = oa_config->b_counter_regs_len;
+
+       dev_priv->perf.oa.flex_regs = oa_config->flex_regs;
+       dev_priv->perf.oa.flex_regs_len = oa_config->flex_regs_len;
+
+       return 0;
+}
+
 static int hsw_enable_metric_set(struct drm_i915_private *dev_priv)
 {
-       int ret = i915_oa_select_metric_set_hsw(dev_priv);
+       int ret;
        int i;

+       if (dev_priv->perf.oa.metrics_set > dev_priv->perf.oa.n_builtin_sets)
+               ret = select_dynamic_metric_set(dev_priv);
+       else
+               ret = i915_oa_select_metric_set_hsw(dev_priv);
+
        if (ret)
                return ret;

@@ -1776,8 +1821,12 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,

 static int gen8_enable_metric_set(struct drm_i915_private *dev_priv)
 {
-       int ret = dev_priv->perf.oa.ops.select_metric_set(dev_priv);
-       int i;
+       int ret, i;
+
+       if (dev_priv->perf.oa.metrics_set > dev_priv->perf.oa.n_builtin_sets)
+               ret = select_dynamic_metric_set(dev_priv);
+       else
+               ret = dev_priv->perf.oa.ops.select_metric_set(dev_priv);

        if (ret)
                return ret;
@@ -2055,7 +2104,7 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
        dev_priv->perf.oa.oa_buffer.format =
                dev_priv->perf.oa.oa_formats[props->oa_format].format;

-       dev_priv->perf.oa.metrics_set = props->metrics_set;
+       dev_priv->perf.oa.metrics_set = stream->metrics_set = props->metrics_set;

        dev_priv->perf.oa.periodic = props->oa_periodic;
        if (dev_priv->perf.oa.periodic)
@@ -2713,7 +2762,9 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
                        break;
                case DRM_I915_PERF_PROP_OA_METRICS_SET:
                        if (value == 0 ||
-                           value > dev_priv->perf.oa.n_builtin_sets) {
+                           (value > dev_priv->perf.oa.n_builtin_sets &&
+                            idr_find(&dev_priv->perf.metrics_idr,
+                                     value) == NULL)) {
                                DRM_DEBUG("Unknown OA metric set ID\n");
                                return -EINVAL;
                        }
@@ -2957,6 +3008,321 @@ 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)
+{
+       int err;
+       int i;
+       struct i915_oa_reg *oa_regs;


      

      
Maybe reverse the order of these?

    

    

    Done.

    

    

      

      

        
+
+       if (!n_regs)
+               return NULL;


      

      
Caller expects ERR_PTR.

    

    

    It's not an error, there is just no registers to allocate.

    

    

      

      

        
+
+       if (!is_valid)
+               return ERR_PTR(-EINVAL);


      

      
GEM_BUG_ON(!is_valid);

    

    

    Indeed!

    

    

      

      

        
+
+       oa_regs = kmalloc(sizeof(*oa_regs) * n_regs, GFP_KERNEL);
+       if (!oa_regs)
+               return ERR_PTR(-ENOMEM);
+
+       for (i = 0; i < n_regs; i++) {
+               int ret;
+               u32 addr, value;
+
+               ret = get_user(addr, regs);
+               if (ret) {


      

      
Get rid of ret, and use err here and below?

    

    

    Done.

    

    

      

      

        
+                       err = ret;
+                       goto addr_err;
+               }
+
+               if (!is_valid(dev_priv, addr)) {
+                       DRM_ERROR("Invalid oa_reg address: %X\n", addr);
+                       err = -EINVAL;
+                       goto addr_err;
+               }
+
+               ret = get_user(value, regs + 1);
+               if (ret) {
+                       err = ret;
+                       goto value_err;
+               }
+
+               oa_regs[i].addr = _MMIO(addr);
+               oa_regs[i].value = value;
+
+               regs += 2;
+       }
+
+       return oa_regs;
+
+addr_err:
+value_err:


      

      
Squash these?

    

    

    Done.

    

    

      

      

        
+       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_dynamic_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_dynamic_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->guid;
+       oa_config->sysfs_metric.attrs = oa_config->attrs;
+
+       return sysfs_create_group(dev_priv->perf.metrics_kobj,
+                                 &oa_config->sysfs_metric);
+}
+
+int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
+                              struct drm_file *file)
+{
+       int err;


      

      
Move this and id to the end of the block?

    

    

    Done.

    

    

      

      

        
+       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_perf_oa_config *args = data;
+       struct i915_oa_dynamic_config *oa_config, *tmp;
+       int id;
+       unsigned int x1, x2, x3, x4, x5;
+
+       if (!dev_priv->perf.initialized) {
+               DRM_DEBUG("i915 perf interface not available for this system\n");
+               return -ENOTSUPP;
+       }
+


      

      
if (!dev_priv->perf.metrics_kobj) {

}

    

    

    Done.

    

    

      

      

        
+       if (!capable(CAP_SYS_ADMIN)) {
+               DRM_ERROR("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_ERROR("No OA registers given\n");
+               return -EINVAL;
+       }
+
+       oa_config = kzalloc(sizeof(*oa_config), GFP_KERNEL);
+       if (!oa_config) {
+               DRM_ERROR("Failed to allocate memory for the OA config\n");
+               return -ENOMEM;
+       }
+
+       err = strncpy_from_user(oa_config->guid, u64_to_user_ptr(args->uuid),
+                               sizeof(oa_config->guid));
+       if (err < 0) {
+               DRM_ERROR("Failed to copy uuid from OA config\n");
+               goto uuid_err;
+       }
+
+       if (sscanf(oa_config->guid, "%08x-%04x-%04x-%04x-%012x", &x1, &x2, &x3,
+                  &x4, &x5) != 5) {
+               DRM_ERROR("Invalid uuid format for OA config\n");
+               err = -EINVAL;
+               goto uuid_err;
+       }
+
+       oa_config->mux_regs_len = args->n_mux_regs;
+       oa_config->mux_regs =
+               alloc_oa_regs(dev_priv,
+                             dev_priv->perf.oa.ops.is_valid_mux_reg,
+                             u64_to_user_ptr(args->mux_regs),
+                             args->n_mux_regs);
+
+       if (IS_ERR(oa_config->mux_regs)) {
+               DRM_ERROR("Failed to create OA config for mux_regs\n");
+               err = PTR_ERR(oa_config->mux_regs);
+               goto mux_err;
+       }
+
+       oa_config->b_counter_regs_len = args->n_boolean_regs;
+       oa_config->b_counter_regs =
+               alloc_oa_regs(dev_priv,
+                             dev_priv->perf.oa.ops.is_valid_b_counter_reg,
+                             u64_to_user_ptr(args->boolean_regs),
+                             args->n_boolean_regs);
+
+       if (IS_ERR(oa_config->b_counter_regs)) {
+               DRM_ERROR("Failed to create OA config for b_counter_regs\n");
+               err = PTR_ERR(oa_config->b_counter_regs);
+               goto boolean_err;
+       }
+
+       if (INTEL_GEN(dev_priv) < 8) {
+               if (args->n_flex_regs != 0)
+                       goto flex_err;
+       } else {
+               oa_config->flex_regs_len = args->n_flex_regs;
+               oa_config->flex_regs =
+                       alloc_oa_regs(dev_priv,
+                                     dev_priv->perf.oa.ops.is_valid_flex_reg,
+                                     u64_to_user_ptr(args->flex_regs),
+                                     args->n_flex_regs);
+
+               if (IS_ERR(oa_config->flex_regs)) {
+                       DRM_ERROR("Failed to create OA config for flex_regs\n");
+                       err = PTR_ERR(oa_config->flex_regs);
+                       goto flex_err;
+               }
+       }
+
+       err = i915_mutex_lock_interruptible(dev);
+       if (err)
+               goto lock_err;
+
+       idr_for_each_entry(&dev_priv->perf.metrics_idr, tmp, id) {
+               if (!strcmp(tmp->guid, oa_config->guid)) {
+                       DRM_ERROR("OA config already exists with this uuid\n");
+                       err = -EADDRINUSE;
+                       goto sysfs_err;
+               }
+       }
+
+       err = create_dynamic_oa_sysfs_entry(dev_priv, oa_config);
+       if (err) {
+               DRM_ERROR("Failed to create sysfs entry for OA config\n");
+               goto sysfs_err;
+       }
+
+       oa_config->id = idr_alloc(&dev_priv->perf.metrics_idr,
+                                 oa_config,
+                                 dev_priv->perf.oa.n_builtin_sets + 1,
+                                 0, GFP_KERNEL);


      

      
Hmm, I think we need to check for the possible error here and unwind
accordingly.

    

    

    Done.

    

    

      

      

        
+
+       mutex_unlock(&dev->struct_mutex);
+
+       return oa_config->id;
+
+sysfs_err:
+       mutex_unlock(&dev->struct_mutex);
+lock_err:
+       kfree(oa_config->flex_regs);
+flex_err:
+       kfree(oa_config->b_counter_regs);
+boolean_err:
+       kfree(oa_config->mux_regs);
+mux_err:
+uuid_err:


      

      
Squash these?

    

    

    Done.

    

    

      

      

        
+       kfree(oa_config);
+
+       DRM_ERROR("Failed to add new OA config\n");
+       return err;
+}
+
+int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
+                                 struct drm_file *file)
+{
+       struct drm_i915_private *dev_priv = dev->dev_private;
+       u64 *arg = data;


      

      
int id = *data;

    

    

    data is void*, I don't think that's going to work.

    

    

      

      

        
+       struct i915_oa_dynamic_config *oa_config;
+       int ret;
+
+       if (!dev_priv->perf.initialized) {
+               DRM_DEBUG("i915 perf interface not available for this system\n");
+               return -ENOTSUPP;
+       }
+
+       if (!capable(CAP_SYS_ADMIN)) {
+               DRM_ERROR("Insufficient privileges to remove i915 OA config\n");
+               return -EACCES;
+       }
+
+       ret = i915_mutex_lock_interruptible(dev);
+       if (ret)
+               goto lock_err;
+
+       if (dev_priv->perf.oa.exclusive_stream &&
+           dev_priv->perf.oa.exclusive_stream->metrics_set == *arg) {
+               DRM_ERROR("Failed to remove config in use\n");
+               ret = -EADDRINUSE;
+               goto config_err;
+       }
+
+       oa_config = idr_find(&dev_priv->perf.metrics_idr, *arg);
+       if (!oa_config) {
+               DRM_ERROR("Failed to remove unknown config\n");
+               ret = -EINVAL;
+               goto config_err;
+       }


      

      
GEM_BUG_ON(id != oa_config->id);

    

    

    Done.

    

    

      

      

        
+
+       idr_remove(&dev_priv->perf.metrics_idr, *arg);
+
+       sysfs_remove_group(dev_priv->perf.metrics_kobj,
+                          &oa_config->sysfs_metric);
+       if (oa_config->flex_regs)
+               kfree(oa_config->flex_regs);
+       if (oa_config->b_counter_regs)
+               kfree(oa_config->b_counter_regs);
+       if (oa_config->mux_regs)
+               kfree(oa_config->mux_regs);


      

      
Just let kfree check for NULL?

    

    

    Done.

    

    

      

      

        
+       kfree(oa_config);
+
+config_err:
+       mutex_unlock(&dev->struct_mutex);
+lock_err:
+       return ret;
+}
+
 static struct ctl_table oa_table[] = {
        {
         .procname = "perf_stream_paranoid",
@@ -3011,8 +3377,14 @@ static struct ctl_table dev_root[] = {
 void i915_perf_init(struct drm_i915_private *dev_priv)
 {
        dev_priv->perf.oa.n_builtin_sets = 0;
+       idr_init(&dev_priv->perf.metrics_idr);


      

      
We should also throw an idr_destroy in perf_fini. Maybe we also need
to cleanup any dynamic configs which haven't been removed also?

    

    

    Indeed! Thanks!

    

    

      

      

        
        if (IS_HASWELL(dev_priv)) {
+               dev_priv->perf.oa.ops.is_valid_b_counter_reg =
+                       gen7_is_valid_b_counter_addr;
+               dev_priv->perf.oa.ops.is_valid_mux_reg =
+                       hsw_is_valid_mux_addr;
+               dev_priv->perf.oa.ops.is_valid_flex_reg = NULL;
                dev_priv->perf.oa.ops.init_oa_buffer = gen7_init_oa_buffer;
                dev_priv->perf.oa.ops.enable_metric_set = hsw_enable_metric_set;
                dev_priv->perf.oa.ops.disable_metric_set = hsw_disable_metric_set;
@@ -3036,6 +3408,13 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
                 * execlist mode by default.
                 */

+               dev_priv->perf.oa.ops.is_valid_b_counter_reg =
+                       gen7_is_valid_b_counter_addr;
+               dev_priv->perf.oa.ops.is_valid_mux_reg =
+                       gen7_is_valid_mux_addr;
+               dev_priv->perf.oa.ops.is_valid_flex_reg =
+                       gen8_is_valid_flex_addr;
+
                if (IS_GEN8(dev_priv)) {
                        dev_priv->perf.oa.ctx_oactxctrl_offset = 0x120;
                        dev_priv->perf.oa.ctx_flexeu0_offset = 0x2ce;
@@ -3050,6 +3429,8 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
                                dev_priv->perf.oa.ops.select_metric_set =
                                        i915_oa_select_metric_set_bdw;
                        } else if (IS_CHERRYVIEW(dev_priv)) {
+                               dev_priv->perf.oa.ops.is_valid_mux_reg =
+                                       chv_is_valid_mux_addr;
                                dev_priv->perf.oa.n_builtin_sets =
                                        i915_oa_n_builtin_metric_sets_chv;
                                dev_priv->perf.oa.ops.select_metric_set =
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 21ab12f4e72a..433fcd7beb72 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -732,6 +732,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define GDT_CHICKEN_BITS    _MMIO(0x9840)
 #define GT_NOA_ENABLE      0x00000080

+#define NOA_WRITE           _MMIO(0x9888)
+
 /*
  * OA Boolean state
  */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7ccbd6a2bbe0..e2ff47732303 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -260,6 +260,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_CONTEXT_GETPARAM  0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM  0x35
 #define DRM_I915_PERF_OPEN             0x36
+#define DRM_I915_PERF_ADD_CONFIG       0x37
+#define DRM_I915_PERF_REMOVE_CONFIG    0x38

 #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)
@@ -315,6 +317,8 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM    DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM    DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
 #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_IOWR(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
+#define DRM_IOCTL_I915_PERF_REMOVE_CONFIG      DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)

 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1467,6 +1471,23 @@ enum drm_i915_perf_record_type {
        DRM_I915_PERF_RECORD_MAX /* non-ABI */
 };

+/**
+ * Structure to upload perf dynamic configuration into the kernel.
+ */
+struct drm_i915_perf_oa_config {
+       /* string formatted like "%08x-%04x-%04x-%04x-%012x" **/


      

      
/** String formatted like: "%08x-%04x-%04x-%04x-%012x" */

    

    

    Done.

    

    

      



      

        
+       __u64 __user uuid;
+
+       __u32 n_mux_regs;
+       __u64 __user mux_regs;
+
+       __u32 n_boolean_regs;
+       __u64 __user boolean_regs;
+
+       __u32 n_flex_regs;
+       __u64 __user flex_regs;
+};
+
 #if defined(__cplusplus)
 }
 #endif
--
2.13.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


      

    

    


    

  


--------------EA01DC6AD336F2D77432B6FA--

--===============0806434591==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4
IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz
dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg==

--===============0806434591==--