[RFC v2 0/3] drm/i915: Trace point callbacks for kernel validation

[RFC v2 0/3] drm/i915: Trace point callbacks for kernel validation

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

The callbacks are useful for validation because they can be used to extract 
real-time information from the kernel.

v2: duplicate the i915_gem_ring_dispatch trace instead of modifying the 
existing one.

I plan to resubmit these patches together with an igt test and a kernel module
that uses the callbacks (as requested), but before doing that I wanted to be 
sure that I've correctly addressed all the comments.

Daniele Ceraolo Spurio (3):
  drm/i915: Add ppgtt init/release trace points
  drm/i915: duplicate i915_gem_ring_dispatch trace and add ctx parameter
  drm/i915: Trace point callbacks for validation

 drivers/gpu/drm/i915/i915_gem_context.c    |  9 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 ++
 drivers/gpu/drm/i915/i915_trace.h          | 90 ++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)

-- 
1.8.5.2

[RFC v2 1/3] drm/i915: Add ppgtt init/release trace points

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

These tracepoints are useful for observing the creation and
destruction of Full PPGTTs.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c |  6 +++++
 drivers/gpu/drm/i915/i915_trace.h       | 41 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index de72a28..133da7b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -88,6 +88,7 @@
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "i915_trace.h"
 
 /* This is a HW constraint. The value below is the largest known requirement
  * I've seen in a spec to date, and that was a workaround for a non-shipping
@@ -136,6 +137,8 @@ static void ppgtt_release(struct kref *kref)
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(kref, struct i915_hw_ppgtt, ref);
 
+	trace_ppgtt_release(ppgtt);
+
 	do_ppgtt_cleanup(ppgtt);
 	kfree(ppgtt);
 }
@@ -245,6 +248,9 @@ create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx)
 	}
 
 	ppgtt->ctx = ctx;
+
+	trace_create_vm_for_ctx(ppgtt);
+
 	return ppgtt;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index f5aa006..9be1421 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -587,6 +587,47 @@ TRACE_EVENT(intel_gpu_freq_change,
 	    TP_printk("new_freq=%u", __entry->freq)
 );
 
+TRACE_EVENT(create_vm_for_ctx,
+
+	TP_PROTO(struct i915_hw_ppgtt *ppgtt),
+
+	TP_ARGS(ppgtt),
+
+	TP_STRUCT__entry(
+			__field(struct i915_address_space *, vm)
+			__field(u32, dev)
+			__field(u32, pid)
+	),
+
+	TP_fast_assign(
+			__entry->vm = &ppgtt->base;
+			__entry->dev = ppgtt->base.dev->primary->index;
+			__entry->pid = (unsigned int)task_pid_nr(current);
+	),
+
+	TP_printk("dev=%u, task_pid=%u, vm=%p",
+		  __entry->dev, __entry->pid, __entry->vm)
+);
+
+TRACE_EVENT(ppgtt_release,
+
+	TP_PROTO(struct i915_hw_ppgtt *ppgtt),
+
+	TP_ARGS(ppgtt),
+
+	TP_STRUCT__entry(
+			__field(struct i915_address_space *, vm)
+			__field(u32, dev)
+	),
+
+	TP_fast_assign(
+		__entry->vm = &ppgtt->base;
+		__entry->dev = ppgtt->base.dev->primary->index;
+	),
+
+	TP_printk("dev=%u, vm=%p", __entry->dev, __entry->vm)
+);
+
 #endif /* _I915_TRACE_H_ */
 
 /* This part must be outside protection */
-- 
1.8.5.2

[RFC v2 2/3] drm/i915: duplicate i915_gem_ring_dispatch trace and add ctx parameter

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

The context used to execute a batchbuffer is becoming increasingly
important. Duplicating to avoid modifications to the original trace.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 ++
 drivers/gpu/drm/i915/i915_trace.h          | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 60998fc..6b0dd9f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1174,6 +1174,8 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	}
 
 	trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
+	trace_i915_gem_ring_dispatch_validation(ring,
+					intel_ring_get_seqno(ring), flags, ctx);
 
 	i915_gem_execbuffer_move_to_active(vmas, ring);
 	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 9be1421..d639d6c 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -374,6 +374,33 @@ TRACE_EVENT(i915_gem_ring_dispatch,
 		      __entry->dev, __entry->ring, __entry->seqno, __entry->flags)
 );
 
+TRACE_EVENT(i915_gem_ring_dispatch_validation,
+	    TP_PROTO(struct intel_engine_cs *ring, u32 seqno, u32 flags,
+		     struct intel_context *ctx),
+	    TP_ARGS(ring, seqno, flags, ctx),
+
+	    TP_STRUCT__entry(
+			     __field(u32, dev)
+			     __field(u32, ring)
+			     __field(u32, seqno)
+			     __field(u32, flags)
+			     __field(struct i915_address_space *, vm)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->dev = ring->dev->primary->index;
+			   __entry->ring = ring->id;
+			   __entry->seqno = seqno;
+			   __entry->flags = flags;
+			   __entry->vm = ctx->vm;
+			   i915_trace_irq_get(ring, seqno);
+			   ),
+
+	    TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x, vm=%p",
+		      __entry->dev, __entry->ring, __entry->seqno,
+		      __entry->flags, __entry->vm)
+);
+
 TRACE_EVENT(i915_gem_ring_flush,
 	    TP_PROTO(struct intel_engine_cs *ring, u32 invalidate, u32 flush),
 	    TP_ARGS(ring, invalidate, flush),
-- 
1.8.5.2

Re: [RFC v2 2/3] drm/i915: duplicate i915_gem_ring_dispatch trace and add ctx parameter

[Chris Wilson]

On Wed, Jul 16, 2014 at 05:22:38PM +0100, daniele.ceraolospurio@intel.com wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> The context used to execute a batchbuffer is becoming increasingly
> important. Duplicating to avoid modifications to the original trace.

I am sure we don't want both. The structure encoding is exposed to
userspace so we are free to update the tracepoints within reason.
I would also like a better ctx identifier than its pointer. Using the
pointer for tracking objects makes it more difficult to read traces
(although it is easy for scripts).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC v2 2/3] drm/i915: duplicate i915_gem_ring_dispatch trace and add ctx parameter

[Ceraolo Spurio, Daniele]

On 7/17/2014 5:25 PM, Chris Wilson wrote:
> On Wed, Jul 16, 2014 at 05:22:38PM +0100, daniele.ceraolospurio@intel.com wrote:
>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> The context used to execute a batchbuffer is becoming increasingly
>> important. Duplicating to avoid modifications to the original trace.
>
> I am sure we don't want both. The structure encoding is exposed to
> userspace so we are free to update the tracepoints within reason.

As you can see by the next patch in the series, I plan to add a callback 
inside the trace. My original patch modified the existing trace, but (if 
I've understood correctly) Daniel asked for a duplicated trace to avoid 
adding the callback into the existing one.

> I would also like a better ctx identifier than its pointer. Using the
> pointer for tracking objects makes it more difficult to read traces
> (although it is easy for scripts).

I use the VM pointer to track the ppgtt; that pointer is also printed by 
several other traces, including the ppgtt init/release ones that I've 
submitted for comments in this series. However, I don't mind changing 
the way we identify the ctx as long as I still have access to the VM 
pointer. I'll have a look at the possible ways of identifying the ctx 
and I'll try to find a better solution than the current one.

thanks,
Daniele

Re: [RFC v2 2/3] drm/i915: duplicate i915_gem_ring_dispatch trace and add ctx parameter

[Daniel Vetter]

On Fri, Jul 18, 2014 at 10:43:36AM +0100, Ceraolo Spurio, Daniele wrote:
> On 7/17/2014 5:25 PM, Chris Wilson wrote:
> >On Wed, Jul 16, 2014 at 05:22:38PM +0100, daniele.ceraolospurio@intel.com wrote:
> >>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>
> >>The context used to execute a batchbuffer is becoming increasingly
> >>important. Duplicating to avoid modifications to the original trace.
> >
> >I am sure we don't want both. The structure encoding is exposed to
> >userspace so we are free to update the tracepoints within reason.
> 
> As you can see by the next patch in the series, I plan to add a callback
> inside the trace. My original patch modified the existing trace, but (if
> I've understood correctly) Daniel asked for a duplicated trace to avoid
> adding the callback into the existing one.

I guess there was a misunderstanding. I was worried about changing the
tracepoint, but apparently Chris disagrees.

The callback in the tracepoint is an unrelated issue and a no-go either
way.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC v2 2/3] drm/i915: duplicate i915_gem_ring_dispatch trace and add ctx parameter

[Ceraolo Spurio, Daniele]

>> I would also like a better ctx identifier than its pointer. Using the
>> pointer for tracking objects makes it more difficult to read traces
>> (although it is easy for scripts).
>
> I use the VM pointer to track the ppgtt; that pointer is also printed by
> several other traces, including the ppgtt init/release ones that I've
> submitted for comments in this series. However, I don't mind changing
> the way we identify the ctx as long as I still have access to the VM
> pointer. I'll have a look at the possible ways of identifying the ctx
> and I'll try to find a better solution than the current one.
>

I've looked for other ways to identify the ctx, but I could't find 
anything more readable than a pointer. The best alternative in my 
opinion is to use the user_handle, but that is dependent on the 
file_priv so it is not enough on its own. Coupling the user_handle with 
the file_priv pointer doesn't seem like a good idea, so I still think 
that using the ctx pointer or the vm pointer is the best way to identify 
the ctx at the moment. Did you have something in mind that I've missed?

Thanks,
Daniele

[RFC v2 3/3] drm/i915: Trace point callbacks for validation

[daniele.ceraolospurio]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

These callbacks can be assigned to specific functions inside an external
validation kernel module. This module can then extract run-time
information to make sure everything is working as expected.

Specifically, these two callbacks extract information about full PPGTT
and batch dispatch.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c    |  3 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  3 +++
 drivers/gpu/drm/i915/i915_trace.h          | 22 ++++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 133da7b..9fc81a2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -90,6 +90,9 @@
 #include "i915_drv.h"
 #include "i915_trace.h"
 
+i915_ppgtt_validation_callback_t ppgtt_validation_callback = NULL;
+EXPORT_SYMBOL_GPL(ppgtt_validation_callback);
+
 /* This is a HW constraint. The value below is the largest known requirement
  * I've seen in a spec to date, and that was a workaround for a non-shipping
  * part. It should be safe to decrease this, but it's more future proof as is.
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6b0dd9f..e5daef10 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -33,6 +33,9 @@
 #include "intel_drv.h"
 #include <linux/dma_remapping.h>
 
+i915_gem_ring_dispatch_callback_t i915_gem_ring_dispatch_validation_callback = NULL;
+EXPORT_SYMBOL_GPL(i915_gem_ring_dispatch_validation_callback);
+
 #define  __EXEC_OBJECT_HAS_PIN (1<<31)
 #define  __EXEC_OBJECT_HAS_FENCE (1<<30)
 #define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index d639d6c..a16e29c 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -15,6 +15,18 @@
 #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
 #define TRACE_INCLUDE_FILE i915_trace
 
+/* validation callbacks */
+
+typedef void (*i915_ppgtt_validation_callback_t)(unsigned int action_code,
+						 struct i915_hw_ppgtt *ppgtt);
+extern i915_ppgtt_validation_callback_t ppgtt_validation_callback;
+
+typedef void (*i915_gem_ring_dispatch_callback_t)(struct intel_engine_cs *ring,
+						  u32 seqno,
+						  u32 flags,
+						  struct intel_context *ctx);
+extern i915_gem_ring_dispatch_callback_t i915_gem_ring_dispatch_validation_callback;
+
 /* pipe updates */
 
 TRACE_EVENT(i915_pipe_update_start,
@@ -394,6 +406,10 @@ TRACE_EVENT(i915_gem_ring_dispatch_validation,
 			   __entry->flags = flags;
 			   __entry->vm = ctx->vm;
 			   i915_trace_irq_get(ring, seqno);
+
+			   if (i915_gem_ring_dispatch_validation_callback)
+				   i915_gem_ring_dispatch_validation_callback(ring,
+						      seqno, flags, ctx);
 			   ),
 
 	    TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x, vm=%p",
@@ -630,6 +646,9 @@ TRACE_EVENT(create_vm_for_ctx,
 			__entry->vm = &ppgtt->base;
 			__entry->dev = ppgtt->base.dev->primary->index;
 			__entry->pid = (unsigned int)task_pid_nr(current);
+
+			if (ppgtt_validation_callback)
+				ppgtt_validation_callback(0, ppgtt);
 	),
 
 	TP_printk("dev=%u, task_pid=%u, vm=%p",
@@ -650,6 +669,9 @@ TRACE_EVENT(ppgtt_release,
 	TP_fast_assign(
 		__entry->vm = &ppgtt->base;
 		__entry->dev = ppgtt->base.dev->primary->index;
+
+		if (ppgtt_validation_callback)
+			ppgtt_validation_callback(1, ppgtt);
 	),
 
 	TP_printk("dev=%u, vm=%p", __entry->dev, __entry->vm)
-- 
1.8.5.2