| Message ID | 20180503002352.11951-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Paulo Zanoni (2018-05-03 03:23:52) > ICL changes the registers and addresses to 64 bits. > > I also briefly looked at implementing an u64 version of the PCI config > read functions, but I concluded this wouldn't be trivial, so it's not > worth doing it for a single user that can't have any racing problems > while reading the register in two separate operations. > > v2: > - Adjust the patch after the i915_stolen_to_dma() changes. > - Remove unused variable (Daniele). > - Update commit message. > v3: > - Fix a missing phys_addr_t->dma_addr_t forgotten in v2 (kbuild bot) > v4: > - Rebase. > v5: > - Fix warnings in arch/x86/kernel/early-quirks.c after rebase. > v6: > - No more TODO list. > - Stay under 80 columns. > - Add debug message to match the other functions. This will only confuse most readers, so please do scrub the internal changelog when sending the first revision on a mailing list. > Issue: VIZ-9250 > Cc: Ingo Molnar <mingo@kernel.org> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: x86@kernel.org > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> # Early review, needs re-check before merging > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > arch/x86/kernel/early-quirks.c | 18 ++++++++++++++++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 38 +++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_reg.h | 1 + > include/drm/i915_drm.h | 4 +++- > 4 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c > index bae0d32e327b..96228bac1c8c 100644 > --- a/arch/x86/kernel/early-quirks.c > +++ b/arch/x86/kernel/early-quirks.c > @@ -340,6 +340,18 @@ static resource_size_t __init gen3_stolen_base(int num, int slot, int func, > return bsm & INTEL_BSM_MASK; > } > > +static resource_size_t __init gen11_stolen_base(int num, int slot, int func, > + resource_size_t stolen_size) > +{ > + u64 bsm; > + > + bsm = read_pci_config(num, slot, func, INTEL_GEN11_BSM_DW0); > + bsm &= INTEL_BSM_MASK; > + bsm |= (u64)read_pci_config(num, slot, func, INTEL_GEN11_BSM_DW1) << 32; > + > + return (resource_size_t)bsm; return bsm; will suffice. > +} > + > static resource_size_t __init i830_stolen_size(int num, int slot, int func) > { > u16 gmch_ctrl; > @@ -500,6 +512,11 @@ static const struct intel_early_ops chv_early_ops __initconst = { > .stolen_size = chv_stolen_size, > }; > > +static const struct intel_early_ops gen11_early_ops __initconst = { > + .stolen_base = gen11_stolen_base, > + .stolen_size = gen9_stolen_size, > +}; > + > static const struct pci_device_id intel_early_ids[] __initconst = { > INTEL_I830_IDS(&i830_early_ops), > INTEL_I845G_IDS(&i845_early_ops), > @@ -531,6 +548,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = { > INTEL_CFL_IDS(&gen9_early_ops), > INTEL_GLK_IDS(&gen9_early_ops), > INTEL_CNL_IDS(&gen9_early_ops), > + INTEL_ICL_11_IDS(&gen11_early_ops), > }; Please split the patch here and add a respective Fixes: tag to when base Icelake support was introduced. Lacking this portion when running ICL will cause random memory corruption so it's important to have this landed early. Regards, Joonas
Em Qui, 2018-05-03 às 12:59 +0300, Joonas Lahtinen escreveu: > Quoting Paulo Zanoni (2018-05-03 03:23:52) > > ICL changes the registers and addresses to 64 bits. > > > > I also briefly looked at implementing an u64 version of the PCI > > config > > read functions, but I concluded this wouldn't be trivial, so it's > > not > > worth doing it for a single user that can't have any racing > > problems > > while reading the register in two separate operations. > > > > v2: > > - Adjust the patch after the i915_stolen_to_dma() changes. > > - Remove unused variable (Daniele). > > - Update commit message. > > v3: > > - Fix a missing phys_addr_t->dma_addr_t forgotten in v2 (kbuild > > bot) > > v4: > > - Rebase. > > v5: > > - Fix warnings in arch/x86/kernel/early-quirks.c after rebase. > > v6: > > - No more TODO list. > > - Stay under 80 columns. > > - Add debug message to match the other functions. > > This will only confuse most readers, so please do scrub the internal > changelog when sending the first revision on a mailing list. We've been doing this for years, why did our opinion change today? > > > Issue: VIZ-9250 > > Cc: Ingo Molnar <mingo@kernel.org> > > Cc: H. Peter Anvin <hpa@zytor.com> > > Cc: x86@kernel.org > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.co > > m> # Early review, needs re-check before merging > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > arch/x86/kernel/early-quirks.c | 18 ++++++++++++++++ > > drivers/gpu/drm/i915/i915_gem_stolen.c | 38 > > +++++++++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > include/drm/i915_drm.h | 4 +++- > > 4 files changed, 59 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/early-quirks.c > > b/arch/x86/kernel/early-quirks.c > > index bae0d32e327b..96228bac1c8c 100644 > > --- a/arch/x86/kernel/early-quirks.c > > +++ b/arch/x86/kernel/early-quirks.c > > @@ -340,6 +340,18 @@ static resource_size_t __init > > gen3_stolen_base(int num, int slot, int func, > > return bsm & INTEL_BSM_MASK; > > } > > > > +static resource_size_t __init gen11_stolen_base(int num, int slot, > > int func, > > + resource_size_t > > stolen_size) > > +{ > > + u64 bsm; > > + > > + bsm = read_pci_config(num, slot, func, > > INTEL_GEN11_BSM_DW0); > > + bsm &= INTEL_BSM_MASK; > > + bsm |= (u64)read_pci_config(num, slot, func, > > INTEL_GEN11_BSM_DW1) << 32; > > + > > + return (resource_size_t)bsm; > > return bsm; will suffice. > > > +} > > + > > static resource_size_t __init i830_stolen_size(int num, int slot, > > int func) > > { > > u16 gmch_ctrl; > > @@ -500,6 +512,11 @@ static const struct intel_early_ops > > chv_early_ops __initconst = { > > .stolen_size = chv_stolen_size, > > }; > > > > +static const struct intel_early_ops gen11_early_ops __initconst = > > { > > + .stolen_base = gen11_stolen_base, > > + .stolen_size = gen9_stolen_size, > > +}; > > + > > static const struct pci_device_id intel_early_ids[] __initconst = > > { > > INTEL_I830_IDS(&i830_early_ops), > > INTEL_I845G_IDS(&i845_early_ops), > > @@ -531,6 +548,7 @@ static const struct pci_device_id > > intel_early_ids[] __initconst = { > > INTEL_CFL_IDS(&gen9_early_ops), > > INTEL_GLK_IDS(&gen9_early_ops), > > INTEL_CNL_IDS(&gen9_early_ops), > > + INTEL_ICL_11_IDS(&gen11_early_ops), > > }; > > Please split the patch here and add a respective Fixes: tag to when > base Icelake support was introduced. Lacking this portion when > running ICL will > cause random memory corruption so it's important to have this landed > early. Icelake is still hidden behind the alpha_support flag, so no point in backporting for i915.ko specifically. If we're talking about backporting just because of the memory reservation without graphics, how far back should we go? > > Regards, Joonas
Quoting Paulo Zanoni (2018-05-03 16:24:47) > Em Qui, 2018-05-03 às 12:59 +0300, Joonas Lahtinen escreveu: > > Please split the patch here and add a respective Fixes: tag to when > > base Icelake support was introduced. Lacking this portion when > > running ICL will > > cause random memory corruption so it's important to have this landed > > early. > > Icelake is still hidden behind the alpha_support flag, so no point in > backporting for i915.ko specifically. If we're talking about > backporting just because of the memory reservation without graphics, > how far back should we go? The danger for the reserved portion is that it is used by the bios/uefi scanout and vgacon. So if the gfx reserved portion is absent from the 820 memmap (buggy bios or whatever), then it may be reused by the system even though it's been written to by the console (and unrelated to i915.ko). That's where the danger comes from, it's happened before so we fear it may happen again. -Chris
Quoting Chris Wilson (2018-05-03 18:35:48) > Quoting Paulo Zanoni (2018-05-03 16:24:47) > > Em Qui, 2018-05-03 às 12:59 +0300, Joonas Lahtinen escreveu: > > > Please split the patch here and add a respective Fixes: tag to when > > > base Icelake support was introduced. Lacking this portion when > > > running ICL will > > > cause random memory corruption so it's important to have this landed > > > early. > > > > Icelake is still hidden behind the alpha_support flag, so no point in > > backporting for i915.ko specifically. If we're talking about > > backporting just because of the memory reservation without graphics, > > how far back should we go? > > The danger for the reserved portion is that it is used by the bios/uefi > scanout and vgacon. So if the gfx reserved portion is absent from the > 820 memmap (buggy bios or whatever), then it may be reused by the system > even though it's been written to by the console (and unrelated to > i915.ko). That's where the danger comes from, it's happened before so we > fear it may happen again. Yep, This same discussion was had for Cannonlake; alpha_support protection is not enough to justify system level memory corruption which has nothing to do with i915 module but all to do with enforcing e820 memory maps not to include with the BIOS framebuffer as regular memory. So this step really should be a separate patch, and should be one of the first patches to upstream. Regards, Joonas
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c index bae0d32e327b..96228bac1c8c 100644 --- a/arch/x86/kernel/early-quirks.c +++ b/arch/x86/kernel/early-quirks.c @@ -340,6 +340,18 @@ static resource_size_t __init gen3_stolen_base(int num, int slot, int func, return bsm & INTEL_BSM_MASK; } +static resource_size_t __init gen11_stolen_base(int num, int slot, int func, + resource_size_t stolen_size) +{ + u64 bsm; + + bsm = read_pci_config(num, slot, func, INTEL_GEN11_BSM_DW0); + bsm &= INTEL_BSM_MASK; + bsm |= (u64)read_pci_config(num, slot, func, INTEL_GEN11_BSM_DW1) << 32; + + return (resource_size_t)bsm; +} + static resource_size_t __init i830_stolen_size(int num, int slot, int func) { u16 gmch_ctrl; @@ -500,6 +512,11 @@ static const struct intel_early_ops chv_early_ops __initconst = { .stolen_size = chv_stolen_size, }; +static const struct intel_early_ops gen11_early_ops __initconst = { + .stolen_base = gen11_stolen_base, + .stolen_size = gen9_stolen_size, +}; + static const struct pci_device_id intel_early_ids[] __initconst = { INTEL_I830_IDS(&i830_early_ops), INTEL_I845G_IDS(&i845_early_ops), @@ -531,6 +548,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = { INTEL_CFL_IDS(&gen9_early_ops), INTEL_GLK_IDS(&gen9_early_ops), INTEL_CNL_IDS(&gen9_early_ops), + INTEL_ICL_11_IDS(&gen11_early_ops), }; struct resource intel_graphics_stolen_res __ro_after_init = DEFINE_RES_MEM(0, 0); diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index ad949cc30928..ede432f6891c 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -343,6 +343,35 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv, *size = stolen_top - *base; } +static void icl_get_stolen_reserved(struct drm_i915_private *dev_priv, + resource_size_t *base, + resource_size_t *size) +{ + uint64_t reg_val = I915_READ64(GEN6_STOLEN_RESERVED); + + DRM_DEBUG_DRIVER("GEN6_STOLEN_RESERVED = 0x%016llx\n", reg_val); + + *base = reg_val & GEN11_STOLEN_RESERVED_ADDR_MASK; + + switch (reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK) { + case GEN8_STOLEN_RESERVED_1M: + *size = 1024 * 1024; + break; + case GEN8_STOLEN_RESERVED_2M: + *size = 2 * 1024 * 1024; + break; + case GEN8_STOLEN_RESERVED_4M: + *size = 4 * 1024 * 1024; + break; + case GEN8_STOLEN_RESERVED_8M: + *size = 8 * 1024 * 1024; + break; + default: + *size = 8 * 1024 * 1024; + MISSING_CASE(reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK); + } +} + int i915_gem_init_stolen(struct drm_i915_private *dev_priv) { resource_size_t reserved_base, stolen_top; @@ -399,7 +428,9 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) gen7_get_stolen_reserved(dev_priv, &reserved_base, &reserved_size); break; - default: + case 8: + case 9: + case 10: if (IS_LP(dev_priv)) chv_get_stolen_reserved(dev_priv, &reserved_base, &reserved_size); @@ -407,6 +438,11 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) bdw_get_stolen_reserved(dev_priv, &reserved_base, &reserved_size); break; + case 11: + default: + icl_get_stolen_reserved(dev_priv, &reserved_base, + &reserved_size); + break; } /* diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 085928c9005e..859d8c697123 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -395,6 +395,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define GEN8_STOLEN_RESERVED_4M (2 << 7) #define GEN8_STOLEN_RESERVED_8M (3 << 7) #define GEN6_STOLEN_RESERVED_ENABLE (1 << 0) +#define GEN11_STOLEN_RESERVED_ADDR_MASK (0xFFFFFFFFFFFULL << 20) /* VGA stuff */ diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h index c9e5a6621b95..c44703f471b3 100644 --- a/include/drm/i915_drm.h +++ b/include/drm/i915_drm.h @@ -95,7 +95,9 @@ extern struct resource intel_graphics_stolen_res; #define I845_TSEG_SIZE_512K (2 << 1) #define I845_TSEG_SIZE_1M (3 << 1) -#define INTEL_BSM 0x5c +#define INTEL_BSM 0x5c +#define INTEL_GEN11_BSM_DW0 0xc0 +#define INTEL_GEN11_BSM_DW1 0xc4 #define INTEL_BSM_MASK (-(1u << 20)) #endif /* _I915_DRM_H_ */