[RFC 0/8] Introduce framework for forwarding generic non-OA performance

[RFC 0/8] Introduce framework for forwarding generic non-OA performance

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This is an updated patch set (changes list at end), which builds upon the
multi context OA patch set introduced earlier at:

http://lists.freedesktop.org/archives/intel-gfx/2015-July/071697.html

The OA unit, as such, is specific to render ring and can't cater to performance
data requirements for other GPU engines.
Specifically, the media workloads may utilize other GPU engines, but there is
currently no framework which can be used to query performance statistics for
non-RCS workloads and provide this data to userspace tools. This patch set
tries to address this specific problem. The aim of this patch series is to
build upon the perf event framework developed earlier and use it for
forwarding performance data of non-RCS engine workloads.

Since the previous PMU is customized to handle OA reports, a new perf PMU is
added to handle generic non-OA performance data. An example of such non-OA
performance data is the timestamps/mmio registers.
This patch set enables framework for capturing the timestamps at
batch buffer boundaries, by inserting commands for the same in ringbuffer,
and forwarding the samples to userspace through perf interface.
Nevertheless, the framework and data structures can be extended to introduce
more performance data types (other than timestamps). The intention here is to
introduce a framework to enable capturing of generic performance data and
forwarding the same to userspace using perf apis.

The reports generated will again have an additional footer for metadata
information such as ctx_id, pid, ring id and tags (in the same way as done
for OA reports specified in the patch series earlier). This information can be
used by userspace tools such as MVP (Modular Video Profiler) to associate
reports with individual contexts and different stages of workload execution.

In this patch set, the timestamps are captured at BB boundaries by inserting
the commands in the ringbuffer at the batchbuffer boundaries. As specified
earlier, for a system wide GPU profiler, the relative complexity of doing this
in kernel is significantly less than supporting this usecase through userspace
command insertion by all the different components.

The final patch in the series tries to extend the data structures to enable
capture of upto 8 MMIO register values, in conjunction with timestamps

v2: This patch series has the following changes wrt the one floated earlier:
    - Removing synchronous waits during event stop/destroy
    - segregating the book-keeping data for the samples from destination buffer
      and collecting it into a separate list
    - managing the lifetime of destination buffer with the help of gem active
      reference tracking
    - having the scope of i915 device mutex limited to places of gem interaction
      and having the pmu data structures protected with a per pmu lock
    - userspace can now control the metadata it wants by requesting the same
      during event init. The sample is sent with the requested metadata in a
      packed format.
    - Some patches merged together and a few more introduced
    - mmio whitelist in place

Sourab Gupta (8):
  drm/i915: Add a new PMU for handling non-OA counter data profiling
    requests
  drm/i915: Add mechanism for forwarding the timestamp data through perf
  drm/i915: Handle event stop and destroy for GPU commands submitted
  drm/i915: Insert commands for capturing timestamps in the ring
  drm/i915: Add support for forwarding ring id in sample metadata
    through perf
  drm/i915: Add support for forwarding pid in timestamp sample metadata
    through perf
  drm/i915: Add support for forwarding execbuffer tags in timestamp
    sample metadata
  drm/i915: Support for retrieving MMIO register values alongwith
    timestamps through perf

 drivers/gpu/drm/i915/i915_dma.c     |   2 +
 drivers/gpu/drm/i915/i915_drv.h     |  41 +++
 drivers/gpu/drm/i915/i915_oa_perf.c | 680 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     |   2 +
 include/uapi/drm/i915_drm.h         |  41 +++
 5 files changed, 766 insertions(+)

-- 
1.8.5.1

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

[RFC 1/8] drm/i915: Add a new PMU for handling non-OA counter data profiling requests

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

The current perf PMU driver is specific for collection of OA counter
statistics (which may be done in a periodic or asynchronous way). Since
this enables us (and limits us) to render ring, we have no means for
collection of data pertaining to other rings.

To overcome this limitation, we need to have a new PMU driver which enables
data collection for other rings also (in a non-OA specific mode).
This patch adds a new perf PMU to i915 device private, for handling
profiling requests for non-OA counter data.This data may encompass
timestamps, mmio register values, etc. for the relevant ring.
The new perf PMU will serve these purposes, without constraining itself to
type of data being dumped (which may restrict the user to specific ring
like in case of OA counters).

The patch introduces this PMU driver alongwith its associated callbacks.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c     |   2 +
 drivers/gpu/drm/i915/i915_drv.h     |  18 +++
 drivers/gpu/drm/i915/i915_oa_perf.c | 220 ++++++++++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0553f20..4b91504 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -822,6 +822,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	 * otherwise i915_oa_context_pin_notify() will lock an un-initialized
 	 * spinlock, upsetting lockdep checks */
 	i915_oa_pmu_register(dev);
+	i915_gen_pmu_register(dev);
 
 	intel_pm_setup(dev);
 
@@ -1072,6 +1073,7 @@ int i915_driver_unload(struct drm_device *dev)
 		return ret;
 	}
 
+	i915_gen_pmu_unregister(dev);
 	i915_oa_pmu_unregister(dev);
 	intel_power_domains_fini(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9409b4a..7e8e77b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1985,6 +1985,20 @@ struct drm_i915_private {
 		int sample_info_flags;
 	} oa_pmu;
 
+	struct {
+		struct pmu pmu;
+		spinlock_t lock;
+		struct hrtimer timer;
+		struct pt_regs dummy_regs;
+		struct perf_event *exclusive_event;
+		bool event_active;
+
+		struct {
+			struct drm_i915_gem_object *obj;
+			u8 *addr;
+		} buffer;
+	} gen_pmu;
+
 	void (*insert_profile_cmd[I915_PROFILE_MAX])
 		(struct intel_ringbuffer *ringbuf, u32 ctx_id, int tag);
 #endif
@@ -3292,10 +3306,14 @@ int i915_parse_cmds(struct intel_engine_cs *ring,
 /* i915_oa_perf.c */
 #ifdef CONFIG_PERF_EVENTS
 extern void i915_oa_pmu_register(struct drm_device *dev);
+extern void i915_gen_pmu_register(struct drm_device *dev);
 extern void i915_oa_pmu_unregister(struct drm_device *dev);
+extern void i915_gen_pmu_unregister(struct drm_device *dev);
 #else
 static inline void i915_oa_pmu_register(struct drm_device *dev) {}
+static inline void i915_gen_pmu_register(struct drm_device *dev) {}
 static inline void i915_oa_pmu_unregister(struct drm_device *dev) {}
+static inline void i915_gen_pmu_unregister(struct drm_device *dev) {}
 #endif
 
 /* i915_suspend.c */
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index 839ebb4..ab965b4 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -408,6 +408,13 @@ void forward_oa_rcs_snapshots_work(struct work_struct *__work)
 	}
 }
 
+static void gen_pmu_flush_snapshots(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!dev_priv->gen_pmu.buffer.addr);
+
+	/* TODO: routine for forwarding snapshots to userspace */
+}
+
 static void
 oa_rcs_buffer_destroy(struct drm_i915_private *i915)
 {
@@ -561,6 +568,35 @@ out:
 	spin_unlock_irqrestore(&dev_priv->oa_pmu.lock, lock_flags);
 }
 
+static void gen_buffer_destroy(struct drm_i915_private *i915)
+{
+	unsigned long lock_flags;
+
+	mutex_lock(&i915->dev->struct_mutex);
+	vunmap(i915->gen_pmu.buffer.addr);
+	i915_gem_object_ggtt_unpin(i915->gen_pmu.buffer.obj);
+	drm_gem_object_unreference(&i915->gen_pmu.buffer.obj->base);
+	mutex_unlock(&i915->dev->struct_mutex);
+
+	spin_lock_irqsave(&i915->gen_pmu.lock, lock_flags);
+	i915->gen_pmu.buffer.obj = NULL;
+	i915->gen_pmu.buffer.addr = NULL;
+	spin_unlock_irqrestore(&i915->gen_pmu.lock, lock_flags);
+}
+
+static void i915_gen_event_destroy(struct perf_event *event)
+{
+	struct drm_i915_private *i915 =
+		container_of(event->pmu, typeof(*i915), gen_pmu.pmu);
+
+	WARN_ON(event->parent);
+
+	gen_buffer_destroy(i915);
+
+	BUG_ON(i915->gen_pmu.exclusive_event != event);
+	i915->gen_pmu.exclusive_event = NULL;
+}
+
 static int alloc_obj(struct drm_i915_private *dev_priv,
 				struct drm_i915_gem_object **obj)
 {
@@ -720,6 +756,40 @@ static int init_oa_rcs_buffer(struct perf_event *event)
 	return 0;
 }
 
+static int init_gen_pmu_buffer(struct perf_event *event)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
+	struct drm_i915_gem_object *bo;
+	int ret;
+
+	BUG_ON(dev_priv->gen_pmu.buffer.obj);
+
+	ret = alloc_obj(dev_priv, &bo);
+	if (ret)
+		return ret;
+
+	dev_priv->gen_pmu.buffer.obj = bo;
+
+	dev_priv->gen_pmu.buffer.addr = vmap_oa_buffer(bo);
+
+	DRM_DEBUG_DRIVER("Gen PMU Buffer initialized, vaddr = %p",
+			 dev_priv->gen_pmu.buffer.addr);
+
+	return 0;
+}
+
+static enum hrtimer_restart hrtimer_sample_gen(struct hrtimer *hrtimer)
+{
+	struct drm_i915_private *i915 =
+		container_of(hrtimer, typeof(*i915), gen_pmu.timer);
+
+	gen_pmu_flush_snapshots(i915);
+
+	hrtimer_forward_now(hrtimer, ns_to_ktime(PERIOD));
+	return HRTIMER_RESTART;
+}
+
 static enum hrtimer_restart hrtimer_sample(struct hrtimer *hrtimer)
 {
 	struct drm_i915_private *i915 =
@@ -1232,6 +1302,111 @@ static int i915_oa_event_event_idx(struct perf_event *event)
 	return 0;
 }
 
+static int i915_gen_event_init(struct perf_event *event)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
+	int ret = 0;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	/* To avoid the complexity of having to accurately filter
+	 * data and marshal to the appropriate client
+	 * we currently only allow exclusive access */
+	if (dev_priv->gen_pmu.buffer.obj)
+		return -EBUSY;
+
+	/*
+	 * We need to check for CAP_SYS_ADMIN capability as we profile all
+	 * the running contexts
+	 */
+	if (!capable(CAP_SYS_ADMIN))
+			return -EACCES;
+
+	ret = init_gen_pmu_buffer(event);
+	if (ret)
+		return ret;
+
+	BUG_ON(dev_priv->gen_pmu.exclusive_event);
+	dev_priv->gen_pmu.exclusive_event = event;
+
+	event->destroy = i915_gen_event_destroy;
+
+	return 0;
+}
+
+static void i915_gen_event_start(struct perf_event *event, int flags)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
+	unsigned long lock_flags;
+
+	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+
+	dev_priv->gen_pmu.event_active = true;
+
+	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+
+	__hrtimer_start_range_ns(&dev_priv->gen_pmu.timer, ns_to_ktime(PERIOD),
+					0, HRTIMER_MODE_REL_PINNED, 0);
+
+	event->hw.state = 0;
+}
+
+static void i915_gen_event_stop(struct perf_event *event, int flags)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
+	unsigned long lock_flags;
+
+	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+	dev_priv->gen_pmu.event_active = false;
+
+	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+
+	hrtimer_cancel(&dev_priv->gen_pmu.timer);
+	gen_pmu_flush_snapshots(dev_priv);
+
+	event->hw.state = PERF_HES_STOPPED;
+}
+
+static int i915_gen_event_add(struct perf_event *event, int flags)
+{
+	if (flags & PERF_EF_START)
+		i915_gen_event_start(event, flags);
+
+	return 0;
+}
+
+static void i915_gen_event_del(struct perf_event *event, int flags)
+{
+	i915_gen_event_stop(event, flags);
+}
+
+static void i915_gen_event_read(struct perf_event *event)
+{
+	struct drm_i915_private *i915 =
+		container_of(event->pmu, typeof(*i915), gen_pmu.pmu);
+
+	/* XXX: What counter would be useful here? */
+	local64_set(&event->count, 0);
+}
+
+static int i915_gen_event_flush(struct perf_event *event)
+{
+	struct drm_i915_private *i915 =
+		container_of(event->pmu, typeof(*i915), gen_pmu.pmu);
+
+	gen_pmu_flush_snapshots(i915);
+	return 0;
+}
+
+static int i915_gen_event_event_idx(struct perf_event *event)
+{
+	return 0;
+}
+
 void i915_oa_context_pin_notify(struct drm_i915_private *dev_priv,
 				struct intel_context *context)
 {
@@ -1360,3 +1535,48 @@ void i915_oa_pmu_unregister(struct drm_device *dev)
 	perf_pmu_unregister(&i915->oa_pmu.pmu);
 	i915->oa_pmu.pmu.event_init = NULL;
 }
+
+void i915_gen_pmu_register(struct drm_device *dev)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+
+	if (!(IS_HASWELL(dev) || IS_VALLEYVIEW(dev) || IS_BROADWELL(dev)))
+		return;
+
+	i915->gen_pmu.dummy_regs = *task_pt_regs(current);
+
+	hrtimer_init(&i915->gen_pmu.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	i915->gen_pmu.timer.function = hrtimer_sample_gen;
+
+	spin_lock_init(&i915->gen_pmu.lock);
+
+	i915->gen_pmu.pmu.capabilities  = PERF_PMU_CAP_IS_DEVICE;
+
+	/* Effectively disallow opening an event with a specific pid
+	 * since we aren't interested in processes running on the cpu...
+	 */
+	i915->gen_pmu.pmu.task_ctx_nr   = perf_invalid_context;
+
+	i915->gen_pmu.pmu.event_init    = i915_gen_event_init;
+	i915->gen_pmu.pmu.add	       = i915_gen_event_add;
+	i915->gen_pmu.pmu.del	       = i915_gen_event_del;
+	i915->gen_pmu.pmu.start	       = i915_gen_event_start;
+	i915->gen_pmu.pmu.stop	       = i915_gen_event_stop;
+	i915->gen_pmu.pmu.read	       = i915_gen_event_read;
+	i915->gen_pmu.pmu.flush	       = i915_gen_event_flush;
+	i915->gen_pmu.pmu.event_idx     = i915_gen_event_event_idx;
+
+	if (perf_pmu_register(&i915->gen_pmu.pmu, "i915_gen", -1))
+		i915->gen_pmu.pmu.event_init = NULL;
+}
+
+void i915_gen_pmu_unregister(struct drm_device *dev)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+
+	if (i915->gen_pmu.pmu.event_init == NULL)
+		return;
+
+	perf_pmu_unregister(&i915->gen_pmu.pmu);
+	i915->gen_pmu.pmu.event_init = NULL;
+}
-- 
1.8.5.1

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

[RFC 2/8] drm/i915: Add mechanism for forwarding the timestamp data through perf

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch adds the mechanism for forwarding the timestamp data to
userspace using the Gen PMU perf event interface.

The timestamps will be captured in a gem buffer object. The metadata
information (ctx_id right now) pertaining to snapshot is maintained in a
list, whose each node has offsets into the gem buffer object for each
snapshot captured.
In order to track whether the gpu has completed processing the node,
a field pertaining to corresponding gem request is added. The request is
expected to be referenced whenever the gpu command is submitted.

Each snapshot collected is forwarded as a separate perf sample. The perf
sample will have raw timestamp data followed by metadata information
pertaining to that sample.
While forwarding the samples, we check whether the gem request is completed
and dereference the corresponding request. The need to dereference the
request necessitates a worker here, which will be scheduled when the
hrtimer triggers.
While flushing the samples, we have to wait for the requests already
scheduled, before forwarding the samples. This wait is done in a lockless
fashion.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  11 ++++
 drivers/gpu/drm/i915/i915_oa_perf.c | 118 +++++++++++++++++++++++++++++++++++-
 include/uapi/drm/i915_drm.h         |  10 +++
 3 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7e8e77b..6984150 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1675,6 +1675,13 @@ struct i915_oa_rcs_node {
 	u32 tag;
 };
 
+struct i915_gen_pmu_node {
+	struct list_head head;
+	struct drm_i915_gem_request *req;
+	u32 offset;
+	u32 ctx_id;
+};
+
 extern const struct i915_oa_reg i915_oa_3d_mux_config_hsw[];
 extern const int i915_oa_3d_mux_config_hsw_len;
 extern const struct i915_oa_reg i915_oa_3d_b_counter_config_hsw[];
@@ -1996,7 +2003,11 @@ struct drm_i915_private {
 		struct {
 			struct drm_i915_gem_object *obj;
 			u8 *addr;
+			u32 node_size;
+			u32 node_count;
 		} buffer;
+		struct list_head node_list;
+		struct work_struct work_timer;
 	} gen_pmu;
 
 	void (*insert_profile_cmd[I915_PROFILE_MAX])
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index ab965b4..350b560 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -408,11 +408,108 @@ void forward_oa_rcs_snapshots_work(struct work_struct *__work)
 	}
 }
 
+int i915_gen_pmu_wait_gpu(struct drm_i915_private *dev_priv)
+{
+	struct i915_gen_pmu_node *last_entry;
+	unsigned long lock_flags;
+	int ret;
+
+	/*
+	 * Wait for the last scheduled request to complete. This would
+	 * implicitly wait for the prior submitted requests. The refcount
+	 * of the requests is not decremented here.
+	 */
+	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+
+	if (list_empty(&dev_priv->gen_pmu.node_list)) {
+		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+		return 0;
+	}
+	last_entry = list_last_entry(&dev_priv->gen_pmu.node_list,
+			struct i915_gen_pmu_node, head);
+	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+
+	if (last_entry && last_entry->req) {
+		ret = __i915_wait_request(last_entry->req, atomic_read(
+				&dev_priv->gpu_error.reset_counter),
+				dev_priv->mm.interruptible, NULL, NULL);
+		if (ret) {
+			DRM_ERROR("failed to wait\n");
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
+				struct i915_gen_pmu_node *node)
+{
+	struct perf_sample_data data;
+	struct perf_event *event = dev_priv->gen_pmu.exclusive_event;
+	int ts_size, snapshot_size;
+	u8 *snapshot;
+	struct drm_i915_ts_node_ctx_id *ctx_info;
+	struct perf_raw_record raw;
+
+	ts_size = sizeof(struct drm_i915_ts_data);
+	snapshot_size = ts_size + sizeof(*ctx_info);
+	snapshot = dev_priv->gen_pmu.buffer.addr + node->offset;
+
+	ctx_info = (struct drm_i915_ts_node_ctx_id *)(snapshot + ts_size);
+	ctx_info->ctx_id = node->ctx_id;
+
+	perf_sample_data_init(&data, 0, event->hw.last_period);
+
+	/* Note: the combined u32 raw->size member + raw data itself must be 8
+	 * byte aligned. (See note in init_gen_pmu_buffer for more details) */
+	raw.size = snapshot_size + 4;
+	raw.data = snapshot;
+
+	data.raw = &raw;
+
+	perf_event_overflow(event, &data, &dev_priv->gen_pmu.dummy_regs);
+}
+
+void forward_gen_pmu_snapshots_work(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(__work, typeof(*dev_priv), gen_pmu.work_timer);
+	struct i915_gen_pmu_node *entry, *next;
+	struct drm_i915_gem_request *req;
+	unsigned long lock_flags;
+	int ret;
+
+	list_for_each_entry_safe
+		(entry, next, &dev_priv->gen_pmu.node_list, head) {
+		req = entry->req;
+		if (req && i915_gem_request_completed(req, true)) {
+			forward_one_gen_pmu_sample(dev_priv, entry);
+			ret = i915_mutex_lock_interruptible(dev_priv->dev);
+			if (ret)
+				break;
+			i915_gem_request_assign(&entry->req, NULL);
+			mutex_unlock(&dev_priv->dev->struct_mutex);
+		} else
+			break;
+
+		/*
+		 * Do we instead need to protect whole loop? If so, we would
+		 * need to *list_move_tail* to a deferred list, from where
+		 * i915 device mutex could be taken to deference the requests,
+		 * and free the node.
+		 */
+		spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+		list_del(&entry->head);
+		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+		kfree(entry);
+	}
+}
+
 static void gen_pmu_flush_snapshots(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!dev_priv->gen_pmu.buffer.addr);
 
-	/* TODO: routine for forwarding snapshots to userspace */
+	schedule_work(&dev_priv->gen_pmu.work_timer);
 }
 
 static void
@@ -761,7 +858,7 @@ static int init_gen_pmu_buffer(struct perf_event *event)
 	struct drm_i915_private *dev_priv =
 		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
 	struct drm_i915_gem_object *bo;
-	int ret;
+	int ret, node_size;
 
 	BUG_ON(dev_priv->gen_pmu.buffer.obj);
 
@@ -772,6 +869,15 @@ static int init_gen_pmu_buffer(struct perf_event *event)
 	dev_priv->gen_pmu.buffer.obj = bo;
 
 	dev_priv->gen_pmu.buffer.addr = vmap_oa_buffer(bo);
+	INIT_LIST_HEAD(&dev_priv->gen_pmu.node_list);
+
+	node_size = sizeof(struct drm_i915_ts_data) +
+			sizeof(struct drm_i915_ts_node_ctx_id);
+
+	/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
+	node_size = ALIGN(node_size, 8);
+	dev_priv->gen_pmu.buffer.node_size = node_size;
+	dev_priv->gen_pmu.buffer.node_count = bo->base.size / node_size;
 
 	DRM_DEBUG_DRIVER("Gen PMU Buffer initialized, vaddr = %p",
 			 dev_priv->gen_pmu.buffer.addr);
@@ -1397,6 +1503,11 @@ static int i915_gen_event_flush(struct perf_event *event)
 {
 	struct drm_i915_private *i915 =
 		container_of(event->pmu, typeof(*i915), gen_pmu.pmu);
+	int ret;
+
+	ret = i915_gen_pmu_wait_gpu(i915);
+	if (ret)
+		return ret;
 
 	gen_pmu_flush_snapshots(i915);
 	return 0;
@@ -1548,6 +1659,7 @@ void i915_gen_pmu_register(struct drm_device *dev)
 	hrtimer_init(&i915->gen_pmu.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	i915->gen_pmu.timer.function = hrtimer_sample_gen;
 
+	INIT_WORK(&i915->gen_pmu.work_timer, forward_gen_pmu_snapshots_work);
 	spin_lock_init(&i915->gen_pmu.lock);
 
 	i915->gen_pmu.pmu.capabilities  = PERF_PMU_CAP_IS_DEVICE;
@@ -1577,6 +1689,8 @@ void i915_gen_pmu_unregister(struct drm_device *dev)
 	if (i915->gen_pmu.pmu.event_init == NULL)
 		return;
 
+	cancel_work_sync(&i915->gen_pmu.work_timer);
+
 	perf_pmu_unregister(&i915->gen_pmu.pmu);
 	i915->gen_pmu.pmu.event_init = NULL;
 }
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 1084178..9c083a2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -140,6 +140,16 @@ struct drm_i915_oa_node_tag {
 	__u32 pad;
 };
 
+struct drm_i915_ts_data {
+	__u32 ts_low;
+	__u32 ts_high;
+};
+
+struct drm_i915_ts_node_ctx_id {
+	__u32 ctx_id;
+	__u32 pad;
+};
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
1.8.5.1

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

[RFC 3/8] drm/i915: Handle event stop and destroy for GPU commands submitted

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch handles the event stop and destroy callbacks taking into account
the fact that there may be commands scheduled on GPU which may utilize the
destination buffer.

The event stop would just set the event state, and stop forwarding data to
userspace. From userspace perspective, for all purposes, the event sampling
is stopped.A subsequent event start (without event destroy) would start
forwarding samples again.

The event destroy releases the local copy of the dest buffer. But since it
is expected that the active reference of buffer is taken while inserting
commands, we can rest assured that buffer is freed up only after GPU is
done with it.
Still there is a need to schedule a worker from event destroy, because we
need to do some further stuff like freeing up request references.

The ideal solution here would be to have a callback when the last request
is finished on GPU, so that we can do this stuff there (WIP:Chris'
retire-notification mechanism). Till the time, a worker thread will do.

A subsequent event init would have to wait for previously submitted RPC
commands to complete or return -EBUSY. Currently, for the sake of
simplicity, we are returning -EBUSY if such a case is detected.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |   2 +
 drivers/gpu/drm/i915/i915_oa_perf.c | 104 +++++++++++++++++++++++++++++++-----
 2 files changed, 92 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6984150..41a01bd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1679,6 +1679,7 @@ struct i915_gen_pmu_node {
 	struct list_head head;
 	struct drm_i915_gem_request *req;
 	u32 offset;
+	bool discard;
 	u32 ctx_id;
 };
 
@@ -2008,6 +2009,7 @@ struct drm_i915_private {
 		} buffer;
 		struct list_head node_list;
 		struct work_struct work_timer;
+		struct work_struct work_event_destroy;
 	} gen_pmu;
 
 	void (*insert_profile_cmd[I915_PROFILE_MAX])
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index 350b560..107570e 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -441,6 +441,36 @@ int i915_gen_pmu_wait_gpu(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+void i915_gen_pmu_release_request_ref(struct drm_i915_private *dev_priv)
+{
+	struct i915_gen_pmu_node *entry, *next;
+	struct drm_i915_gem_request *req;
+	unsigned long lock_flags;
+	int ret;
+
+	list_for_each_entry_safe
+		(entry, next, &dev_priv->gen_pmu.node_list, head) {
+		req = entry->req;
+		if (req) {
+			ret = i915_mutex_lock_interruptible(dev_priv->dev);
+			if (ret)
+				break;
+			i915_gem_request_assign(&entry->req, NULL);
+			mutex_unlock(&dev_priv->dev->struct_mutex);
+		}
+
+		/*
+		 * This fn won't be running concurrently with forward snapshots
+		 * work fn. These are the only two places where list entries
+		 * will be deleted. So no need of protecting full loop?
+		 */
+		spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+		list_del(&entry->head);
+		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+		kfree(entry);
+	}
+}
+
 static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 				struct i915_gen_pmu_node *node)
 {
@@ -479,11 +509,19 @@ void forward_gen_pmu_snapshots_work(struct work_struct *__work)
 	unsigned long lock_flags;
 	int ret;
 
+	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+	if (dev_priv->gen_pmu.event_active == false) {
+		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+		return;
+	}
+	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+
 	list_for_each_entry_safe
 		(entry, next, &dev_priv->gen_pmu.node_list, head) {
 		req = entry->req;
 		if (req && i915_gem_request_completed(req, true)) {
-			forward_one_gen_pmu_sample(dev_priv, entry);
+			if (!entry->discard)
+				forward_one_gen_pmu_sample(dev_priv, entry);
 			ret = i915_mutex_lock_interruptible(dev_priv->dev);
 			if (ret)
 				break;
@@ -667,18 +705,11 @@ out:
 
 static void gen_buffer_destroy(struct drm_i915_private *i915)
 {
-	unsigned long lock_flags;
-
 	mutex_lock(&i915->dev->struct_mutex);
 	vunmap(i915->gen_pmu.buffer.addr);
 	i915_gem_object_ggtt_unpin(i915->gen_pmu.buffer.obj);
 	drm_gem_object_unreference(&i915->gen_pmu.buffer.obj->base);
 	mutex_unlock(&i915->dev->struct_mutex);
-
-	spin_lock_irqsave(&i915->gen_pmu.lock, lock_flags);
-	i915->gen_pmu.buffer.obj = NULL;
-	i915->gen_pmu.buffer.addr = NULL;
-	spin_unlock_irqrestore(&i915->gen_pmu.lock, lock_flags);
 }
 
 static void i915_gen_event_destroy(struct perf_event *event)
@@ -688,10 +719,44 @@ static void i915_gen_event_destroy(struct perf_event *event)
 
 	WARN_ON(event->parent);
 
-	gen_buffer_destroy(i915);
+	cancel_work_sync(&i915->gen_pmu.work_timer);
 
 	BUG_ON(i915->gen_pmu.exclusive_event != event);
 	i915->gen_pmu.exclusive_event = NULL;
+
+	/* We can deference our local copy of dest buffer here, since
+	 * an active reference of buffer would be taken while
+	 * inserting commands. So the buffer would be freed up only
+	 * after GPU is done with it.
+	 */
+	gen_buffer_destroy(i915);
+
+	schedule_work(&i915->gen_pmu.work_event_destroy);
+}
+
+void i915_gen_pmu_event_destroy_work(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(__work, typeof(*dev_priv),
+			gen_pmu.work_event_destroy);
+	unsigned long lock_flags;
+	int ret;
+
+	ret = i915_gen_pmu_wait_gpu(dev_priv);
+	if (ret)
+		goto out;
+
+	i915_gen_pmu_release_request_ref(dev_priv);
+
+out:
+	/*
+	 * Done at end, as this excludes a new event till we've done processing
+	 * the old one
+	 */
+	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+	dev_priv->gen_pmu.buffer.obj = NULL;
+	dev_priv->gen_pmu.buffer.addr = NULL;
+	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
 }
 
 static int alloc_obj(struct drm_i915_private *dev_priv,
@@ -1412,6 +1477,7 @@ static int i915_gen_event_init(struct perf_event *event)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
+	unsigned long lock_flags;
 	int ret = 0;
 
 	if (event->attr.type != event->pmu->type)
@@ -1420,8 +1486,12 @@ static int i915_gen_event_init(struct perf_event *event)
 	/* To avoid the complexity of having to accurately filter
 	 * data and marshal to the appropriate client
 	 * we currently only allow exclusive access */
-	if (dev_priv->gen_pmu.buffer.obj)
+	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+	if (dev_priv->gen_pmu.buffer.obj) {
+		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
 		return -EBUSY;
+	}
+	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
 
 	/*
 	 * We need to check for CAP_SYS_ADMIN capability as we profile all
@@ -1464,16 +1534,18 @@ static void i915_gen_event_stop(struct perf_event *event, int flags)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
+	struct i915_gen_pmu_node *entry;
 	unsigned long lock_flags;
 
+	hrtimer_cancel(&dev_priv->gen_pmu.timer);
+	gen_pmu_flush_snapshots(dev_priv);
+
 	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
 	dev_priv->gen_pmu.event_active = false;
-
+	list_for_each_entry(entry, &dev_priv->gen_pmu.node_list, head)
+		entry->discard = true;
 	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
 
-	hrtimer_cancel(&dev_priv->gen_pmu.timer);
-	gen_pmu_flush_snapshots(dev_priv);
-
 	event->hw.state = PERF_HES_STOPPED;
 }
 
@@ -1660,6 +1732,9 @@ void i915_gen_pmu_register(struct drm_device *dev)
 	i915->gen_pmu.timer.function = hrtimer_sample_gen;
 
 	INIT_WORK(&i915->gen_pmu.work_timer, forward_gen_pmu_snapshots_work);
+	INIT_WORK(&i915->gen_pmu.work_event_destroy,
+			i915_gen_pmu_event_destroy_work);
+
 	spin_lock_init(&i915->gen_pmu.lock);
 
 	i915->gen_pmu.pmu.capabilities  = PERF_PMU_CAP_IS_DEVICE;
@@ -1690,6 +1765,7 @@ void i915_gen_pmu_unregister(struct drm_device *dev)
 		return;
 
 	cancel_work_sync(&i915->gen_pmu.work_timer);
+	cancel_work_sync(&i915->gen_pmu.work_event_destroy);
 
 	perf_pmu_unregister(&i915->gen_pmu.pmu);
 	i915->gen_pmu.pmu.event_init = NULL;
-- 
1.8.5.1

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

[RFC 4/8] drm/i915: Insert commands for capturing timestamps in the ring

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch adds the routines through which one can insert commands in the
ringbuf for capturing timestamps, which are used to insert these commands
around the batchbuffer.

While inserting the commands, we keep a reference of associated request.
This will be released when we are forwarding the samples to userspace
(or when the event is being destroyed).
Also, an active reference of the destination buffer is taken here, so that
we can be assured that the buffer is freed up only after GPU is done with
it, even if the local reference of the buffer is released.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_oa_perf.c | 73 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     |  2 +
 3 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 41a01bd..59d23d0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1662,6 +1662,7 @@ enum i915_oa_event_state {
 
 enum i915_profile_mode {
 	I915_PROFILE_OA = 0,
+	I915_PROFILE_TS,
 	I915_PROFILE_MAX,
 };
 
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index 107570e..1780de42 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -102,6 +102,77 @@ void i915_oa_insert_cmd(struct intel_ringbuffer *ringbuf, u32 ctx_id, int tag)
 	i915_vma_move_to_active(i915_gem_obj_to_ggtt(obj), ring);
 }
 
+void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
+				int tag)
+{
+	struct intel_engine_cs *ring = ringbuf->ring;
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_i915_gem_object *obj = dev_priv->gen_pmu.buffer.obj;
+	struct i915_gen_pmu_node *entry;
+	unsigned long lock_flags;
+	u32 addr = 0;
+	int ret;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (entry == NULL) {
+		DRM_ERROR("alloc failed\n");
+		return;
+	}
+	entry->ctx_id = ctx_id;
+	i915_gem_request_assign(&entry->req, ring->outstanding_lazy_request);
+
+	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
+	if (list_empty(&dev_priv->gen_pmu.node_list))
+		entry->offset = 0;
+	else {
+		struct i915_gen_pmu_node *last_entry;
+		int max_offset = dev_priv->gen_pmu.buffer.node_count *
+				dev_priv->gen_pmu.buffer.node_size;
+
+		last_entry = list_last_entry(&dev_priv->gen_pmu.node_list,
+					struct i915_gen_pmu_node, head);
+		entry->offset = last_entry->offset +
+				dev_priv->gen_pmu.buffer.node_size;
+
+		if (entry->offset > max_offset)
+			entry->offset = 0;
+	}
+	list_add_tail(&entry->head, &dev_priv->gen_pmu.node_list);
+	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
+
+	addr = i915_gem_obj_ggtt_offset(obj) + entry->offset;
+
+	if (ring->id == RCS) {
+		ret = intel_ring_begin(ring, 6);
+		if (ret)
+			return;
+
+		intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(5));
+		intel_ring_emit(ring,
+				PIPE_CONTROL_GLOBAL_GTT_IVB |
+				PIPE_CONTROL_TIMESTAMP_WRITE);
+		intel_ring_emit(ring, addr | PIPE_CONTROL_GLOBAL_GTT);
+		intel_ring_emit(ring, 0); /* imm low, must be zero */
+		intel_ring_emit(ring, 0); /* imm high, must be zero */
+		intel_ring_emit(ring, MI_NOOP);
+		intel_ring_advance(ring);
+	} else {
+		ret = intel_ring_begin(ring, 4);
+		if (ret)
+			return;
+
+		intel_ring_emit(ring,
+				MI_FLUSH_DW | MI_FLUSH_DW_OP_STAMP);
+		intel_ring_emit(ring, addr | MI_FLUSH_DW_USE_GTT);
+		intel_ring_emit(ring, 0); /* imm low, must be zero */
+		intel_ring_emit(ring, 0); /* imm high, must be zero */
+		intel_ring_advance(ring);
+	}
+
+	obj->base.write_domain = I915_GEM_DOMAIN_RENDER;
+	i915_vma_move_to_active(i915_gem_obj_to_ggtt(obj), ring);
+}
+
 static void forward_one_oa_snapshot_to_event(struct drm_i915_private *dev_priv,
 					     u8 *snapshot,
 					     struct perf_event *event)
@@ -1521,6 +1592,7 @@ static void i915_gen_event_start(struct perf_event *event, int flags)
 	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
 
 	dev_priv->gen_pmu.event_active = true;
+	dev_priv->insert_profile_cmd[I915_PROFILE_TS] = i915_gen_insert_cmd_ts;
 
 	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
 
@@ -1542,6 +1614,7 @@ static void i915_gen_event_stop(struct perf_event *event, int flags)
 
 	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
 	dev_priv->gen_pmu.event_active = false;
+	dev_priv->insert_profile_cmd[I915_PROFILE_TS] = NULL;
 	list_for_each_entry(entry, &dev_priv->gen_pmu.node_list, head)
 		entry->discard = true;
 	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c9955968..f816b08 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -347,6 +347,7 @@
 #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)
 #define   MI_INVALIDATE_TLB		(1<<18)
 #define   MI_FLUSH_DW_OP_STOREDW	(1<<14)
+#define   MI_FLUSH_DW_OP_STAMP		(3<<14)
 #define   MI_FLUSH_DW_OP_MASK		(3<<14)
 #define   MI_FLUSH_DW_NOTIFY		(1<<8)
 #define   MI_INVALIDATE_BSD		(1<<7)
@@ -423,6 +424,7 @@
 #define   PIPE_CONTROL_MEDIA_STATE_CLEAR		(1<<16)
 #define   PIPE_CONTROL_QW_WRITE				(1<<14)
 #define   PIPE_CONTROL_POST_SYNC_OP_MASK                (3<<14)
+#define   PIPE_CONTROL_TIMESTAMP_WRITE			(3<<14)
 #define   PIPE_CONTROL_DEPTH_STALL			(1<<13)
 #define   PIPE_CONTROL_WRITE_FLUSH			(1<<12)
 #define   PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH	(1<<12) /* gen6+ */
-- 
1.8.5.1

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

[RFC 5/8] drm/i915: Add support for forwarding ring id in sample metadata through perf

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch introduces flags and adds support for having ring id output with
the timestamp samples and forwarding them through perf.

When the userspace expresses its interest in listening to the ring id
through a gen pmu attr field during event init, the samples generated would
have an additional field appended with the ring id information. This patch
enables this framework, which can be expanded upon to introduce further
fields in the gen pmu attr through which additional metadata information
can be appended to samples.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++
 drivers/gpu/drm/i915/i915_oa_perf.c | 90 ++++++++++++++++++++++++++++++++++++-
 include/uapi/drm/i915_drm.h         | 13 ++++++
 3 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 59d23d0..cf0528e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1682,6 +1682,7 @@ struct i915_gen_pmu_node {
 	u32 offset;
 	bool discard;
 	u32 ctx_id;
+	u32 ring;
 };
 
 extern const struct i915_oa_reg i915_oa_3d_mux_config_hsw[];
@@ -2011,6 +2012,8 @@ struct drm_i915_private {
 		struct list_head node_list;
 		struct work_struct work_timer;
 		struct work_struct work_event_destroy;
+#define I915_GEN_PMU_SAMPLE_RING		(1<<0)
+		int sample_info_flags;
 	} gen_pmu;
 
 	void (*insert_profile_cmd[I915_PROFILE_MAX])
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index 1780de42..5915720 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -102,6 +102,9 @@ void i915_oa_insert_cmd(struct intel_ringbuffer *ringbuf, u32 ctx_id, int tag)
 	i915_vma_move_to_active(i915_gem_obj_to_ggtt(obj), ring);
 }
 
+/* Returns the ring's ID mask (i.e. I915_EXEC_<ring>) */
+#define ring_id_mask(ring) ((ring)->id + 1)
+
 void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
 				int tag)
 {
@@ -119,6 +122,8 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
 		return;
 	}
 	entry->ctx_id = ctx_id;
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_RING)
+		entry->ring = ring_id_mask(ring);
 	i915_gem_request_assign(&entry->req, ring->outstanding_lazy_request);
 
 	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
@@ -548,8 +553,9 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 	struct perf_sample_data data;
 	struct perf_event *event = dev_priv->gen_pmu.exclusive_event;
 	int ts_size, snapshot_size;
-	u8 *snapshot;
+	u8 *snapshot, *current_ptr;
 	struct drm_i915_ts_node_ctx_id *ctx_info;
+	struct drm_i915_ts_node_ring_id *ring_info;
 	struct perf_raw_record raw;
 
 	ts_size = sizeof(struct drm_i915_ts_data);
@@ -558,6 +564,14 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 
 	ctx_info = (struct drm_i915_ts_node_ctx_id *)(snapshot + ts_size);
 	ctx_info->ctx_id = node->ctx_id;
+	current_ptr = snapshot + snapshot_size;
+
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_RING) {
+		ring_info = (struct drm_i915_ts_node_ring_id *)current_ptr;
+		ring_info->ring = node->ring;
+		snapshot_size += sizeof(*ring_info);
+		current_ptr = snapshot + snapshot_size;
+	}
 
 	perf_sample_data_init(&data, 0, event->hw.last_period);
 
@@ -1010,6 +1024,9 @@ static int init_gen_pmu_buffer(struct perf_event *event)
 	node_size = sizeof(struct drm_i915_ts_data) +
 			sizeof(struct drm_i915_ts_node_ctx_id);
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_RING)
+		node_size += sizeof(struct drm_i915_ts_node_ring_id);
+
 	/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
 	node_size = ALIGN(node_size, 8);
 	dev_priv->gen_pmu.buffer.node_size = node_size;
@@ -1544,16 +1561,87 @@ static int i915_oa_event_event_idx(struct perf_event *event)
 	return 0;
 }
 
+static int i915_gen_pmu_copy_attr(struct drm_i915_gen_pmu_attr __user *uattr,
+			     struct drm_i915_gen_pmu_attr *attr)
+{
+	u32 size;
+	int ret;
+
+	if (!access_ok(VERIFY_WRITE, uattr, I915_GEN_PMU_ATTR_SIZE_VER0))
+		return -EFAULT;
+
+	/*
+	 * zero the full structure, so that a short copy will be nice.
+	 */
+	memset(attr, 0, sizeof(*attr));
+
+	ret = get_user(size, &uattr->size);
+	if (ret)
+		return ret;
+
+	if (size > PAGE_SIZE)	/* silly large */
+		goto err_size;
+
+	if (size < I915_GEN_PMU_ATTR_SIZE_VER0)
+		goto err_size;
+
+	/*
+	 * If we're handed a bigger struct than we know of,
+	 * ensure all the unknown bits are 0 - i.e. new
+	 * user-space does not rely on any kernel feature
+	 * extensions we dont know about yet.
+	 */
+	if (size > sizeof(*attr)) {
+		unsigned char __user *addr;
+		unsigned char __user *end;
+		unsigned char val;
+
+		addr = (void __user *)uattr + sizeof(*attr);
+		end  = (void __user *)uattr + size;
+
+		for (; addr < end; addr++) {
+			ret = get_user(val, addr);
+			if (ret)
+				return ret;
+			if (val)
+				goto err_size;
+		}
+		size = sizeof(*attr);
+	}
+
+	ret = copy_from_user(attr, uattr, size);
+	if (ret)
+		return -EFAULT;
+
+out:
+	return ret;
+
+err_size:
+	put_user(sizeof(*attr), &uattr->size);
+	ret = -E2BIG;
+	goto out;
+}
+
 static int i915_gen_event_init(struct perf_event *event)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
+	struct drm_i915_gen_pmu_attr gen_attr;
 	unsigned long lock_flags;
 	int ret = 0;
 
 	if (event->attr.type != event->pmu->type)
 		return -ENOENT;
 
+	ret = i915_gen_pmu_copy_attr(to_user_ptr(event->attr.config),
+				&gen_attr);
+	if (ret)
+		return ret;
+
+	if (gen_attr.sample_ring)
+		dev_priv->gen_pmu.sample_info_flags |=
+				I915_GEN_PMU_SAMPLE_RING;
+
 	/* To avoid the complexity of having to accurately filter
 	 * data and marshal to the appropriate client
 	 * we currently only allow exclusive access */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 9c083a2..fd52926 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -81,6 +81,8 @@
 
 #define I915_OA_ATTR_SIZE_VER0		32  /* sizeof first published struct */
 
+#define I915_GEN_PMU_ATTR_SIZE_VER0	8  /* sizeof first published struct */
+
 typedef struct _drm_i915_oa_attr {
 	__u32 size;
 
@@ -98,6 +100,12 @@ typedef struct _drm_i915_oa_attr {
 		__reserved_1:60;
 } drm_i915_oa_attr_t;
 
+struct drm_i915_gen_pmu_attr {
+	__u32 size;
+	__u32 sample_ring:1,
+		__reserved_1:31;
+};
+
 /* Header for PERF_RECORD_DEVICE type events */
 typedef struct _drm_i915_oa_event_header {
 	__u32 type;
@@ -150,6 +158,11 @@ struct drm_i915_ts_node_ctx_id {
 	__u32 pad;
 };
 
+struct drm_i915_ts_node_ring_id {
+	__u32 ring;
+	__u32 pad;
+};
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
1.8.5.1

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

[RFC 6/8] drm/i915: Add support for forwarding pid in timestamp sample metadata through perf

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch introduces flags and adds support for having pid output with the
timestamp samples and forwarding them through perf.

When the userspace expresses its interest in listening to the pid through a
gen pmu attr field during event init, the samples generated would have an
additional field appended with the pid information.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 ++
 drivers/gpu/drm/i915/i915_oa_perf.c | 16 ++++++++++++++++
 include/uapi/drm/i915_drm.h         |  8 +++++++-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cf0528e..c23c5be 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1683,6 +1683,7 @@ struct i915_gen_pmu_node {
 	bool discard;
 	u32 ctx_id;
 	u32 ring;
+	u32 pid;
 };
 
 extern const struct i915_oa_reg i915_oa_3d_mux_config_hsw[];
@@ -2013,6 +2014,7 @@ struct drm_i915_private {
 		struct work_struct work_timer;
 		struct work_struct work_event_destroy;
 #define I915_GEN_PMU_SAMPLE_RING		(1<<0)
+#define I915_GEN_PMU_SAMPLE_PID			(1<<1)
 		int sample_info_flags;
 	} gen_pmu;
 
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index 5915720..a195c37 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -124,6 +124,8 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
 	entry->ctx_id = ctx_id;
 	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_RING)
 		entry->ring = ring_id_mask(ring);
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_PID)
+		entry->pid = current->pid;
 	i915_gem_request_assign(&entry->req, ring->outstanding_lazy_request);
 
 	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
@@ -556,6 +558,7 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 	u8 *snapshot, *current_ptr;
 	struct drm_i915_ts_node_ctx_id *ctx_info;
 	struct drm_i915_ts_node_ring_id *ring_info;
+	struct drm_i915_ts_node_pid *pid_info;
 	struct perf_raw_record raw;
 
 	ts_size = sizeof(struct drm_i915_ts_data);
@@ -573,6 +576,13 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 		current_ptr = snapshot + snapshot_size;
 	}
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_PID) {
+		pid_info = (struct drm_i915_ts_node_pid *)current_ptr;
+		pid_info->pid = node->pid;
+		snapshot_size += sizeof(*pid_info);
+		current_ptr = snapshot + snapshot_size;
+	}
+
 	perf_sample_data_init(&data, 0, event->hw.last_period);
 
 	/* Note: the combined u32 raw->size member + raw data itself must be 8
@@ -1027,6 +1037,9 @@ static int init_gen_pmu_buffer(struct perf_event *event)
 	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_RING)
 		node_size += sizeof(struct drm_i915_ts_node_ring_id);
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_PID)
+		node_size += sizeof(struct drm_i915_ts_node_pid);
+
 	/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
 	node_size = ALIGN(node_size, 8);
 	dev_priv->gen_pmu.buffer.node_size = node_size;
@@ -1642,6 +1655,9 @@ static int i915_gen_event_init(struct perf_event *event)
 		dev_priv->gen_pmu.sample_info_flags |=
 				I915_GEN_PMU_SAMPLE_RING;
 
+	if (gen_attr.sample_pid)
+		dev_priv->gen_pmu.sample_info_flags |= I915_GEN_PMU_SAMPLE_PID;
+
 	/* To avoid the complexity of having to accurately filter
 	 * data and marshal to the appropriate client
 	 * we currently only allow exclusive access */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index fd52926..393f4ec 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -103,7 +103,8 @@ typedef struct _drm_i915_oa_attr {
 struct drm_i915_gen_pmu_attr {
 	__u32 size;
 	__u32 sample_ring:1,
-		__reserved_1:31;
+		sample_pid:1,
+		__reserved_1:30;
 };
 
 /* Header for PERF_RECORD_DEVICE type events */
@@ -163,6 +164,11 @@ struct drm_i915_ts_node_ring_id {
 	__u32 pad;
 };
 
+struct drm_i915_ts_node_pid {
+	__u32 pid;
+	__u32 pad;
+};
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
1.8.5.1

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

[RFC 7/8] drm/i915: Add support for forwarding execbuffer tags in timestamp sample metadata

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch enables userspace to specify tags (per workload), provided via
execbuffer ioctl, which could be added to timestamps samples, to help
associate samples with the corresponding workloads.

There may be multiple stages within a single context, from a userspace
perspective. An ability is needed to individually associate the samples
with their corresponding workloads(execbuffers), which may not be possible
solely with ctx_id or pid information.
This patch enables such this mechanism.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 ++
 drivers/gpu/drm/i915/i915_oa_perf.c | 16 ++++++++++++++++
 include/uapi/drm/i915_drm.h         |  8 +++++++-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c23c5be..f2fe8d0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1684,6 +1684,7 @@ struct i915_gen_pmu_node {
 	u32 ctx_id;
 	u32 ring;
 	u32 pid;
+	u32 tag;
 };
 
 extern const struct i915_oa_reg i915_oa_3d_mux_config_hsw[];
@@ -2015,6 +2016,7 @@ struct drm_i915_private {
 		struct work_struct work_event_destroy;
 #define I915_GEN_PMU_SAMPLE_RING		(1<<0)
 #define I915_GEN_PMU_SAMPLE_PID			(1<<1)
+#define I915_GEN_PMU_SAMPLE_TAG			(1<<2)
 		int sample_info_flags;
 	} gen_pmu;
 
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index a195c37..1cc16ef 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -126,6 +126,8 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
 		entry->ring = ring_id_mask(ring);
 	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_PID)
 		entry->pid = current->pid;
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_TAG)
+		entry->tag = tag;
 	i915_gem_request_assign(&entry->req, ring->outstanding_lazy_request);
 
 	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
@@ -559,6 +561,7 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 	struct drm_i915_ts_node_ctx_id *ctx_info;
 	struct drm_i915_ts_node_ring_id *ring_info;
 	struct drm_i915_ts_node_pid *pid_info;
+	struct drm_i915_ts_node_tag *tag_info;
 	struct perf_raw_record raw;
 
 	ts_size = sizeof(struct drm_i915_ts_data);
@@ -583,6 +586,13 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 		current_ptr = snapshot + snapshot_size;
 	}
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_TAG) {
+		tag_info = (struct drm_i915_ts_node_tag *)current_ptr;
+		tag_info->tag = node->tag;
+		snapshot_size += sizeof(*tag_info);
+		current_ptr = snapshot + snapshot_size;
+	}
+
 	perf_sample_data_init(&data, 0, event->hw.last_period);
 
 	/* Note: the combined u32 raw->size member + raw data itself must be 8
@@ -1040,6 +1050,9 @@ static int init_gen_pmu_buffer(struct perf_event *event)
 	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_PID)
 		node_size += sizeof(struct drm_i915_ts_node_pid);
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_TAG)
+		node_size += sizeof(struct drm_i915_ts_node_tag);
+
 	/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
 	node_size = ALIGN(node_size, 8);
 	dev_priv->gen_pmu.buffer.node_size = node_size;
@@ -1658,6 +1671,9 @@ static int i915_gen_event_init(struct perf_event *event)
 	if (gen_attr.sample_pid)
 		dev_priv->gen_pmu.sample_info_flags |= I915_GEN_PMU_SAMPLE_PID;
 
+	if (gen_attr.sample_tag)
+		dev_priv->gen_pmu.sample_info_flags |= I915_GEN_PMU_SAMPLE_TAG;
+
 	/* To avoid the complexity of having to accurately filter
 	 * data and marshal to the appropriate client
 	 * we currently only allow exclusive access */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 393f4ec..7ab4972 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -104,7 +104,8 @@ struct drm_i915_gen_pmu_attr {
 	__u32 size;
 	__u32 sample_ring:1,
 		sample_pid:1,
-		__reserved_1:30;
+		sample_tag:1,
+		__reserved_1:29;
 };
 
 /* Header for PERF_RECORD_DEVICE type events */
@@ -169,6 +170,11 @@ struct drm_i915_ts_node_pid {
 	__u32 pad;
 };
 
+struct drm_i915_ts_node_tag {
+	__u32 tag;
+	__u32 pad;
+};
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
1.8.5.1

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

[RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch adds support for retrieving MMIO register values alongwith
timestamps and forwarding them to userspace through perf.
The userspace can request upto 8 MMIO register values to be dumped.
The addresses of upto 8 MMIO registers can be passed through perf attr
config. The registers are checked against a whitelist before passing them
on. The commands to dump the values of these MMIO registers are then
inserted into the ring alongwith commands to dump the timestamps.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 +
 drivers/gpu/drm/i915/i915_oa_perf.c | 87 ++++++++++++++++++++++++++++++++++---
 include/uapi/drm/i915_drm.h         | 10 ++++-
 3 files changed, 92 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f2fe8d0..e114175 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2017,7 +2017,9 @@ struct drm_i915_private {
 #define I915_GEN_PMU_SAMPLE_RING		(1<<0)
 #define I915_GEN_PMU_SAMPLE_PID			(1<<1)
 #define I915_GEN_PMU_SAMPLE_TAG			(1<<2)
+#define I915_GEN_PMU_SAMPLE_MMIO		(1<<3)
 		int sample_info_flags;
+		u32 mmio_list[8];
 	} gen_pmu;
 
 	void (*insert_profile_cmd[I915_PROFILE_MAX])
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index 1cc16ef..a9d44e0 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -113,8 +113,8 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
 	struct drm_i915_gem_object *obj = dev_priv->gen_pmu.buffer.obj;
 	struct i915_gen_pmu_node *entry;
 	unsigned long lock_flags;
-	u32 addr = 0;
-	int ret;
+	u32 mmio_addr, addr = 0;
+	int ret, i;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (entry == NULL) {
@@ -150,6 +150,7 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
 	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
 
 	addr = i915_gem_obj_ggtt_offset(obj) + entry->offset;
+	mmio_addr = addr + sizeof(struct drm_i915_ts_data);
 
 	if (ring->id == RCS) {
 		ret = intel_ring_begin(ring, 6);
@@ -177,6 +178,25 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
 		intel_ring_emit(ring, 0); /* imm high, must be zero */
 		intel_ring_advance(ring);
 	}
+	for (i = 0; i < 8; i++) {
+		if (0 == dev_priv->gen_pmu.mmio_list[i])
+			break;
+
+		addr = mmio_addr +
+			i * sizeof(dev_priv->gen_pmu.mmio_list[i]);
+
+		ret = intel_ring_begin(ring, 4);
+		if (ret)
+			return;
+
+		intel_ring_emit(ring,
+				MI_STORE_REGISTER_MEM(1) |
+				MI_SRM_LRM_GLOBAL_GTT);
+		intel_ring_emit(ring, dev_priv->gen_pmu.mmio_list[i]);
+		intel_ring_emit(ring, addr);
+		intel_ring_emit(ring, MI_NOOP);
+		intel_ring_advance(ring);
+	}
 
 	obj->base.write_domain = I915_GEM_DOMAIN_RENDER;
 	i915_vma_move_to_active(i915_gem_obj_to_ggtt(obj), ring);
@@ -556,7 +576,7 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 {
 	struct perf_sample_data data;
 	struct perf_event *event = dev_priv->gen_pmu.exclusive_event;
-	int ts_size, snapshot_size;
+	int ts_size, mmio_size, snapshot_size;
 	u8 *snapshot, *current_ptr;
 	struct drm_i915_ts_node_ctx_id *ctx_info;
 	struct drm_i915_ts_node_ring_id *ring_info;
@@ -565,10 +585,17 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 	struct perf_raw_record raw;
 
 	ts_size = sizeof(struct drm_i915_ts_data);
-	snapshot_size = ts_size + sizeof(*ctx_info);
+
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_MMIO)
+		mmio_size = sizeof(struct drm_i915_mmio_data);
+	else
+		mmio_size = 0;
+
 	snapshot = dev_priv->gen_pmu.buffer.addr + node->offset;
+	snapshot_size = ts_size + mmio_size + sizeof(*ctx_info);
 
-	ctx_info = (struct drm_i915_ts_node_ctx_id *)(snapshot + ts_size);
+	ctx_info = (struct drm_i915_ts_node_ctx_id *)
+			(snapshot + mmio_size + ts_size);
 	ctx_info->ctx_id = node->ctx_id;
 	current_ptr = snapshot + snapshot_size;
 
@@ -1053,6 +1080,9 @@ static int init_gen_pmu_buffer(struct perf_event *event)
 	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_TAG)
 		node_size += sizeof(struct drm_i915_ts_node_tag);
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_MMIO)
+		node_size += sizeof(struct drm_i915_mmio_data);
+
 	/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
 	node_size = ALIGN(node_size, 8);
 	dev_priv->gen_pmu.buffer.node_size = node_size;
@@ -1648,6 +1678,42 @@ err_size:
 	goto out;
 }
 
+#define GEN_RANGE(l, h) GENMASK(h, l)
+
+/* Some embargoed entries missing from whitelist */
+static const struct register_whitelist {
+	uint64_t offset;
+	uint32_t size;
+	/* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */
+	uint32_t gen_bitmask;
+} whitelist[] = {
+	{ GEN6_GT_GFX_RC6, 4, GEN_RANGE(7, 9) },
+	{ GEN6_GT_GFX_RC6p, 4, GEN_RANGE(7, 9) },
+};
+
+static int check_mmio_whitelist(struct drm_i915_private *dev_priv,
+				struct drm_i915_gen_pmu_attr *gen_attr)
+{
+	struct register_whitelist const *entry = whitelist;
+	int i, count;
+
+	for (count = 0; count < 8; count++) {
+		if (!gen_attr->mmio_list[count])
+			break;
+
+		for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
+			if (entry->offset == gen_attr->mmio_list[count] &&
+			    (1 << INTEL_INFO(dev_priv->dev)->gen &
+					entry->gen_bitmask))
+				break;
+		}
+
+		if (i == ARRAY_SIZE(whitelist))
+			return -EINVAL;
+	}
+	return 0;
+}
+
 static int i915_gen_event_init(struct perf_event *event)
 {
 	struct drm_i915_private *dev_priv =
@@ -1674,6 +1740,17 @@ static int i915_gen_event_init(struct perf_event *event)
 	if (gen_attr.sample_tag)
 		dev_priv->gen_pmu.sample_info_flags |= I915_GEN_PMU_SAMPLE_TAG;
 
+	if (gen_attr.sample_mmio) {
+		ret = check_mmio_whitelist(dev_priv, &gen_attr);
+		if (ret)
+			return ret;
+
+		dev_priv->gen_pmu.sample_info_flags |=
+				I915_GEN_PMU_SAMPLE_MMIO;
+		memcpy(dev_priv->gen_pmu.mmio_list, gen_attr.mmio_list,
+				sizeof(dev_priv->gen_pmu.mmio_list));
+	}
+
 	/* To avoid the complexity of having to accurately filter
 	 * data and marshal to the appropriate client
 	 * we currently only allow exclusive access */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7ab4972..65bc39d 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -81,7 +81,7 @@
 
 #define I915_OA_ATTR_SIZE_VER0		32  /* sizeof first published struct */
 
-#define I915_GEN_PMU_ATTR_SIZE_VER0	8  /* sizeof first published struct */
+#define I915_GEN_PMU_ATTR_SIZE_VER0	40  /* sizeof first published struct */
 
 typedef struct _drm_i915_oa_attr {
 	__u32 size;
@@ -105,7 +105,9 @@ struct drm_i915_gen_pmu_attr {
 	__u32 sample_ring:1,
 		sample_pid:1,
 		sample_tag:1,
-		__reserved_1:29;
+		sample_mmio:1,
+		__reserved_1:28;
+	__u32 mmio_list[8];
 };
 
 /* Header for PERF_RECORD_DEVICE type events */
@@ -155,6 +157,10 @@ struct drm_i915_ts_data {
 	__u32 ts_high;
 };
 
+struct drm_i915_mmio_data {
+	__u32 mmio[8];
+};
+
 struct drm_i915_ts_node_ctx_id {
 	__u32 ctx_id;
 	__u32 pad;
-- 
1.8.5.1

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

Re: [RFC 2/8] drm/i915: Add mechanism for forwarding the timestamp data through perf

[Chris Wilson]

On Wed, Jul 15, 2015 at 02:21:40PM +0530, sourab.gupta@intel.com wrote:
> From: Sourab Gupta <sourab.gupta@intel.com>
> 
> This patch adds the mechanism for forwarding the timestamp data to
> userspace using the Gen PMU perf event interface.
> 
> The timestamps will be captured in a gem buffer object. The metadata
> information (ctx_id right now) pertaining to snapshot is maintained in a
> list, whose each node has offsets into the gem buffer object for each
> snapshot captured.

What is the definition of ctx_id? The only persistent one is the
user_handle which is only valid within the file_priv namespace. There is
no guid for ctx at the moment.

> In order to track whether the gpu has completed processing the node,
> a field pertaining to corresponding gem request is added. The request is
> expected to be referenced whenever the gpu command is submitted.
> 
> Each snapshot collected is forwarded as a separate perf sample. The perf
> sample will have raw timestamp data followed by metadata information
> pertaining to that sample.
> While forwarding the samples, we check whether the gem request is completed
> and dereference the corresponding request. The need to dereference the
> request necessitates a worker here, which will be scheduled when the
> hrtimer triggers.
> While flushing the samples, we have to wait for the requests already
> scheduled, before forwarding the samples. This wait is done in a lockless
> fashion.
> 
> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  11 ++++
>  drivers/gpu/drm/i915/i915_oa_perf.c | 118 +++++++++++++++++++++++++++++++++++-
>  include/uapi/drm/i915_drm.h         |  10 +++
>  3 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7e8e77b..6984150 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1675,6 +1675,13 @@ struct i915_oa_rcs_node {
>  	u32 tag;
>  };
>  
> +struct i915_gen_pmu_node {
> +	struct list_head head;
> +	struct drm_i915_gem_request *req;
> +	u32 offset;
> +	u32 ctx_id;
> +};
> +
>  extern const struct i915_oa_reg i915_oa_3d_mux_config_hsw[];
>  extern const int i915_oa_3d_mux_config_hsw_len;
>  extern const struct i915_oa_reg i915_oa_3d_b_counter_config_hsw[];
> @@ -1996,7 +2003,11 @@ struct drm_i915_private {
>  		struct {
>  			struct drm_i915_gem_object *obj;
>  			u8 *addr;
> +			u32 node_size;
> +			u32 node_count;
>  		} buffer;
> +		struct list_head node_list;
> +		struct work_struct work_timer;
>  	} gen_pmu;
>  
>  	void (*insert_profile_cmd[I915_PROFILE_MAX])
> diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
> index ab965b4..350b560 100644
> --- a/drivers/gpu/drm/i915/i915_oa_perf.c
> +++ b/drivers/gpu/drm/i915/i915_oa_perf.c
> @@ -408,11 +408,108 @@ void forward_oa_rcs_snapshots_work(struct work_struct *__work)
>  	}
>  }
>  
> +int i915_gen_pmu_wait_gpu(struct drm_i915_private *dev_priv)

Why so many exports from this file? And why are half of them privately
named?

> +{
> +	struct i915_gen_pmu_node *last_entry;
> +	unsigned long lock_flags;
> +	int ret;
> +
> +	/*
> +	 * Wait for the last scheduled request to complete. This would
> +	 * implicitly wait for the prior submitted requests. The refcount
> +	 * of the requests is not decremented here.
> +	 */
> +	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);

Urm, are we really going to be called in irq context? I really hope you
are not planning on hooking in the irq handlers...

Afaict, you are just using the list from a timer context, so
spin_lock_bh() would be sufficient. Apparently it isn't a timer (thanks
for the misleading name!) so just spin_lock().

> +	if (list_empty(&dev_priv->gen_pmu.node_list)) {
> +		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
> +		return 0;
> +	}
> +	last_entry = list_last_entry(&dev_priv->gen_pmu.node_list,
> +			struct i915_gen_pmu_node, head);
> +	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);

You could write this a little neater with just one path through the
crticial section.

> +	if (last_entry && last_entry->req) {

last_entry cannot be NULL.

When is req NULL? Does the last_entry->req being NULL guarrantee that
all previous req are NULL?

> +		ret = __i915_wait_request(last_entry->req, atomic_read(
> +				&dev_priv->gpu_error.reset_counter),
> +				dev_priv->mm.interruptible, NULL, NULL);

Invalid use of dev_priv->mm.interruptible (just pass true).

> +		if (ret) {
> +			DRM_ERROR("failed to wait\n");
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
> +				struct i915_gen_pmu_node *node)
> +{
> +	struct perf_sample_data data;
> +	struct perf_event *event = dev_priv->gen_pmu.exclusive_event;
> +	int ts_size, snapshot_size;
> +	u8 *snapshot;
> +	struct drm_i915_ts_node_ctx_id *ctx_info;
> +	struct perf_raw_record raw;
> +
> +	ts_size = sizeof(struct drm_i915_ts_data);
> +	snapshot_size = ts_size + sizeof(*ctx_info);

If you kept these as compile time constants it will make the rest a bit
easier to follow.

> +	snapshot = dev_priv->gen_pmu.buffer.addr + node->offset;
> +
> +	ctx_info = (struct drm_i915_ts_node_ctx_id *)(snapshot + ts_size);
> +	ctx_info->ctx_id = node->ctx_id;
> +
> +	perf_sample_data_init(&data, 0, event->hw.last_period);
> +
> +	/* Note: the combined u32 raw->size member + raw data itself must be 8
> +	 * byte aligned. (See note in init_gen_pmu_buffer for more details) */

You mean this comment?
/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
That's not particularly enlightening.

Missing BUILD_BUG tests to assert that your structure sizes are what you
claim they should be. To illustrate this comment, you could do

BUILD_BUG_ON(sizeof(struct drm_i915_ts_data) != 8);
BUILD_BUG_ON(sizeof(struct drm_i915_ts_mode_ctx_id) != 8);
BUILD_BUG_ON((snapshot_size + 4 + sizeof(raw.size) % 8) == 0);

Otherwise the comment doesn't really make it clear exactly what has to
be aligned to 8 or *why*.

> +	raw.size = snapshot_size + 4;
> +	raw.data = snapshot;

> +	data.raw = &raw;
> +
> +	perf_event_overflow(event, &data, &dev_priv->gen_pmu.dummy_regs);
> +}
> +
> +void forward_gen_pmu_snapshots_work(struct work_struct *__work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(__work, typeof(*dev_priv), gen_pmu.work_timer);
> +	struct i915_gen_pmu_node *entry, *next;
> +	struct drm_i915_gem_request *req;
> +	unsigned long lock_flags;
> +	int ret;
> +
> +	list_for_each_entry_safe
> +		(entry, next, &dev_priv->gen_pmu.node_list, head) {
> +		req = entry->req;
> +		if (req && i915_gem_request_completed(req, true)) {

Negate the test and reduce indentation for ease of reading.

> +			forward_one_gen_pmu_sample(dev_priv, entry);
> +			ret = i915_mutex_lock_interruptible(dev_priv->dev);
> +			if (ret)
> +				break;
> +			i915_gem_request_assign(&entry->req, NULL);
> +			mutex_unlock(&dev_priv->dev->struct_mutex);

This is just i915_gem_request_unreference_unlocked().

> +		} else
> +			break;
> +
> +		/*
> +		 * Do we instead need to protect whole loop? If so, we would
> +		 * need to *list_move_tail* to a deferred list, from where
> +		 * i915 device mutex could be taken to deference the requests,
> +		 * and free the node.
> +		 */
> +		spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
> +		list_del(&entry->head);
> +		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
> +		kfree(entry);
> +	}
> +}
> +
>  static void gen_pmu_flush_snapshots(struct drm_i915_private *dev_priv)
>  {
>  	WARN_ON(!dev_priv->gen_pmu.buffer.addr);
>  
> -	/* TODO: routine for forwarding snapshots to userspace */
> +	schedule_work(&dev_priv->gen_pmu.work_timer);

Why are we scheduling a timer? Might be a bad name for a work item to
infer that it is a timer.

Why is this in a work queue if you already blocked during the flush?

>  }
>  
>  static void
> @@ -761,7 +858,7 @@ static int init_gen_pmu_buffer(struct perf_event *event)
>  	struct drm_i915_private *dev_priv =
>  		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
>  	struct drm_i915_gem_object *bo;
> -	int ret;
> +	int ret, node_size;
>  
>  	BUG_ON(dev_priv->gen_pmu.buffer.obj);
>  
> @@ -772,6 +869,15 @@ static int init_gen_pmu_buffer(struct perf_event *event)
>  	dev_priv->gen_pmu.buffer.obj = bo;
>  
>  	dev_priv->gen_pmu.buffer.addr = vmap_oa_buffer(bo);
> +	INIT_LIST_HEAD(&dev_priv->gen_pmu.node_list);
> +
> +	node_size = sizeof(struct drm_i915_ts_data) +
> +			sizeof(struct drm_i915_ts_node_ctx_id);
> +
> +	/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
> +	node_size = ALIGN(node_size, 8);
> +	dev_priv->gen_pmu.buffer.node_size = node_size;
> +	dev_priv->gen_pmu.buffer.node_count = bo->base.size / node_size;
>  
>  	DRM_DEBUG_DRIVER("Gen PMU Buffer initialized, vaddr = %p",
>  			 dev_priv->gen_pmu.buffer.addr);
> @@ -1397,6 +1503,11 @@ static int i915_gen_event_flush(struct perf_event *event)
>  {
>  	struct drm_i915_private *i915 =
>  		container_of(event->pmu, typeof(*i915), gen_pmu.pmu);
> +	int ret;
> +
> +	ret = i915_gen_pmu_wait_gpu(i915);
> +	if (ret)
> +		return ret;
>  
>  	gen_pmu_flush_snapshots(i915);

Wait for idle, then schedule a task???
-Chris
>   

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 2/8] drm/i915: Add mechanism for forwarding the timestamp data through perf

[Gupta, Sourab]

On Wed, 2015-07-15 at 09:40 +0000, Chris Wilson wrote:
> On Wed, Jul 15, 2015 at 02:21:40PM +0530, sourab.gupta@intel.com wrote:
> > From: Sourab Gupta <sourab.gupta@intel.com>
> > 
> > This patch adds the mechanism for forwarding the timestamp data to
> > userspace using the Gen PMU perf event interface.
> > 
> > The timestamps will be captured in a gem buffer object. The metadata
> > information (ctx_id right now) pertaining to snapshot is maintained in a
> > list, whose each node has offsets into the gem buffer object for each
> > snapshot captured.
> 
> What is the definition of ctx_id? The only persistent one is the
> user_handle which is only valid within the file_priv namespace. There is
> no guid for ctx at the moment.

Well, this patch set makes assumption on the availability of a globally
unique ctx_id. The first patch in the series proposes the same:
http://lists.freedesktop.org/archives/intel-gfx/2015-July/071698.html
Not sure, whether that would be acceptable though.
> 
> > In order to track whether the gpu has completed processing the node,
> > a field pertaining to corresponding gem request is added. The request is
> > expected to be referenced whenever the gpu command is submitted.
> > 
> > Each snapshot collected is forwarded as a separate perf sample. The perf
> > sample will have raw timestamp data followed by metadata information
> > pertaining to that sample.
> > While forwarding the samples, we check whether the gem request is completed
> > and dereference the corresponding request. The need to dereference the
> > request necessitates a worker here, which will be scheduled when the
> > hrtimer triggers.
> > While flushing the samples, we have to wait for the requests already
> > scheduled, before forwarding the samples. This wait is done in a lockless
> > fashion.
> > 
> > Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h     |  11 ++++
> >  drivers/gpu/drm/i915/i915_oa_perf.c | 118 +++++++++++++++++++++++++++++++++++-
> >  include/uapi/drm/i915_drm.h         |  10 +++
> >  3 files changed, 137 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7e8e77b..6984150 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1675,6 +1675,13 @@ struct i915_oa_rcs_node {
> >  	u32 tag;
> >  };
> >  
> > +struct i915_gen_pmu_node {
> > +	struct list_head head;
> > +	struct drm_i915_gem_request *req;
> > +	u32 offset;
> > +	u32 ctx_id;
> > +};
> > +
> >  extern const struct i915_oa_reg i915_oa_3d_mux_config_hsw[];
> >  extern const int i915_oa_3d_mux_config_hsw_len;
> >  extern const struct i915_oa_reg i915_oa_3d_b_counter_config_hsw[];
> > @@ -1996,7 +2003,11 @@ struct drm_i915_private {
> >  		struct {
> >  			struct drm_i915_gem_object *obj;
> >  			u8 *addr;
> > +			u32 node_size;
> > +			u32 node_count;
> >  		} buffer;
> > +		struct list_head node_list;
> > +		struct work_struct work_timer;
> >  	} gen_pmu;
> >  
> >  	void (*insert_profile_cmd[I915_PROFILE_MAX])
> > diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
> > index ab965b4..350b560 100644
> > --- a/drivers/gpu/drm/i915/i915_oa_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_oa_perf.c
> > @@ -408,11 +408,108 @@ void forward_oa_rcs_snapshots_work(struct work_struct *__work)
> >  	}
> >  }
> >  
> > +int i915_gen_pmu_wait_gpu(struct drm_i915_private *dev_priv)
> 
> Why so many exports from this file? And why are half of them privately
> named?

Acknowledged. The exports are not needed. Will have a static
declaration.
> 
> > +{
> > +	struct i915_gen_pmu_node *last_entry;
> > +	unsigned long lock_flags;
> > +	int ret;
> > +
> > +	/*
> > +	 * Wait for the last scheduled request to complete. This would
> > +	 * implicitly wait for the prior submitted requests. The refcount
> > +	 * of the requests is not decremented here.
> > +	 */
> > +	spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
> 
> Urm, are we really going to be called in irq context? I really hope you
> are not planning on hooking in the irq handlers...
> 
> Afaict, you are just using the list from a timer context, so
> spin_lock_bh() would be sufficient. Apparently it isn't a timer (thanks
> for the misleading name!) so just spin_lock().

No, it won't be called from irq context. I'll just use spin_lock() here.
> 
> > +	if (list_empty(&dev_priv->gen_pmu.node_list)) {
> > +		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
> > +		return 0;
> > +	}
> > +	last_entry = list_last_entry(&dev_priv->gen_pmu.node_list,
> > +			struct i915_gen_pmu_node, head);
> > +	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
> 
> You could write this a little neater with just one path through the
> crticial section.

Will rework on this.
> 
> > +	if (last_entry && last_entry->req) {
> 
> last_entry cannot be NULL.
> 
> When is req NULL? Does the last_entry->req being NULL guarrantee that
> all previous req are NULL?

Sorry, these extraneous checks may have crept in. Agreed that last_entry
can't be NULL, and same for req, since we have refcounted the req. Will
remove these.  
> 
> > +		ret = __i915_wait_request(last_entry->req, atomic_read(
> > +				&dev_priv->gpu_error.reset_counter),
> > +				dev_priv->mm.interruptible, NULL, NULL);
> 
> Invalid use of dev_priv->mm.interruptible (just pass true).

Will make this change.
> 
> > +		if (ret) {
> > +			DRM_ERROR("failed to wait\n");
> > +			return ret;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
> > +				struct i915_gen_pmu_node *node)
> > +{
> > +	struct perf_sample_data data;
> > +	struct perf_event *event = dev_priv->gen_pmu.exclusive_event;
> > +	int ts_size, snapshot_size;
> > +	u8 *snapshot;
> > +	struct drm_i915_ts_node_ctx_id *ctx_info;
> > +	struct perf_raw_record raw;
> > +
> > +	ts_size = sizeof(struct drm_i915_ts_data);
> > +	snapshot_size = ts_size + sizeof(*ctx_info);
> 
> If you kept these as compile time constants it will make the rest a bit
> easier to follow.
Will make this change.
> 
> > +	snapshot = dev_priv->gen_pmu.buffer.addr + node->offset;
> > +
> > +	ctx_info = (struct drm_i915_ts_node_ctx_id *)(snapshot + ts_size);
> > +	ctx_info->ctx_id = node->ctx_id;
> > +
> > +	perf_sample_data_init(&data, 0, event->hw.last_period);
> > +
> > +	/* Note: the combined u32 raw->size member + raw data itself must be 8
> > +	 * byte aligned. (See note in init_gen_pmu_buffer for more details) */
> 
> You mean this comment?
> /* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
> That's not particularly enlightening.
> 
> Missing BUILD_BUG tests to assert that your structure sizes are what you
> claim they should be. To illustrate this comment, you could do
> 
> BUILD_BUG_ON(sizeof(struct drm_i915_ts_data) != 8);
> BUILD_BUG_ON(sizeof(struct drm_i915_ts_mode_ctx_id) != 8);
> BUILD_BUG_ON((snapshot_size + 4 + sizeof(raw.size) % 8) == 0);
> 
> Otherwise the comment doesn't really make it clear exactly what has to
> be aligned to 8 or *why*.
> 

Actually, this check is for ensuring that the combined size of raw data
+ the size of 'raw->size' member field should be multiple of 8 bytes.
This check can be seen at kernel/events/core.c: perf_prepare_sample()
(the place where PERF_SAMPLE_RAW is processed).
Will reword the comment to make it clearer, in addition to having the
BUILD_BUG_ON macro checks.

> > +	raw.size = snapshot_size + 4;
> > +	raw.data = snapshot;
> 
> > +	data.raw = &raw;
> > +
> > +	perf_event_overflow(event, &data, &dev_priv->gen_pmu.dummy_regs);
> > +}
> > +
> > +void forward_gen_pmu_snapshots_work(struct work_struct *__work)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > +		container_of(__work, typeof(*dev_priv), gen_pmu.work_timer);
> > +	struct i915_gen_pmu_node *entry, *next;
> > +	struct drm_i915_gem_request *req;
> > +	unsigned long lock_flags;
> > +	int ret;
> > +
> > +	list_for_each_entry_safe
> > +		(entry, next, &dev_priv->gen_pmu.node_list, head) {
> > +		req = entry->req;
> > +		if (req && i915_gem_request_completed(req, true)) {
> 
> Negate the test and reduce indentation for ease of reading.

Ok, will do.
> 
> > +			forward_one_gen_pmu_sample(dev_priv, entry);
> > +			ret = i915_mutex_lock_interruptible(dev_priv->dev);
> > +			if (ret)
> > +				break;
> > +			i915_gem_request_assign(&entry->req, NULL);
> > +			mutex_unlock(&dev_priv->dev->struct_mutex);
> 
> This is just i915_gem_request_unreference_unlocked().

Right. Missed it. Will have this function.
> 
> > +		} else
> > +			break;
> > +
> > +		/*
> > +		 * Do we instead need to protect whole loop? If so, we would
> > +		 * need to *list_move_tail* to a deferred list, from where
> > +		 * i915 device mutex could be taken to deference the requests,
> > +		 * and free the node.
> > +		 */
> > +		spin_lock_irqsave(&dev_priv->gen_pmu.lock, lock_flags);
> > +		list_del(&entry->head);
> > +		spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
> > +		kfree(entry);
> > +	}
> > +}
> > +
> >  static void gen_pmu_flush_snapshots(struct drm_i915_private *dev_priv)
> >  {
> >  	WARN_ON(!dev_priv->gen_pmu.buffer.addr);
> >  
> > -	/* TODO: routine for forwarding snapshots to userspace */
> > +	schedule_work(&dev_priv->gen_pmu.work_timer);
> 
> Why are we scheduling a timer? Might be a bad name for a work item to
> infer that it is a timer.
> 
Well, this is a case of bad name and it's a work item really.

> Why is this in a work queue if you already blocked during the flush?
> 

Actually, the gen_pmu_flush_snapshots() fn is called from 3 places:
hrtimer, event_stop and event_flush. And we can block only from event
flush. So, this function not really tied with event flush only.

Also, the job of forwarding samples needs mutex to be taken (to
dereference the request : i915_gem_request_unreference_unlocked), and
since the forwarding has to be initiated from atomic context (e.g.
hrtimer), I created a wq for the same, with the intention of not having
code duplication for event flush.

Probably, the better sense would be to have the forwarding functionality
implemented in a seperate function. The work item (which would be
scheduled from hrtimer/event stop) would be calling that function. And
the the event flush would directly call this forwarding fn, without the
extraneous work item scheduled. Does this seem ok?

> >  }
> >  
> >  static void
> > @@ -761,7 +858,7 @@ static int init_gen_pmu_buffer(struct perf_event *event)
> >  	struct drm_i915_private *dev_priv =
> >  		container_of(event->pmu, typeof(*dev_priv), gen_pmu.pmu);
> >  	struct drm_i915_gem_object *bo;
> > -	int ret;
> > +	int ret, node_size;
> >  
> >  	BUG_ON(dev_priv->gen_pmu.buffer.obj);
> >  
> > @@ -772,6 +869,15 @@ static int init_gen_pmu_buffer(struct perf_event *event)
> >  	dev_priv->gen_pmu.buffer.obj = bo;
> >  
> >  	dev_priv->gen_pmu.buffer.addr = vmap_oa_buffer(bo);
> > +	INIT_LIST_HEAD(&dev_priv->gen_pmu.node_list);
> > +
> > +	node_size = sizeof(struct drm_i915_ts_data) +
> > +			sizeof(struct drm_i915_ts_node_ctx_id);
> > +
> > +	/* size has to be aligned to 8 bytes (required by relevant gpu cmds) */
> > +	node_size = ALIGN(node_size, 8);
> > +	dev_priv->gen_pmu.buffer.node_size = node_size;
> > +	dev_priv->gen_pmu.buffer.node_count = bo->base.size / node_size;
> >  
> >  	DRM_DEBUG_DRIVER("Gen PMU Buffer initialized, vaddr = %p",
> >  			 dev_priv->gen_pmu.buffer.addr);
> > @@ -1397,6 +1503,11 @@ static int i915_gen_event_flush(struct perf_event *event)
> >  {
> >  	struct drm_i915_private *i915 =
> >  		container_of(event->pmu, typeof(*i915), gen_pmu.pmu);
> > +	int ret;
> > +
> > +	ret = i915_gen_pmu_wait_gpu(i915);
> > +	if (ret)
> > +		return ret;
> >  
> >  	gen_pmu_flush_snapshots(i915);
> 
> Wait for idle, then schedule a task???
> -Chris
> >   
> 

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

Re: [RFC 2/8] drm/i915: Add mechanism for forwarding the timestamp data through perf

[Chris Wilson]

On Wed, Jul 15, 2015 at 11:30:13AM +0000, Gupta, Sourab wrote:
> On Wed, 2015-07-15 at 09:40 +0000, Chris Wilson wrote:
> > >  static void gen_pmu_flush_snapshots(struct drm_i915_private *dev_priv)
> > >  {
> > >  	WARN_ON(!dev_priv->gen_pmu.buffer.addr);
> > >  
> > > -	/* TODO: routine for forwarding snapshots to userspace */
> > > +	schedule_work(&dev_priv->gen_pmu.work_timer);
> > 
> > Why are we scheduling a timer? Might be a bad name for a work item to
> > infer that it is a timer.
> > 
> Well, this is a case of bad name and it's a work item really.
> 
> > Why is this in a work queue if you already blocked during the flush?
> > 
> 
> Actually, the gen_pmu_flush_snapshots() fn is called from 3 places:
> hrtimer, event_stop and event_flush. And we can block only from event
> flush. So, this function not really tied with event flush only.
> 
> Also, the job of forwarding samples needs mutex to be taken (to
> dereference the request : i915_gem_request_unreference_unlocked), and
> since the forwarding has to be initiated from atomic context (e.g.
> hrtimer), I created a wq for the same, with the intention of not having
> code duplication for event flush.
> 
> Probably, the better sense would be to have the forwarding functionality
> implemented in a seperate function. The work item (which would be
> scheduled from hrtimer/event stop) would be calling that function. And
> the the event flush would directly call this forwarding fn, without the
> extraneous work item scheduled. Does this seem ok?

Indeed that's an improvement for flush, where the semantics may be such
that after calling it we do expect to be able to process the samples (in
userspace) immediately.

Having a comment at the top the forward function would also help the
reader to understand the contexts from which we can be called (and why).
It's something we often lack, but is very useful for review.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[Chris Wilson]

On Wed, Jul 15, 2015 at 02:21:46PM +0530, sourab.gupta@intel.com wrote:
> From: Sourab Gupta <sourab.gupta@intel.com>
> 
> This patch adds support for retrieving MMIO register values alongwith
> timestamps and forwarding them to userspace through perf.
> The userspace can request upto 8 MMIO register values to be dumped.
> The addresses of upto 8 MMIO registers can be passed through perf attr
> config. The registers are checked against a whitelist before passing them
> on. The commands to dump the values of these MMIO registers are then
> inserted into the ring alongwith commands to dump the timestamps.
> 
> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +
>  drivers/gpu/drm/i915/i915_oa_perf.c | 87 ++++++++++++++++++++++++++++++++++---
>  include/uapi/drm/i915_drm.h         | 10 ++++-
>  3 files changed, 92 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f2fe8d0..e114175 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2017,7 +2017,9 @@ struct drm_i915_private {
>  #define I915_GEN_PMU_SAMPLE_RING		(1<<0)
>  #define I915_GEN_PMU_SAMPLE_PID			(1<<1)
>  #define I915_GEN_PMU_SAMPLE_TAG			(1<<2)
> +#define I915_GEN_PMU_SAMPLE_MMIO		(1<<3)
>  		int sample_info_flags;
> +		u32 mmio_list[8];
>  	} gen_pmu;
>  
>  	void (*insert_profile_cmd[I915_PROFILE_MAX])
> diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
> index 1cc16ef..a9d44e0 100644
> --- a/drivers/gpu/drm/i915/i915_oa_perf.c
> +++ b/drivers/gpu/drm/i915/i915_oa_perf.c
> @@ -113,8 +113,8 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
>  	struct drm_i915_gem_object *obj = dev_priv->gen_pmu.buffer.obj;
>  	struct i915_gen_pmu_node *entry;
>  	unsigned long lock_flags;
> -	u32 addr = 0;
> -	int ret;
> +	u32 mmio_addr, addr = 0;
> +	int ret, i;
>  
>  	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>  	if (entry == NULL) {
> @@ -150,6 +150,7 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
>  	spin_unlock_irqrestore(&dev_priv->gen_pmu.lock, lock_flags);
>  
>  	addr = i915_gem_obj_ggtt_offset(obj) + entry->offset;
> +	mmio_addr = addr + sizeof(struct drm_i915_ts_data);
>  
>  	if (ring->id == RCS) {
>  		ret = intel_ring_begin(ring, 6);
> @@ -177,6 +178,25 @@ void i915_gen_insert_cmd_ts(struct intel_ringbuffer *ringbuf, u32 ctx_id,
>  		intel_ring_emit(ring, 0); /* imm high, must be zero */
>  		intel_ring_advance(ring);
>  	}
> +	for (i = 0; i < 8; i++) {
> +		if (0 == dev_priv->gen_pmu.mmio_list[i])
> +			break;
> +
> +		addr = mmio_addr +
> +			i * sizeof(dev_priv->gen_pmu.mmio_list[i]);
> +
> +		ret = intel_ring_begin(ring, 4);
> +		if (ret)
> +			return;
> +
> +		intel_ring_emit(ring,
> +				MI_STORE_REGISTER_MEM(1) |
> +				MI_SRM_LRM_GLOBAL_GTT);
> +		intel_ring_emit(ring, dev_priv->gen_pmu.mmio_list[i]);
> +		intel_ring_emit(ring, addr);
> +		intel_ring_emit(ring, MI_NOOP);
> +		intel_ring_advance(ring);

Premature optimisation, but can we store the register array with a single command?

for (i = 0; i < 8; i++) {
	if (0 == dev_priv->gen_pmu.mmio_list[i])
		break;

ret = intel_ring_begin(ring, 2*i + 2);
if (ret)
	return;

intel_ring_emit(MI_STORE_REGISTER_MEM(i));
	for (i = 0; i < 8; i++) {
		if (0 == dev_priv->gen_pmu.mmio_list[i])
			break;
		intel_ring_emit(dev_priv->gen_pmu.mmio_list[i]);
		intel_ring_emit(addr + i*4);
	}
intel_ring_emit(MI_NOOP);

You should also consider ensuring that we have a command barrier between
these and 3d operations. If only we had engine->emit_flush(I915_COMMAND_BARRIER).

> +/* Some embargoed entries missing from whitelist */
> +static const struct register_whitelist {
> +	uint64_t offset;
> +	uint32_t size;
> +	/* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */
> +	uint32_t gen_bitmask;
> +} whitelist[] = {
> +	{ GEN6_GT_GFX_RC6, 4, GEN_RANGE(7, 9) },
> +	{ GEN6_GT_GFX_RC6p, 4, GEN_RANGE(7, 9) },
> +};
> +
> +static int check_mmio_whitelist(struct drm_i915_private *dev_priv,
> +				struct drm_i915_gen_pmu_attr *gen_attr)

Just what I came looking for.

> +{
> +	struct register_whitelist const *entry = whitelist;
> +	int i, count;
> +
> +	for (count = 0; count < 8; count++) {

hardcoded value, ARRAY_SIZE(gen_attr->mmio_list)

> +		if (!gen_attr->mmio_list[count])
> +			break;
> +
> +		for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {

Interesting choice of continuation in the ABI. Seems a litte forced.
Move the whitelist into the function, no one else should think of
accessing it (right?), then just use whitelist[i].

> +			if (entry->offset == gen_attr->mmio_list[count] &&
> +			    (1 << INTEL_INFO(dev_priv->dev)->gen &

dev_priv->dev->dev_priv->info, you have to be kidding me.

INTEL_INFO(dev_priv) is the natural form.

> +					entry->gen_bitmask))
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(whitelist))
> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int i915_gen_event_init(struct perf_event *event)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -1674,6 +1740,17 @@ static int i915_gen_event_init(struct perf_event *event)
>  	if (gen_attr.sample_tag)
>  		dev_priv->gen_pmu.sample_info_flags |= I915_GEN_PMU_SAMPLE_TAG;
>  
> +	if (gen_attr.sample_mmio) {
> +		ret = check_mmio_whitelist(dev_priv, &gen_attr);
> +		if (ret)
> +			return ret;

Global hw state should only be inspectable by root (unless the system
has relaxed perf permissions).

> +		dev_priv->gen_pmu.sample_info_flags |=
> +				I915_GEN_PMU_SAMPLE_MMIO;
> +		memcpy(dev_priv->gen_pmu.mmio_list, gen_attr.mmio_list,
> +				sizeof(dev_priv->gen_pmu.mmio_list));
> +	}
> +
>  	/* To avoid the complexity of having to accurately filter
>  	 * data and marshal to the appropriate client
>  	 * we currently only allow exclusive access */
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 7ab4972..65bc39d 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -81,7 +81,7 @@
>  
>  #define I915_OA_ATTR_SIZE_VER0		32  /* sizeof first published struct */
>  
> -#define I915_GEN_PMU_ATTR_SIZE_VER0	8  /* sizeof first published struct */
> +#define I915_GEN_PMU_ATTR_SIZE_VER0	40  /* sizeof first published struct */
>  
>  typedef struct _drm_i915_oa_attr {
>  	__u32 size;
> @@ -105,7 +105,9 @@ struct drm_i915_gen_pmu_attr {
>  	__u32 sample_ring:1,
>  		sample_pid:1,
>  		sample_tag:1,
> -		__reserved_1:29;
> +		sample_mmio:1,
> +		__reserved_1:28;
> +	__u32 mmio_list[8];
>  };

seems like having a
BUILD_BUG_ON(sizeof(struct _drm_i915_oa_attr) != I915_GEN_PMU_ATTR_SIZE_VER0) is in order.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[sourab.gupta]

From: Sourab Gupta <sourab.gupta@intel.com>

This patch adds support for retrieving MMIO register values alongwith
timestamps and forwarding them to userspace through perf.
The userspace can request upto 8 MMIO register values to be dumped.
The addresses of upto 8 MMIO registers can be passed through perf attr
config. The registers are checked against a whitelist before passing them
on. The commands to dump the values of these MMIO registers are then
inserted into the ring alongwith commands to dump the timestamps.

v2: Implement suggestions by Chris, pertaining to code restructuring, using
BUILD_BUG_ON etc.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 +
 drivers/gpu/drm/i915/i915_oa_perf.c | 99 ++++++++++++++++++++++++++++++++++---
 include/uapi/drm/i915_drm.h         | 11 ++++-
 3 files changed, 104 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c3e823f..5c6e37a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2022,7 +2022,9 @@ struct drm_i915_private {
 #define I915_GEN_PMU_SAMPLE_RING		(1<<0)
 #define I915_GEN_PMU_SAMPLE_PID			(1<<1)
 #define I915_GEN_PMU_SAMPLE_TAG			(1<<2)
+#define I915_GEN_PMU_SAMPLE_MMIO		(1<<3)
 		int sample_info_flags;
+		u32 mmio_list[I915_PMU_MMIO_NUM];
 	} gen_pmu;
 
 	void (*emit_profiling_data[I915_PROFILE_MAX])
diff --git a/drivers/gpu/drm/i915/i915_oa_perf.c b/drivers/gpu/drm/i915/i915_oa_perf.c
index e065e06..4197dbd 100644
--- a/drivers/gpu/drm/i915/i915_oa_perf.c
+++ b/drivers/gpu/drm/i915/i915_oa_perf.c
@@ -12,6 +12,7 @@
 #define PERIOD max_t(u64, 10000, NSEC_PER_SEC / FREQUENCY)
 
 #define TS_DATA_SIZE sizeof(struct drm_i915_ts_data)
+#define MMIO_DATA_SIZE sizeof(struct drm_i915_mmio_data)
 #define CTX_INFO_SIZE sizeof(struct drm_i915_ts_node_ctx_id)
 #define RING_INFO_SIZE sizeof(struct drm_i915_ts_node_ring_id)
 #define PID_INFO_SIZE sizeof(struct drm_i915_ts_node_pid)
@@ -129,8 +130,8 @@ static void i915_gen_emit_ts_data(struct drm_i915_gem_request *req,
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_object *obj = dev_priv->gen_pmu.buffer.obj;
 	struct i915_gen_pmu_node *entry;
-	u32 addr = 0;
-	int ret;
+	u32 mmio_addr, addr = 0;
+	int ret, i, count = 0;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (entry == NULL) {
@@ -138,7 +139,12 @@ static void i915_gen_emit_ts_data(struct drm_i915_gem_request *req,
 		return;
 	}
 
-	ret = intel_ring_begin(ring, 6);
+	for (count = 0; count < I915_PMU_MMIO_NUM; count++) {
+		if (0 == dev_priv->gen_pmu.mmio_list[count])
+			break;
+	}
+
+	ret = intel_ring_begin(ring, 6 + 4*count);
 	if (ret) {
 		kfree(entry);
 		return;
@@ -173,6 +179,7 @@ static void i915_gen_emit_ts_data(struct drm_i915_gem_request *req,
 	spin_unlock(&dev_priv->gen_pmu.lock);
 
 	addr = dev_priv->gen_pmu.buffer.gtt_offset + entry->offset;
+	mmio_addr = addr + TS_DATA_SIZE;
 
 	if (ring->id == RCS) {
 		intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(5));
@@ -192,6 +199,32 @@ static void i915_gen_emit_ts_data(struct drm_i915_gem_request *req,
 		intel_ring_emit(ring, MI_NOOP);
 		intel_ring_emit(ring, MI_NOOP);
 	}
+
+	/*
+	 * Note:
+	 * 1) The optimization to store the register array with a single
+	 * command doesn't seem to be working with SRM commands. Hence, have a
+	 * loop with a single SRM command repeated. Missing anything here?
+	 * 2) This fn is presently called before and after batch buffer. As
+	 * such, there should already be the CS stall commands called after BB.
+	 * Is there a need/necessity for a command barrier to be inserted in
+	 * ring here? If so, which commands? (CS Stall?)
+	 */
+	for (i = 0; i < I915_PMU_MMIO_NUM; i++) {
+		if (0 == dev_priv->gen_pmu.mmio_list[i])
+			break;
+
+		addr = mmio_addr +
+			i * sizeof(dev_priv->gen_pmu.mmio_list[i]);
+
+		intel_ring_emit(ring,
+				MI_STORE_REGISTER_MEM(1) |
+				MI_SRM_LRM_GLOBAL_GTT);
+		intel_ring_emit(ring, dev_priv->gen_pmu.mmio_list[i]);
+		intel_ring_emit(ring, addr);
+		intel_ring_emit(ring, MI_NOOP);
+	}
+
 	intel_ring_advance(ring);
 
 	obj->base.write_domain = I915_GEM_DOMAIN_RENDER;
@@ -553,7 +586,7 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 {
 	struct perf_sample_data data;
 	struct perf_event *event = dev_priv->gen_pmu.exclusive_event;
-	int snapshot_size;
+	int snapshot_size, mmio_size;
 	u8 *snapshot, *current_ptr;
 	struct drm_i915_ts_node_ctx_id *ctx_info;
 	struct drm_i915_ts_node_ring_id *ring_info;
@@ -565,10 +598,16 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
 			(RING_INFO_SIZE != 8) || (PID_INFO_SIZE != 8) ||
 			(TAG_INFO_SIZE != 8));
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_MMIO)
+		mmio_size = MMIO_DATA_SIZE;
+	else
+		mmio_size = 0;
+
 	snapshot = dev_priv->gen_pmu.buffer.addr + node->offset;
-	snapshot_size = TS_DATA_SIZE + CTX_INFO_SIZE;
+	snapshot_size = TS_DATA_SIZE + mmio_size + CTX_INFO_SIZE;
 
-	ctx_info = (struct drm_i915_ts_node_ctx_id *)(snapshot + TS_DATA_SIZE);
+	ctx_info = (struct drm_i915_ts_node_ctx_id *)
+				(snapshot + mmio_size +	TS_DATA_SIZE);
 	ctx_info->ctx_id = node->ctx_id;
 	current_ptr = snapshot + snapshot_size;
 
@@ -1046,6 +1085,9 @@ static int init_gen_pmu_buffer(struct perf_event *event)
 	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_TAG)
 		node_size += TAG_INFO_SIZE;
 
+	if (dev_priv->gen_pmu.sample_info_flags & I915_GEN_PMU_SAMPLE_MMIO)
+		node_size += MMIO_DATA_SIZE;
+
 	/* size has to be aligned to 8 bytes */
 	node_size = ALIGN(node_size, 8);
 	dev_priv->gen_pmu.buffer.node_size = node_size;
@@ -1641,6 +1683,40 @@ err_size:
 	goto out;
 }
 
+
+static int check_mmio_whitelist(struct drm_i915_private *dev_priv,
+				struct drm_i915_gen_pmu_attr *gen_attr)
+{
+
+#define GEN_RANGE(l, h) GENMASK(h, l)
+	static const struct register_whitelist {
+		uint64_t offset;
+		uint32_t size;
+		/* supported gens, 0x10 for 4, 0x30 for 4 and 5, etc. */
+		uint32_t gen_bitmask;
+	} whitelist[] = {
+		{ GEN6_GT_GFX_RC6, 4, GEN_RANGE(7, 9) },
+		{ GEN6_GT_GFX_RC6p, 4, GEN_RANGE(7, 9) },
+	};
+	int i, count;
+
+	for (count = 0; count < I915_PMU_MMIO_NUM; count++) {
+		if (!gen_attr->mmio_list[count])
+			break;
+
+		for (i = 0; i < ARRAY_SIZE(whitelist); i++) {
+			if (whitelist[i].offset == gen_attr->mmio_list[count] &&
+			    (1 << INTEL_INFO(dev_priv)->gen &
+					whitelist[i].gen_bitmask))
+				break;
+		}
+
+		if (i == ARRAY_SIZE(whitelist))
+			return -EINVAL;
+	}
+	return 0;
+}
+
 static int i915_gen_event_init(struct perf_event *event)
 {
 	struct drm_i915_private *dev_priv =
@@ -1670,6 +1746,17 @@ static int i915_gen_event_init(struct perf_event *event)
 	if (gen_attr.sample_tag)
 		dev_priv->gen_pmu.sample_info_flags |= I915_GEN_PMU_SAMPLE_TAG;
 
+	if (gen_attr.sample_mmio) {
+		ret = check_mmio_whitelist(dev_priv, &gen_attr);
+		if (ret)
+			return ret;
+
+		dev_priv->gen_pmu.sample_info_flags |=
+				I915_GEN_PMU_SAMPLE_MMIO;
+		memcpy(dev_priv->gen_pmu.mmio_list, gen_attr.mmio_list,
+				sizeof(dev_priv->gen_pmu.mmio_list));
+	}
+
 	/* To avoid the complexity of having to accurately filter
 	 * data and marshal to the appropriate client
 	 * we currently only allow exclusive access */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index db91098..4153cdf 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -81,7 +81,7 @@
 
 #define I915_OA_ATTR_SIZE_VER0		32  /* sizeof first published struct */
 
-#define I915_GEN_PMU_ATTR_SIZE_VER0	8  /* sizeof first published struct */
+#define I915_GEN_PMU_ATTR_SIZE_VER0	40  /* sizeof first published struct */
 
 typedef struct _drm_i915_oa_attr {
 	__u32 size;
@@ -105,7 +105,9 @@ struct drm_i915_gen_pmu_attr {
 	__u32 sample_ring:1,
 		sample_pid:1,
 		sample_tag:1,
-		__reserved_1:29;
+		sample_mmio:1,
+		__reserved_1:28;
+	__u32 mmio_list[8];
 };
 
 /* Header for PERF_RECORD_DEVICE type events */
@@ -155,6 +157,11 @@ struct drm_i915_ts_data {
 	__u32 ts_high;
 };
 
+struct drm_i915_mmio_data {
+#define I915_PMU_MMIO_NUM	8
+	__u32 mmio[I915_PMU_MMIO_NUM];
+};
+
 struct drm_i915_ts_node_ctx_id {
 	__u32 ctx_id;
 	__u32 pad;
-- 
1.8.5.1

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

Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[Chris Wilson]

On Wed, Aug 05, 2015 at 11:25:44AM +0530, sourab.gupta@intel.com wrote:
> From: Sourab Gupta <sourab.gupta@intel.com>
> 
> This patch adds support for retrieving MMIO register values alongwith
> timestamps and forwarding them to userspace through perf.
> The userspace can request upto 8 MMIO register values to be dumped.
> The addresses of upto 8 MMIO registers can be passed through perf attr
> config. The registers are checked against a whitelist before passing them
> on. The commands to dump the values of these MMIO registers are then
> inserted into the ring alongwith commands to dump the timestamps.

The values reported to userspace are deltas across batches right? We
don't expose the global value to an unprivileged user? It would be nice
to clarify that in perf_init so that the reviewer is aware that
the issue of unprivileged information leak is addressed (or at least
reminded that the register values do not leak!).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[Gupta, Sourab]

On Wed, 2015-08-05 at 10:03 +0000, Chris Wilson wrote:
> On Wed, Aug 05, 2015 at 11:25:44AM +0530, sourab.gupta@intel.com wrote:
> > From: Sourab Gupta <sourab.gupta@intel.com>
> > 
> > This patch adds support for retrieving MMIO register values alongwith
> > timestamps and forwarding them to userspace through perf.
> > The userspace can request upto 8 MMIO register values to be dumped.
> > The addresses of upto 8 MMIO registers can be passed through perf attr
> > config. The registers are checked against a whitelist before passing them
> > on. The commands to dump the values of these MMIO registers are then
> > inserted into the ring alongwith commands to dump the timestamps.
> 
> The values reported to userspace are deltas across batches right? We
> don't expose the global value to an unprivileged user? It would be nice
> to clarify that in perf_init so that the reviewer is aware that
> the issue of unprivileged information leak is addressed (or at least
> reminded that the register values do not leak!).
> -Chris
> 
Hi Chris,
Two things here:
1) Only root is allowed to call event_init for gen pmu. This restriction
is there in event_init. (The thought behind this restriction being that
we are profiling data across contexts here, so a process wishing to
listen to global activity happening in system across all contexts ought
to have root priviliges). Is this thought process correct? Should we be
supporting non-root users too?

2) Being already a root, do we need to worry about the unauthorized mmio
access while exposing these mmio values through the interface?

In the current patches, the full mmio register value is dumped to be
passed on to userspace (no deltas across batches), provided the register
is there in the whitelist. Does the question of unpriviliged information
leak arise here(the user being root)?

Regards,
Sourab
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[Chris Wilson]

On Wed, Aug 05, 2015 at 10:18:50AM +0000, Gupta, Sourab wrote:
> On Wed, 2015-08-05 at 10:03 +0000, Chris Wilson wrote:
> > On Wed, Aug 05, 2015 at 11:25:44AM +0530, sourab.gupta@intel.com wrote:
> > > From: Sourab Gupta <sourab.gupta@intel.com>
> > > 
> > > This patch adds support for retrieving MMIO register values alongwith
> > > timestamps and forwarding them to userspace through perf.
> > > The userspace can request upto 8 MMIO register values to be dumped.
> > > The addresses of upto 8 MMIO registers can be passed through perf attr
> > > config. The registers are checked against a whitelist before passing them
> > > on. The commands to dump the values of these MMIO registers are then
> > > inserted into the ring alongwith commands to dump the timestamps.
> > 
> > The values reported to userspace are deltas across batches right? We
> > don't expose the global value to an unprivileged user? It would be nice
> > to clarify that in perf_init so that the reviewer is aware that
> > the issue of unprivileged information leak is addressed (or at least
> > reminded that the register values do not leak!).
> > -Chris
> > 
> Hi Chris,
> Two things here:
> 1) Only root is allowed to call event_init for gen pmu. This restriction
> is there in event_init. (The thought behind this restriction being that
> we are profiling data across contexts here, so a process wishing to
> listen to global activity happening in system across all contexts ought
> to have root priviliges). Is this thought process correct? Should we be
> supporting non-root users too?

That is not clear in this patch, so you need to address such concerns at
least in the changelog, and preferrably with a reminder in the
whitelist (that these register reads are safe because they are being
done from a privileged context only - we then have a red flag in case we
lower it).

What is the privilege check you are using here exactly?

For gen pmu, I want it user accessible. How long does it take to execute
my batches is a common developer query. We may even be able to make
anonymised information freely available ala top (per-process GPU usage,
memory usage, though cgroups/namespacing rules probably apply here).

> 2) Being already a root, do we need to worry about the unauthorized mmio
> access while exposing these mmio values through the interface?

Yes. See above, the information here can be anonymised and useful for
user processes exactly like TIMESTAMP.
 
> In the current patches, the full mmio register value is dumped to be
> passed on to userspace (no deltas across batches), provided the register
> is there in the whitelist. Does the question of unpriviliged information
> leak arise here(the user being root)?

Not for root.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[Gupta, Sourab]

On Wed, 2015-08-05 at 10:30 +0000, Chris Wilson wrote:
> On Wed, Aug 05, 2015 at 10:18:50AM +0000, Gupta, Sourab wrote:
> > On Wed, 2015-08-05 at 10:03 +0000, Chris Wilson wrote:
> > > On Wed, Aug 05, 2015 at 11:25:44AM +0530, sourab.gupta@intel.com wrote:
> > > > From: Sourab Gupta <sourab.gupta@intel.com>
> > > > 
> > > > This patch adds support for retrieving MMIO register values alongwith
> > > > timestamps and forwarding them to userspace through perf.
> > > > The userspace can request upto 8 MMIO register values to be dumped.
> > > > The addresses of upto 8 MMIO registers can be passed through perf attr
> > > > config. The registers are checked against a whitelist before passing them
> > > > on. The commands to dump the values of these MMIO registers are then
> > > > inserted into the ring alongwith commands to dump the timestamps.
> > > 
> > > The values reported to userspace are deltas across batches right? We
> > > don't expose the global value to an unprivileged user? It would be nice
> > > to clarify that in perf_init so that the reviewer is aware that
> > > the issue of unprivileged information leak is addressed (or at least
> > > reminded that the register values do not leak!).
> > > -Chris
> > > 
> > Hi Chris,
> > Two things here:
> > 1) Only root is allowed to call event_init for gen pmu. This restriction
> > is there in event_init. (The thought behind this restriction being that
> > we are profiling data across contexts here, so a process wishing to
> > listen to global activity happening in system across all contexts ought
> > to have root priviliges). Is this thought process correct? Should we be
> > supporting non-root users too?
> 
> That is not clear in this patch, so you need to address such concerns at
> least in the changelog, and preferrably with a reminder in the
> whitelist (that these register reads are safe because they are being
> done from a privileged context only - we then have a red flag in case we
> lower it).
> 
> What is the privilege check you are using here exactly?

In the current patch set, during the gen pmu event_init, I'm checking
for root access using the below check:
+	if (!capable(CAP_SYS_ADMIN))
+			return -EACCES;

> 
> For gen pmu, I want it user accessible. How long does it take to execute
> my batches is a common developer query. We may even be able to make
> anonymised information freely available ala top (per-process GPU usage,
> memory usage, though cgroups/namespacing rules probably apply here).
> 

So, aiui the privilige access should be controlled as below:
- For gen pmu, no need to restrict only to root processes. This would
imply that user processes would now be able to gather timestamps for all
the batches (no unpriviliged information leak since timestamps are
inherently anonymised) ..

- For the collection of mmio register data, we have following options:
    - If it is a root process, allow access (is whitelist check
necessary in this case?).
    - If not root, one option is to disallow mmio register dump(probably
not a preferable option?).
    - If not root, second option is to allow mmio dump (after checking
against the whitelist?). In this case, do we send the mmio register
values as they exist or do we anonymise them?. Since my impression was
that perf is expected to simply return the dump of mmio registers
requested, and throw an access error in case of unpriviliged operation.
And if required, how do we anonymise the mmio data?

can you let me know your opinion here wrt the above points. And the
mechanism to anonymise the mmio data.

> > 2) Being already a root, do we need to worry about the unauthorized mmio
> > access while exposing these mmio values through the interface?
> 
> Yes. See above, the information here can be anonymised and useful for
> user processes exactly like TIMESTAMP.
>  
> > In the current patches, the full mmio register value is dumped to be
> > passed on to userspace (no deltas across batches), provided the register
> > is there in the whitelist. Does the question of unpriviliged information
> > leak arise here(the user being root)?
> 
> Not for root.
> -Chris
> 

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

Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[Robert Bragg]

On Wed, Aug 5, 2015 at 6:55 AM,  <sourab.gupta@intel.com> wrote:
> From: Sourab Gupta <sourab.gupta@intel.com>
>
> This patch adds support for retrieving MMIO register values alongwith
> timestamps and forwarding them to userspace through perf.
> The userspace can request upto 8 MMIO register values to be dumped.
> The addresses of upto 8 MMIO registers can be passed through perf attr
> config. The registers are checked against a whitelist before passing them
> on. The commands to dump the values of these MMIO registers are then
> inserted into the ring alongwith commands to dump the timestamps.

Considering the discussion had so far with Peter: one thing raised was
a preference for exposing individual counters via separate events. In
the case of OA metrics I don't think that's at all as straight forward
as it sounds due to the way the OA unit is configured and reports
counters but for mmio based counters the configurations are completely
orthogonal (just an address) so I don't know that there's a need to
configure multiple reads per event and I imagine we should be able to
avoid the arbitrary limit of 8 reads.

Perf allows users to group event fds together which signifies to the
kernel that it wants the counters to be reported in the same buffer
(the buffer of the group leader).

A more extensible list of registers that should be read via the SRM
commands could be indirectly derived by maintaining a list of the
active mmio-read events.

I think something else to raise here is that it could help if we had
some more concrete use cases and at least some prototype userspace
code for this interface. I guess the requirements around privileges
could depend a bit on what specific registers you're interested in.

If security requirements may vary for different counters I do also
wonder if instead of a generic mmio event it might be appropriate to
enumerate what we're interested in and have a separate event for each
specific counter considering requirements on a case by case basis.

I wonder if we should also consider exposing 64bit counters such as
the pipeline statistics here. intel_gpu_top tries to expose pipeline
statistics but one problem if faces is that these are per-context
counters so it would be better to read them via the command stream
with a mechanism like this instead of periodically so that the reads
can be reliably mapped to a context.

In general a mechanism like this could be a good fit for exposing
per-context metrics to a system compositor (metrics not well suited to
period sampling).

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