[Intel-xe] [17/21] drm/xe/oa: Remove filtering reports on context id
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Fri Oct 20 17:00:26 UTC 2023
On Fri, Oct 20, 2023 at 10:30:15AM +0300, Lionel Landwerlin wrote:
> 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.
Maybe I didn't understand the filtering use case in i915 correctly.
The OA buffer access is/was always privileged. Filtering was only
squashing out the irrelevant contexts in the OA buffer. However those
squashed contexts are also copied over to the user buffer. My thought
was to just return everything unsquashed since this is a privileged
operation.
>
> I guess this also makes DRM_XE_OA_PROP_EXEC_QUEUE_ID useless?
>
> It should probably be removed from the uapi.
DRM_XE_OA_PROP_EXEC_QUEUE_ID was still used to enable OAR on a specific
context. If we drop this, we could enable it on all RCS instances. The
problem comes when OAC is used. If there are multiple CCS instances on a
tile, OAC can only be enabled on one instance.
Thanks,
Umesh
>
> -Lionel
>
>
> Suggested-by: Umesh Nerlige Ramappa [1]<umesh.nerlige.ramappa at intel.com>
> Signed-off-by: Ashutosh Dixit [2]<ashutosh.dixit at 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
> *
>
>References
>
> Visible links
> 1. mailto:umesh.nerlige.ramappa at intel.com
> 2. mailto:ashutosh.dixit at intel.com
More information about the Intel-xe
mailing list