[PATCH] drm/i915: Add aub debug support for kernel

[PATCH] drm/i915: Add aub debug support for kernel

[Yuanhan Liu]

The AUB file is a file format used by Intel's internal simulation
and other validation tools. The content of an aub file is a subset
collection of all the data needed by GPU.

Here now just collects the data that are going to be written into
register, since the others data like ring buffer, buffer objects
are dumped at user space by libdrm.

The currentl implementation: hooks the I915_WRITEx macro. The basic
idea is write down all the reg offset and reg val before writing
them to register.

Since it's a debug feature, it's disabled by default. You can add
the following line into kernel cmdline in your bootloader's config
file to enable it:

i915.aub_debug=1

This comes two files under ${debugfs_mntpoint}/dri/0/ named:
i915.aub	-- the aub file
i915_aub_reset  -- read this file to reset the aub data buffer, as
		   the size of aub data buffer is so limite, while
		   the aub data is so huge. Well, I'm not sure I
		   should add this debug file.

comments?

Signed-off-by: Yuanhan Liu <yuanhan.liu@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  173 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c     |    3 +
 drivers/gpu/drm/i915/i915_drv.h     |   76 +++++++++++++++-
 3 files changed, 249 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1f4f3ce..b953547 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -29,6 +29,7 @@
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
 #include "drmP.h"
 #include "drm.h"
 #include "intel_drv.h"
@@ -1015,6 +1016,165 @@ static int i915_wedged_create(struct dentry *root, struct drm_minor *minor)
 	return drm_add_fake_info_node(minor, ent, &i915_wedged_fops);
 }
 
+static void * i915_aub_data = NULL;
+static struct i915_aub_data * i915_aub_data_ptr = NULL;
+static struct mutex i915_aub_data_lock;
+
+static inline void i915_aub_out(struct seq_file *m, uint32_t data)
+{
+	seq_write(m, &data, 4);
+}
+
+static inline void 
+i915_aub_out_data(struct seq_file *m, void *data, size_t size)
+{
+	seq_write(m, data, size);
+}
+
+
+/*
+ * Check should we trace this register. Since some registers, 
+ * like GPIOX,  writes too often, which will fill up the 
+ * i915_aub_data buffer soon. That's really not good!
+ *
+ * You can add your own filter here.
+ */
+static int i915_aub_trace_reg(uint32_t reg)
+{
+#if 0
+	if ((reg - PCH_GPIOA) >= 0 && 
+	    (reg - PCH_GPIOA) <= (PCH_GPIOF - PCH_GPIOA))
+		return 0;
+	if (reg == CURABASE || reg == CURAPOS ||
+	    reg == CURBBASE || reg == CURBPOS)
+		return 0;
+#endif
+	return 1;
+}
+
+void i915_aub_write_reg(uint32_t reg, uint64_t val, int len)
+{
+	if (!i915_aub_trace_reg(reg))
+		return;
+	
+	mutex_lock(&i915_aub_data_lock);
+	if ((void *)i915_aub_data_ptr < i915_aub_data + AUB_DATA_SIZE) {
+		i915_aub_data_ptr->type = AUB_TYPE_REG;
+		i915_aub_data_ptr->size = len;
+		i915_aub_data_ptr->addr = reg;
+		i915_aub_data_ptr->data  = val;
+		i915_aub_data_ptr++;
+	}
+	mutex_unlock(&i915_aub_data_lock);
+}
+
+
+/*
+ * Write out the aub header
+ */
+static void i915_aub_out_header(struct seq_file *m)
+{
+	char app_name[AUB_APP_NAME_LEN] = "kernel-i915";
+
+	i915_aub_out(m, AUB_CMD_HEADER | (13 - 2));
+	i915_aub_out(m, AUB_VERSION);
+	i915_aub_out_data(m, app_name, AUB_APP_NAME_LEN);
+	i915_aub_out(m, 0); /* timestamp */
+	i915_aub_out(m, 0); /* timestamp */
+	i915_aub_out(m, 0); /* comment len */
+}
+
+static void i915_aub_out_reg(struct seq_file *m, struct i915_aub_data *reg)
+{
+	i915_aub_out(m, AUB_CMD_TRACE_HEADER_BLOCK | (5 - 2));
+	i915_aub_out(m, AUB_TRACE_OP_MMIO_WRITE);
+	i915_aub_out(m, 0);
+	i915_aub_out(m, reg->addr); /* reg offset */
+	i915_aub_out(m, reg->size); /* size */
+	i915_aub_out_data(m, &reg->data, reg->size); /* data */
+}
+
+/* 
+ * Set up the GTT. The max we can handle is 256M 
+ *
+ * FIXME: is 256M OK?
+ */
+static void i915_aub_setup_gtt(struct seq_file *m)
+{
+	int i;
+	uint32_t entry = 0x200003;
+
+	for (i = 0x0; i < 0x10000; i += 4, entry += 0x1000) {
+		i915_aub_out(m, AUB_CMD_TRACE_HEADER_BLOCK | (5 - 2));
+		i915_aub_out(m, AUB_TRACE_MEMTYPE_NONLOCAL | 0 | AUB_TRACE_OP_DATA_WRITE);
+		i915_aub_out(m, 0);
+		i915_aub_out(m, i);
+		i915_aub_out(m, 4);
+		i915_aub_out(m, entry);
+	}
+}
+
+/*
+ * It's time to tell fulsim to handle these data
+ */
+static void i915_aub_draw(struct seq_file *m)
+{
+	/*  AUB_CMD_DRAW */
+	i915_aub_out(m, AUB_CMD_DRAW | 5);
+	
+	/* FIXME: currently write all ZERO */
+	i915_aub_out(m, 0x0); /* Lenght */
+	i915_aub_out(m, 0x0); /* Flags */
+	i915_aub_out(m, 0x0); /* XLeft, Ytop */
+	i915_aub_out(m, 0x0); /* Width, Height */
+	i915_aub_out(m, 0x0); /* BitsPerPixels,  Pitch */
+	i915_aub_out(m, 0x0); /* SpanGrid */
+}
+
+static int i915_aub(struct seq_file *m, void *unused)
+{
+	struct i915_aub_data *p = (struct i915_aub_data *)i915_aub_data;
+	struct i915_aub_data *end = i915_aub_data_ptr;
+
+	if (i915_aub_debug == 0) {
+		seq_printf(m, "aub debug is disabled! You can add 'i915.aub_debug=1' "
+			   "at the end of boot command line to enable it.\n");
+		return 0;
+	}
+	
+	i915_aub_out_header(m);
+
+	if ((void *)i915_aub_data_ptr >= i915_aub_data + AUB_DATA_SIZE)
+		printk("WARNING: i915_aub_data_ptr exceed the size of aub data"
+		       " buffer. You may want to enlarge the size\n");
+
+	/* FIXME: As we are now in kernel, should we do thing? */
+	i915_aub_setup_gtt(m);
+
+	while (p < end) {
+		if (p->type == AUB_TYPE_REG)
+			i915_aub_out_reg(m, p);
+		else
+			DRM_ERROR("Invaled aub type(%d)\n", p->type);
+
+		p++;
+	}
+
+	i915_aub_draw(m);
+
+	return 0;
+}
+
+static int i915_aub_reset(struct seq_file *m, void *unused)
+{
+	if (i915_aub_debug) {
+		mutex_lock(&i915_aub_data_lock);
+		i915_aub_data_ptr = (struct i915_aub_data *)i915_aub_data;
+		mutex_unlock(&i915_aub_data_lock);
+	}
+	return 0;
+}
+
 static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
@@ -1044,6 +1204,8 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
+	{"i915.aub", i915_aub, 0},
+	{"i915_aub_reset", i915_aub_reset, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
@@ -1055,6 +1217,14 @@ int i915_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
+	if (i915_aub_debug) {
+		i915_aub_data = vmalloc(AUB_DATA_SIZE);
+		if (!i915_aub_data)
+			return -ENOMEM;
+		i915_aub_data_ptr = (struct i915_aub_data *)i915_aub_data;
+		mutex_init(&i915_aub_data_lock);
+	}
+
 	return drm_debugfs_create_files(i915_debugfs_list,
 					I915_DEBUGFS_ENTRIES,
 					minor->debugfs_root, minor);
@@ -1066,6 +1236,9 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 				 I915_DEBUGFS_ENTRIES, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_wedged_fops,
 				 1, minor);
+
+	if (i915_aub_debug)
+		vfree(i915_aub_data);
 }
 
 #endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3467dd4..9b6589c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -40,6 +40,9 @@
 static int i915_modeset = -1;
 module_param_named(modeset, i915_modeset, int, 0400);
 
+int i915_aub_debug = 0;
+module_param_named(aub_debug, i915_aub_debug, int, 0400);
+
 unsigned int i915_fbpercrtc = 0;
 module_param_named(fbpercrtc, i915_fbpercrtc, int, 0400);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c2c19b..3c8f4b4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -880,6 +880,50 @@ enum intel_chip_family {
 	CHIP_I965 = 0x08,
 };
 
+#define AUB_CMD				(7 << 29)
+#define AUB_APP_NAME_LEN		32
+
+/*
+ * 32 Page is really not enough. Since some registers, like CURAPOS and
+ * CURABASE will fill up the aub data buffer soon!
+ *
+ * So, if you want to record everything(impossible, in fact)!, change 
+ * the size here.
+ */
+#define AUB_DATA_SIZE 			(32 << PAGE_SHIFT)
+
+#define AUB_HEADER_MAJOR_SHIFT		24
+#define AUB_HEADER_MINOR_SHIFT		16
+#define AUB_VERSION 			((4 << AUB_HEADER_MAJOR_SHIFT) | \
+			 		 (0 << AUB_HEADER_MINOR_SHIFT))
+
+#define AUB_CMD_HEADER			(AUB_CMD | (1 << 23) | (0x05 << 16))
+#define AUB_CMD_TRACE_HEADER_BLOCK	(AUB_CMD | (1 << 23) | (0x41 << 16))
+#define AUB_CMD_DRAW               	(AUB_CMD | (1 << 23) | (0xaf << 16))
+
+
+#define AUB_TRACE_MEMTYPE_GTT		(0 << 16)
+#define AUB_TRACE_MEMTYPE_LOCAL		(1 << 16)
+#define AUB_TRACE_MEMTYPE_NONLOCAL	(2 << 16)
+
+#define AUB_TRACE_OP_DATA_WRITE		0x00000001
+#define AUB_TRACE_OP_MMIO_WRITE		0x00000003
+
+struct i915_aub_data {
+	uint32_t type:8;
+	uint32_t size:24;
+	uint32_t addr;
+	uint64_t data;
+};
+
+/* Currently support one type of data */
+enum i915_aub_type {
+	AUB_TYPE_REG = 1,
+};
+
+
+extern int i915_aub_debug;
+
 extern struct drm_ioctl_desc i915_ioctls[];
 extern int i915_max_ioctl;
 extern unsigned int i915_fbpercrtc;
@@ -1105,6 +1149,7 @@ void i915_gem_dump_object(struct drm_gem_object *obj, int len,
 /* i915_debugfs.c */
 int i915_debugfs_init(struct drm_minor *minor);
 void i915_debugfs_cleanup(struct drm_minor *minor);
+void i915_aub_write_reg(uint32_t reg, uint64_t val, int len);
 
 /* i915_suspend.c */
 extern int i915_save_state(struct drm_device *dev);
@@ -1195,18 +1240,43 @@ static inline u32 i915_read(struct drm_i915_private *dev_priv, u32 reg)
 static inline void i915_write(struct drm_i915_private *dev_priv, u32 reg,
 			      u32 val)
 {
+	if (i915_aub_debug)
+		i915_aub_write_reg(reg, val, 4);
 	writel(val, dev_priv->regs + reg);
 	if (dev_priv->debug_flags & I915_DEBUG_WRITE)
 		printk(KERN_ERR "wrote 0x%08x to 0x%08x\n", val, reg);
 }
 
+static inline void i915_writeb(struct drm_i915_private *dev_priv, u32 reg,
+			      u8 val)
+{
+	if (i915_aub_debug)
+		i915_aub_write_reg(reg, val, 1);
+	writeb(val, dev_priv->regs + reg);
+}
+
+static inline void i915_writew(struct drm_i915_private *dev_priv, u32 reg,
+			      u16 val)
+{
+	if (i915_aub_debug)
+		i915_aub_write_reg(reg, val, 2);
+	writew(val, dev_priv->regs + reg);
+}
+
+static inline void i915_writeq(struct drm_i915_private *dev_priv, u32 reg,
+			      u64 val)
+{
+	if (i915_aub_debug)
+		i915_aub_write_reg(reg, val, 8);
+	writeq(val, dev_priv->regs + reg);
+}
 #define I915_READ(reg)          i915_read(dev_priv, (reg))
 #define I915_WRITE(reg, val)    i915_write(dev_priv, (reg), (val))
 #define I915_READ16(reg)	readw(dev_priv->regs + (reg))
-#define I915_WRITE16(reg, val)	writel(val, dev_priv->regs + (reg))
+#define I915_WRITE16(reg, val)	i915_writew(dev_priv, (reg), (val))
 #define I915_READ8(reg)		readb(dev_priv->regs + (reg))
-#define I915_WRITE8(reg, val)	writeb(val, dev_priv->regs + (reg))
-#define I915_WRITE64(reg, val)	writeq(val, dev_priv->regs + (reg))
+#define I915_WRITE8(reg, val)	i915_writeb(dev_priv, (reg), (val))
+#define I915_WRITE64(reg, val)	i915_writeq(dev_priv, (reg), (val))
 #define I915_READ64(reg)	readq(dev_priv->regs + (reg))
 #define POSTING_READ(reg)	(void)I915_READ(reg)
 #define POSTING_READ16(reg)	(void)I915_READ16(reg)
-- 
1.7.0.1

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Chris Wilson]

On Tue,  2 Nov 2010 17:11:36 +0800, Yuanhan Liu <yuanhan.liu@intel.com> wrote:
> The AUB file is a file format used by Intel's internal simulation
> and other validation tools. The content of an aub file is a subset
> collection of all the data needed by GPU.
> 
> Here now just collects the data that are going to be written into
> register, since the others data like ring buffer, buffer objects
> are dumped at user space by libdrm.
> 
> The currentl implementation: hooks the I915_WRITEx macro. The basic
> idea is write down all the reg offset and reg val before writing
> them to register.

This part looks possible to achieve just using tracepoints (and replace
the current debug printk in the process). The advantage of that is we
can then tap into a low overhead tracing mechanism with a *ringbuffer*!
;-) It will require a bit of userspace scripting to do the .aub format
conversion.

I presume the plan will be to include batchbuffer+auxiliary buffer
contents as well in order to capture the complete temporal sequence for
debugging+replay?

Being able to trace the register writes using ftrace is good... In fact we
can remove our own printks and just use mmiotrace. So is it possible to
post-process mmiotrace into .aub?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu Aleaxander]

Hi Chris,

On Tue, Nov 2, 2010 at 6:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue,  2 Nov 2010 17:11:36 +0800, Yuanhan Liu <yuanhan.liu@intel.com> wrote:
>> The AUB file is a file format used by Intel's internal simulation
>> and other validation tools. The content of an aub file is a subset
>> collection of all the data needed by GPU.
>>
>> Here now just collects the data that are going to be written into
>> register, since the others data like ring buffer, buffer objects
>> are dumped at user space by libdrm.
>>
>> The currentl implementation: hooks the I915_WRITEx macro. The basic
>> idea is write down all the reg offset and reg val before writing
>> them to register.
>
> This part looks possible to achieve just using tracepoints (and replace
> the current debug printk in the process). The advantage of that is we
> can then tap into a low overhead tracing mechanism with a *ringbuffer*!
> ;-) It will require a bit of userspace scripting to do the .aub format
> conversion.
>
> I presume the plan will be to include batchbuffer+auxiliary buffer
> contents as well in order to capture the complete temporal sequence for
> debugging+replay?

First of all, thanks for your comments.

Dumping batchbuffer and auxiliary buffer at kernel is good, but I'm not sure
is there any plan for this. Since the job is done well at user space by libdrm.
And it's a bit easier to control the aub debug feature at user space, which is
done by 'export INTEL_DEBUG=aub' then run some 3D apps, then an
i965.aub file generated. Of course, you can use libdrm trace media data, too.

While, we may need to dump some buffers, as some register's content refers
to a page address.  Anyway, that's why i915_aub_data is defined but not
i915_aub_reg.

>
> Being able to trace the register writes using ftrace is good... In fact we
> can remove our own printks and just use mmiotrace. So is it possible to
> post-process mmiotrace into .aub?

As I found mmiotrace can handle the operation(read or write), the
register address
and the value's size, yes, it is(I guess).  I will do more
investigation tomorrow.

-- 
regards
Yuanhan Liu

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu, Yuanhan]

On Tue, 2010-11-02 at 22:55 +0800, Liu Aleaxander wrote:
>
> > Being able to trace the register writes using ftrace is good... In fact we
> > can remove our own printks and just use mmiotrace. So is it possible to
> > post-process mmiotrace into .aub?
> 
> As I found mmiotrace can handle the operation(read or write), the
> register address
> and the value's size, yes, it is(I guess).  I will do more
> investigation tomorrow.
> 
> Hi,

One should do this 'echo mmiotrace > /sys/kernel/debug/tracing' to enable 
mmio trace. And this should be done before calling ioremap function, AKA, 
before loading a module. So, first, user should have to stop i915 modules 
from loading at the boot time. Then enable mmiotrace like said above, then 
modprobe i915 by hand. And yes, all the register data are recorded. But just 
a while, I get a data file about 1G(Since it recorded everything, and some 
register write and read too often).

So, I'm not sure this good for debug. One great advantage of aub file is that 
user can gave an aub file to us, then we may know what was going wrong, 
just like we often ask user for something like dmesg log. As it's a bit hard for 
user to generate an aub file, then it's not good for us to debug.

So, ideas and comments? (And feel free to correct me if I'm wrong)


Thanks!


--
regards
Yuanhan Liu

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu, Yuanhan]

On Thu, 2010-11-04 at 11:44 +0800, Liu, Yuanhan wrote:
One should do this 'echo mmiotrace > /sys/kernel/debug/tracing' to enable 
> mmio trace. 
> 
Oops, an error, it's 
$ echo mmiotrace >/sys/kernel/debug/tracing/current_tracer 

Thanks.

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Chris Wilson]

On Thu, 4 Nov 2010 11:44:39 +0800, "Liu, Yuanhan" <yuanhan.liu@intel.com> wrote:
> 
> On Tue, 2010-11-02 at 22:55 +0800, Liu Aleaxander wrote:
> >
> > > Being able to trace the register writes using ftrace is good... In fact we
> > > can remove our own printks and just use mmiotrace. So is it possible to
> > > post-process mmiotrace into .aub?
> > 
> > As I found mmiotrace can handle the operation(read or write), the
> > register address
> > and the value's size, yes, it is(I guess).  I will do more
> > investigation tomorrow.
> > 
> > Hi,
> 
> One should do this 'echo mmiotrace > /sys/kernel/debug/tracing' to enable 
> mmio trace. And this should be done before calling ioremap function, AKA, 
> before loading a module. So, first, user should have to stop i915 modules 
> from loading at the boot time. Then enable mmiotrace like said above, then 
> modprobe i915 by hand.

Ok, that is tricky for userland debugging.

> And yes, all the register data are recorded. But just 
> a while, I get a data file about 1G(Since it recorded everything, and some 
> register write and read too often).

Painful, but we could pipe the output from ftrace through a filter to
eliminate those.

> So, I'm not sure this good for debug. One great advantage of aub file is that 
> user can gave an aub file to us, then we may know what was going wrong, 
> just like we often ask user for something like dmesg log. As it's a bit hard for 
> user to generate an aub file, then it's not good for us to debug.
> 
> So, ideas and comments? (And feel free to correct me if I'm wrong)

I'm not going to give up on tracepoints just yet since they solve the
complex issue of exporting the data to userspace. The alternative is to
instrument I915_READ/I915_WRITE with a pair of our own tracepoints (so we
can selectively choose to record reads or writes) and introduce
I915_WRITE_UNTRACED/I915_READ_UNTRACED for the raw variants to be used in
GPIO.

Still requires post-processing to add the aub headers, but this will allow
runtime enabling of the capture. How does this sound?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu Aleaxander]

On Thu, Nov 4, 2010 at 5:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 4 Nov 2010 11:44:39 +0800, "Liu, Yuanhan" <yuanhan.liu@intel.com> wrote:
>>
>> On Tue, 2010-11-02 at 22:55 +0800, Liu Aleaxander wrote:
>> >
>> > > Being able to trace the register writes using ftrace is good... In fact we
>> > > can remove our own printks and just use mmiotrace. So is it possible to
>> > > post-process mmiotrace into .aub?
>> >
>> > As I found mmiotrace can handle the operation(read or write), the
>> > register address
>> > and the value's size, yes, it is(I guess).  I will do more
>> > investigation tomorrow.
>> >
>> > Hi,
>>
>> One should do this 'echo mmiotrace > /sys/kernel/debug/tracing' to enable
>> mmio trace. And this should be done before calling ioremap function, AKA,
>> before loading a module. So, first, user should have to stop i915 modules
>> from loading at the boot time. Then enable mmiotrace like said above, then
>> modprobe i915 by hand.
>
> Ok, that is tricky for userland debugging.
>
>> And yes, all the register data are recorded. But just
>> a while, I get a data file about 1G(Since it recorded everything, and some
>> register write and read too often).
>
> Painful, but we could pipe the output from ftrace through a filter to
> eliminate those.
>
>> So, I'm not sure this good for debug. One great advantage of aub file is that
>> user can gave an aub file to us, then we may know what was going wrong,
>> just like we often ask user for something like dmesg log. As it's a bit hard for
>> user to generate an aub file, then it's not good for us to debug.
>>
>> So, ideas and comments? (And feel free to correct me if I'm wrong)
>
> I'm not going to give up on tracepoints just yet since they solve the
> complex issue of exporting the data to userspace.

Yes, indeed.

> The alternative is to
> instrument I915_READ/I915_WRITE with a pair of our own tracepoints (so we
> can selectively choose to record reads or writes) and introduce
> I915_WRITE_UNTRACED/I915_READ_UNTRACED for the raw variants to be used in
> GPIO.

Sorry, I'm not sure I understand you. Our own tracepoints? what's the
difference with mmio
trace?

Assume we will use mmio trace to generate an aub file, I think the
steps would be something
like this:

1. make sure i915.ko module isn't loaded at boot time.
2. mount -t debugfs nodev /sys/kernel/debug
3. echo mmiotrace >/sys/kernel/debug/tracing/current_tracer   # enable
mmio trace
4. cat /sys/kernel/debug/tracing/trace_pipe >/tmp/out&            #
dump the data to a file
5. modprobe i915

After these step, all register data read and write is recorded in
/tmp/out file, it's a bit big, but yes,
just like Chris said, we can filter out something not useful for us,
so this would not be a problem.
Then use a script to convert these data to an aub file, and that's how
the aub file is generated
by using mmio trace.

Well, what are the steps you excepted to get an aub file? Please tell
me, then I may know your
points better.


>
> Still requires post-processing to add the aub headers, but this will allow
> runtime enabling of the capture. How does this sound?

Yes, runtime enabling of the capture is great. And post-processing
isn't that bad at all.
What I concerned is how much hard or easy for user to generate an aub
file. As Nanhai said,
if user met some problem, we can ask him for an aub file, then we
might know what was going
wrong. This is the biggest advantage of aub file. So, I'm not trying
to ask you give up the
tracepoints(it really nice for tracing something down), I just want an
easy way to get an aub file
for us, and especially for user.

Thanks!


-- 
regards
Yuanhan Liu

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Chris Wilson]

On Sat, 6 Nov 2010 00:43:46 +0800, Liu Aleaxander <aleaxander@gmail.com> wrote:
> Well, what are the steps you excepted to get an aub file? Please tell
> me, then I may know your
> points better.
> 
> 
> >
> > Still requires post-processing to add the aub headers, but this will allow
> > runtime enabling of the capture. How does this sound?
> 
> Yes, runtime enabling of the capture is great. And post-processing
> isn't that bad at all.
> What I concerned is how much hard or easy for user to generate an aub
> file. As Nanhai said,
> if user met some problem, we can ask him for an aub file, then we
> might know what was going
> wrong. This is the biggest advantage of aub file. So, I'm not trying
> to ask you give up the
> tracepoints(it really nice for tracing something down), I just want an
> easy way to get an aub file
> for us, and especially for user.

Right. If we want to ask the user to gather some debug info, it
essentially has to be from within X and be as simple as run
'intel-gpu-trace myapp'. Using ftrace is the simplest way to achieve
that. Having to rmmod i915.ko rules out mmiotrace as a viable candidate.
But we can easy add two tracepoints to I915_READ and I915_WRITE which can
be enabled at runtime, exported via /sys/kernel/debug/tracing and
automated in a little tool.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu Aleaxander]

On Sat, Nov 6, 2010 at 1:11 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Right. If we want to ask the user to gather some debug info, it
> essentially has to be from within X and be as simple as run
> 'intel-gpu-trace myapp'.

Some notes here: let's me try to make it clear this time. As I had
said, we can easily get an aub file at user space for Mesa and the
media part by:
$ export INTEL_DEBUG=aub
$ run-you-app-here.

Then an aub file is generated(i965.aub for mesa). This work is done at
libdrm, and for now, it works pretty well. While, for some reason, we
didn't release it out, but it should be soon;)

While, what we can't do at user space is to get the KMS data to make
an aub file. And this is what the patch for.

So, on the whole. We use libdrm to get an aub file, which collects the
batchbuffer and all the auxiliary buffers, at user space. While, this
patch to get an aub file for the KMS phase.

>Using ftrace is the simplest way to achieve that.

Then I got your idea;)  I will check this tomorrow.

Thanks!

>Having to rmmod i915.ko rules out mmiotrace as a viable candidate.
> But we can easy add two tracepoints to I915_READ and I915_WRITE which can
> be enabled at runtime, exported via /sys/kernel/debug/tracing and
> automated in a little tool.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
>



-- 
regards
Yuanhan Liu

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu Aleaxander]

On 11/6/10, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Right. If we want to ask the user to gather some debug info, it
> essentially has to be from within X and be as simple as run
> 'intel-gpu-trace myapp'. Using ftrace is the simplest way to achieve
> that. Having to rmmod i915.ko rules out mmiotrace as a viable candidate.
> But we can easy add two tracepoints to I915_READ and I915_WRITE which can
> be enabled at runtime, exported via /sys/kernel/debug/tracing and
> automated in a little tool.

I wrote a new patch(no post-processing yet).
Chris, is this what you want?

Thanks.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6212342..0537650 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -32,6 +32,7 @@

 #include "i915_reg.h"
 #include "intel_bios.h"
+#include "i915_trace.h"
 #include "intel_ringbuffer.h"
 #include <linux/io-mapping.h>
 #include <linux/i2c.h>
@@ -1173,14 +1174,58 @@ extern void
intel_overlay_print_error_state(struct seq_file *m, struct intel_ove
 		LOCK_TEST_WITH_RETURN(dev, file_priv);			\
 } while (0)

-#define I915_READ(reg)          readl(dev_priv->regs + (reg))
-#define I915_WRITE(reg, val)     writel(val, dev_priv->regs + (reg))
-#define I915_READ16(reg)	readw(dev_priv->regs + (reg))
-#define I915_WRITE16(reg, val)	writel(val, dev_priv->regs + (reg))
-#define I915_READ8(reg)		readb(dev_priv->regs + (reg))
-#define I915_WRITE8(reg, val)	writeb(val, dev_priv->regs + (reg))
-#define I915_WRITE64(reg, val)	writeq(val, dev_priv->regs + (reg))
-#define I915_READ64(reg)	readq(dev_priv->regs + (reg))
+static inline u32 i915_read(struct drm_i915_private *dev_priv, u32
reg, int len)
+{
+	u64 val = 0;
+
+	switch (len) {
+	case 8:
+		val = readq(dev_priv->regs + reg);
+		break;
+	case 4:
+		val = readl(dev_priv->regs + reg);
+		break;
+	case 2:
+		val = readw(dev_priv->regs + reg);
+		break;
+	case 1:
+		val = readb(dev_priv->regs + reg);
+		break;
+	}
+	trace_i915_reg_rw('R', reg, val, len);
+
+	return val;
+}
+
+static inline void
+i915_write(struct drm_i915_private *dev_priv, u32 reg, u64 val, int len)
+{
+	/* Trace down the write operation before the real write */
+	trace_i915_reg_rw('W', reg, val, len);
+	switch (len) {
+	case 8:
+		writeq(val, dev_priv->regs + reg);
+		break;
+	case 4:
+		writel(val, dev_priv->regs + reg);
+		break;
+	case 2:
+		writew(val, dev_priv->regs + reg);
+		break;
+	case 1:
+		writeb(val, dev_priv->regs + reg);
+		break;
+	}
+}
+
+#define I915_READ(reg)          i915_read(dev_priv, (reg), 4)
+#define I915_WRITE(reg, val)    i915_write(dev_priv, (reg), (val), 4)
+#define I915_READ16(reg)	i915_read(dev_priv, (reg), 2)
+#define I915_WRITE16(reg, val)	i915_write(dev_priv, (reg), (val), 2)
+#define I915_READ8(reg)		i915_read(dev_priv, (reg), 1)
+#define I915_WRITE8(reg, val)	i915_write(dev_priv, (reg), (val), 1)
+#define I915_WRITE64(reg, val)	i915_write(dev_priv, (reg), (val), 8)
+#define I915_READ64(reg)	i915_read(dev_priv, (reg), 8)
 #define POSTING_READ(reg)	(void)I915_READ(reg)
 #define POSTING_READ16(reg)	(void)I915_READ16(reg)

diff --git a/drivers/gpu/drm/i915/i915_trace.h
b/drivers/gpu/drm/i915/i915_trace.h
index 0b1049f..7113f29 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -301,6 +301,29 @@ TRACE_EVENT(i915_flip_complete,
 	    TP_printk("plane=%d, obj=%p", __entry->plane, __entry->obj)
 );

+TRACE_EVENT(i915_reg_rw,
+	    TP_PROTO(int cmd, uint32_t reg, uint64_t val, int len),
+
+	    TP_ARGS(cmd, reg, val, len),
+
+	    TP_STRUCT__entry(
+		    __field(int, cmd)
+		    __field(uint32_t, reg)
+		    __field(uint64_t, val)
+		    __field(int, len)
+		    ),
+
+	    TP_fast_assign(
+	    	    __entry->cmd = cmd;
+		    __entry->reg = reg;
+		    __entry->val = (uint64_t)val;
+		    __entry->len = len;
+		    ),
+
+	    TP_printk("cmd=%c, reg=0x%x, val=0x%llx, len=%d",
+	     	      __entry->cmd, __entry->reg, __entry->val, __entry->len)
+);
+
 #endif /* _I915_TRACE_H_ */

 /* This part must be outside protection */



-- 
regards
Yuanhan Liu

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Chris Wilson]

On Sun, 7 Nov 2010 22:08:00 +0800, Liu Aleaxander <aleaxander@gmail.com> wrote:
> I wrote a new patch(no post-processing yet).
> Chris, is this what you want?

Yes, that looks good. The compiler should be able to do its job and reduce
that code nicely.

I think you want, in addition, a pair of I915_READ_NOTRACE and
I915_WRITE_NOTRACE for the GPIO registers. I would suggest converting the
calls that you don't want traced as a separate patch to keep the commits
clean.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu Aleaxander]

On Sun, Nov 7, 2010 at 10:23 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sun, 7 Nov 2010 22:08:00 +0800, Liu Aleaxander <aleaxander@gmail.com> wrote:
>> I wrote a new patch(no post-processing yet).
>> Chris, is this what you want?
>
> Yes, that looks good.
Thanks:)

>The compiler should be able to do its job and reduce
> that code nicely.
>
> I think you want, in addition, a pair of I915_READ_NOTRACE and
> I915_WRITE_NOTRACE for the GPIO registers. I would suggest converting the
> calls that you don't want traced as a separate patch to keep the commits
> clean.
Sure.


But back to that issue again, how can we get the data of register
read/write at the KMS phase. i195:* trace event does exist only when
i915 module is loaded. Meanwhile, the KMS phase is passed by when you
are trying to enable i915_reg_rw event.

Chris, your ideas?

Thanks.


-- 
regards
Yuanhan Liu

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Chris Wilson]

On Sun, 7 Nov 2010 23:04:40 +0800, Liu Aleaxander <aleaxander@gmail.com> wrote:
> But back to that issue again, how can we get the data of register
> read/write at the KMS phase. i195:* trace event does exist only when
> i915 module is loaded. Meanwhile, the KMS phase is passed by when you
> are trying to enable i915_reg_rw event.
> 
> Chris, your ideas?

The hacky approach is a module parameter (i915.trace_load=1) that we parse
in i915_init() and enable the trace points manually. I remember this being
possible, once upon a time...

I'll keep digging to see how we can do it today.

The long term solution is to ask for a generic method to enable module
tracepoints upon module load.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Liu Aleaxander]

On Sun, Nov 7, 2010 at 11:33 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> The hacky approach is a module parameter (i915.trace_load=1) that we parse
> in i915_init() and enable the trace points manually. I remember this being
> possible, once upon a time...
Yes, I will check if it is possible tomorrow(night here).

>
> I'll keep digging to see how we can do it today.
>
> The long term solution is to ask for a generic method to enable module
> tracepoints upon module load.
agreed.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
>

Thanks.


-- 
regards
Yuanhan Liu

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Chris Wilson]

Oh my, this turns out to be quite hacky indeed...

if (i915_trace_on_load) {
	const struct ftrace_event_call enable_list[] = {
#define EVENT(name) event_call_##name
		&EVENT(i915_reg),
		NULL,
	}, *call = enable_list;
	do {
		if ((*call)->class->reg(*call, TRACE_REG_REGISTER) == 0)
			(*call)->flags |= TRACE_EVENT_FL_ENABLED;
		else
			DRM_DEBUG("failed to enable tracepoint '%s'\n",
				(*call)->name);
	} while (*++call);
}

[Not even compile tested! ;)]

That would seem to do the trick. The alternative would be to override the
use of trace_event_raw_init in the include/trace/ftrace.h macros. In light
of that the above seems much simpler.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Yuanhan Liu]

I just send two patches out, without trace on load patch for
now(explained at below).

On Mon, 2010-11-08 at 00:03 +0800, Chris Wilson wrote:
> Oh my, this turns out to be quite hacky indeed...
> 
> if (i915_trace_on_load) {
> 	const struct ftrace_event_call enable_list[] = {
> #define EVENT(name) event_call_##name
> 		&EVENT(i915_reg),
> 		NULL,
> 	}, *call = enable_list;
> 	do {
> 		if ((*call)->class->reg(*call, TRACE_REG_REGISTER) == 0)
> 			(*call)->flags |= TRACE_EVENT_FL_ENABLED;
> 		else
> 			DRM_DEBUG("failed to enable tracepoint '%s'\n",
> 				(*call)->name);
> 	} while (*++call);
> }
> 
> [Not even compile tested! ;)]

Sorry, it can't compile for not being able to find the event_call_*. 

While, that doesn't matter. As you may know, I send a patch to upstream 
to export trace_set_clr_event function. Then we can simply add the 
following line in i915_init():

trace_set_clr_event("i915", "i915_reg_rw", 1)

then the i915:i915_reg_rw event is enabled at the load time.

While, I am not saying I am waiting upstream to receive that patch. I 
mean we can hold on for a while: I think the trace-event can export 
some more friendly interface for enabling specified event at module 
load time. (I might make some patches if I have time).

Thanks.
> 
> That would seem to do the trick. The alternative would be to override the
> use of trace_event_raw_init in the include/trace/ftrace.h macros. In light
> of that the above seems much simpler.
> -Chris
> 

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Chris Wilson]

On Mon, 08 Nov 2010 17:23:28 +0800, Yuanhan Liu <yuanhan.liu@intel.com> wrote:
> I just send two patches out, without trace on load patch for
> now(explained at below).
> 
> On Mon, 2010-11-08 at 00:03 +0800, Chris Wilson wrote:
> > Oh my, this turns out to be quite hacky indeed...
> > 
> > if (i915_trace_on_load) {
> > 	const struct ftrace_event_call enable_list[] = {
> > #define EVENT(name) event_call_##name
> > 		&EVENT(i915_reg),
> > 		NULL,
> > 	}, *call = enable_list;
> > 	do {
> > 		if ((*call)->class->reg(*call, TRACE_REG_REGISTER) == 0)
> > 			(*call)->flags |= TRACE_EVENT_FL_ENABLED;
> > 		else
> > 			DRM_DEBUG("failed to enable tracepoint '%s'\n",
> > 				(*call)->name);
> > 	} while (*++call);
> > }
> > 
> > [Not even compile tested! ;)]
> 
> Sorry, it can't compile for not being able to find the event_call_*. 
> 
> While, that doesn't matter. As you may know, I send a patch to upstream 
> to export trace_set_clr_event function. Then we can simply add the 
> following line in i915_init():
> 
> trace_set_clr_event("i915", "i915_reg_rw", 1)
> 
> then the i915:i915_reg_rw event is enabled at the load time.

Much neater. I missed that when scanning trace/ for a hook in the event
system.

> While, I am not saying I am waiting upstream to receive that patch. I 
> mean we can hold on for a while: I think the trace-event can export 
> some more friendly interface for enabling specified event at module 
> load time. (I might make some patches if I have time).

So long as you can enable it locally to develop the debug tools that will
do for the time being.  You'll want to send the
EXPORT_SYMBOL_GPL(trace_set_clr_event) upstream and cc Steven Rostedt with
an outline of why we want it, he may be able to show us the best way of
integrating an module hook to parse tracing options.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add aub debug support for kernel

[Yuanhan Liu]

On Mon, Nov 08, 2010 at 09:55:23AM +0000, Chris Wilson wrote:
> > While, I am not saying I am waiting upstream to receive that patch. I 
> > mean we can hold on for a while: I think the trace-event can export 
> > some more friendly interface for enabling specified event at module 
> > load time. (I might make some patches if I have time).
> 
> So long as you can enable it locally to develop the debug tools that will
> do for the time being.  You'll want to send the
> EXPORT_SYMBOL_GPL(trace_set_clr_event) upstream and cc Steven Rostedt with
> an outline of why we want it, he may be able to show us the best way of
> integrating an module hook to parse tracing options.

Yes, he did. And seems that a friendly interface for enabling speified
event on module load is on the way;)

Thanks.

--
Yuanhan Liu