From: Jani Nikula <jani.nikula@linux.intel.com>
To: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>,
intel-gfx@lists.freedesktop.org,
Lionel G Landwerlin <lionel.g.landwerlin@intel.com>,
Ashutosh Dixit <ashutosh.dixit@intel.com>
Subject: Re: [Intel-gfx] [PATCH 12/19] drm/i915/perf: Parse 64bit report header formats correctly
Date: Tue, 23 Aug 2022 10:51:06 +0300 [thread overview]
Message-ID: <87a67vmm8l.fsf@intel.com> (raw)
In-Reply-To: <20220823000342.281222-13-umesh.nerlige.ramappa@intel.com>
On Tue, 23 Aug 2022, Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> wrote:
> Now that OA formats come in flavor of 64 bit reports, the report header
> has 64 bit report-id, timestamp, context-id and gpu-ticks fields. When
> filtering these reports, use the right width for these fields.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
> drivers/gpu/drm/i915/i915_perf.c | 109 ++++++++++++++++++++-----
> drivers/gpu/drm/i915/i915_perf_types.h | 6 ++
> 2 files changed, 94 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 41634d614ba5..c3183aedc712 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -324,8 +324,8 @@ static const struct i915_oa_format oa_formats[I915_OA_FORMAT_MAX] = {
> [I915_OA_FORMAT_A32u40_A4u32_B8_C8] = { 5, 256 },
> [I915_OAR_FORMAT_A32u40_A4u32_B8_C8] = { 5, 256 },
> [I915_OA_FORMAT_A24u40_A14u32_B8_C8] = { 5, 256 },
> - [I915_OAR_FORMAT_A36u64_B8_C8] = { 1, 384 },
> - [I915_OA_FORMAT_A38u64_R2u64_B8_C8] = { 1, 448 },
> + [I915_OAR_FORMAT_A36u64_B8_C8] = { 1, 384, HDR_64_BIT },
> + [I915_OA_FORMAT_A38u64_R2u64_B8_C8] = { 1, 448, HDR_64_BIT },
> };
>
> #define SAMPLE_OA_REPORT (1<<0)
> @@ -457,6 +457,75 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream)
> return oastatus1 & GEN7_OASTATUS1_TAIL_MASK;
> }
>
> +#define oa_report_header_64bit(__s) \
> + ((__s)->oa_buffer.format->header == HDR_64_BIT)
> +
> +static inline u64
> +oa_report_id(struct i915_perf_stream *stream, void *report)
> +{
> + return oa_report_header_64bit(stream) ? *(u64 *)report : *(u32 *)report;
> +}
Drive-by-comment, I suspect you're not really gaining anything by making
these "inline". Just let the compiler do its thing.
Actually making them inline prevents you from getting warnings for
unused functions if they ever become unused.
BR,
Jani.
> +static inline u64
> +oa_report_reason(struct i915_perf_stream *stream, void *report)
> +{
> + return (oa_report_id(stream, report) >> OAREPORT_REASON_SHIFT) &
> + (GRAPHICS_VER(stream->perf->i915) == 12 ?
> + OAREPORT_REASON_MASK_EXTENDED :
> + OAREPORT_REASON_MASK);
> +}
> +
> +static inline void
> +oa_report_id_clear(struct i915_perf_stream *stream, u32 *report)
> +{
> + if (oa_report_header_64bit(stream))
> + *(u64 *)report = 0;
> + else
> + *report = 0;
> +}
> +
> +static inline bool
> +oa_report_ctx_invalid(struct i915_perf_stream *stream, void *report)
> +{
> + return !(oa_report_id(stream, report) &
> + stream->perf->gen8_valid_ctx_bit) &&
> + GRAPHICS_VER(stream->perf->i915) <= 11;
> +}
> +
> +static inline u64
> +oa_timestamp(struct i915_perf_stream *stream, void *report)
> +{
> + return oa_report_header_64bit(stream) ?
> + *((u64 *)report + 1) :
> + *((u32 *)report + 1);
> +}
> +
> +static inline void
> +oa_timestamp_clear(struct i915_perf_stream *stream, u32 *report)
> +{
> + if (oa_report_header_64bit(stream))
> + *(u64 *)&report[2] = 0;
> + else
> + report[1] = 0;
> +}
> +
> +static inline u32
> +oa_context_id(struct i915_perf_stream *stream, u32 *report)
> +{
> + u32 ctx_id = oa_report_header_64bit(stream) ? report[4] : report[2];
> +
> + return ctx_id & stream->specific_ctx_id_mask;
> +}
> +
> +static inline void
> +oa_context_id_squash(struct i915_perf_stream *stream, u32 *report)
> +{
> + if (oa_report_header_64bit(stream))
> + report[4] = INVALID_CTX_ID;
> + else
> + report[2] = INVALID_CTX_ID;
> +}
> +
> /**
> * oa_buffer_check_unlocked - check for data and update tail ptr state
> * @stream: i915 stream instance
> @@ -546,9 +615,10 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> * If not : (╯°□°）╯︵ ┻━┻
> */
> while (_oa_taken(stream, tail, aged_tail) >= report_size) {
> - u32 *report32 = (void *)(stream->oa_buffer.vaddr + tail);
> + void *report = stream->oa_buffer.vaddr + tail;
>
> - if (report32[0] != 0 || report32[1] != 0)
> + if (oa_report_id(stream, report) ||
> + oa_timestamp(stream, report))
> break;
>
> tail = _rewind_tail(stream, tail, report_size);
> @@ -741,23 +811,19 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
> u8 *report = oa_buf_base + head;
> u32 *report32 = (void *)report;
> u32 ctx_id;
> - u32 reason;
> + u64 reason;
>
> /*
> * The reason field includes flags identifying what
> * triggered this specific report (mostly timer
> * triggered or e.g. due to a context switch).
> *
> - * This field is never expected to be zero so we can
> - * check that the report isn't invalid before copying
> - * it to userspace...
> + * In MMIO triggered reports, some platforms do not set the
> + * reason bit in this field and it is valid to have a reason
> + * field of zero.
> */
> - reason = ((report32[0] >> OAREPORT_REASON_SHIFT) &
> - (GRAPHICS_VER(stream->perf->i915) == 12 ?
> - OAREPORT_REASON_MASK_EXTENDED :
> - OAREPORT_REASON_MASK));
> -
> - ctx_id = report32[2] & stream->specific_ctx_id_mask;
> + reason = oa_report_reason(stream, report);
> + ctx_id = oa_context_id(stream, report32);
>
> /*
> * Squash whatever is in the CTX_ID field if it's marked as
> @@ -767,9 +833,10 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
> * Note: that we don't clear the valid_ctx_bit so userspace can
> * understand that the ID has been squashed by the kernel.
> */
> - if (!(report32[0] & stream->perf->gen8_valid_ctx_bit) &&
> - GRAPHICS_VER(stream->perf->i915) <= 11)
> - ctx_id = report32[2] = INVALID_CTX_ID;
> + if (oa_report_ctx_invalid(stream, report)) {
> + ctx_id = INVALID_CTX_ID;
> + oa_context_id_squash(stream, report32);
> + }
>
> /*
> * NB: For Gen 8 the OA unit no longer supports clock gating
> @@ -813,7 +880,7 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
> */
> if (stream->ctx &&
> stream->specific_ctx_id != ctx_id) {
> - report32[2] = INVALID_CTX_ID;
> + oa_context_id_squash(stream, report32);
> }
>
> ret = append_oa_sample(stream, buf, count, offset,
> @@ -825,11 +892,11 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
> }
>
> /*
> - * Clear out the first 2 dword as a mean to detect unlanded
> + * Clear out the report id and timestamp as a means to detect unlanded
> * reports.
> */
> - report32[0] = 0;
> - report32[1] = 0;
> + oa_report_id_clear(stream, report32);
> + oa_timestamp_clear(stream, report32);
> }
>
> if (start_offset != *offset) {
> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index e0c96b44eda8..68db5f94bc58 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -30,9 +30,15 @@ struct i915_vma;
> struct intel_context;
> struct intel_engine_cs;
>
> +enum report_header {
> + HDR_32_BIT = 0,
> + HDR_64_BIT,
> +};
> +
> struct i915_oa_format {
> u32 format;
> int size;
> + enum report_header header;
> };
>
> struct i915_oa_reg {
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2022-08-23 7:51 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-23 0:03 [Intel-gfx] [PATCH 00/19] Add DG2 OA support Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 01/19] drm/i915/perf: Fix OA filtering logic for GuC mode Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 02/19] drm/i915/perf: Add OA formats for DG2 Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 03/19] drm/i915/perf: Fix noa wait predication " Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 04/19] drm/i915/perf: Determine gen12 oa ctx offset at runtime Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 05/19] drm/i915/perf: Enable commands per clock reporting in OA Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 06/19] drm/i915/perf: Use helpers to process reports w.r.t. OA buffer size Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 07/19] drm/i915/perf: Simply use stream->ctx Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 08/19] drm/i915/perf: Move gt-specific data from i915->perf to gt->perf Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 09/19] drm/i915/perf: Replace gt->perf.lock with stream->lock for file ops Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 10/19] drm/i915/perf: Use gt-specific ggtt for OA and noa-wait buffers Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 11/19] drm/i915/perf: Store a pointer to oa_format in oa_buffer Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 12/19] drm/i915/perf: Parse 64bit report header formats correctly Umesh Nerlige Ramappa
2022-08-23 7:51 ` Jani Nikula [this message]
2022-08-23 0:03 ` [Intel-gfx] [PATCH 13/19] drm/i915/perf: Add Wa_16010703925:dg2 Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 14/19] drm/i915/perf: Add Wa_1608133521:dg2 Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 15/19] drm/i915/perf: Add Wa_1508761755:dg2 Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 16/19] drm/i915/perf: Apply Wa_18013179988 Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 17/19] drm/i915/perf: Save/restore EU flex counters across reset Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 18/19] drm/i915/guc: Support OA when Wa_16011777198 is enabled Umesh Nerlige Ramappa
2022-08-23 0:03 ` [Intel-gfx] [PATCH 19/19] drm/i915/perf: Enable OA for DG2 Umesh Nerlige Ramappa
-- strict thread matches above, loose matches on Subject: below --
2022-08-23 20:41 [Intel-gfx] [PATCH 00/19] Add DG2 OA support Umesh Nerlige Ramappa
2022-08-23 20:41 ` [Intel-gfx] [PATCH 12/19] drm/i915/perf: Parse 64bit report header formats correctly Umesh Nerlige Ramappa
2022-09-16 0:47 ` Dixit, Ashutosh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a67vmm8l.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=ashutosh.dixit@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lionel.g.landwerlin@intel.com \
--cc=umesh.nerlige.ramappa@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox