[PATCH i-g-t,v1 0/4] L3 bank mask
Lucas De Marchi
lucas.demarchi at intel.com
Fri Apr 19 16:05:12 UTC 2024
On Thu, Apr 18, 2024 at 03:27:20PM GMT, Francois Dugast wrote:
>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
patch 1 tries to fix something that only happened because neither of
these solutions are being used. Then later in the series you just
reintroduce the same problem.
>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.
just remember to dedicate a xe header for that, don't conflate with the
i915 one.
thanks
Lucas De Marchi
More information about the igt-dev
mailing list