| Message ID | 1414164652-24281-1-git-send-email-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 24, 2014 at 04:30:52PM +0100, daniele.ceraolospurio@intel.com wrote: > From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > These tracepoints are useful for observing the creation and > destruction of Full PPGTTs. > > v4: add DOC information > v5: pull the DOC in drm.tmpl > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > +TRACE_EVENT(i915_ppgtt_create, > + TP_PROTO(struct i915_address_space *vm), > + > + TP_ARGS(vm), > + > + TP_STRUCT__entry( > + __field(struct i915_address_space *, vm) > + __field(u32, dev) > + __field(int, pid) > + ), > + > + TP_fast_assign( > + __entry->vm = vm; > + __entry->dev = vm->dev->primary->index; > + __entry->pid = (int)task_pid_nr(current); This is redundant. Current pid is part of the perf header (iirc at least). Besides which storing the creator pid is useful elsewhere when debugging vm (especially as now vm->pid != file->pid). > + ), > + > + TP_printk("dev=%u, task_pid=%d, vm=%p", > + __entry->dev, __entry->pid, __entry->vm) > +); > + > +TRACE_EVENT(i915_ppgtt_release, > + > + TP_PROTO(struct i915_address_space *vm), > + > + TP_ARGS(vm), > + > + TP_STRUCT__entry( > + __field(struct i915_address_space *, vm) > + __field(u32, dev) > + ), > + > + TP_fast_assign( > + __entry->vm = vm; > + __entry->dev = vm->dev->primary->index; > + ), > + > + TP_printk("dev=%u, vm=%p", __entry->dev, __entry->vm) > +); So what about switch_mm (accounting for both execlists/non-execlists)? ppgtt_close is also another important point in the lifetime, and so you also want ppgtt_open for symmetry. -Chris
On 10/27/2014 8:49 AM, Chris Wilson wrote: > On Fri, Oct 24, 2014 at 04:30:52PM +0100, daniele.ceraolospurio@intel.com wrote: >> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> These tracepoints are useful for observing the creation and >> destruction of Full PPGTTs. >> >> v4: add DOC information >> v5: pull the DOC in drm.tmpl >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> +TRACE_EVENT(i915_ppgtt_create, >> + TP_PROTO(struct i915_address_space *vm), >> + >> + TP_ARGS(vm), >> + >> + TP_STRUCT__entry( >> + __field(struct i915_address_space *, vm) >> + __field(u32, dev) >> + __field(int, pid) >> + ), >> + >> + TP_fast_assign( >> + __entry->vm = vm; >> + __entry->dev = vm->dev->primary->index; >> + __entry->pid = (int)task_pid_nr(current); > > This is redundant. Current pid is part of the perf header (iirc at > least). Besides which storing the creator pid is useful elsewhere when > debugging vm (especially as now vm->pid != file->pid). > You're right, I'll just remove it. >> + ), >> + >> + TP_printk("dev=%u, task_pid=%d, vm=%p", >> + __entry->dev, __entry->pid, __entry->vm) >> +); >> + >> +TRACE_EVENT(i915_ppgtt_release, >> + >> + TP_PROTO(struct i915_address_space *vm), >> + >> + TP_ARGS(vm), >> + >> + TP_STRUCT__entry( >> + __field(struct i915_address_space *, vm) >> + __field(u32, dev) >> + ), >> + >> + TP_fast_assign( >> + __entry->vm = vm; >> + __entry->dev = vm->dev->primary->index; >> + ), >> + >> + TP_printk("dev=%u, vm=%p", __entry->dev, __entry->vm) >> +); > > So what about switch_mm (accounting for both execlists/non-execlists)? I'll add a couple of tracepoints to cover them. > ppgtt_close is also another important point in the lifetime, and so you > also want ppgtt_open for symmetry. > -Chris > What do you mean with ppgtt_close and ppgtt_open? I don't see anything like that in my local d-i-n tree (pulled this morning). Thanks, Daniele
On Mon, Oct 27, 2014 at 11:03:26AM +0000, Ceraolo Spurio, Daniele wrote: > On 10/27/2014 8:49 AM, Chris Wilson wrote: > >On Fri, Oct 24, 2014 at 04:30:52PM +0100, daniele.ceraolospurio@intel.com wrote: > >>From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> > >>These tracepoints are useful for observing the creation and > >>destruction of Full PPGTTs. > >> > >>v4: add DOC information > >>v5: pull the DOC in drm.tmpl > >> > >>Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >>+TRACE_EVENT(i915_ppgtt_create, > >>+ TP_PROTO(struct i915_address_space *vm), > >>+ > >>+ TP_ARGS(vm), > >>+ > >>+ TP_STRUCT__entry( > >>+ __field(struct i915_address_space *, vm) > >>+ __field(u32, dev) > >>+ __field(int, pid) > >>+ ), > >>+ > >>+ TP_fast_assign( > >>+ __entry->vm = vm; > >>+ __entry->dev = vm->dev->primary->index; > >>+ __entry->pid = (int)task_pid_nr(current); > > > >This is redundant. Current pid is part of the perf header (iirc at > >least). Besides which storing the creator pid is useful elsewhere when > >debugging vm (especially as now vm->pid != file->pid). > > > > You're right, I'll just remove it. > > >>+ ), > >>+ > >>+ TP_printk("dev=%u, task_pid=%d, vm=%p", > >>+ __entry->dev, __entry->pid, __entry->vm) > >>+); > >>+ > >>+TRACE_EVENT(i915_ppgtt_release, > >>+ > >>+ TP_PROTO(struct i915_address_space *vm), > >>+ > >>+ TP_ARGS(vm), > >>+ > >>+ TP_STRUCT__entry( > >>+ __field(struct i915_address_space *, vm) > >>+ __field(u32, dev) > >>+ ), > >>+ > >>+ TP_fast_assign( > >>+ __entry->vm = vm; > >>+ __entry->dev = vm->dev->primary->index; > >>+ ), > >>+ > >>+ TP_printk("dev=%u, vm=%p", __entry->dev, __entry->vm) > >>+); > > > >So what about switch_mm (accounting for both execlists/non-execlists)? > > I'll add a couple of tracepoints to cover them. > > >ppgtt_close is also another important point in the lifetime, and so you > >also want ppgtt_open for symmetry. > >-Chris > > > > What do you mean with ppgtt_close and ppgtt_open? I don't see > anything like that in my local d-i-n tree (pulled this morning). I was thinking of context open/close. They need tracepoints too. The issue is that we may hold onto the vm long after the process is closed so tracing that would be useful. -Chris
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index f6a9d7b..a372f52 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -3963,6 +3963,19 @@ int num_ioctls;</synopsis> !Idrivers/gpu/drm/i915/intel_lrc.c </sect2> </sect1> + + <sect1> + <title> Tracing </title> + <para> + This sections covers all things related to the tracepoints implemented in + the i915 driver. + </para> + <sect2> + <title> i915_ppgtt_create and i915_ppgtt_release </title> +!Pdrivers/gpu/drm/i915/i915_trace.h i915_ppgtt_create and i915_ppgtt_release tracepoints + </sect2> + </sect1> + </chapter> !Cdrivers/gpu/drm/i915/i915_irq.c </part> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index e0bcba0..ed9ec67 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1174,6 +1174,8 @@ i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv) ppgtt->file_priv = fpriv; + trace_i915_ppgtt_create(&ppgtt->base); + return ppgtt; } @@ -1182,6 +1184,8 @@ void i915_ppgtt_release(struct kref *kref) struct i915_hw_ppgtt *ppgtt = container_of(kref, struct i915_hw_ppgtt, ref); + trace_i915_ppgtt_release(&ppgtt->base); + /* vmas should already be unbound */ WARN_ON(!list_empty(&ppgtt->base.active_list)); WARN_ON(!list_empty(&ppgtt->base.inactive_list)); diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index f5aa006..525be1b 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -587,6 +587,56 @@ TRACE_EVENT(intel_gpu_freq_change, TP_printk("new_freq=%u", __entry->freq) ); +/** + * DOC: i915_ppgtt_create and i915_ppgtt_release tracepoints + * + * With full ppgtt enabled each process using drm will allocate at least one + * translation table. With these traces it is possible to keep track of the + * allocation and of the lifetime of the tables; this can be used during + * testing/debug to verify that we are not leaking ppgtts. + * These traces identify the ppgtt through the vm pointer, which is also printed + * by the i915_vma_bind and i915_vma_unbind tracepoints. + */ +TRACE_EVENT(i915_ppgtt_create, + TP_PROTO(struct i915_address_space *vm), + + TP_ARGS(vm), + + TP_STRUCT__entry( + __field(struct i915_address_space *, vm) + __field(u32, dev) + __field(int, pid) + ), + + TP_fast_assign( + __entry->vm = vm; + __entry->dev = vm->dev->primary->index; + __entry->pid = (int)task_pid_nr(current); + ), + + TP_printk("dev=%u, task_pid=%d, vm=%p", + __entry->dev, __entry->pid, __entry->vm) +); + +TRACE_EVENT(i915_ppgtt_release, + + TP_PROTO(struct i915_address_space *vm), + + TP_ARGS(vm), + + TP_STRUCT__entry( + __field(struct i915_address_space *, vm) + __field(u32, dev) + ), + + TP_fast_assign( + __entry->vm = vm; + __entry->dev = vm->dev->primary->index; + ), + + TP_printk("dev=%u, vm=%p", __entry->dev, __entry->vm) +); + #endif /* _I915_TRACE_H_ */ /* This part must be outside protection */