Re: [PATCH v4 03/23] drm/xe: Move 'va_bits' flag back to platform descriptor

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Oct 07, 2025 at 05:02:57PM -0500, Lucas De Marchi wrote:
> On Tue, Oct 07, 2025 at 01:48:33PM -0700, Matt Roper wrote:
> > The number of virtual address bits is something that should be tracked
> > at the platform level rather than the IP level.  Even when mixing and
> > matching various graphics, media, and display IP blocks, the platform as
> > a whole has to have consistent page table handling.  This is also a
> > trait that should be tied to the platform even if the graphics IP itself
> > is not present (e.g., if we disable the primary GT via configfs).
> > 
> > v2:
> > - Drop the default value of 48 and explicitly set it in each relevant
> >   descriptor.  (Lucas, Michal)
> > v3:
> > - Drop an outdated comment about default value.  (Michal)
> > 
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_pci.c       | 21 +++++++++++++++------
> > drivers/gpu/drm/xe/xe_pci_types.h |  2 +-
> > 2 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > index 3f42b91efa28..69ed987fef67 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -52,13 +52,11 @@ __diag_ignore_all("-Woverride-init", "Allow field overrides in table");
> > static const struct xe_graphics_desc graphics_xelp = {
> > 	.hw_engine_mask = BIT(XE_HW_ENGINE_RCS0) | BIT(XE_HW_ENGINE_BCS0),
> > 
> > -	.va_bits = 48,
> > 	.vm_max_level = 3,
> > };
> > 
> > #define XE_HP_FEATURES \
> > 	.has_range_tlb_inval = true, \
> > -	.va_bits = 48, \
> > 	.vm_max_level = 3
> > 
> > static const struct xe_graphics_desc graphics_xehpg = {
> > @@ -84,7 +82,6 @@ static const struct xe_graphics_desc graphics_xehpc = {
> > 		BIT(XE_HW_ENGINE_CCS2) | BIT(XE_HW_ENGINE_CCS3),
> > 
> > 	XE_HP_FEATURES,
> > -	.va_bits = 57,
> > 	.vm_max_level = 4,
> > 	.vram_flags = XE_VRAM_FLAGS_NEED64K,
> > 
> > @@ -108,7 +105,6 @@ static const struct xe_graphics_desc graphics_xelpg = {
> > 	.has_range_tlb_inval = 1, \
> > 	.has_usm = 1, \
> > 	.has_64bit_timestamp = 1, \
> > -	.va_bits = 48, \
> > 	.vm_max_level = 4, \
> > 	.hw_engine_mask = \
> > 		BIT(XE_HW_ENGINE_RCS0) | \
> > @@ -174,6 +170,7 @@ static const struct xe_device_desc tgl_desc = {
> > 	.has_sriov = true,
> > 	.max_gt_per_tile = 1,
> > 	.require_force_probe = true,
> > +	.va_bits = 48,
> > };
> > 
> > static const struct xe_device_desc rkl_desc = {
> > @@ -185,6 +182,7 @@ static const struct xe_device_desc rkl_desc = {
> > 	.has_llc = true,
> > 	.max_gt_per_tile = 1,
> > 	.require_force_probe = true,
> > +	.va_bits = 48,
> > };
> > 
> > static const u16 adls_rpls_ids[] = { INTEL_RPLS_IDS(NOP), 0 };
> > @@ -203,6 +201,7 @@ static const struct xe_device_desc adl_s_desc = {
> > 		{ XE_SUBPLATFORM_ALDERLAKE_S_RPLS, "RPLS", adls_rpls_ids },
> > 		{},
> > 	},
> > +	.va_bits = 48,
> > };
> > 
> > static const u16 adlp_rplu_ids[] = { INTEL_RPLU_IDS(NOP), 0 };
> > @@ -221,6 +220,7 @@ static const struct xe_device_desc adl_p_desc = {
> > 		{ XE_SUBPLATFORM_ALDERLAKE_P_RPLU, "RPLU", adlp_rplu_ids },
> > 		{},
> > 	},
> > +	.va_bits = 48,
> > };
> > 
> > static const struct xe_device_desc adl_n_desc = {
> > @@ -233,6 +233,7 @@ static const struct xe_device_desc adl_n_desc = {
> > 	.has_sriov = true,
> > 	.max_gt_per_tile = 1,
> > 	.require_force_probe = true,
> > +	.va_bits = 48,
> > };
> > 
> > #define DGFX_FEATURES \
> > @@ -249,6 +250,7 @@ static const struct xe_device_desc dg1_desc = {
> > 	.has_heci_gscfi = 1,
> > 	.max_gt_per_tile = 1,
> > 	.require_force_probe = true,
> > +	.va_bits = 48,
> > };
> > 
> > static const u16 dg2_g10_ids[] = { INTEL_DG2_G10_IDS(NOP), INTEL_ATS_M150_IDS(NOP), 0 };
> > @@ -265,7 +267,8 @@ static const u16 dg2_g12_ids[] = { INTEL_DG2_G12_IDS(NOP), 0 };
> > 		{ XE_SUBPLATFORM_DG2_G11, "G11", dg2_g11_ids }, \
> > 		{ XE_SUBPLATFORM_DG2_G12, "G12", dg2_g12_ids }, \
> > 		{ } \
> > -	}
> > +	}, \
> > +	.va_bits = 48
> > 
> > static const struct xe_device_desc ats_m_desc = {
> > 	.pre_gmdid_graphics_ip = &graphics_ip_xehpg,
> > @@ -303,6 +306,7 @@ static const __maybe_unused struct xe_device_desc pvc_desc = {
> > 	.max_gt_per_tile = 1,
> > 	.max_remote_tiles = 1,
> > 	.require_force_probe = true,
> > +	.va_bits = 57,
> > 	.has_mbx_power_limits = false,
> > };
> > 
> > @@ -314,6 +318,7 @@ static const struct xe_device_desc mtl_desc = {
> > 	.has_display = true,
> > 	.has_pxp = true,
> > 	.max_gt_per_tile = 2,
> > +	.va_bits = 48,
> > };
> > 
> > static const struct xe_device_desc lnl_desc = {
> > @@ -323,6 +328,7 @@ static const struct xe_device_desc lnl_desc = {
> > 	.has_pxp = true,
> > 	.max_gt_per_tile = 2,
> > 	.needs_scratch = true,
> > +	.va_bits = 48,
> 
> as a follow up, maybe do a s/dma_mask_size/pa_bits/, so it's easy for
> people enabling new platforms to reference e.g. bspec 70817 where they
> are noted as "Virtual Address Range" and "Physical Address Range".

I'm a little bit hesitant about making that rename, because the reality
is more complicated and confusing than that.  For example, bspec 53650
will tell you the physical address range of ADL-S is 46-bits, but we
still need to limit ourselves to 39 bits in the driver.  We were
confused during early ADL-S enabling because we thought the 46-bit
address space in the spec meant we should just set the mask size to
46-bits, but that caused a bunch of DMAR faults.

Rodrigo eventually tracked down a hardware architect who gave us some
more insight into what it all means at the platform level.  If I
understand correctly, the host physical addresses are indeed 46-bits
when MKTME + IOMMU is in use (HPA[45:42] is a MKTME key ID and
HPA[41:39] is always 0).  However that's only really relevant to the
IOMMU and hypervisor in those specific configurations.  Guest physical
addresses are still 39-bits, and when MKTME is disabled the platform is
limited to 39-bit addresses.

Basically the real details here wind up being a platform-level thing
that isn't always clearly documented or explained in the bspec.  Simply
taking the single value that the bspec calls "physical address range"
(with no other explanation) and plugging that into the driver sometimes
isn't the right thing to do.

dma_mask_size probably isn't the most descriptive structure field name
either, but at least it matches the Linux DMA mapping functions that we
need to call during init.


Matt

> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Lucas De Marchi
> 
> > };
> > 
> > static const struct xe_device_desc bmg_desc = {
> > @@ -338,6 +344,7 @@ static const struct xe_device_desc bmg_desc = {
> > 	.has_sriov = true,
> > 	.max_gt_per_tile = 2,
> > 	.needs_scratch = true,
> > +	.va_bits = 48,
> > };
> > 
> > static const struct xe_device_desc ptl_desc = {
> > @@ -347,6 +354,7 @@ static const struct xe_device_desc ptl_desc = {
> > 	.has_sriov = true,
> > 	.max_gt_per_tile = 2,
> > 	.needs_scratch = true,
> > +	.va_bits = 48,
> > };
> > 
> > #undef PLATFORM
> > @@ -584,6 +592,8 @@ static int xe_info_init_early(struct xe_device *xe,
> > 		subplatform_desc->subplatform : XE_SUBPLATFORM_NONE;
> > 
> > 	xe->info.dma_mask_size = desc->dma_mask_size;
> > +	xe->info.va_bits = desc->va_bits;
> > +
> > 	xe->info.is_dgfx = desc->is_dgfx;
> > 	xe->info.has_fan_control = desc->has_fan_control;
> > 	xe->info.has_mbx_power_limits = desc->has_mbx_power_limits;
> > @@ -713,7 +723,6 @@ static int xe_info_init(struct xe_device *xe,
> > 	}
> > 
> > 	xe->info.vram_flags = graphics_desc->vram_flags;
> > -	xe->info.va_bits = graphics_desc->va_bits;
> > 	xe->info.vm_max_level = graphics_desc->vm_max_level;
> > 	xe->info.has_asid = graphics_desc->has_asid;
> > 	xe->info.has_atomic_enable_pte_bit = graphics_desc->has_atomic_enable_pte_bit;
> > diff --git a/drivers/gpu/drm/xe/xe_pci_types.h b/drivers/gpu/drm/xe/xe_pci_types.h
> > index 9b9766a3baa3..796439571abe 100644
> > --- a/drivers/gpu/drm/xe/xe_pci_types.h
> > +++ b/drivers/gpu/drm/xe/xe_pci_types.h
> > @@ -30,6 +30,7 @@ struct xe_device_desc {
> > 	u8 dma_mask_size;
> > 	u8 max_remote_tiles:2;
> > 	u8 max_gt_per_tile:2;
> > +	u8 va_bits;
> > 
> > 	u8 require_force_probe:1;
> > 	u8 is_dgfx:1;
> > @@ -51,7 +52,6 @@ struct xe_device_desc {
> > };
> > 
> > struct xe_graphics_desc {
> > -	u8 va_bits;
> > 	u8 vm_max_level;
> > 	u8 vram_flags;
> > 
> > -- 
> > 2.51.0
> > 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation