[Intel-gfx] [RFC 4/4] drm/i915/perf: Send system clock monotonic time in perf samples

[Sagar Arun Kamble]

On 11/15/2017 6:01 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-11-15 12:13:54)
>> From: Sourab Gupta <sourab.gupta at intel.com>
>>
>> Currently, we have the ability to only forward the GPU timestamps in the
>> samples (which are generated via OA reports). This limits the ability to
>> correlate these samples with the system events.
>>
>> An ability is therefore needed to report timestamps in different clock
>> domains, such as CLOCK_MONOTONIC, in the perf samples to be of more
>> practical use to the userspace. This ability becomes important
>> when we want to correlate/plot GPU events/samples with other system events
>> on the same timeline (e.g. vblank events, or timestamps when work was
>> submitted to kernel, etc.)
>>
>> The patch here proposes a mechanism to achieve this. The correlation
>> between gpu time and system time is established using the timestamp clock
>> associated with the command stream, abstracted as timecounter/cyclecounter
>> to retrieve gpu/system time correlated values.
>>
>> v2: Added i915_driver_init_late() function to capture the new late init
>> phase for perf (Chris)
>>
>> v3: Removed cross-timestamp changes.
>>
>> Signed-off-by: Sourab Gupta <sourab.gupta at intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Sourab Gupta <sourab.gupta at intel.com>
>> Cc: Matthew Auld <matthew.auld at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_perf.c | 27 +++++++++++++++++++++++++++
>>   include/uapi/drm/i915_drm.h      |  7 +++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 3b721d7..94ee924 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -336,6 +336,7 @@
>>   
>>   #define SAMPLE_OA_REPORT       BIT(0)
>>   #define SAMPLE_GPU_TS          BIT(1)
>> +#define SAMPLE_SYSTEM_TS       BIT(2)
>>   
>>   /**
>>    * struct perf_open_properties - for validated properties given to open a stream
>> @@ -622,6 +623,7 @@ static int append_oa_sample(struct i915_perf_stream *stream,
>>          struct drm_i915_perf_record_header header;
>>          u32 sample_flags = stream->sample_flags;
>>          u64 gpu_ts = 0;
>> +       u64 system_ts = 0;
>>   
>>          header.type = DRM_I915_PERF_RECORD_SAMPLE;
>>          header.pad = 0;
>> @@ -647,6 +649,23 @@ static int append_oa_sample(struct i915_perf_stream *stream,
>>   
>>                  if (copy_to_user(buf, &gpu_ts, I915_PERF_TS_SAMPLE_SIZE))
>>                          return -EFAULT;
>> +               buf += I915_PERF_TS_SAMPLE_SIZE;
>> +       }
>> +
>> +       if (sample_flags & SAMPLE_SYSTEM_TS) {
>> +               gpu_ts = get_gpu_ts_from_oa_report(stream, report);
> Scope your variables. Stops us from being confused as to where else
> gpu_ts or sys_ts may be reused. For instance I first thought you were
> using SAMPLE_GPU_TS to initialise gpu_ts
Yes
>> +               /*
>> +                * XXX: timecounter_cyc2time considers time backwards if delta
>> +                * timestamp is more than half the max ns time covered by
>> +                * counter. It will be ~35min for 36 bit counter. If this much
>> +                * sampling duration is needed we will have to update tc->nsec
>> +                * by explicitly reading the timecounter (timecounter_read)
>> +                * before this duration.
>> +                */
>> +               system_ts = timecounter_cyc2time(&stream->tc, gpu_ts);
>> +
>> +               if (copy_to_user(buf, &system_ts, I915_PERF_TS_SAMPLE_SIZE))
>> +                       return -EFAULT;
> Advance buf.
Had kept advance logic only if there is new field to be added as this 
advance is missing for
SAMPLE_OA_REPORT currently in drm-tip. Will fix.
Thanks for the review :)