Re: [PATCH 00/17] Add OA functionality to Xe

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2024-03-14 at 18:35 -0700, Ashutosh Dixit wrote:
> Please see cover letter for v7 here:
> https://patchwork.freedesktop.org/series/121084/#rev7
> 
> For changes in v8 through v10, see:
> https://patchwork.freedesktop.org/series/128993/
> 
> For changes in v11, see:
> https://patchwork.freedesktop.org/series/130705/
> 
> This series is also available at:
>         https://gitlab.freedesktop.org/adixit/kernel/-/tree/xe-oa
> 
> The series has been tested against this IGT series:
>         https://gitlab.freedesktop.org/adixit/igt-gpu-tools/-/tree/xe-oa, or,
> 	https://patchwork.freedesktop.org/series/130033/
> 
> v2: Fix build
> v3: Rebase, due to s/xe_engine/xe_exec_queue/
> v4: Re-run for testing
> v5: Address review comments, new patches 11 through 17
> v6: New patches 18 through 21
> v7: Patches are completely redone and don't start with i915 version of the uapi
> v8: See https://patchwork.freedesktop.org/patch/575214/?series=128993&rev=1
> v9: See https://patchwork.freedesktop.org/patch/577441/?series=128993&rev=2
> v10: See https://patchwork.freedesktop.org/patch/577943/?series=128993&rev=3
> v11: See https://patchwork.freedesktop.org/patch/581239/?series=130705&rev=1
> v12: Add last two new patches to enable Xe2+ overrun mode
> v13: Update last two patches after code review completion

Hi

I hope I'm replying to the latest version...

Just a few comments I have in the uAPI:

- Rename DRM_XE_OA_PROPERTY_OA_EXPONENT to DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, for better clarity.
- Some Perf uAPIs added here don't have usage in Mesa, see those below. Not sure if MDAPI or gviz will make use of it, if not the approach is to
remove uAPIs until there is UMD using it:
	- DRM_XE_PERF_IOCTL_INFO + drm_xe_oa_stream_info
	- DRM_XE_DEVICE_QUERY_OA_UNITS + drm_xe_query_oa_units + drm_xe_oa_unit
- DRM_XE_OASTATUS_* values are based on HW definition, I think it should not follow HW at least for DRM_XE_OASTATUS_MMIO_TRG_Q_FULL and avoid have a
blank space of bits not used in the uAPI
- Perf stream read() should give us a hint that drm_xe_oa_stream_status needs to be read, that discussion is already in progress thank you for that

Other than that the uAPI LGTM.

There is the hold_preemption feature that is missing but I think that can be added later...

thank you

> 
> Ashutosh Dixit (17):
>   drm/xe/perf/uapi: "Perf" layer to support multiple perf counter stream
>     types
>   drm/xe/perf/uapi: Add perf_stream_paranoid sysctl
>   drm/xe/oa/uapi: Add OA data formats
>   drm/xe/oa/uapi: Initialize OA units
>   drm/xe/oa/uapi: Add/remove OA config perf ops
>   drm/xe/oa/uapi: Define and parse OA stream properties
>   drm/xe/oa: OA stream initialization (OAG)
>   drm/xe/oa/uapi: Expose OA stream fd
>   drm/xe/oa/uapi: Read file_operation
>   drm/xe/oa: Add OAR support
>   drm/xe/oa: Add OAC support
>   drm/xe/oa/uapi: Query OA unit properties
>   drm/xe/oa/uapi: OA buffer mmap
>   drm/xe/oa: Add MMIO trigger support
>   drm/xe/oa: Override GuC RC with OA on PVC
>   drm/xe/oa: Changes to OA_TAKEN
>   drm/xe/oa: Enable Xe2+ overrun mode
> 
>  drivers/gpu/drm/xe/Makefile                   |    2 +
>  .../gpu/drm/xe/instructions/xe_mi_commands.h  |    3 +
>  drivers/gpu/drm/xe/regs/xe_engine_regs.h      |    4 +-
>  drivers/gpu/drm/xe/regs/xe_gt_regs.h          |    3 +
>  drivers/gpu/drm/xe/regs/xe_oa_regs.h          |   99 +
>  drivers/gpu/drm/xe/xe_device.c                |   18 +-
>  drivers/gpu/drm/xe/xe_device_types.h          |    4 +
>  drivers/gpu/drm/xe/xe_gt_types.h              |    4 +
>  drivers/gpu/drm/xe/xe_guc_pc.c                |   56 +
>  drivers/gpu/drm/xe/xe_guc_pc.h                |    3 +
>  drivers/gpu/drm/xe/xe_hw_engine_types.h       |    2 +
>  drivers/gpu/drm/xe/xe_lrc.c                   |   11 +-
>  drivers/gpu/drm/xe/xe_lrc.h                   |    1 +
>  drivers/gpu/drm/xe/xe_module.c                |    6 +
>  drivers/gpu/drm/xe/xe_oa.c                    | 2334 +++++++++++++++++
>  drivers/gpu/drm/xe/xe_oa.h                    |   30 +
>  drivers/gpu/drm/xe/xe_oa_types.h              |  229 ++
>  drivers/gpu/drm/xe/xe_perf.c                  |   67 +
>  drivers/gpu/drm/xe/xe_perf.h                  |   20 +
>  drivers/gpu/drm/xe/xe_query.c                 |   77 +
>  drivers/gpu/drm/xe/xe_reg_whitelist.c         |   24 +-
>  include/uapi/drm/xe_drm.h                     |  286 ++
>  22 files changed, 3275 insertions(+), 8 deletions(-)
>  create mode 100644 drivers/gpu/drm/xe/regs/xe_oa_regs.h
>  create mode 100644 drivers/gpu/drm/xe/xe_oa.c
>  create mode 100644 drivers/gpu/drm/xe/xe_oa.h
>  create mode 100644 drivers/gpu/drm/xe/xe_oa_types.h
>  create mode 100644 drivers/gpu/drm/xe/xe_perf.c
>  create mode 100644 drivers/gpu/drm/xe/xe_perf.h
>