[1/8] drm/i915: Add i915 perf infrastructure

Submitted by Robert Bragg on Feb. 3, 2016, 6:39 p.m.

Details

Message ID 1454524753-13218-2-git-send-email-robert@sixbynine.org
State New
Headers show
Series "Enable Gen 7 Observation Architecture" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Robert Bragg Feb. 3, 2016, 6:39 p.m.
Adds base i915 perf infrastructure for Gen performance metrics.

This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an array of uint64
properties to configure a stream of metrics and returns a new fd usable
with standard VFS system calls including read() to read typed and sized
records; ioctl() to enable or disable capture and poll() to wait for
data.

A stream is opened something like:

  uint64_t properties[] = {
      /* Single context sampling */
      DRM_I915_PERF_CTX_HANDLE_PROP,        ctx_handle,

      /* Include OA reports in samples */
      DRM_I915_PERF_SAMPLE_OA_PROP,         true,

      /* OA unit configuration */
      DRM_I915_PERF_OA_METRICS_SET_PROP,    metrics_set_id,
      DRM_I915_PERF_OA_FORMAT_PROP,         report_format,
      DRM_I915_PERF_OA_EXPONENT_PROP,       period_exponent,
   };
   struct drm_i915_perf_open_param parm = {
      .flags = I915_PERF_FLAG_FD_CLOEXEC |
               I915_PERF_FLAG_FD_NONBLOCK |
               I915_PERF_FLAG_DISABLED,
      .properties = properties,
      .n_properties = sizeof(properties) / 16,
   };
   int fd = drmIoctl(drm_fd, DRM_IOCTL_I915_PERF_OPEN, &param);

Records read all start with a common { type, size } header with
DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample records
contain an extensible number of fields and it's the
DRM_I915_PERF_SAMPLE_xyz properties given when opening that determine
what's included in every sample.

No specific streams are supported yet so any attempt to open a stream
will return an error.

Signed-off-by: Robert Bragg <robert@sixbynine.org>
---
 drivers/gpu/drm/i915/Makefile    |   3 +
 drivers/gpu/drm/i915/i915_dma.c  |   7 +
 drivers/gpu/drm/i915/i915_drv.h  |  88 ++++++++
 drivers/gpu/drm/i915/i915_perf.c | 442 +++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h      |  69 ++++++
 5 files changed, 609 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/i915_perf.c

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0851de07..af8ecbb 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -94,6 +94,9 @@  i915-y += dvo_ch7017.o \
 # virtual gpu code
 i915-y += i915_vgpu.o
 
+# perf code
+i915-y += i915_perf.o
+
 # legacy horrors
 i915-y += i915_dma.o
 
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a42eb58..4f4c9ba 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1012,6 +1012,11 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (ret < 0)
 		goto out_free_priv;
 
+	/* Must at least be initialized before trying to pin any context
+	 * which i915_perf hooks into.
+	 */
+	i915_perf_init(dev);
+
 	intel_pm_setup(dev);
 
 	intel_runtime_pm_get(dev_priv);
@@ -1203,6 +1208,7 @@  int i915_driver_unload(struct drm_device *dev)
 		return ret;
 	}
 
+	i915_perf_fini(dev);
 	intel_power_domains_fini(dev_priv);
 
 	intel_gpu_ips_teardown();
@@ -1379,6 +1385,7 @@  const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW),
 	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),
 };
 
 int i915_max_ioctl = ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 65a2cd0..7748383 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1732,6 +1732,81 @@  struct intel_wm_config {
 	bool sprites_scaled;
 };
 
+struct i915_perf_read_state {
+	int count;
+	ssize_t read;
+	char __user *buf;
+};
+
+struct i915_perf_stream {
+	struct drm_i915_private *dev_priv;
+
+	struct list_head link;
+
+	u32 sample_flags;
+
+	struct intel_context *ctx;
+	bool enabled;
+
+	/* Enables the collection of HW samples, either in response to
+	 * I915_PERF_IOCTL_ENABLE or implicitly called when stream is
+	 * opened without I915_PERF_FLAG_DISABLED.
+	 */
+	void (*enable)(struct i915_perf_stream *stream);
+
+	/* Disables the collection of HW samples, either in response to
+	 * I915_PERF_IOCTL_DISABLE or implicitly called before
+	 * destroying the stream.
+	 */
+	void (*disable)(struct i915_perf_stream *stream);
+
+	/* Return: true if any i915 perf records are ready to read()
+	 * for this stream.
+	 */
+	bool (*can_read)(struct i915_perf_stream *stream);
+
+	/* Call poll_wait, passing a wait queue that will be woken
+	 * once there is something ready to read() for the stream
+	 */
+	void (*poll_wait)(struct i915_perf_stream *stream,
+			  struct file *file,
+			  poll_table *wait);
+
+	/* For handling a blocking read, wait until there is something
+	 * to ready to read() for the stream. E.g. wait on the same
+	 * wait queue that would be passed to poll_wait() until
+	 * ->can_read() returns true (if its safe to call ->can_read()
+	 * without the i915 perf lock held).
+	 */
+	int (*wait_unlocked)(struct i915_perf_stream *stream);
+
+	/* Copy as many buffered i915 perf samples and records for
+	 * this stream to userspace as will fit in the given buffer.
+	 *
+	 * Only write complete records.
+	 *
+	 * read_state->count is the length of read_state->buf
+	 *
+	 * Update read_state->read with the number of bytes written.
+	 *
+	 * Return: the number of records appended (may be zero if not
+	 * enough space), or errno.
+	 *
+	 * Only return an EFAULT from copying to the userspace buffer
+	 * if zero records have been appended, otherwise squash the
+	 * EFAULT error and report what records were successfully
+	 * copied.
+	 */
+	int (*read)(struct i915_perf_stream *stream,
+		    struct i915_perf_read_state *read_state);
+
+	/* Cleanup any stream specific resources.
+	 *
+	 * The stream will always be disabled before this is called.
+	 */
+	void (*destroy)(struct i915_perf_stream *stream);
+};
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *objects;
@@ -1983,6 +2058,12 @@  struct drm_i915_private {
 
 	struct i915_runtime_pm pm;
 
+	struct {
+		bool initialized;
+		struct mutex lock;
+		struct list_head streams;
+	} perf;
+
 	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
 	struct {
 		int (*execbuf_submit)(struct i915_execbuffer_params *params,
@@ -3251,6 +3332,9 @@  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv);
 
+int i915_perf_open_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file);
+
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct drm_device *dev,
 					  struct i915_address_space *vm,
@@ -3366,6 +3450,10 @@  int i915_parse_cmds(struct intel_engine_cs *ring,
 		    u32 batch_len,
 		    bool is_master);
 
+/* i915_perf.c */
+extern void i915_perf_init(struct drm_device *dev);
+extern void i915_perf_fini(struct drm_device *dev);
+
 /* i915_suspend.c */
 extern int i915_save_state(struct drm_device *dev);
 extern int i915_restore_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
new file mode 100644
index 0000000..9af9e52
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -0,0 +1,442 @@ 
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <linux/anon_inodes.h>
+#include <linux/sizes.h>
+
+#include "i915_drv.h"
+
+struct perf_open_properties {
+	u32 sample_flags;
+
+	u64 single_context:1;
+	u64 ctx_handle;
+};
+
+static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
+				     struct file *file,
+				     char __user *buf,
+				     size_t count,
+				     loff_t *ppos)
+{
+	struct drm_i915_private *dev_priv = stream->dev_priv;
+	struct i915_perf_read_state state = { count, 0, buf };
+	int ret;
+
+	if (file->f_flags & O_NONBLOCK) {
+		if (!stream->can_read(stream))
+			return -EAGAIN;
+	} else {
+		mutex_unlock(&dev_priv->perf.lock);
+		ret = stream->wait_unlocked(stream);
+		mutex_lock(&dev_priv->perf.lock);
+
+		if (ret)
+			return ret;
+	}
+
+	ret = stream->read(stream, &state);
+	if (ret < 0) {
+		/* can relax this later if needs be, but this is the only
+		 * errno expected here atm...
+		 */
+		WARN_ON(ret != -EFAULT);
+		return ret;
+	}
+
+	if (ret == 0)
+		return -ENOSPC;
+
+	return state.read;
+}
+
+static ssize_t i915_perf_read(struct file *file,
+			      char __user *buf,
+			      size_t count,
+			      loff_t *ppos)
+{
+	struct i915_perf_stream *stream = file->private_data;
+	struct drm_i915_private *dev_priv = stream->dev_priv;
+	ssize_t ret;
+
+	mutex_lock(&dev_priv->perf.lock);
+	ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+	mutex_unlock(&dev_priv->perf.lock);
+
+	return ret;
+}
+
+static unsigned int i915_perf_poll_locked(struct i915_perf_stream *stream,
+					  struct file *file,
+					  poll_table *wait)
+{
+	unsigned int streams = 0;
+
+	stream->poll_wait(stream, file, wait);
+
+	if (stream->can_read(stream))
+		streams |= POLLIN;
+
+	return streams;
+}
+
+static unsigned int i915_perf_poll(struct file *file, poll_table *wait)
+{
+	struct i915_perf_stream *stream = file->private_data;
+	struct drm_i915_private *dev_priv = stream->dev_priv;
+	int ret;
+
+	mutex_lock(&dev_priv->perf.lock);
+	ret = i915_perf_poll_locked(stream, file, wait);
+	mutex_unlock(&dev_priv->perf.lock);
+
+	return ret;
+}
+
+static void i915_perf_enable_locked(struct i915_perf_stream *stream)
+{
+	if (stream->enabled)
+		return;
+
+	/* Allow stream->enable() to refer to this */
+	stream->enabled = true;
+
+	if (stream->enable)
+		stream->enable(stream);
+}
+
+static void i915_perf_disable_locked(struct i915_perf_stream *stream)
+{
+	if (!stream->enabled)
+		return;
+
+	/* Allow stream->disable() to refer to this */
+	stream->enabled = false;
+
+	if (stream->disable)
+		stream->disable(stream);
+}
+
+static long i915_perf_ioctl_locked(struct i915_perf_stream *stream,
+				   unsigned int cmd,
+				   unsigned long arg)
+{
+	switch (cmd) {
+	case I915_PERF_IOCTL_ENABLE:
+		i915_perf_enable_locked(stream);
+		return 0;
+	case I915_PERF_IOCTL_DISABLE:
+		i915_perf_disable_locked(stream);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static long i915_perf_ioctl(struct file *file,
+			    unsigned int cmd,
+			    unsigned long arg)
+{
+	struct i915_perf_stream *stream = file->private_data;
+	struct drm_i915_private *dev_priv = stream->dev_priv;
+	long ret;
+
+	mutex_lock(&dev_priv->perf.lock);
+	ret = i915_perf_ioctl_locked(stream, cmd, arg);
+	mutex_unlock(&dev_priv->perf.lock);
+
+	return ret;
+}
+
+static void i915_perf_destroy_locked(struct i915_perf_stream *stream)
+{
+	struct drm_i915_private *dev_priv = stream->dev_priv;
+
+	if (stream->enabled)
+		i915_perf_disable_locked(stream);
+
+	if (stream->destroy)
+		stream->destroy(stream);
+
+	list_del(&stream->link);
+
+	if (stream->ctx) {
+		mutex_lock(&dev_priv->dev->struct_mutex);
+		i915_gem_context_unreference(stream->ctx);
+		mutex_unlock(&dev_priv->dev->struct_mutex);
+	}
+
+	kfree(stream);
+}
+
+static int i915_perf_release(struct inode *inode, struct file *file)
+{
+	struct i915_perf_stream *stream = file->private_data;
+	struct drm_i915_private *dev_priv = stream->dev_priv;
+
+	mutex_lock(&dev_priv->perf.lock);
+	i915_perf_destroy_locked(stream);
+	mutex_unlock(&dev_priv->perf.lock);
+
+	return 0;
+}
+
+
+static const struct file_operations fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.release	= i915_perf_release,
+	.poll		= i915_perf_poll,
+	.read		= i915_perf_read,
+	.unlocked_ioctl	= i915_perf_ioctl,
+};
+
+static struct intel_context *
+lookup_context(struct drm_i915_private *dev_priv,
+	       struct file *user_filp,
+	       u32 ctx_user_handle)
+{
+	struct intel_context *ctx;
+
+	mutex_lock(&dev_priv->dev->struct_mutex);
+	list_for_each_entry(ctx, &dev_priv->context_list, link) {
+		struct drm_file *drm_file;
+
+		if (!ctx->file_priv)
+			continue;
+
+		drm_file = ctx->file_priv->file;
+
+		if (user_filp->private_data == drm_file &&
+		    ctx->user_handle == ctx_user_handle) {
+			i915_gem_context_reference(ctx);
+			mutex_unlock(&dev_priv->dev->struct_mutex);
+
+			return ctx;
+		}
+	}
+	mutex_unlock(&dev_priv->dev->struct_mutex);
+
+	return NULL;
+}
+
+int i915_perf_open_ioctl_locked(struct drm_device *dev,
+				struct drm_i915_perf_open_param *param,
+				struct perf_open_properties *props,
+				struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_context *specific_ctx = NULL;
+	struct i915_perf_stream *stream = NULL;
+	unsigned long f_flags = 0;
+	int stream_fd;
+	int ret = 0;
+
+	if (props->single_context) {
+		u32 ctx_handle = props->ctx_handle;
+
+		specific_ctx = lookup_context(dev_priv, file->filp, ctx_handle);
+		if (!specific_ctx) {
+			DRM_ERROR("Failed to look up context with ID %u for opening perf stream\n",
+				  ctx_handle);
+			ret = -EINVAL;
+			goto err;
+		}
+	}
+
+	if (!specific_ctx && !capable(CAP_SYS_ADMIN)) {
+		DRM_ERROR("Insufficient privileges to open system-wide i915 perf stream\n");
+		ret = -EACCES;
+		goto err_ctx;
+	}
+
+	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+	if (!stream) {
+		ret = -ENOMEM;
+		goto err_ctx;
+	}
+
+	stream->sample_flags = props->sample_flags;
+	stream->dev_priv = dev_priv;
+	stream->ctx = specific_ctx;
+
+	/*
+	 * TODO: support sampling something
+	 *
+	 * For now this is as far as we can go.
+	 */
+	DRM_ERROR("Unsupported i915 perf stream configuration\n");
+	ret = -EINVAL;
+	goto err_alloc;
+
+	list_add(&stream->link, &dev_priv->perf.streams);
+
+	if (param->flags & I915_PERF_FLAG_FD_CLOEXEC)
+		f_flags |= O_CLOEXEC;
+	if (param->flags & I915_PERF_FLAG_FD_NONBLOCK)
+		f_flags |= O_NONBLOCK;
+
+	stream_fd = anon_inode_getfd("[i915_perf]", &fops, stream, f_flags);
+	if (stream_fd < 0) {
+		ret = stream_fd;
+		goto err_open;
+	}
+
+	if (!(param->flags & I915_PERF_FLAG_DISABLED))
+		i915_perf_enable_locked(stream);
+
+	return stream_fd;
+
+err_open:
+	list_del(&stream->link);
+	if (stream->destroy)
+		stream->destroy(stream);
+err_alloc:
+	kfree(stream);
+err_ctx:
+	if (specific_ctx) {
+		mutex_lock(&dev_priv->dev->struct_mutex);
+		i915_gem_context_unreference(specific_ctx);
+		mutex_unlock(&dev_priv->dev->struct_mutex);
+	}
+err:
+	return ret;
+}
+
+/* Note we copy the properties from userspace outside of the i915 perf
+ * mutex to avoid an awkward lockdep with mmap_sem.
+ *
+ * Note this function only validates properties in isolation it doesn't
+ * validate that the combination of properties makes sense or that all
+ * properties necessary for a particular kind of stream have been set.
+ */
+static int read_properties_unlocked(struct drm_i915_private *dev_priv,
+				    u64 __user *uprops,
+				    u32 n_props,
+				    struct perf_open_properties *props)
+{
+	u64 __user *uprop = uprops;
+	int i;
+
+	memset(props, 0, sizeof(struct perf_open_properties));
+
+	if (!n_props) {
+		DRM_ERROR("No i915 perf properties given");
+		return -EINVAL;
+	}
+
+	if (n_props > DRM_I915_PERF_PROP_MAX) {
+		DRM_ERROR("More i915 perf properties specified than exist");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < n_props; i++) {
+		u64 id, value;
+		int ret;
+
+		ret = get_user(id, (u64 __user *)uprop);
+		if (ret)
+			return ret;
+
+		if (id == 0 || id >= DRM_I915_PERF_PROP_MAX) {
+			DRM_ERROR("Unknown i915 perf property ID");
+			return -EINVAL;
+		}
+
+		ret = get_user(value, (u64 __user *)uprop + 1);
+		if (ret)
+			return ret;
+
+		switch ((enum drm_i915_perf_property_id)id) {
+		case DRM_I915_PERF_CTX_HANDLE_PROP:
+			props->single_context = 1;
+			props->ctx_handle = value;
+			break;
+
+		case DRM_I915_PERF_PROP_MAX:
+			BUG();
+		}
+
+		uprop += 2;
+	}
+
+	return 0;
+}
+
+int i915_perf_open_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_perf_open_param *param = data;
+	struct perf_open_properties props;
+	u32 known_open_flags = 0;
+	int ret;
+
+	if (!dev_priv->perf.initialized) {
+		DRM_ERROR("i915 perf interface not available for this system");
+		return -ENOTSUPP;
+	}
+
+	known_open_flags = I915_PERF_FLAG_FD_CLOEXEC |
+			   I915_PERF_FLAG_FD_NONBLOCK |
+			   I915_PERF_FLAG_DISABLED;
+	if (param->flags & ~known_open_flags) {
+		DRM_ERROR("Unknown drm_i915_perf_open_param flag\n");
+		return -EINVAL;
+	}
+
+	ret = read_properties_unlocked(dev_priv,
+				       to_user_ptr(param->properties),
+				       param->n_properties,
+				       &props);
+	if (ret)
+		return ret;
+
+	mutex_lock(&dev_priv->perf.lock);
+	ret = i915_perf_open_ioctl_locked(dev, param, &props, file);
+	mutex_unlock(&dev_priv->perf.lock);
+
+	return ret;
+}
+
+void i915_perf_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	INIT_LIST_HEAD(&dev_priv->perf.streams);
+	mutex_init(&dev_priv->perf.lock);
+
+	dev_priv->perf.initialized = true;
+}
+
+void i915_perf_fini(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (!dev_priv->perf.initialized)
+		return;
+
+	/* Currently nothing to clean up */
+
+	dev_priv->perf.initialized = false;
+}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index a5524cc..68ca26e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -230,6 +230,7 @@  typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_USERPTR		0x33
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
+#define DRM_I915_PERF_OPEN		0x36
 
 #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)
@@ -283,6 +284,7 @@  typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_USERPTR			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_USERPTR, struct drm_i915_gem_userptr)
 #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_IOWR(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1170,4 +1172,71 @@  struct drm_i915_gem_context_param {
 	__u64 value;
 };
 
+#define I915_PERF_FLAG_FD_CLOEXEC	(1<<0)
+#define I915_PERF_FLAG_FD_NONBLOCK	(1<<1)
+#define I915_PERF_FLAG_DISABLED		(1<<2)
+
+enum drm_i915_perf_property_id {
+	/**
+	 * Open the stream for a specific context handle (as used with
+	 * execbuffer2). A stream opened for a specific context this way
+	 * won't typically require root privileges.
+	 */
+	DRM_I915_PERF_CTX_HANDLE_PROP = 1,
+
+	DRM_I915_PERF_PROP_MAX /* non-ABI */
+};
+
+struct drm_i915_perf_open_param {
+	/** CLOEXEC, NONBLOCK... */
+	__u32 flags;
+
+	/**
+	 * Pointer to array of u64 (id, value) pairs configuring the stream
+	 * to open.
+	 */
+	__u64 __user properties;
+
+	/** The number of u64 (id, value) pairs */
+	__u32 n_properties;
+};
+
+#define I915_PERF_IOCTL_ENABLE	_IO('i', 0x0)
+#define I915_PERF_IOCTL_DISABLE	_IO('i', 0x1)
+
+/**
+ * Common to all i915 perf records
+ */
+struct drm_i915_perf_record_header {
+	__u32 type;
+	__u16 pad;
+	__u16 size;
+};
+
+enum drm_i915_perf_record_type {
+
+	/**
+	 * Samples are the work horse record type whose contents are extensible
+	 * and defined when opening an i915 perf stream based on the given
+	 * properties.
+	 *
+	 * Boolean properties following the naming convention
+	 * DRM_I915_PERF_SAMPLE_xyz_PROP request the inclusion of 'xyz' data in
+	 * every sample.
+	 *
+	 * The order of these sample properties given by userspace has no
+	 * affect on the ordering of data within a sample. The order will be
+	 * documented here.
+	 *
+	 * struct {
+	 *     struct drm_i915_perf_record_header header;
+	 *
+	 *     TODO: itemize extensible sample data here
+	 * };
+	 */
+	DRM_I915_PERF_RECORD_SAMPLE = 1,
+
+	DRM_I915_PERF_RECORD_MAX /* non-ABI */
+};
+
 #endif /* _UAPI_I915_DRM_H_ */

Comments

On 3 February 2016 at 18:39, Robert Bragg <robert@sixbynine.org> wrote:

> index a5524cc..68ca26e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h

> @@ -1170,4 +1172,71 @@ struct drm_i915_gem_context_param {
>         __u64 value;
>  };
>
> +#define I915_PERF_FLAG_FD_CLOEXEC      (1<<0)
> +#define I915_PERF_FLAG_FD_NONBLOCK     (1<<1)
> +#define I915_PERF_FLAG_DISABLED                (1<<2)
> +
> +enum drm_i915_perf_property_id {
> +       /**
> +        * Open the stream for a specific context handle (as used with
> +        * execbuffer2). A stream opened for a specific context this way
> +        * won't typically require root privileges.
> +        */
> +       DRM_I915_PERF_CTX_HANDLE_PROP = 1,
> +
Wouldn't DRM_I915_PERF_PROP_CTX_HANDLE be a better name ? It's more
obvious at least :-P

> +       DRM_I915_PERF_PROP_MAX /* non-ABI */
Isn't the use of enums (in drm UAPI) discouraged ? Afaics only the old
DRI1 i915 code has them.
Same question applies throughout the patch/series.

> +};
> +
> +struct drm_i915_perf_open_param {
> +       /** CLOEXEC, NONBLOCK... */
Some other places in i915 define the macros just after the __u32
flags. This will allow you to drop the comment ;-)

> +       __u32 flags;
> +
And ... we broke 32 bit userspare on 64 bit kernels. Please check for
holes and other issues as described in Daniel Vetter's
article/documentation [1]

[1] https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.txt

> +       /**
> +        * Pointer to array of u64 (id, value) pairs configuring the stream
> +        * to open.
> +        */
> +       __u64 __user properties;
> +
> +       /** The number of u64 (id, value) pairs */
> +       __u32 n_properties;
The rest of the file uses num_foo or foo_count. Can we opt for one of those ?

> +};
> +
> +#define I915_PERF_IOCTL_ENABLE _IO('i', 0x0)
> +#define I915_PERF_IOCTL_DISABLE        _IO('i', 0x1)
> +
> +/**
> + * Common to all i915 perf records
> + */
> +struct drm_i915_perf_record_header {
> +       __u32 type;
> +       __u16 pad;
Move pad at the end ? This way we can (maybe?) reuse it in the future.

> +       __u16 size;
> +};
> +

Daniel, is there a consensus about zeroing the pad from userspace and
checking it for garbage in the ioctl ? The documentation says "do it",
yet I have a hunch that we're missing a few. Also what error should
the ioctl return in such a case - EINVAL or EFAULT ? Worth
documenting, just in case ?

-Emil
On Thu, Feb 4, 2016 at 1:42 AM, Emil Velikov <emil.l.velikov@gmail.com>
wrote:

> On 3 February 2016 at 18:39, Robert Bragg <robert@sixbynine.org> wrote:
>
> > index a5524cc..68ca26e 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
>
> > @@ -1170,4 +1172,71 @@ struct drm_i915_gem_context_param {
> >         __u64 value;
> >  };
> >
> > +#define I915_PERF_FLAG_FD_CLOEXEC      (1<<0)
> > +#define I915_PERF_FLAG_FD_NONBLOCK     (1<<1)
> > +#define I915_PERF_FLAG_DISABLED                (1<<2)
> > +
> > +enum drm_i915_perf_property_id {
> > +       /**
> > +        * Open the stream for a specific context handle (as used with
> > +        * execbuffer2). A stream opened for a specific context this way
> > +        * won't typically require root privileges.
> > +        */
> > +       DRM_I915_PERF_CTX_HANDLE_PROP = 1,
> > +
> Wouldn't DRM_I915_PERF_PROP_CTX_HANDLE be a better name ? It's more
> obvious at least :-P
>

Yeah would be more consistent to keep the common namespacing at the front.


>
> > +       DRM_I915_PERF_PROP_MAX /* non-ABI */
> Isn't the use of enums (in drm UAPI) discouraged ? Afaics only the old
> DRI1 i915 code has them.
> Same question applies throughout the patch/series.
>

An enum here seems like a pretty good technical fit. I think the potential
for different enums to have different sizes is a likely reason for being
cautious about using them as part of a kernel ABI (so probably unwise to
have enum based members in an ioctl structure) but in this case these IDs
are always passed to the kernel as a u64. We benefit from the compiler
reminding us to handle all properties in the driver if we switch on this
enum which I like, as well as having the _MAX value to refer to in the
driver which is perhaps a little more reliable at the end of an enum vs a
#define which needs to be explicitly updated for each addition.

For reference, the first iteration of this driver was based on the core
pref infrastructure and in this case the enum + _MAX /* non-ABI */ approach
is borrowed from there (ref: include/uapi/linux/perf_event.h)


> > +};
> > +
> > +struct drm_i915_perf_open_param {
> > +       /** CLOEXEC, NONBLOCK... */
> Some other places in i915 define the macros just after the __u32
> flags. This will allow you to drop the comment ;-)
>
> > +       __u32 flags;
> > +
> And ... we broke 32 bit userspare on 64 bit kernels. Please check for
> holes and other issues as described in Daniel Vetter's
> article/documentation [1]
>
> [1] https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.txt


Ah yeah, don't think this would break 32bit userspace, but still would be
good to remove that hole, this has been through a few iterations and there
used to be a __u32 type here, well spotted thanks.

I think I'll bump the flags to be 64bit.


>
>
> > +       /**
> > +        * Pointer to array of u64 (id, value) pairs configuring the
> stream
> > +        * to open.
> > +        */
> > +       __u64 __user properties;
> > +
> > +       /** The number of u64 (id, value) pairs */
> > +       __u32 n_properties;
> The rest of the file uses num_foo or foo_count. Can we opt for one of
> those ?
>

Ah yep, num_properties looks good to me.


>
> > +};
> > +
> > +#define I915_PERF_IOCTL_ENABLE _IO('i', 0x0)
> > +#define I915_PERF_IOCTL_DISABLE        _IO('i', 0x1)
> > +
> > +/**
> > + * Common to all i915 perf records
> > + */
> > +struct drm_i915_perf_record_header {
> > +       __u32 type;
> > +       __u16 pad;
> Move pad at the end ? This way we can (maybe?) reuse it in the future.
>

This header was originally based on struct perf_event_header which is layed
out with the padding in the middle too. I can't currently think of a strong
reason to maintain a record header that's ABI compatible with perf, but
/maybe/ it could be convenient in some case to have a common layout for
profiling tools that deal with both. I think we can consider it reserved
for future use either way.


>
> > +       __u16 size;
> > +};
> > +
>
> Daniel, is there a consensus about zeroing the pad from userspace and
> checking it for garbage in the ioctl ? The documentation says "do it",
> yet I have a hunch that we're missing a few. Also what error should
> the ioctl return in such a case - EINVAL or EFAULT ? Worth
> documenting, just in case ?
>

In case you're wondering about the padding in the header above, note that
this header never gets passed in from userspace, it's a header for data
read by userspace and the driver zeros the padding.

For the hole in the ioctl I'll probably fill that by extending the flags
member and the driver currently returns -EINVAL if any unknown flag bits
are set by userspace.


Thanks for looking through this,
- Robert


>
> -Emil
>
On Thu, Feb 4, 2016 at 1:17 PM, Robert Bragg <robert@sixbynine.org> wrote:

>
> On Thu, Feb 4, 2016 at 1:42 AM, Emil Velikov <emil.l.velikov@gmail.com>
> wrote:
>
>> On 3 February 2016 at 18:39, Robert Bragg <robert@sixbynine.org> wrote:
>>
>>>
>>> > +};
>>> > +
>>> > +struct drm_i915_perf_open_param {
>>> > +       /** CLOEXEC, NONBLOCK... */
>>> > +       __u32 flags;
>>> > +
>>> And ... we broke 32 bit userspare on 64 bit kernels. Please check for
>>> holes and other issues as described in Daniel Vetter's
>>> article/documentation [1]
>>>
>>> [1]
>>> https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.txt
>>
>>
>> Ah yeah, don't think this would break 32bit userspace, but still would be
>> good to remove that hole, this has been through a few iterations and there
>> used to be a __u32 type here, well spotted thanks.
>>
>> I think I'll bump the flags to be 64bit.
>>
>>
>
Actually, just noticed that since the structure has a u32 hole at the end
too I can move the trailing u32 num_properties up into here instead.

Am also renaming properties to properties_ptr which seems the norm in
i915_drm.h.

- Robert