| Message ID | 1489644855-25562-3-git-send-email-sourab.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 16, 2017 at 6:14 AM, <sourab.gupta@intel.com> wrote: > From: Sourab Gupta <sourab.gupta@intel.com> > > This patch exposes a new sample source field to userspace. This field can > be populated to specify the origin of the OA report. > Currently, the OA samples are being generated only periodically, and hence > there's only source flag enum definition right now, but there are other > means of generating OA samples, such as via MI_RPC commands. The OA_SOURCE > sample type is introducing a mechanism (for userspace) to distinguish various > OA reports generated via different sources. Maybe we could clarify that it's not intended as a replacement for the reason field that's part of Gen8+ OA reports. For automatically triggered reports written to the OABUFFER then the reason field will distinguish e.g. periodic vs ctx-switch vs GO transition reasons for the OA unit writing a report. The reason field is overloaded as the RPT_ID field for MI_RPC reports so we need our own way of tracking the difference. > > Signed-off-by: Sourab Gupta <sourab.gupta@intel.com> > Signed-off-by: Robert Bragg <robert@sixbynine.org> > --- > drivers/gpu/drm/i915/i915_perf.c | 18 ++++++++++++++++++ > include/uapi/drm/i915_drm.h | 14 ++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index 4b1db73..540c5b2 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -324,6 +324,7 @@ > }; > > #define SAMPLE_OA_REPORT (1<<0) > +#define SAMPLE_OA_SOURCE_INFO (1<<1) > > /** > * struct perf_open_properties - for validated properties given to open a stream > @@ -659,6 +660,15 @@ static int append_oa_sample(struct i915_perf_stream *stream, > return -EFAULT; > buf += sizeof(header); > I think I'd add a note mentioning that the ordering of _OA_SOURCE_INFO before the _OA_REPORT forms part of the uapi - just as an extra guard against someone reorganising code not realising that. Maybe over paranoid though. > + if (sample_flags & SAMPLE_OA_SOURCE_INFO) { > + enum drm_i915_perf_oa_event_source source = > + I915_PERF_OA_EVENT_SOURCE_PERIODIC; > + > + if (copy_to_user(buf, &source, 4)) > + return -EFAULT; > + buf += 4; > + } I think we should keep all sample sections 8 byte aligned to keep any embedded u64 values we might want within records naturally aligned. Currently we know OA reports all have power-of-two sizes of 64, 128 or 256 bytes. The _OA_REPORT_LOST and _OA_BUFFER_LOST records only have an 8 byte header. 'PERIODIC' may not be an accurate differentiator. As mentioned above with the comment about then Gen8+ 'reason' field there are a number of different automatic triggers for writing OA reports to the OABUFFER. Maybe 'OABUFFER' would be better. I'm not sure the final '_INFO' suffix is necessary here. To me I think I'd expect richer, more-detailed data with that suffix and I'd expect the data to about the source while this is just about identifying the source. (not too important at this stage - just mentioning how it comes across when I read it currently). > + > if (sample_flags & SAMPLE_OA_REPORT) { > if (copy_to_user(buf, report, report_size)) > return -EFAULT; > @@ -2030,6 +2040,11 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream, > stream->sample_flags |= SAMPLE_OA_REPORT; > stream->sample_size += format_size; > > + if (props->sample_flags & SAMPLE_OA_SOURCE_INFO) { > + stream->sample_flags |= SAMPLE_OA_SOURCE_INFO; > + stream->sample_size += 4; > + } 8 - as commented above. > + > dev_priv->perf.oa.oa_buffer.format_size = format_size; > if (WARN_ON(dev_priv->perf.oa.oa_buffer.format_size == 0)) > return -EINVAL; > @@ -2814,6 +2829,9 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv, > props->oa_periodic = true; > props->oa_period_exponent = value; > break; > + case DRM_I915_PERF_PROP_SAMPLE_OA_SOURCE: > + props->sample_flags |= SAMPLE_OA_SOURCE_INFO; > + break; > default: > MISSING_CASE(id); > DRM_DEBUG("Unknown i915 perf property ID\n"); > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 835e711..c597e36 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1312,6 +1312,12 @@ enum drm_i915_oa_format { > I915_OA_FORMAT_MAX /* non-ABI */ > }; > > +enum drm_i915_perf_oa_event_source { > + I915_PERF_OA_EVENT_SOURCE_PERIODIC, > + > + I915_PERF_OA_EVENT_SOURCE_MAX /* non-ABI */ > +}; > + > enum drm_i915_perf_property_id { > /** > * Open the stream for a specific context handle (as used with > @@ -1346,6 +1352,13 @@ enum drm_i915_perf_property_id { > */ > DRM_I915_PERF_PROP_OA_EXPONENT, > > + /** > + * The value of this property set to 1 requests inclusion of sample > + * source field to be given to userspace. The sample source field > + * specifies the origin of OA report. > + */ > + DRM_I915_PERF_PROP_SAMPLE_OA_SOURCE, > + > DRM_I915_PERF_PROP_MAX /* non-ABI */ > }; > > @@ -1411,6 +1424,7 @@ enum drm_i915_perf_record_type { > * struct { > * struct drm_i915_perf_record_header header; > * > + * { u32 source_info; } && DRM_I915_PERF_PROP_SAMPLE_OA_SOURCE u64 Considering how 8 bytes is obviously overkill for the info here I'm wondering if we should consider maintaining the ability to pack more data into the same section in the future. Maybe u64 source_info:4, reserved:60 bitfields could keep it easier to combine with more data later? (this is a convention taken from from uapi/linux/perf_event.h > * { u32 oa_report[]; } && DRM_I915_PERF_PROP_SAMPLE_OA > * }; > */ > -- > 1.9.1 >
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 4b1db73..540c5b2 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -324,6 +324,7 @@ }; #define SAMPLE_OA_REPORT (1<<0) +#define SAMPLE_OA_SOURCE_INFO (1<<1) /** * struct perf_open_properties - for validated properties given to open a stream @@ -659,6 +660,15 @@ static int append_oa_sample(struct i915_perf_stream *stream, return -EFAULT; buf += sizeof(header); + if (sample_flags & SAMPLE_OA_SOURCE_INFO) { + enum drm_i915_perf_oa_event_source source = + I915_PERF_OA_EVENT_SOURCE_PERIODIC; + + if (copy_to_user(buf, &source, 4)) + return -EFAULT; + buf += 4; + } + if (sample_flags & SAMPLE_OA_REPORT) { if (copy_to_user(buf, report, report_size)) return -EFAULT; @@ -2030,6 +2040,11 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream, stream->sample_flags |= SAMPLE_OA_REPORT; stream->sample_size += format_size; + if (props->sample_flags & SAMPLE_OA_SOURCE_INFO) { + stream->sample_flags |= SAMPLE_OA_SOURCE_INFO; + stream->sample_size += 4; + } + dev_priv->perf.oa.oa_buffer.format_size = format_size; if (WARN_ON(dev_priv->perf.oa.oa_buffer.format_size == 0)) return -EINVAL; @@ -2814,6 +2829,9 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv, props->oa_periodic = true; props->oa_period_exponent = value; break; + case DRM_I915_PERF_PROP_SAMPLE_OA_SOURCE: + props->sample_flags |= SAMPLE_OA_SOURCE_INFO; + break; default: MISSING_CASE(id); DRM_DEBUG("Unknown i915 perf property ID\n"); diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 835e711..c597e36 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1312,6 +1312,12 @@ enum drm_i915_oa_format { I915_OA_FORMAT_MAX /* non-ABI */ }; +enum drm_i915_perf_oa_event_source { + I915_PERF_OA_EVENT_SOURCE_PERIODIC, + + I915_PERF_OA_EVENT_SOURCE_MAX /* non-ABI */ +}; + enum drm_i915_perf_property_id { /** * Open the stream for a specific context handle (as used with @@ -1346,6 +1352,13 @@ enum drm_i915_perf_property_id { */ DRM_I915_PERF_PROP_OA_EXPONENT, + /** + * The value of this property set to 1 requests inclusion of sample + * source field to be given to userspace. The sample source field + * specifies the origin of OA report. + */ + DRM_I915_PERF_PROP_SAMPLE_OA_SOURCE, + DRM_I915_PERF_PROP_MAX /* non-ABI */ }; @@ -1411,6 +1424,7 @@ enum drm_i915_perf_record_type { * struct { * struct drm_i915_perf_record_header header; * + * { u32 source_info; } && DRM_I915_PERF_PROP_SAMPLE_OA_SOURCE * { u32 oa_report[]; } && DRM_I915_PERF_PROP_SAMPLE_OA * }; */