[PATCH i-g-t,v1 0/4] L3 bank mask
Lucas De Marchi
lucas.demarchi at intel.com
Fri Apr 19 16:02:18 UTC 2024
On Fri, Apr 19, 2024 at 03:14:52PM GMT, Kamil Konieczny wrote:
>Hi,
>On 2024-04-17 at 11:07:54 -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.
>>
>> 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:
>
>We already have similar solution for i915, located in
>./lib/i915/i915_drm_local.h
which I mentioned above with the LOCAL_* defines and below
with the header we are talking about.
>
>imho we could have it here: ./lib/xe/xe_drm_local.h
>
>Regards,
>Kamil
>
>>
>> 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?
>>
>> 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
^ here
The problem with that is spreading throughout
the code the "#include <..._local.h>" and then removal in follow up
commits. A single -I.../local/ avoids that.
Lucas De Marchi
>>
>> 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
>> >
More information about the igt-dev
mailing list