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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5182 bytes --]

On 19/09/2023 19:10, 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.


If you're removing this, then the opening of the perf stream should 
always be a privileged operation.

I guess this also makes DRM_XE_OA_PROP_EXEC_QUEUE_ID useless?

It should probably be removed from the uapi.

-Lionel


>
> Suggested-by: Umesh Nerlige Ramappa<umesh.nerlige.ramappa@intel.com>
> Signed-off-by: Ashutosh Dixit<ashutosh.dixit@intel.com>
> ---
>   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
>   		 *


[-- Attachment #2: Type: text/html, Size: 5913 bytes --]