On Tue, 23 Aug 2022 13:41:48 -0700, Umesh Nerlige Ramappa wrote:
>
Hi Umesh,
> @@ -740,23 +802,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
> @@ -766,9 +824,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
> @@ -812,7 +871,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,
> @@ -824,11 +883,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);
Because we now have these new functions, why do we now need two pointers
report and report32 pointing to the same location? I think we can just have
a single 'void *report' which we can pass into all these functions,
correct?
With this change, this is:
Reviewed-by: Ashutosh Dixit <[email protected]>
