[PATCH i-g-t,v1 0/4] L3 bank mask

Thu Apr 18 13:27:20 UTC 2024

Hi Lucas,

On Wed, Apr 17, 2024 at 11:07:54AM -0500, Lucas De Marchi wrote:
> On Wed, Apr 17, 2024 at 02:50:45PM GMT, Francois Dugast wrote:
> > Align the XeKMD header with the current version and add the L3 bank mask
> > topology query proposed in this KMD series:
> > https://patchwork.freedesktop.org/series/132075/
> > 
> > This series first includes some patches to align the XeKMD header which
> > have already been reviewed but not yet applied, from this other series:
> > https://patchwork.freedesktop.org/series/131816
> > 
> > Francois Dugast (4):
> >  drm-uapi/xe: Align header with drm-xe-next
> >  drm-uapi/xe: Define topology types as indexes rather than masks
> >  drm-uapi/xe: Expose the L3 bank mask
> 
> no, I don't think we want to merge these as is in this series. It can be
> used for premerge testing, but this is just reintroducing the same
> problem fixed in patch 1.

The first two patches were not meant to be merged with this series, I
should have made that clear in the cover letter.

> 
> I applied patch 1 locally and then did a diff with the current
> drm-xe-next:
> 
> diff --git a/include/drm-uapi/xe_drm.h b/include/drm-uapi/xe_drm.h
> index 4353595a4..0b709b374 100644
> --- a/include/drm-uapi/xe_drm.h
> +++ b/include/drm-uapi/xe_drm.h
> @@ -518,9 +518,9 @@ struct drm_xe_query_topology_mask {
>         /** @gt_id: GT ID the mask is associated with */
>         __u16 gt_id;
> 
> -#define DRM_XE_TOPO_DSS_GEOMETRY       (1 << 0)
> -#define DRM_XE_TOPO_DSS_COMPUTE                (1 << 1)
> -#define DRM_XE_TOPO_EU_PER_DSS         (1 << 2)
> +#define DRM_XE_TOPO_DSS_GEOMETRY       1
> +#define DRM_XE_TOPO_DSS_COMPUTE                2
> +#define DRM_XE_TOPO_EU_PER_DSS         4
>         /** @type: type of mask */
>         __u16 type;
> 
> @@ -871,10 +871,12 @@ struct drm_xe_vm_destroy {
>   *  - %DRM_XE_VM_BIND_OP_PREFETCH
>   *
>   * and the @flags can be:
> - *  - %DRM_XE_VM_BIND_FLAG_READONLY
> - *  - %DRM_XE_VM_BIND_FLAG_IMMEDIATE - Valid on a faulting VM only, do the
> + *  - %DRM_XE_VM_BIND_FLAG_READONLY - Setup the page tables as read-only
> + *    to ensure write protection
> + *  - %DRM_XE_VM_BIND_FLAG_IMMEDIATE - On a faulting VM, do the
>   *    MAP operation immediately rather than deferring the MAP to the page
> - *    fault handler.
> + *    fault handler. This is implied on a non-faulting VM as there is no
> + *    fault handler to defer to.
>   *  - %DRM_XE_VM_BIND_FLAG_NULL - When the NULL flag is set, the page
>   *    tables are setup with a special bit which indicates writes are
>   *    dropped and all reads return zero. In the future, the NULL flags
> @@ -1085,19 +1087,6 @@ struct drm_xe_exec_queue_create {
>  #define DRM_XE_EXEC_QUEUE_EXTENSION_SET_PROPERTY               0
>  #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY              0
>  #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_TIMESLICE             1
> -#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PREEMPTION_TIMEOUT    2
> -#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_JOB_TIMEOUT           4
> -#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_ACC_TRIGGER           5
> -#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_ACC_NOTIFY            6
> -#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_ACC_GRANULARITY       7
> -/* Monitor 128KB contiguous region with 4K sub-granularity */
> -#define     DRM_XE_ACC_GRANULARITY_128K                                0
> -/* Monitor 2MB contiguous region with 64KB sub-granularity */
> -#define     DRM_XE_ACC_GRANULARITY_2M                          1
> -/* Monitor 16MB contiguous region with 512KB sub-granularity */
> -#define     DRM_XE_ACC_GRANULARITY_16M                         2
> -/* Monitor 64MB contiguous region with 2M sub-granularity */
> -#define     DRM_XE_ACC_GRANULARITY_64M                         3
> 
>         /** @extensions: Pointer to the first extension struct, if any */
> 
> I seems we don't need patch 1 and 2. We only need the commit used to be
> closer to drm-xe-next.
> 
> include/drm-uapi/xe_drm.h should always and only be updated by copying
> from kernel from specific commits documented in the commit message.
> 
> WIP changes while the kernel side is not merged should be moved to
> another header. Traditionally we used LOCAL_* defines for that. But that
> comes with its own problems as it's more work to  go find them all and
> drop when we sync the headers.
> 
> What about doing this?
> 
> Add include/drm-uapi/local/ to -I in build system *before* the non-local
> one. Something like:
> 
> meson.build:
> 	inc = include_directories('include', 'include/drm-uapi/local' 'include/drm-uapi', 'include/linux-uapi', 'lib', 'lib/stubs/syscalls', '.')
> 
> Example content for
> include/drm-uapi/local/drm_xe.h:
> 	#pragma once
> 	#include_next <xe_drm.h>
> 
> 	/*
> 	 * Local definitions - delta with WIP UAPI while the upstream defines
> 	 * are not yet merged. These are supposed to conflict whenever the
> 	 * upstream header is updated and break the build. That is intended.
> 	 * Commit updating the upstream header should garbage collect these
> 	 * local definitions.
> 	 */
> 
> 	#ifdef DRM_XE_TOPO_L3_BANK
> 	#error "Already defined by upstream - remove"
> 	#define DRM_XE_TOPO_L3_BANK		3
> 	#endif
> 
> The ifdef + error could be optional as it's harmless (and allowed by
> compiler) to redefine it multiple times to the same value.
> 
> Thoughts?

Neither this solution nor the LOCAL_* defines have been in place for Xe
until now, so is there a particular reason to introduce it now? Not
questioning the benefit of doing it and I am all up for continuous
improvement, just wondering if I did something different here.

The proposal looks good, once it is in place it will be easy to add new
values. I prefer the stricter but cleaner ifdef + error to prevent
duplicates and to keep the local header to the minimum. But I will let
others chime in especially if there are lessons learned from i915.

> 
> If we don't want to decide on this and you only want to unblock this
> test for the kernel addition, then maybe just do the same LOCAL_
> approach currently being used. See e.g. lib/i915/i915_drm_local.h

Sure, will do for next versions until a solution is in place.

Thanks,
Francois

> 
> Lucas De Marchi
> 
> >  lib/xe/xe_query: Add L3 bank mask test
> > 
> > include/drm-uapi/xe_drm.h | 27 +++-------------
> > tests/intel/xe_query.c    | 66 +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 71 insertions(+), 22 deletions(-)
> > 
> > -- 
> > 2.34.1
> > 