All of lore.kernel.org
 help / color / mirror / Atom feed
* uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
@ 2018-12-18  1:26 Michael Sartain
  2018-12-18  2:14 ` Steven Rostedt
  2018-12-18 11:33 ` Chris Wilson
  0 siblings, 2 replies; 11+ messages in thread
From: Michael Sartain @ 2018-12-18  1:26 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ingo Molnar, Pierre-Loup Griffais, Steven Rostedt

I'm writing to try and make a case for Tvrtko's "Remove
DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig" patch:

  https://lists.freedesktop.org/archives/intel-gfx/2018-June/169052.html

Including the tracing team maintainers because this conversation intersects
with their area. Also Pierre-Loup from Valve since he convinced me to start and
work on gpuvis*, helped designed it, and Valve and Steam game developers are
using it.

For background, Valve Software paid me to write gpuvis to use with Steam,
games, and the AMD open source driver starting in early 2017. Gpuvis is a tool
much like GPUView** on Windows. These have some similarities to KernelShark,
but with the added functionality of parsing specific gpu driver tracepoints and
graphing detailed information about GPU work such as submit time, SW queuing,
HW execution times, etc. We also use the ftrace trace_marker functionality to
allow userland applications to insert data which can also be shown, plotted,
graphed, etc.

Gpuvis is hooked into Valve's SteamVR platform. When the developer clicks a
checkbox, ftrace is started in the background capturing the last ~ 3-5 seconds
of trace data in kernel ring buffers. Hitting F6 captures that data using
trace-cmd, saves it, and then launches gpuvis. Here is a screenshot from a
capture with GPU info and userland print events:

  https://i.imgur.com/XhuMuZu.png

Since Intel also has an open source driver with similar tracepoints, I added
i915 support to gpuvis as well. There are eight tracepoints that the i915
driver currently exposes. Five are enabled all the time, three are exposed only
when the DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option is enabled, which it
typically isn't by default. This essentially means that you have to rebuild the
Linux kernel to get the full functionality for Intel traces that you get with
AMD out of the box.

  * i915_request_queue
  * i915_request_add
  * i915_request_wait_begin
  * i915_request_wait_end
  * intel_engine_notify
  * i915_request_submit [LOW_LEVEL]
  * i915_request_in [LOW_LEVEL]
  * i915_request_out [LOW_LEVEL]

The patch Tvrtko submitted removes the DRM_I915_LOW_LEVEL_TRACEPOINTS ifdef and
brings full Intel tracing functionality without requiring kernel rebuilds.

The two main arguments I've heard against this patch are:

(a) Why would application developers care for seeing level of detail which they
    can't control?

Gpuvis is also for driver developers but even if not, please don't try to
withhold information from us game developers. Valve paid for this tool and
worked directly with driver devs to get it working *exactly* like all of them
wanted. Oftentimes traces of game capture glitches are sent to driver
developers, and conversations go back and forth between the two. Gpuvis can
also present data in an easier to digest format turning low level tracepoints
like amdgpu_cs_ioctl, amdgpu_sched_run_job, and fence_signaled into "SW queue",
"HW queue", and "Execution durations". If you ever do need to look into the
black box though, that data is also there.

(b) We can't expose tracepoints since they'll now be a stable uABI, and things
    will change in the future.

This argument intersects the discussion going on with tracepoints being
exposed as stable uABIs: What happens when these tracepoints disappear, change,
or are un-implementable in the future and break user space. Tracepoints limit
our future freedom, etc.

> The introduced interface will have to provide the information for years
> and kernel versions to come ...

And:

> We need to be cautious about inventing too many metrics here. As Joonas
> pointed out several times, once we add these into a uAPI, we are stuck with
> them, and that could stifle our freedom to tweak the GuC scheduling policies.

And:

> We don't have the option where you can make a promise that "we won't
> complain if the interface breaks". If gpuvis is to be bundled to some
> distro and the users would experience breakage when updating their
> kernel (say, due to security update), it'd be irrelevant what the
> gpuvis team's opinions are and kernel team would be forced to fix the
> fallout due to the kernel policies.

For what it's worth, I agree with Ingo on this subject:

  http://lkml.iu.edu/hypermail/linux/kernel/1011.2/00991.html

That post was from 2010, but I think it's still quite relevant to keep in mind.

Also:

 1) Valve, game developers, and AMD driver developers have already been using
    gpuvis with tracepoints for over 1.5 years now.
 2) The AMD and Intel tracepoints have *already* each changed and gpuvis was
    just updated to work with the new tracepoints.
 3) To be clear, "break" in this case just means the app showing unknown
    tracepoints without pretty graphs, falling back to something ~ KernelShark.
 4) Five of the eight Intel tracepoints gpuvis is using are already outside the
    LOW_LEVEL Kconfig. Are these stable uABIs?

But ok, instead of using the current already working Linux tracepoint system,
we're going to write a new userland API for this functionality. Something like:

> My initial idea would be to have DRM subsystem level profiling
> interface with versioning and vendor-specific extensions. Similar to
> i915 error capture, but from sysfs. So DRM drivers (when instructed to
> do so) would start capturing the profiling information in ring buffer
> and freeze it when the application asks to. Then it could be restarted
> after reading the previous log, similar to i915 error capture.
>
> With the DRM subsystem level profiling interface, if we can make the
> usermode drivers amend to the log information with their API level
> information. And further, if through debugging extensions we allow the
> user to amend to the log all the way from shader level, I think we've
> far bypassed ftrace's purpose of tracing what is going on inside the
> kernel.

Linux currently has ftrace and perf, and now we're going to write another
profiling subsystem that exposes Intel gpu data? This system sure sounds a lot
like perf/ftrace. And btw, I'm still going to have to use perf/ftrace to get to
data from the other subsystems like sched, ext4, and CPU counters, etc. Plus
getting Valve to update their use of trace_marker, and we'll also have a new
set of tools like "perf" and "trace-cmd" for this?

All because exposing tracepoints automatically introduce uABIs that can't ever
change?

Ftrace and perf are fantastic, stable, very well known, and documented with
ecosystems built around them. AMD already is doing exactly what we are asking
for with tracepoints, and Intel has tracepoints that work right now. Please,
there must be a way we can enable those and use the current tracing systems
without it introducing a stable tracepoint uABI which promises it'll never
change until the end of days?

Thank you.
 -Mike

Full disclosure: I'm currently working at Intel right now.

[*] https://github.com/mikesart/gpuvis
[**] https://graphics.stanford.edu/~mdfisher/GPUView.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-18  1:26 uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig Michael Sartain
@ 2018-12-18  2:14 ` Steven Rostedt
  2018-12-18 19:56   ` Pierre-Loup A. Griffais
  2018-12-19  8:45   ` Joonas Lahtinen
  2018-12-18 11:33 ` Chris Wilson
  1 sibling, 2 replies; 11+ messages in thread
From: Steven Rostedt @ 2018-12-18  2:14 UTC (permalink / raw)
  To: Michael Sartain; +Cc: intel-gfx, Ingo Molnar, Pierre-Loup Griffais

On Mon, 17 Dec 2018 18:26:03 -0700
Michael Sartain <mikesart@fastmail.com> wrote:

> Ftrace and perf are fantastic, stable, very well known, and documented with
> ecosystems built around them. AMD already is doing exactly what we are asking
> for with tracepoints, and Intel has tracepoints that work right now. Please,
> there must be a way we can enable those and use the current tracing systems
> without it introducing a stable tracepoint uABI which promises it'll never
> change until the end of days?

Would it be helpful if we implemented a way that trace events would not
show up in the tracefs filesystem unless a specifc kernel command line
(or module) parameter was supplied?

That would make for trace events not to be visible by default, but
allow for the same kernel to make them visible with a simple reboot (or
module reload). Like adding an option: i915_debug_tracepoints ?

Or perhaps a sysrq switch?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-18  1:26 uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig Michael Sartain
  2018-12-18  2:14 ` Steven Rostedt
@ 2018-12-18 11:33 ` Chris Wilson
  2018-12-18 18:14   ` Pierre-Loup A. Griffais
  1 sibling, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2018-12-18 11:33 UTC (permalink / raw)
  To: Michael Sartain, intel-gfx
  Cc: Ingo Molnar, Steven Rostedt, Pierre-Loup Griffais

Quoting Michael Sartain (2018-12-18 01:26:03)
> I'm writing to try and make a case for Tvrtko's "Remove
> DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig" patch:
> 
>   https://lists.freedesktop.org/archives/intel-gfx/2018-June/169052.html

I'd recommend we just remove the tracepoints as in the very near future
we will break them and the outlook seems bleak that we can support the
same information in the future.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-18 11:33 ` Chris Wilson
@ 2018-12-18 18:14   ` Pierre-Loup A. Griffais
  2018-12-19 10:08     ` Joonas Lahtinen
  0 siblings, 1 reply; 11+ messages in thread
From: Pierre-Loup A. Griffais @ 2018-12-18 18:14 UTC (permalink / raw)
  To: Chris Wilson, Michael Sartain, intel-gfx; +Cc: Ingo Molnar, Steven Rostedt


On 12/18/18 3:33 AM, Chris Wilson wrote:
> Quoting Michael Sartain (2018-12-18 01:26:03)
>> I'm writing to try and make a case for Tvrtko's "Remove
>> DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig" patch:
>>
>>    https://lists.freedesktop.org/archives/intel-gfx/2018-June/169052.html
> 
> I'd recommend we just remove the tracepoints as in the very near future
> we will break them and the outlook seems bleak that we can support the
> same information in the future.

Is it due to moving to a more async (or HW-assisted) submission model? 
If so, I assume/hope there's still a way to reflect on when things 
completed after the fact? You could still emit tracepoints at the time 
when that becomes known, with the time delta marked in the tracepoint 
itself. gpuvis supports this, but the syntax it currently uses for it 
isn't necessarily important. There's not much value in looking at these 
tracepoints fly by in realtime, so being deferred a bit works well for 
all scenarios where examining a GPU scheduling trace is useful.

> -Chris
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-18  2:14 ` Steven Rostedt
@ 2018-12-18 19:56   ` Pierre-Loup A. Griffais
  2018-12-19  8:45   ` Joonas Lahtinen
  1 sibling, 0 replies; 11+ messages in thread
From: Pierre-Loup A. Griffais @ 2018-12-18 19:56 UTC (permalink / raw)
  To: Steven Rostedt, Michael Sartain; +Cc: intel-gfx, Ingo Molnar

On 12/17/18 6:14 PM, Steven Rostedt wrote:
> On Mon, 17 Dec 2018 18:26:03 -0700
> Michael Sartain <mikesart@fastmail.com> wrote:
> 
>> Ftrace and perf are fantastic, stable, very well known, and documented with
>> ecosystems built around them. AMD already is doing exactly what we are asking
>> for with tracepoints, and Intel has tracepoints that work right now. Please,
>> there must be a way we can enable those and use the current tracing systems
>> without it introducing a stable tracepoint uABI which promises it'll never
>> change until the end of days?
> 
> Would it be helpful if we implemented a way that trace events would not
> show up in the tracefs filesystem unless a specifc kernel command line
> (or module) parameter was supplied?
> 
> That would make for trace events not to be visible by default, but
> allow for the same kernel to make them visible with a simple reboot (or
> module reload). Like adding an option: i915_debug_tracepoints ?
> 
> Or perhaps a sysrq switch?

If something like that was really needed, being able to toggle it (as 
root) without rebooting would be good, otherwise the user experience of 
using such tracing facilities to debug a stutter would suffer quite a bit.

> 
> -- Steve
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-18  2:14 ` Steven Rostedt
  2018-12-18 19:56   ` Pierre-Loup A. Griffais
@ 2018-12-19  8:45   ` Joonas Lahtinen
  1 sibling, 0 replies; 11+ messages in thread
From: Joonas Lahtinen @ 2018-12-19  8:45 UTC (permalink / raw)
  To: Michael Sartain, Steven Rostedt
  Cc: David Airlie, intel-gfx, Ingo Molnar, Linus Torvalds,
	Pierre-Loup Griffais

+ Adding Linus, Dave and Daniel
(because by my knowledge, we're beating a dead horse here)

Quoting Steven Rostedt (2018-12-18 04:14:17)
> On Mon, 17 Dec 2018 18:26:03 -0700
> Michael Sartain <mikesart@fastmail.com> wrote:
> 
> > Ftrace and perf are fantastic, stable, very well known, and documented with
> > ecosystems built around them. AMD already is doing exactly what we are asking
> > for with tracepoints, and Intel has tracepoints that work right now. Please,
> > there must be a way we can enable those and use the current tracing systems
> > without it introducing a stable tracepoint uABI which promises it'll never
> > change until the end of days?
> 
> Would it be helpful if we implemented a way that trace events would not
> show up in the tracefs filesystem unless a specifc kernel command line
> (or module) parameter was supplied?
> 
> That would make for trace events not to be visible by default, but
> allow for the same kernel to make them visible with a simple reboot (or
> module reload). Like adding an option: i915_debug_tracepoints ?
> 
> Or perhaps a sysrq switch?

This wouldn't really change much for the user, would it? After they
update their kernel, the application from their distro that worked
yesterday for their profiling needs, stops working. Kernel parameters
are really not that hard to add. We're today seeing from bug reports
existing debug-only module parameters in daily use causing trouble,
so that we're heavily reducing them already.

Those exact tracepoints are not enabled by default because we've
specifically made an assesment that we'll have hard time maintaining
their delivery in the future with all the changes in our pipeline, even
for the existing platforms. So once you update to a kernel with more
advanced i915 request scheduling, you can wave a goodbye to those.

I'm bit unsure how this situation would be any different from what is
described here by Linus:

https://lkml.org/lkml/2016/11/25/733

If we add new tracepoints that are much closer to the real hardware
events (and thus, less of implementation detail of today), going
forward we'd need to be able to insert trace entries that were captured
by hardware.

The hacky way of doing this is to have the kernel emit tracepoints
from the hardware feed, where the timestamp is an argument. But that
already feels like we've gone beyond what tracepoints were intended
for, and reduced the usefulness of the existing tools.

That combined with the stall of versioning discussion for tracepoints
has kind of got me thinking that a more special purpose uAPI is
needed to solve this elegantly in reasonable time.

But if the tables have turned and we foresee a path forwards solving
the above mentioned issues with tracepoints, I'm happy to hear that
too.

Regards, Joonas

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-18 18:14   ` Pierre-Loup A. Griffais
@ 2018-12-19 10:08     ` Joonas Lahtinen
  2018-12-19 19:22       ` Steven Rostedt
  0 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2018-12-19 10:08 UTC (permalink / raw)
  To: Pierre-Loup A. Griffais, Chris Wilson, Michael Sartain, intel-gfx
  Cc: Ingo Molnar, Steven Rostedt

Quoting Pierre-Loup A. Griffais (2018-12-18 20:14:49)
> 
> On 12/18/18 3:33 AM, Chris Wilson wrote:
> > Quoting Michael Sartain (2018-12-18 01:26:03)
> >> I'm writing to try and make a case for Tvrtko's "Remove
> >> DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig" patch:
> >>
> >>    https://lists.freedesktop.org/archives/intel-gfx/2018-June/169052.html
> > 
> > I'd recommend we just remove the tracepoints as in the very near future
> > we will break them and the outlook seems bleak that we can support the
> > same information in the future.
> 
> Is it due to moving to a more async (or HW-assisted) submission model? 
> If so, I assume/hope there's still a way to reflect on when things 
> completed after the fact? You could still emit tracepoints at the time 
> when that becomes known, with the time delta marked in the tracepoint 
> itself. gpuvis supports this, but the syntax it currently uses for it 
> isn't necessarily important. There's not much value in looking at these 
> tracepoints fly by in realtime, so being deferred a bit works well for 
> all scenarios where examining a GPU scheduling trace is useful.

When you combine HW-assisted scheduling with the Virtual Engine work we
have ongoing, there will basically be a situation where the KMD only
indicates to hardware which contexts are ready to execute, and it'll
just care when *some* requests finish that userspace is making wait
IOCTLs on.

So kernel's view will just be that it wrote a request to logical ring,
indicated that it's runnable (if it previously wasn't) and then it might
or might not want an interrupt on completion.

The motive is quite naturally to reduce the flow of unnecessary
information between the CPU and GPU during execution. And that
reduces a lot with HW-assisted scheduling. But it also reduces
the ability to introspect that information from kernel driver.

Wiring that information to flow back from the hardware is one task.
But forcing kernel to shape hardware events back to tracepoints seems
forceful considering current state of tracepoints.

There would have to be a triggered kernel activity reading the hardware
event ring and converting it to special tracepoints with the timestamp
as parameter information specifically for gpuvis to understand. Or you
could try to do that live but you would have to basically reintroduce the
communication between GPU and kernel driver we just used enormous
engineering efforts to get rid of :) Either way, triggered or live, the
usefulness with generic ftrace tooling would be reduced.

And all this would have to be stable, enabled-by-default in distros
with varying application versions (gpuvis), kernel versions and firmware
versions (HW-assisted scheduling) through the unversioned tracepoint
interface.

Having to jump through such hoops makes one think why does the GPU event
log have to come through tracepoints? It seem backwards to make kernel go
the extra mile, when userspace could just grab the data from an alternative
source in well-defined form and interleave the information when displaying.

For all that is worth, the thread to read the GPU events and turn them into
special tracepoints could be implemented in a userspace daemon, just
like you might import syslog from remote system.

To me, it seems almost as if folks are too preoccupied with thinking if
we somehow can do this through tracepoints, to stop and actually think
if we should.

Regards, Joonas

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-19 10:08     ` Joonas Lahtinen
@ 2018-12-19 19:22       ` Steven Rostedt
  2018-12-20 12:11         ` Joonas Lahtinen
  2018-12-20 18:27         ` Michael Sartain
  0 siblings, 2 replies; 11+ messages in thread
From: Steven Rostedt @ 2018-12-19 19:22 UTC (permalink / raw)
  To: Joonas Lahtinen; +Cc: intel-gfx, Ingo Molnar, Pierre-Loup A. Griffais

On Wed, 19 Dec 2018 12:08:18 +0200
Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:

> To me, it seems almost as if folks are too preoccupied with thinking if
> we somehow can do this through tracepoints, to stop and actually think
> if we should.

Regardless of whether it should or shouldn't, one solution to this is
to make the trace event in question record basically nothing but a
pointer.

DECLARE_EVENT_CLASS(i915_hw_request,
	    TP_PROTO(struct i915_request *rq),
	    TP_ARGS(rq),

	    TP_STRUCT__entry(
				__field(void *, rq)
			     ),

	    TP_fast_assign(
			__entry->rq = rq;
			   ),

	    TP_printk("rq=%p", __entry->rq)
);

Define the events from that:

DEFINE_EVENT(i915_hw_request, i915_request_submit,
	     TP_PROTO(struct i915_request *rq),
	     TP_ARGS(rq)
);


No tool can use that information. But for those that want to, you make a
separate module that you can load that has:

DECLARE_EVENT_CLASS(i915_specific_hw_request,
	    TP_PROTO(struct i915_request *rq),
	    TP_ARGS(rq),

	    TP_STRUCT__entry(
			     __field(u32, dev)
			     __field(u32, hw_id)
			     __field(u64, ctx)
			     __field(u16, class)
			     __field(u16, instance)
			     __field(u32, seqno)
			     __field(u32, global)
			     ),

	    TP_fast_assign(
			   __entry->dev = rq->i915->drm.primary->index;
			   __entry->hw_id = rq->gem_context->hw_id;
			   __entry->class = rq->engine->uabi_class;
			   __entry->instance = rq->engine->instance;
			   __entry->ctx = rq->fence.context;
			   __entry->seqno = rq->fence.seqno;
			   __entry->global = rq->global_seqno;
			   ),

	    TP_printk("dev=%u, engine=%u:%u, hw_id=%u, ctx=%llu, seqno=%u, global=%u",
		      __entry->dev, __entry->class, __entry->instance,
		      __entry->hw_id, __entry->ctx, __entry->seqno,
		      __entry->global)
);

DEFINE_EVENT_FN(i915_specific_hw_request, i915_specific_request_submit,
	     TP_PROTO(struct i915_request *rq),
	     TP_ARGS(rq),
	     start_i915_request, stop_i915_request)
);


In the module:

static void do_i915_hw_request(void *data, struct i915_request *rq)
{
	trace_i915_specific_request_submit(rq);
}

static bool i915_trace_enabled;

static void enable_disable_tracepoints(struct work_struct *work)
{
	if (i915_trace_enabled)
		/* Can not enable tracepoints from a tracepoint callback */
		register_trace_i915_hw_request(do_i915_hw_request, NULL);
	else
		unregister_trace_i915_hw_request(do_i915_hw_request, NULL);
}

int start_i915_request(void)
{
	i915_trace_enabled = true;
	schedule_work(work_call_enable_disable_tracepoints);
}

int stop_i915_request(void)
{
	i915_trace_enabled = false;
	schedule_work(work_call_enable_disable_tracepoints);
}


Note, the above can be racy, but I'm just trying to show a possible
solution. Which is to have the tracepoint there but exposing no useful
data (event the pointer shown will be mangled by the KASLR security).
But if someone wants a tool, they can create a new tracepoint that is
called by a handler of this tracepoint (yes its a double tracepoint).

The above work, handle would enable/disable the upstream tracepoint
when the other is enabled. Or that may not even be needed, just have
the tool enable both tracepoints when it wants the second one.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-19 19:22       ` Steven Rostedt
@ 2018-12-20 12:11         ` Joonas Lahtinen
  2018-12-20 18:27         ` Michael Sartain
  1 sibling, 0 replies; 11+ messages in thread
From: Joonas Lahtinen @ 2018-12-20 12:11 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: intel-gfx, Ingo Molnar, Pierre-Loup A. Griffais

Quoting Steven Rostedt (2018-12-19 21:22:57)
> On Wed, 19 Dec 2018 12:08:18 +0200
> Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> 
> > To me, it seems almost as if folks are too preoccupied with thinking if
> > we somehow can do this through tracepoints, to stop and actually think
> > if we should.
> 
> Regardless of whether it should or shouldn't, one solution to this is
> to make the trace event in question record basically nothing but a
> pointer.
> 
> DECLARE_EVENT_CLASS(i915_hw_request,
>             TP_PROTO(struct i915_request *rq),
>             TP_ARGS(rq),
> 
>             TP_STRUCT__entry(
>                                 __field(void *, rq)
>                              ),
> 
>             TP_fast_assign(
>                         __entry->rq = rq;
>                            ),
> 
>             TP_printk("rq=%p", __entry->rq)
> );
> 
> Define the events from that:
> 
> DEFINE_EVENT(i915_hw_request, i915_request_submit,
>              TP_PROTO(struct i915_request *rq),
>              TP_ARGS(rq)
> );
> 
> 
> No tool can use that information. But for those that want to, you make a
> separate module that you can load that has:

That's a nifty idea.

If the module is built out-of-tree from gpuvis package, you'd
still lose the functionality on kernel update, though. But I
guess it'd then be a problem of the distro that chooses to
package gpuvis/gputop/whatever profiler in stable manner.

On the other hand, if we would have eBPF based (kudos to Chris for
mentioning) module in-tree for userspace to hook to, it would
make things more easily fixable from userspace

You could for example automate the download a matching piece
of eBPF to execute on the module for a kernel version.

So the lines definitely get blurred here, about what is breaking
userspace and what is userspace deliberately setting itself up for
breaking.

*But* before we seek for a judgement on that. We're still having
the peculiarity that the tracepoints would shortly have to be
generated based from hardware event log.

Do you have an idea how to solve that elegantly? Or do you rule such
thing to be outside of the scope for tracepoints?

So if we still end up with a hybrid solution where another uAPI is
needed for hardware events, I'm not sure if above is worthy the hassle
when we can just add to the other uAPI the same events.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-19 19:22       ` Steven Rostedt
  2018-12-20 12:11         ` Joonas Lahtinen
@ 2018-12-20 18:27         ` Michael Sartain
  2018-12-21  7:45           ` Joonas Lahtinen
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Sartain @ 2018-12-20 18:27 UTC (permalink / raw)
  To: Steven Rostedt, Joonas Lahtinen
  Cc: intel-gfx, Ingo Molnar, Pierre-Loup A. Griffais

On Wed, Dec 19, 2018, at 12:22 PM, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 12:08:18 +0200
> Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
>·
> > To me, it seems almost as if folks are too preoccupied with thinking if
> > we somehow can do this through tracepoints, to stop and actually think
> > if we should.
>·
> Regardless of whether it should or shouldn't, one solution to this is
> to make the trace event in question record basically nothing but a
> pointer.

Right now, these are the events I'm capturing w/ an AMD gpu: 

  amdgpu_cs:0-1150  [002] 630662.649417: amdgpu_cs_ioctl:      sched_job=3490671, timeline=gfx, context=105, seqno=3081096, ring_name=ffff91cb1ab1bdd0, num_ibs=3 
          gfx-190   [000] 630662.649451: amdgpu_sched_run_job: sched_job=3490671, timeline=gfx, context=105, seqno=3081096, ring_name=ffff91cb1ab1bdd0, num_ibs=3 
          gfx-190   [000] 630662.649454: dma_fence_signaled:   driver=amd_sched timeline=gfx context=104 seqno=3081096

With Intel gpu (and rebuilt kernel w/ CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS):

        <idle>-0     [002]   821.717208: intel_engine_notify:  dev=0, engine=0:0, seqno=38896, waiters=1
  RenderThread-1024  [002]   825.722358: i915_request_queue:   dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, flags=0x0
  RenderThread-1024  [002]   825.722371: i915_request_add:     dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, global=0
  RenderThread-1024  [002]   825.722372: i915_request_submit:  dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, global=0
  RenderThread-1024  [002]   825.745964: i915_request_execute: dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, global=42199
  RenderThread-1024  [002]   825.745964: i915_request_in:      dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, prio=0, global=42199, port=1
        <idle>-0     [002]   825.755943: intel_engine_notify:  dev=0, engine=0:0, seqno=42199, waiters=1

It's quite obvious that just because gpuvis sees those amdgpu tracepoints when 
running with the AMD card and parses and displays those, it does *not* get
those same tracepoints when I run with an Intel gpu.

And with my Iris Pro Graphics 580 Gen9, I can reasonably expect to get the
above i915 tracepoints.

But if I install a new Intel Xe Gen11, why should I expect to see those Gen9
i915 tracepoint events? Is it because we are tying tracepoints and the
created uABI to *kernel modules* and not the hardware?

I'm asking, because personally I would expect the hardware to drive these
tracepoint events, much like I check cpu flags to see whether I can run AVX
code, or perf has intel_pt recording on one machine, but not another.

Right now gpuvis graphs the above events in an easy to understand view.
Occasionally, it's really nice to use trace-cmd to get textual representation
for grepping, etc. Storing pointers would obviously break that. I guess if it's
what we need to do to avoid the uABI problem, then it's what we do - still
better than using an entirely new tracing system if we can avoid that.

Thanks much.
 -Mike
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig
  2018-12-20 18:27         ` Michael Sartain
@ 2018-12-21  7:45           ` Joonas Lahtinen
  0 siblings, 0 replies; 11+ messages in thread
From: Joonas Lahtinen @ 2018-12-21  7:45 UTC (permalink / raw)
  To: Michael Sartain, Steven Rostedt
  Cc: intel-gfx, Ingo Molnar, Pierre-Loup A. Griffais

Quoting Michael Sartain (2018-12-20 20:27:19)
> On Wed, Dec 19, 2018, at 12:22 PM, Steven Rostedt wrote:
> > On Wed, 19 Dec 2018 12:08:18 +0200
> > Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> >·
> > > To me, it seems almost as if folks are too preoccupied with thinking if
> > > we somehow can do this through tracepoints, to stop and actually think
> > > if we should.
> >·
> > Regardless of whether it should or shouldn't, one solution to this is
> > to make the trace event in question record basically nothing but a
> > pointer.
> 
> Right now, these are the events I'm capturing w/ an AMD gpu: 
> 
>   amdgpu_cs:0-1150  [002] 630662.649417: amdgpu_cs_ioctl:      sched_job=3490671, timeline=gfx, context=105, seqno=3081096, ring_name=ffff91cb1ab1bdd0, num_ibs=3 
>           gfx-190   [000] 630662.649451: amdgpu_sched_run_job: sched_job=3490671, timeline=gfx, context=105, seqno=3081096, ring_name=ffff91cb1ab1bdd0, num_ibs=3 
>           gfx-190   [000] 630662.649454: dma_fence_signaled:   driver=amd_sched timeline=gfx context=104 seqno=3081096
> 
> With Intel gpu (and rebuilt kernel w/ CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS):
> 
>         <idle>-0     [002]   821.717208: intel_engine_notify:  dev=0, engine=0:0, seqno=38896, waiters=1
>   RenderThread-1024  [002]   825.722358: i915_request_queue:   dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, flags=0x0
>   RenderThread-1024  [002]   825.722371: i915_request_add:     dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, global=0
>   RenderThread-1024  [002]   825.722372: i915_request_submit:  dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, global=0
>   RenderThread-1024  [002]   825.745964: i915_request_execute: dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, global=42199
>   RenderThread-1024  [002]   825.745964: i915_request_in:      dev=0, engine=0:0, hw_id=9, ctx=30, seqno=38896, prio=0, global=42199, port=1
>         <idle>-0     [002]   825.755943: intel_engine_notify:  dev=0, engine=0:0, seqno=42199, waiters=1
> 
> It's quite obvious that just because gpuvis sees those amdgpu tracepoints when 
> running with the AMD card and parses and displays those, it does *not* get
> those same tracepoints when I run with an Intel gpu.
> 
> And with my Iris Pro Graphics 580 Gen9, I can reasonably expect to get the
> above i915 tracepoints.
> 
> But if I install a new Intel Xe Gen11, why should I expect to see those Gen9
> i915 tracepoint events? Is it because we are tying tracepoints and the
> created uABI to *kernel modules* and not the hardware?

You're on the correct track here. The issue is that even for Gen9, we
foresee to unnecessary maintenance burden to keep the tracepoints, as
we work on the scheduler or them disappearing from kernel scope when
HW-assisted scheduling is enabled.

And the lack of versioning on tracepoints does not make it easy on
userspace to do graceful degradiation or to detect the underneath
platform. Stuffing the versioning info to every tracepoint, to make sure
it's in the captured ring buffer being inspected, is not too elegant, so
some auxilary interface would still have to be probed.

> I'm asking, because personally I would expect the hardware to drive these
> tracepoint events, much like I check cpu flags to see whether I can run AVX
> code, or perf has intel_pt recording on one machine, but not another.

If we wanted to make sure we can keep them stable within a gen, we would
have to move them closer to the point we talk to hardware and would
basically just emit information that a) came from userspace (which is
stable due to uABI) or b) is going to hardware (we don't expect the
underlying hardware magically change).

> Right now gpuvis graphs the above events in an easy to understand view.
> Occasionally, it's really nice to use trace-cmd to get textual representation
> for grepping, etc. Storing pointers would obviously break that.

Steve's idea kind of solves that.

There would be an auxilary module build out-of-tree (say, from gpuvis),
that would emit a new tracepoint "b" with more information on triggering
tracepoint "a". So basically you would stop looking for tracepoint "a",
and load your module and just look for "b".

It's bit on the grey area when it comes to breaking userspace and the
philosophical question is, is it us breaking userspace or userspace setting
itself a trap. But I guess it might be OK, if the distros knowingly
bundle such out-of-tree module (which is not subject to kernel
stability).

> I guess if it's
> what we need to do to avoid the uABI problem, then it's what we do - still
> better than using an entirely new tracing system if we can avoid that.

The bigger problem that I'd still like to hear some ideas for is before
drawing conclusion is about elegantly sourcing the tracepoints from hardware
events. Trying to do live conversion from hardware generated ring buffer during
execution just to make sure it interleaves with the software generated ring
buffer and works under same trigger, sounds not so performant.

Usefulness of HW related "special" tracepoints without gpuvis doing the
time sorting based on parameters, not timestamp, could be too low to be
used with the general tooling like you mentioned. Then we have to think
about how much effort is it worth put into solving the HW to SW tracepoint
injection if we could with less total effort have a secondary interface
for hardware events.

Anyways, Happy Holidays all! I'll be back after New Year.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-12-21  7:45 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-18  1:26 uABI / Removing DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig Michael Sartain
2018-12-18  2:14 ` Steven Rostedt
2018-12-18 19:56   ` Pierre-Loup A. Griffais
2018-12-19  8:45   ` Joonas Lahtinen
2018-12-18 11:33 ` Chris Wilson
2018-12-18 18:14   ` Pierre-Loup A. Griffais
2018-12-19 10:08     ` Joonas Lahtinen
2018-12-19 19:22       ` Steven Rostedt
2018-12-20 12:11         ` Joonas Lahtinen
2018-12-20 18:27         ` Michael Sartain
2018-12-21  7:45           ` Joonas Lahtinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.