On Thu, Mar 16, 2017 at 6:14 AM, <sourab.gupta@xxxxxxxxx> wrote:
> From: Sourab Gupta <sourab.gupta@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Robert Bragg <robert@xxxxxxxxxxxxx>
> ---
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
