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

Lionel Landwerlin lionel.g.landwerlin at intel.com
Tue Jul 18 14:52:29 UTC 2017


On 18/07/17 11:09, Matthew Auld wrote:
> On 07/17, Lionel Landwerlin wrote:
>> 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.
>>
>> v2: Drop DRM_ERROR for userspace errors (Matthew)
>>      Add padding to userspace structure (Matthew)
>>      s/guid/uuid/ (Matthew)
>>
>> v3: Use u32 instead of int to iterate through registers (Matthew)
>>
>> v4: Lock access to dynamic config list (Lionel)
>>
>> 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  |  48 +++++
>>   drivers/gpu/drm/i915/i915_perf.c | 413 +++++++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_reg.h  |   2 +
>>   include/uapi/drm/i915_drm.h      |  24 +++
>>   5 files changed, 475 insertions(+), 14 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 2b824f8875c4..607484737f3d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1917,6 +1917,9 @@ struct i915_oa_config {
>>   	struct attribute_group sysfs_metric;
>>   	struct attribute *attrs[2];
>>   	struct device_attribute sysfs_metric_id;
>> +
>> +	/* Only updated while holding dev_priv->perf.metrics_lock. */
>> +	bool in_use;
>>   };
>>   
>>   struct i915_perf_stream;
>> @@ -2043,6 +2046,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.
>>   	 *
>> @@ -2430,10 +2452,32 @@ struct drm_i915_private {
>>   		struct kobject *metrics_kobj;
>>   		struct ctl_table_header *sysctl_header;
>>   
>> +		/*
>> +		 * Lock associated with adding/modifying/removing OA configs
>> +		 * in dev_priv->perf.metrics_idr.
>> +		 */
>> +		struct mutex metrics_lock;
>> +
>> +		/*
>> +		 * List of dynamic configurations, you need to hold
>> +		 * dev_priv->perf.metrics_lock to access it.
>> +		 */
>> +		struct idr metrics_idr;
>> +
>> +		/*
>> +		 * Lock associated with anything below within this structure
>> +		 * except exclusive_stream.
>> +		 */
>>   		struct mutex lock;
>>   		struct list_head streams;
>>   
>>   		struct {
>> +			/*
>> +			 * The stream currently using the OA unit. If accessed
>> +			 * outside a syscall associated to its file
>> +			 * descriptor, you need to hold
>> +			 * dev_priv->drm.struct_mutex.
>> +			 */
>>   			struct i915_perf_stream *exclusive_stream;
>>   
>>   			u32 specific_ctx_id;
>> @@ -3599,6 +3643,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 e95e7755a19b..dd788cf34d79 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -357,6 +357,43 @@ struct perf_open_properties {
>>   	int oa_period_exponent;
>>   };
>>   
>> +static int get_oa_config(struct drm_i915_private *dev_priv,
>> +			 int metrics_set,
>> +			 struct i915_oa_config **out_config)
>> +{
>> +	int ret;
>> +
>> +	if (metrics_set == 1) {
>> +		*out_config = &dev_priv->perf.oa.test_config;
>> +		return 0;
>> +	}
>> +
>> +	ret = mutex_lock_interruptible(&dev_priv->perf.metrics_lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*out_config = idr_find(&dev_priv->perf.metrics_idr, metrics_set);
>> +	if (!*out_config)
>> +		ret = -EINVAL;
>> +	else
>> +		(*out_config)->in_use = true;
>> +
>> +	mutex_unlock(&dev_priv->perf.metrics_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static void put_oa_config(struct drm_i915_private *dev_priv,
>> +			  struct i915_oa_config *oa_config)
>> +{
>> +	if (oa_config == &dev_priv->perf.oa.test_config)
>> +		return;
>> +
>> +	mutex_lock(&dev_priv->perf.metrics_lock);
>> +	oa_config->in_use = false;
>> +	mutex_unlock(&dev_priv->perf.metrics_lock);
>> +}
>> +
>>   static u32 gen8_oa_hw_tail_read(struct drm_i915_private *dev_priv)
>>   {
>>   	return I915_READ(GEN8_OATAILPTR) & GEN8_OATAILPTR_MASK;
>> @@ -1246,10 +1283,12 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
>>   	BUG_ON(stream != dev_priv->perf.oa.exclusive_stream);
>>   
>>   	/*
>> -	 * Unset exclusive_stream first, it might be checked while
>> -	 * disabling the metric set on gen8+.
>> +	 * Unset exclusive_stream first, it will be checked while disabling
>> +	 * the metric set on gen8+.
>>   	 */
>> +	mutex_lock(&dev_priv->drm.struct_mutex);
>>   	dev_priv->perf.oa.exclusive_stream = NULL;
>> +	mutex_unlock(&dev_priv->drm.struct_mutex);
>>   
>>   	dev_priv->perf.oa.ops.disable_metric_set(dev_priv);
>>   
>> @@ -1261,6 +1300,8 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
>>   	if (stream->ctx)
>>   		oa_put_render_ctx_id(stream);
>>   
>> +	put_oa_config(dev_priv, stream->oa_config);
>> +
>>   	if (dev_priv->perf.oa.spurious_report_rs.missed) {
>>   		DRM_NOTE("%d spurious OA report notices suppressed due to ratelimiting\n",
>>   			 dev_priv->perf.oa.spurious_report_rs.missed);
>> @@ -1954,15 +1995,6 @@ static const struct i915_perf_stream_ops i915_oa_stream_ops = {
>>   	.read = i915_oa_read,
>>   };
>>   
>> -static struct i915_oa_config *get_oa_config(struct drm_i915_private *dev_priv,
>> -					    int metrics_set)
>> -{
>> -	if (metrics_set == 1)
>> -		return &dev_priv->perf.oa.test_config;
>> -
>> -	return NULL;
>> -}
>> -
>>   /**
>>    * i915_oa_stream_init - validate combined props for OA stream and init
>>    * @stream: An i915 perf stream
>> @@ -2066,9 +2098,9 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>>   			return ret;
>>   	}
>>   
>> -	stream->oa_config = get_oa_config(dev_priv, props->metrics_set);
>> -	if (!stream->oa_config)
>> -		return -EINVAL;
>> +	ret = get_oa_config(dev_priv, props->metrics_set, &stream->oa_config);
>> +	if (ret)
>> +		return ret;
> goto err_config;

Done, thanks a lot.

>
>>   
>>   	/* PRM - observability performance counters:
>>   	 *
>> @@ -2121,6 +2153,8 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>>   	if (stream->ctx)
>>   		oa_put_render_ctx_id(stream);
>>   
>> +	put_oa_config(dev_priv, stream->oa_config);
>> +
>>   	return ret;
> err_oa_buf_alloc:
>      ...
>      put_oa_config(dev_priv, stream->oa_config);
>
> err_config:
>      if (stream->ctx)
>          ...
>
>      return ret;

Done.

>
>>   }
>>   
>> @@ -2131,6 +2165,8 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>>   	struct drm_i915_private *dev_priv = engine->i915;
>>   	struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
>>   
>> +	lockdep_assert_held(&dev_priv->drm.struct_mutex);
>> +
>>   	if (engine->id != RCS)
>>   		return;
>>   
>> @@ -2929,6 +2965,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;
>> +	u32 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);
> oa_regs = kmalloc_array(n_regs, sizeof(*oa_regs), GFP_KERNEL);

Done.

>
>> +	if (!oa_regs)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (i = 0; i < n_regs; i++) {
>> +		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),
>> +				UUID_STRING_LEN);
>> +	if (err < 0) {
>> +		DRM_DEBUG("Failed to copy uuid from OA config\n");
>> +		goto mux_err;
>> +	}
>> +
> /* TODO: eventually replace with uuid_is_valid() */

Oh actually, I should use that.

>> +	if (sscanf(oa_config->uuid, "%08x-%04x-%04x-%04x-%012x", &x1, &x2, &x3,
>> +		   &x4, &x5) != 5) {
>> +		DRM_DEBUG("Invalid uuid format for OA config\n");
>> +		err = -EINVAL;
>> +		goto mux_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_DEBUG("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_DEBUG("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_DEBUG("Failed to create OA config for flex_regs\n");
>> +			err = PTR_ERR(oa_config->flex_regs);
>> +			goto flex_err;
>> +		}
>> +	}
>> +
>> +	err = mutex_lock_interruptible(&dev_priv->perf.metrics_lock);
>> +	if (err)
>> +		goto lock_err;
>> +
>> +	idr_for_each_entry(&dev_priv->perf.metrics_idr, tmp, id) {
>> +		if (!strcmp(tmp->uuid, oa_config->uuid)) {
>> +			DRM_DEBUG("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_DEBUG("Failed to create sysfs entry for OA config\n");
>> +		goto sysfs_err;
>> +	}
>> +
>> +	oa_config->id = idr_alloc(&dev_priv->perf.metrics_idr,
>> +				  oa_config, 2,
>> +				  0, GFP_KERNEL);
>> +	if (oa_config->id < 0) {
>> +		DRM_DEBUG("Failed to create sysfs entry for OA config\n");
> err = oa_config->id;

Done, thanks!

>
>> +		goto idr_err;
>> +	}
>> +
>> +	mutex_unlock(&dev_priv->perf.metrics_lock);
>> +
>> +	return oa_config->id;
>> +
>> +idr_err:
>> +	sysfs_remove_group(dev_priv->perf.metrics_kobj,
>> +			   &oa_config->sysfs_metric);
>> +sysfs_err:
>> +	mutex_unlock(&dev_priv->perf.metrics_lock);
>> +lock_err:
>> +	kfree(oa_config->flex_regs);
>> +flex_err:
>> +	kfree(oa_config->b_counter_regs);
>> +boolean_err:
>> +	kfree(oa_config->mux_regs);
>> +mux_err:
>> +	kfree(oa_config);
>> +
>> +	DRM_DEBUG("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;
>> +	struct i915_oa_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_DEBUG("Insufficient privileges to remove i915 OA config\n");
>> +		return -EACCES;
>> +	}
>> +
>> +	ret = mutex_lock_interruptible(&dev_priv->perf.metrics_lock);
>> +	if (ret)
>> +		goto lock_err;
>> +
>> +	oa_config = idr_find(&dev_priv->perf.metrics_idr, *arg);
>> +	if (!oa_config) {
>> +		DRM_DEBUG("Failed to remove unknown OA config\n");
>> +		ret = -EINVAL;
>> +		goto config_err;
>> +	}
>> +
>> +	if (oa_config->in_use) {
>> +		DRM_DEBUG("Failed to in use OA config\n");
> "Failed to remove in use OA config"

Done.

>
>> +		ret = -EADDRINUSE;
>> +		goto config_err;
>> +	}
>> +
>> +	GEM_BUG_ON(*arg != oa_config->id);
>> +
>> +	idr_remove(&dev_priv->perf.metrics_idr, *arg);
>> +	destroy_config(oa_config->id, oa_config, dev_priv);
>> +
>> +config_err:
>> +	mutex_unlock(&dev_priv->perf.metrics_lock);
>> +lock_err:
>> +	return ret;
>> +}
>> +
>>   static struct ctl_table oa_table[] = {
>>   	{
>>   	 .procname = "perf_stream_paranoid",
>> @@ -2983,8 +3346,14 @@ static struct ctl_table dev_root[] = {
>>   void i915_perf_init(struct drm_i915_private *dev_priv)
>>   {
>>   	dev_priv->perf.oa.timestamp_frequency = 0;
>> +	idr_init(&dev_priv->perf.metrics_idr);
>>   
>>   	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;
>> @@ -3004,6 +3373,12 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
>>   		 * worth the complexity to maintain now that BDW+ enable
>>   		 * 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;
>>   
>>   		dev_priv->perf.oa.ops.init_oa_buffer = gen8_init_oa_buffer;
>>   		dev_priv->perf.oa.ops.enable_metric_set = gen8_enable_metric_set;
>> @@ -3022,6 +3397,10 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
>>   			dev_priv->perf.oa.timestamp_frequency = 12500000;
>>   
>>   			dev_priv->perf.oa.gen8_valid_ctx_bit = (1<<25);
>> +			if (IS_CHERRYVIEW(dev_priv)) {
>> +				dev_priv->perf.oa.ops.is_valid_mux_reg =
>> +					chv_is_valid_mux_addr;
>> +			}
>>   		} else if (IS_GEN9(dev_priv)) {
>>   			dev_priv->perf.oa.ctx_oactxctrl_offset = 0x128;
>>   			dev_priv->perf.oa.ctx_flexeu0_offset = 0x3de;
>> @@ -3060,6 +3439,9 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
>>   			dev_priv->perf.oa.timestamp_frequency / 2;
>>   		dev_priv->perf.sysctl_header = register_sysctl_table(dev_root);
>>   
>> +		mutex_init(&dev_priv->perf.metrics_lock);
>> +		idr_init(&dev_priv->perf.metrics_idr);
>> +
>>   		dev_priv->perf.initialized = true;
>>   	}
>>   }
>> @@ -3073,6 +3455,9 @@ void i915_perf_fini(struct drm_i915_private *dev_priv)
>>   	if (!dev_priv->perf.initialized)
>>   		return;
>>   
>> +	idr_for_each(&dev_priv->perf.metrics_idr, destroy_config, dev_priv);
>> +	idr_destroy(&dev_priv->perf.metrics_idr);
>> +
>>   	unregister_sysctl_table(dev_priv->perf.sysctl_header);
>>   
>>   	memset(&dev_priv->perf.oa.ops, 0, sizeof(dev_priv->perf.oa.ops));
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index c712d01f92ab..fc5934063c83 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..82b3b873a0f3 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)
> These need to be DRM_IOW(), or we'll end up doing do a needless copy_to_user()

Done.

>
>>   
>>   /* Allow drivers to submit batchbuffers directly to hardware, relying
>>    * on the security mechanisms provided by hardware.
>> @@ -1467,6 +1471,26 @@ 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" */
>> +	__u64 __user uuid;
>> +
>> +	__u32 n_mux_regs;
>> +	__u32 pad0;
>> +	__u64 __user mux_regs;
>> +
>> +	__u32 n_boolean_regs;
>> +	__u32 pad1;
>> +	__u64 __user boolean_regs;
>> +
>> +	__u32 n_flex_regs;
>> +	__u32 pad2;
>> +	__u64 __user flex_regs;
>> +};
> Not sure if it's worth checking if all the padding is indeed zeroed and reject
> if not, in case we ever want to extend?

Other parts of the driver do it, so let's be consistent!

>
> Otherwise couldn't find anything else to nitpick :)

Thanks a lot for you time, much appreciated.
Sending a v5 shortly.

>
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>> -- 
>> 2.13.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx




More information about the Intel-gfx mailing list