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

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

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@intel.com>
> Signed-off-by: Ashutosh Dixit [2]<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
>                  *
>
>References
>
>   Visible links
>   1. mailto:umesh.nerlige.ramappa@intel.com
>   2. mailto:ashutosh.dixit@intel.com