Re: [Intel-xe] [PATCH 17/21] drm/xe/oa: Remove filtering reports on context id

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Sep 19, 2023 at 09:10:45AM -0700, Ashutosh Dixit wrote:
>At present XE OA code does not obtain context id's from GuC. Even if these
>context id's were available it is not clear if included reports for
>userspace should be filtered on context id's. Till these issues are
>resolved remove filtering reports based on context id's.
>
>Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

For review, it would be easier to just drop whatever is not used rather 
than adding it in earlier patches and removing it later. I guess you 
want to highlight what has changed between i915 and Xe, but I guess we 
can review everything as new code and refer to i915 offline.

Thanks,
Umesh
>---
> drivers/gpu/drm/xe/xe_oa.c       | 33 +++-----------------------------
> drivers/gpu/drm/xe/xe_oa_types.h |  9 ---------
> 2 files changed, 3 insertions(+), 39 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>index 8648652e05aa5..077698a0c5628 100644
>--- a/drivers/gpu/drm/xe/xe_oa.c
>+++ b/drivers/gpu/drm/xe/xe_oa.c
>@@ -184,13 +184,6 @@ static void oa_timestamp_clear(struct xe_oa_stream *stream, u32 *report)
> 		report[1] = 0;
> }
>
>-static u32 oa_context_id(struct xe_oa_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 void oa_context_id_squash(struct xe_oa_stream *stream, u32 *report)
> {
> 	if (oa_report_header_64bit(stream))
>@@ -368,7 +361,7 @@ static int xe_oa_append_reports(struct xe_oa_stream *stream, char __user *buf,
>
> 	for (; OA_TAKEN(tail, head); head = (head + report_size) & mask) {
> 		u8 *report = oa_buf_base + head;
>-		u32 ctx_id, *report32 = (void *)report;
>+		u32 *report32 = (void *)report;
> 		u64 reason;
>
> 		/*
>@@ -379,7 +372,6 @@ static int xe_oa_append_reports(struct xe_oa_stream *stream, char __user *buf,
> 		 * this field and it is valid to have a reason field of zero.
> 		 */
> 		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 invalid to be
>@@ -388,10 +380,8 @@ static int xe_oa_append_reports(struct xe_oa_stream *stream, char __user *buf,
> 		 * Note: we don't clear the valid_ctx_bit so userspace can understand that
> 		 * the ID has been squashed by the kernel.
> 		 */
>-		if (oa_report_ctx_invalid(stream, report)) {
>-			ctx_id = INVALID_CTX_ID;
>+		if (oa_report_ctx_invalid(stream, report))
> 			oa_context_id_squash(stream, report32);
>-		}
>
> 		/*
> 		 * NB: The OA unit does not support clock gating off for a specific
>@@ -419,21 +409,10 @@ static int xe_oa_append_reports(struct xe_oa_stream *stream, char __user *buf,
> 		 * since it's not-uncommon for periodic samples to identify a switch
> 		 * before any 'context switch' report.
> 		 */
>-		if (!stream->exec_q || stream->specific_ctx_id == ctx_id ||
>-		    stream->oa_buffer.last_ctx_id == stream->specific_ctx_id ||
>-		    reason & OAREPORT_REASON_CTX_SWITCH) {
>-			/*
>-			 * While filtering for a single context we avoid
>-			 * leaking the IDs of other contexts.
>-			 */
>-			if (stream->exec_q && stream->specific_ctx_id != ctx_id)
>-				oa_context_id_squash(stream, report32);
>-
>+		if (!stream->exec_q || reason & OAREPORT_REASON_CTX_SWITCH) {
> 			ret = xe_oa_append_sample(stream, buf, count, offset, report);
> 			if (ret)
> 				break;
>-
>-			stream->oa_buffer.last_ctx_id = ctx_id;
> 		}
>
> 		if (is_power_of_2(report_size)) {
>@@ -497,12 +476,6 @@ static void xe_oa_init_oa_buffer(struct xe_oa_stream *stream)
> 	/* Mark that we need updated tail pointers to read from... */
> 	stream->oa_buffer.tail = 0;
>
>-	/*
>-	 * Reset state used to recognise context switches, affecting which reports we will
>-	 * forward to userspace while filtering for a single context.
>-	 */
>-	stream->oa_buffer.last_ctx_id = INVALID_CTX_ID;
>-
> 	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
>
> 	/* Zero out the OA buffer since we rely on zero report id and timestamp fields */
>diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
>index 3cc1d88fe4a51..7566fef55b0ab 100644
>--- a/drivers/gpu/drm/xe/xe_oa_types.h
>+++ b/drivers/gpu/drm/xe/xe_oa_types.h
>@@ -222,12 +222,6 @@ struct xe_oa_stream {
> 	 */
> 	struct llist_head oa_config_bos;
>
>-	/** @specific_ctx_id: id of the context used for filtering reports */
>-	u32 specific_ctx_id;
>-
>-	/** @specific_ctx_id_mask: The mask used to masking specific_ctx_id bits */
>-	u32 specific_ctx_id_mask;
>-
> 	/**
> 	 * @poll_check_timer: High resolution timer that will periodically
> 	 * check for data in the circular OA buffer for notifying userspace
>@@ -261,9 +255,6 @@ struct xe_oa_stream {
> 		/** @vaddr: mapped vaddr of the OA buffer */
> 		u8 *vaddr;
>
>-		/** @last_ctx_id: last context id for OA data added */
>-		u32 last_ctx_id;
>-
> 		/**
> 		 * @ptr_lock: Locks reads and writes to all head/tail state
> 		 *
>-- 
>2.41.0
>