x86/gpu: add JSL stolen memory support

Commit Message

Tejas Upadhyay Nov. 4, 2020, 12:05 p.m. UTC

JSL re-uses the same stolen memory as ICL and EHL.

Cc: Lucas De Marchi <[e-mail]>
Cc: Matt Roper <[e-mail]>
Signed-off-by: Tejas Upadhyay <[e-mail]>
---
 arch/x86/kernel/early-quirks.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Bjorn Helgaas Nov. 4, 2020, 5:35 p.m. UTC | #1

[+cc Jani, Joonas, Rodrigo, David, Daniel]

On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> JSL re-uses the same stolen memory as ICL and EHL.
> 
> Cc: Lucas De Marchi <[e-mail]>
> Cc: Matt Roper <[e-mail]>
> Signed-off-by: Tejas Upadhyay <[e-mail]>

I don't plan to do anything with this since previous similar patches
have gone through some other tree, so this is just kibitzing.

But the fact that we have this long list of Intel devices [1] that
constantly needs updates [2] is a hint that something is wrong.

IIUC the general idea is that we need to discover Intel gfx memory by
looking at device-dependent config space and add it to the E820 map.
Apparently the quirks discover this via PCI config registers like
I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
global "intel_graphics_stolen_res"?

That's not the way this should work.  There should some generic, non
device-dependent PCI or ACPI method to discover the memory used, or at
least some way to do it in the driver instead of early arch code.

How is this *supposed* to work?  Is there something we can do in E820
or other resource management that would make this easier?

> ---
>  arch/x86/kernel/early-quirks.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index a4b5af03dcc1..534cc3f78c6b 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
>  	INTEL_CNL_IDS(&gen9_early_ops),
>  	INTEL_ICL_11_IDS(&gen11_early_ops),
>  	INTEL_EHL_IDS(&gen11_early_ops),
> +	INTEL_JSL_IDS(&gen11_early_ops),
>  	INTEL_TGL_12_IDS(&gen11_early_ops),
>  	INTEL_RKL_IDS(&gen11_early_ops),
>  };

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518

[2]
  May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
  Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
  Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
  May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
  Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
  Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
  Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
  Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
  Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
  ...

Joonas Lahtinen Nov. 5, 2020, 9:46 a.m. UTC | #2

Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> [+cc Jani, Joonas, Rodrigo, David, Daniel]
> 
> On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > JSL re-uses the same stolen memory as ICL and EHL.
> > 
> > Cc: Lucas De Marchi <[e-mail]>
> > Cc: Matt Roper <[e-mail]>
> > Signed-off-by: Tejas Upadhyay <[e-mail]>
> 
> I don't plan to do anything with this since previous similar patches
> have gone through some other tree, so this is just kibitzing.
> 
> But the fact that we have this long list of Intel devices [1] that
> constantly needs updates [2] is a hint that something is wrong.

We add an entry for every new integrated graphics platform. Once the
platform is added, there have not been changes lately.

> IIUC the general idea is that we need to discover Intel gfx memory by
> looking at device-dependent config space and add it to the E820 map.
> Apparently the quirks discover this via PCI config registers like
> I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> global "intel_graphics_stolen_res"?

We discover what is called the graphics data stolen memory. It is regular
system memory range that is not CPU accessible. It is accessible by the
integrated graphics only.

See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9

> That's not the way this should work.  There should some generic, non
> device-dependent PCI or ACPI method to discover the memory used, or at
> least some way to do it in the driver instead of early arch code.

It's used by the early BIOS/UEFI code to set up initial framebuffer.
Even if i915 driver is never loaded, the memory ranges still need to
be fixed. They source of the problem is that the OEM BIOS which are
not under our control get the programming wrong.

We used to detect the memory region size again at i915 initialization
but wanted to eliminate the code duplication and resulting subtle bugs
that caused. Conclusion back then was that storing the struct resource
in memory is the best trade-off.

> How is this *supposed* to work?  Is there something we can do in E820
> or other resource management that would make this easier?

The code was added around Haswell (HSW) device generation to mitigate
bugs in BIOS. It is traditionally hard to get all OEMs to fix their
BIOS when things work for Windows. It's only later years when some
laptop models are intended to be sold with Linux.

The alternative would be to get all the OEM to fix their BIOS for Linux,
but that is not very realistic given past experiences. So it seems
a better choice to to add new line per platform generation to make
sure the users can boot to Linux.

Regards, Joonas

> > ---
> >  arch/x86/kernel/early-quirks.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > index a4b5af03dcc1..534cc3f78c6b 100644
> > --- a/arch/x86/kernel/early-quirks.c
> > +++ b/arch/x86/kernel/early-quirks.c
> > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> >       INTEL_CNL_IDS(&gen9_early_ops),
> >       INTEL_ICL_11_IDS(&gen11_early_ops),
> >       INTEL_EHL_IDS(&gen11_early_ops),
> > +     INTEL_JSL_IDS(&gen11_early_ops),
> >       INTEL_TGL_12_IDS(&gen11_early_ops),
> >       INTEL_RKL_IDS(&gen11_early_ops),
> >  };
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> 
> [2]
>   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
>   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
>   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
>   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
>   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
>   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
>   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
>   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
>   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
>   ...

Bjorn Helgaas Nov. 5, 2020, 2:17 p.m. UTC | #3

On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > 
> > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > JSL re-uses the same stolen memory as ICL and EHL.
> > > 
> > > Cc: Lucas De Marchi <[e-mail]>
> > > Cc: Matt Roper <[e-mail]>
> > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > 
> > I don't plan to do anything with this since previous similar patches
> > have gone through some other tree, so this is just kibitzing.
> > 
> > But the fact that we have this long list of Intel devices [1] that
> > constantly needs updates [2] is a hint that something is wrong.
> 
> We add an entry for every new integrated graphics platform. Once the
> platform is added, there have not been changes lately.
> 
> > IIUC the general idea is that we need to discover Intel gfx memory by
> > looking at device-dependent config space and add it to the E820 map.
> > Apparently the quirks discover this via PCI config registers like
> > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > global "intel_graphics_stolen_res"?
> 
> We discover what is called the graphics data stolen memory. It is regular
> system memory range that is not CPU accessible. It is accessible by the
> integrated graphics only.
> 
> See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> 
> > That's not the way this should work.  There should some generic, non
> > device-dependent PCI or ACPI method to discover the memory used, or at
> > least some way to do it in the driver instead of early arch code.
> 
> It's used by the early BIOS/UEFI code to set up initial framebuffer.
> Even if i915 driver is never loaded, the memory ranges still need to
> be fixed. They source of the problem is that the OEM BIOS which are
> not under our control get the programming wrong.
> 
> We used to detect the memory region size again at i915 initialization
> but wanted to eliminate the code duplication and resulting subtle bugs
> that caused. Conclusion back then was that storing the struct resource
> in memory is the best trade-off.
> 
> > How is this *supposed* to work?  Is there something we can do in E820
> > or other resource management that would make this easier?
> 
> The code was added around Haswell (HSW) device generation to mitigate
> bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> BIOS when things work for Windows. It's only later years when some
> laptop models are intended to be sold with Linux.
> 
> The alternative would be to get all the OEM to fix their BIOS for Linux,
> but that is not very realistic given past experiences. So it seems
> a better choice to to add new line per platform generation to make
> sure the users can boot to Linux.

How does Windows do this?  Do they have to add similar code for each
new platform?

> > > ---
> > >  arch/x86/kernel/early-quirks.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > index a4b5af03dcc1..534cc3f78c6b 100644
> > > --- a/arch/x86/kernel/early-quirks.c
> > > +++ b/arch/x86/kernel/early-quirks.c
> > > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> > >       INTEL_CNL_IDS(&gen9_early_ops),
> > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > >       INTEL_RKL_IDS(&gen11_early_ops),
> > >  };
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > 
> > [2]
> >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
> >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
> >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
> >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
> >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
> >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
> >   ...

Daniel Vetter Nov. 6, 2020, 9:39 a.m. UTC | #4

On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
>
> On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > >
> > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > >
> > > > Cc: Lucas De Marchi <[e-mail]>
> > > > Cc: Matt Roper <[e-mail]>
> > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > >
> > > I don't plan to do anything with this since previous similar patches
> > > have gone through some other tree, so this is just kibitzing.
> > >
> > > But the fact that we have this long list of Intel devices [1] that
> > > constantly needs updates [2] is a hint that something is wrong.
> >
> > We add an entry for every new integrated graphics platform. Once the
> > platform is added, there have not been changes lately.
> >
> > > IIUC the general idea is that we need to discover Intel gfx memory by
> > > looking at device-dependent config space and add it to the E820 map.
> > > Apparently the quirks discover this via PCI config registers like
> > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > > global "intel_graphics_stolen_res"?
> >
> > We discover what is called the graphics data stolen memory. It is regular
> > system memory range that is not CPU accessible. It is accessible by the
> > integrated graphics only.
> >
> > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> >
> > > That's not the way this should work.  There should some generic, non
> > > device-dependent PCI or ACPI method to discover the memory used, or at
> > > least some way to do it in the driver instead of early arch code.
> >
> > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > Even if i915 driver is never loaded, the memory ranges still need to
> > be fixed. They source of the problem is that the OEM BIOS which are
> > not under our control get the programming wrong.
> >
> > We used to detect the memory region size again at i915 initialization
> > but wanted to eliminate the code duplication and resulting subtle bugs
> > that caused. Conclusion back then was that storing the struct resource
> > in memory is the best trade-off.
> >
> > > How is this *supposed* to work?  Is there something we can do in E820
> > > or other resource management that would make this easier?
> >
> > The code was added around Haswell (HSW) device generation to mitigate
> > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> > BIOS when things work for Windows. It's only later years when some
> > laptop models are intended to be sold with Linux.
> >
> > The alternative would be to get all the OEM to fix their BIOS for Linux,
> > but that is not very realistic given past experiences. So it seems
> > a better choice to to add new line per platform generation to make
> > sure the users can boot to Linux.
>
> How does Windows do this?  Do they have to add similar code for each
> new platform?

Windows is chicken and doesn't move any mmio bar around on its own.
Except if the bios explicitly told it somehow (e.g. for the 64bit bar
stuff amd recently announced for windows, that linux supports since
years by moving the bar). So except if you want to preemptively
disable the pci code that does this anytime there's an intel gpu, this
is what we have to do.

And given that this 64bit mmio bar support in windows still requires
an explicit bios upgrade for the explicit opt in I don't think this
will change anytime soon.

We have a similar ugly problem with kvm, since you can't use these
ranges freely (they're very special in hw), and the kvm maintainers
are equally annoyed that they have to keep supporting RRMR to block
that range, just because of intel integrated graphics. Apparently
windows is again totally fine with this.
-Daniel


>
> > > > ---
> > > >  arch/x86/kernel/early-quirks.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > > index a4b5af03dcc1..534cc3f78c6b 100644
> > > > --- a/arch/x86/kernel/early-quirks.c
> > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> > > >       INTEL_CNL_IDS(&gen9_early_ops),
> > > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > > >       INTEL_RKL_IDS(&gen11_early_ops),
> > > >  };
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > >
> > > [2]
> > >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> > >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> > >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
> > >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
> > >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> > >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
> > >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
> > >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
> > >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
> > >   ...

Tejas Upadhyay Nov. 18, 2020, 12:10 p.m. UTC | #5

Just checking, can somebody review/merge the patch? 

Thanks,
Tejas

> -----Original Message-----
> From: Daniel Vetter <[e-mail]>
> Sent: 06 November 2020 15:09
> To: Bjorn Helgaas <[e-mail]>
> Cc: Joonas Lahtinen <[e-mail]>; Surendrakumar
> Upadhyay, TejaskumarX
> <[e-mail]>; Linux PCI <linux-
> [e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> Matthew D <[e-mail]>; Pandey, Hariom
> <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> 
> On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> >
> > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > >
> > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > >
> > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > Cc: Matt Roper <[e-mail]>
> > > > > Signed-off-by: Tejas Upadhyay
> > > > > <[e-mail]>
> > > >
> > > > I don't plan to do anything with this since previous similar
> > > > patches have gone through some other tree, so this is just kibitzing.
> > > >
> > > > But the fact that we have this long list of Intel devices [1] that
> > > > constantly needs updates [2] is a hint that something is wrong.
> > >
> > > We add an entry for every new integrated graphics platform. Once the
> > > platform is added, there have not been changes lately.
> > >
> > > > IIUC the general idea is that we need to discover Intel gfx memory
> > > > by looking at device-dependent config space and add it to the E820
> map.
> > > > Apparently the quirks discover this via PCI config registers like
> > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via
> > > > the global "intel_graphics_stolen_res"?
> > >
> > > We discover what is called the graphics data stolen memory. It is
> > > regular system memory range that is not CPU accessible. It is
> > > accessible by the integrated graphics only.
> > >
> > > See:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/c
> > > ommit/arch/x86/kernel/early-quirks.c?h=v5.10-
> rc2&id=814c5f1f52a4beb3
> > > 710317022acd6ad34fc0b6b9
> > >
> > > > That's not the way this should work.  There should some generic,
> > > > non device-dependent PCI or ACPI method to discover the memory
> > > > used, or at least some way to do it in the driver instead of early arch
> code.
> > >
> > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > > Even if i915 driver is never loaded, the memory ranges still need to
> > > be fixed. They source of the problem is that the OEM BIOS which are
> > > not under our control get the programming wrong.
> > >
> > > We used to detect the memory region size again at i915
> > > initialization but wanted to eliminate the code duplication and
> > > resulting subtle bugs that caused. Conclusion back then was that
> > > storing the struct resource in memory is the best trade-off.
> > >
> > > > How is this *supposed* to work?  Is there something we can do in
> > > > E820 or other resource management that would make this easier?
> > >
> > > The code was added around Haswell (HSW) device generation to
> > > mitigate bugs in BIOS. It is traditionally hard to get all OEMs to
> > > fix their BIOS when things work for Windows. It's only later years
> > > when some laptop models are intended to be sold with Linux.
> > >
> > > The alternative would be to get all the OEM to fix their BIOS for
> > > Linux, but that is not very realistic given past experiences. So it
> > > seems a better choice to to add new line per platform generation to
> > > make sure the users can boot to Linux.
> >
> > How does Windows do this?  Do they have to add similar code for each
> > new platform?
> 
> Windows is chicken and doesn't move any mmio bar around on its own.
> Except if the bios explicitly told it somehow (e.g. for the 64bit bar stuff amd
> recently announced for windows, that linux supports since years by moving
> the bar). So except if you want to preemptively disable the pci code that does
> this anytime there's an intel gpu, this is what we have to do.
> 
> And given that this 64bit mmio bar support in windows still requires an
> explicit bios upgrade for the explicit opt in I don't think this will change
> anytime soon.
> 
> We have a similar ugly problem with kvm, since you can't use these ranges
> freely (they're very special in hw), and the kvm maintainers are equally
> annoyed that they have to keep supporting RRMR to block that range, just
> because of intel integrated graphics. Apparently windows is again totally fine
> with this.
> -Daniel
> 
> 
> >
> > > > > ---
> > > > >  arch/x86/kernel/early-quirks.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/arch/x86/kernel/early-quirks.c
> > > > > b/arch/x86/kernel/early-quirks.c index
> > > > > a4b5af03dcc1..534cc3f78c6b 100644
> > > > > --- a/arch/x86/kernel/early-quirks.c
> > > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > > @@ -549,6 +549,7 @@ static const struct pci_device_id
> intel_early_ids[] __initconst = {
> > > > >       INTEL_CNL_IDS(&gen9_early_ops),
> > > > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > > > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > > > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > > > >       INTEL_RKL_IDS(&gen11_early_ops),  };
> > > >
> > > > [1]
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > > /tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > > >
> > > > [2]
> > > >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> > > >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> > > >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early
> quirks")
> > > >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen
> memory")
> > > >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> > > >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as
> SKL")
> > > >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as
> SKL")
> > > >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS
> values as Skylake")
> > > >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS
> values as Skylake")
> > > >   ...
> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Bjorn Helgaas Nov. 18, 2020, 4:02 p.m. UTC | #6

On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > >
> > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > >
> > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > Cc: Matt Roper <[e-mail]>
> > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > > >
> > > > I don't plan to do anything with this since previous similar patches
> > > > have gone through some other tree, so this is just kibitzing.
> > > >
> > > > But the fact that we have this long list of Intel devices [1] that
> > > > constantly needs updates [2] is a hint that something is wrong.
> > >
> > > We add an entry for every new integrated graphics platform. Once the
> > > platform is added, there have not been changes lately.
> > >
> > > > IIUC the general idea is that we need to discover Intel gfx memory by
> > > > looking at device-dependent config space and add it to the E820 map.
> > > > Apparently the quirks discover this via PCI config registers like
> > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > > > global "intel_graphics_stolen_res"?
> > >
> > > We discover what is called the graphics data stolen memory. It is regular
> > > system memory range that is not CPU accessible. It is accessible by the
> > > integrated graphics only.
> > >
> > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > >
> > > > That's not the way this should work.  There should some generic, non
> > > > device-dependent PCI or ACPI method to discover the memory used, or at
> > > > least some way to do it in the driver instead of early arch code.
> > >
> > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > > Even if i915 driver is never loaded, the memory ranges still need to
> > > be fixed. They source of the problem is that the OEM BIOS which are
> > > not under our control get the programming wrong.
> > >
> > > We used to detect the memory region size again at i915 initialization
> > > but wanted to eliminate the code duplication and resulting subtle bugs
> > > that caused. Conclusion back then was that storing the struct resource
> > > in memory is the best trade-off.
> > >
> > > > How is this *supposed* to work?  Is there something we can do in E820
> > > > or other resource management that would make this easier?
> > >
> > > The code was added around Haswell (HSW) device generation to mitigate
> > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> > > BIOS when things work for Windows. It's only later years when some
> > > laptop models are intended to be sold with Linux.
> > >
> > > The alternative would be to get all the OEM to fix their BIOS for Linux,
> > > but that is not very realistic given past experiences. So it seems
> > > a better choice to to add new line per platform generation to make
> > > sure the users can boot to Linux.
> >
> > How does Windows do this?  Do they have to add similar code for each
> > new platform?
> 
> Windows is chicken and doesn't move any mmio bar around on its own.
> Except if the bios explicitly told it somehow (e.g. for the 64bit bar
> stuff amd recently announced for windows, that linux supports since
> years by moving the bar). So except if you want to preemptively
> disable the pci code that does this anytime there's an intel gpu, this
> is what we have to do.

I think Windows *does* move BARs (they use the more generic
terminology of "rebalancing PNP resources") in some cases [3,4].  Of
course, I'm pretty sure Windows will only assign PCI resources inside
the windows advertised in the host bridge _CRS.

Linux *used* to ignore that host bridge _CRS and could set BARs to
addresses that appeared available but were in fact used by the
platform somehow.  But Linux has been paying attention to host bridge
_CRS for a long time now, so it should also only assign resources
inside those windows.

So I'm missing what the real problem is.  Joonas mentioned BIOS bugs
above, but I don't know what that means.

Here's what I can figure out so far, tell me if I'm in the weeds:

  - The intel_graphics_stolen() early quirk:
      1) Initializes global struct resource intel_graphics_stolen_res
      2) Adds the region to E820 map as E820_TYPE_RESERVED

  - i915_driver_hw_probe() uses intel_graphics_stolen_res, but this
    happens after the PCI core is initialized, so early quirks
    wouldn't be required for this use.

  - So I guess the E820 map update is what requires the early quirk?
    But I don't know exactly what depends on this update.

I haven't found the connection to the early BIOS/UEFI code and the
initial framebuffer yet.

> And given that this 64bit mmio bar support in windows still requires
> an explicit bios upgrade for the explicit opt in I don't think this
> will change anytime soon.
> 
> We have a similar ugly problem with kvm, since you can't use these
> ranges freely (they're very special in hw), and the kvm maintainers
> are equally annoyed that they have to keep supporting RRMR to block
> that range, just because of intel integrated graphics. Apparently
> windows is again totally fine with this.
> -Daniel
> 
> 
> >
> > > > > ---
> > > > >  arch/x86/kernel/early-quirks.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > > > index a4b5af03dcc1..534cc3f78c6b 100644
> > > > > --- a/arch/x86/kernel/early-quirks.c
> > > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> > > > >       INTEL_CNL_IDS(&gen9_early_ops),
> > > > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > > > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > > > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > > > >       INTEL_RKL_IDS(&gen11_early_ops),
> > > > >  };
> > > >
> > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > > >
> > > > [2]
> > > >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> > > >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> > > >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
> > > >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
> > > >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> > > >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
> > > >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
> > > >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
> > > >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
> > > >   ...

[3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/stopping-a-device-to-rebalance-resources
[4] http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/cpa070_wh06.ppt ("PCI Express In Depth For Windows Vista and Beyond")

Daniel Vetter Nov. 18, 2020, 9:57 p.m. UTC | #7

On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas <[e-mail]> wrote:
>
> On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > >
> > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > >
> > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > > > >
> > > > > I don't plan to do anything with this since previous similar patches
> > > > > have gone through some other tree, so this is just kibitzing.
> > > > >
> > > > > But the fact that we have this long list of Intel devices [1] that
> > > > > constantly needs updates [2] is a hint that something is wrong.
> > > >
> > > > We add an entry for every new integrated graphics platform. Once the
> > > > platform is added, there have not been changes lately.
> > > >
> > > > > IIUC the general idea is that we need to discover Intel gfx memory by
> > > > > looking at device-dependent config space and add it to the E820 map.
> > > > > Apparently the quirks discover this via PCI config registers like
> > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > > > > global "intel_graphics_stolen_res"?
> > > >
> > > > We discover what is called the graphics data stolen memory. It is regular
> > > > system memory range that is not CPU accessible. It is accessible by the
> > > > integrated graphics only.
> > > >
> > > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > >
> > > > > That's not the way this should work.  There should some generic, non
> > > > > device-dependent PCI or ACPI method to discover the memory used, or at
> > > > > least some way to do it in the driver instead of early arch code.
> > > >
> > > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > > > Even if i915 driver is never loaded, the memory ranges still need to
> > > > be fixed. They source of the problem is that the OEM BIOS which are
> > > > not under our control get the programming wrong.
> > > >
> > > > We used to detect the memory region size again at i915 initialization
> > > > but wanted to eliminate the code duplication and resulting subtle bugs
> > > > that caused. Conclusion back then was that storing the struct resource
> > > > in memory is the best trade-off.
> > > >
> > > > > How is this *supposed* to work?  Is there something we can do in E820
> > > > > or other resource management that would make this easier?
> > > >
> > > > The code was added around Haswell (HSW) device generation to mitigate
> > > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> > > > BIOS when things work for Windows. It's only later years when some
> > > > laptop models are intended to be sold with Linux.
> > > >
> > > > The alternative would be to get all the OEM to fix their BIOS for Linux,
> > > > but that is not very realistic given past experiences. So it seems
> > > > a better choice to to add new line per platform generation to make
> > > > sure the users can boot to Linux.
> > >
> > > How does Windows do this?  Do they have to add similar code for each
> > > new platform?
> >
> > Windows is chicken and doesn't move any mmio bar around on its own.
> > Except if the bios explicitly told it somehow (e.g. for the 64bit bar
> > stuff amd recently announced for windows, that linux supports since
> > years by moving the bar). So except if you want to preemptively
> > disable the pci code that does this anytime there's an intel gpu, this
> > is what we have to do.
>
> I think Windows *does* move BARs (they use the more generic
> terminology of "rebalancing PNP resources") in some cases [3,4].  Of
> course, I'm pretty sure Windows will only assign PCI resources inside
> the windows advertised in the host bridge _CRS.
>
> Linux *used* to ignore that host bridge _CRS and could set BARs to
> addresses that appeared available but were in fact used by the
> platform somehow.  But Linux has been paying attention to host bridge
> _CRS for a long time now, so it should also only assign resources
> inside those windows.

If this behaviour is newer than the addition of these quirks then yeah
they're probably not needed anymore, and we can move all this back
into the driver. Do you have the commit when pci core started
observing _CRS on the host bridge?
-Daniel

>
> So I'm missing what the real problem is.  Joonas mentioned BIOS bugs
> above, but I don't know what that means.
>
> Here's what I can figure out so far, tell me if I'm in the weeds:
>
>   - The intel_graphics_stolen() early quirk:
>       1) Initializes global struct resource intel_graphics_stolen_res
>       2) Adds the region to E820 map as E820_TYPE_RESERVED
>
>   - i915_driver_hw_probe() uses intel_graphics_stolen_res, but this
>     happens after the PCI core is initialized, so early quirks
>     wouldn't be required for this use.
>
>   - So I guess the E820 map update is what requires the early quirk?
>     But I don't know exactly what depends on this update.
>
> I haven't found the connection to the early BIOS/UEFI code and the
> initial framebuffer yet.
>
> > And given that this 64bit mmio bar support in windows still requires
> > an explicit bios upgrade for the explicit opt in I don't think this
> > will change anytime soon.
> >
> > We have a similar ugly problem with kvm, since you can't use these
> > ranges freely (they're very special in hw), and the kvm maintainers
> > are equally annoyed that they have to keep supporting RRMR to block
> > that range, just because of intel integrated graphics. Apparently
> > windows is again totally fine with this.
> > -Daniel
> >
> >
> > >
> > > > > > ---
> > > > > >  arch/x86/kernel/early-quirks.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > > > > index a4b5af03dcc1..534cc3f78c6b 100644
> > > > > > --- a/arch/x86/kernel/early-quirks.c
> > > > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > > > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> > > > > >       INTEL_CNL_IDS(&gen9_early_ops),
> > > > > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > > > > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > > > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > > > > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > > > > >       INTEL_RKL_IDS(&gen11_early_ops),
> > > > > >  };
> > > > >
> > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > > > >
> > > > > [2]
> > > > >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> > > > >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> > > > >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
> > > > >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
> > > > >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> > > > >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
> > > > >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
> > > > >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
> > > > >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
> > > > >   ...
>
> [3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/stopping-a-device-to-rebalance-resources
> [4] http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/cpa070_wh06.ppt ("PCI Express In Depth For Windows Vista and Beyond")

Bjorn Helgaas Nov. 18, 2020, 11:14 p.m. UTC | #8

On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas <[e-mail]> wrote:
> > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > >
> > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > >
> > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > > > > >
> > > > > > I don't plan to do anything with this since previous similar patches
> > > > > > have gone through some other tree, so this is just kibitzing.
> > > > > >
> > > > > > But the fact that we have this long list of Intel devices [1] that
> > > > > > constantly needs updates [2] is a hint that something is wrong.
> > > > >
> > > > > We add an entry for every new integrated graphics platform. Once the
> > > > > platform is added, there have not been changes lately.
> > > > >
> > > > > > IIUC the general idea is that we need to discover Intel gfx memory by
> > > > > > looking at device-dependent config space and add it to the E820 map.
> > > > > > Apparently the quirks discover this via PCI config registers like
> > > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > > > > > global "intel_graphics_stolen_res"?
> > > > >
> > > > > We discover what is called the graphics data stolen memory. It is regular
> > > > > system memory range that is not CPU accessible. It is accessible by the
> > > > > integrated graphics only.
> > > > >
> > > > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > >
> > > > > > That's not the way this should work.  There should some generic, non
> > > > > > device-dependent PCI or ACPI method to discover the memory used, or at
> > > > > > least some way to do it in the driver instead of early arch code.
> > > > >
> > > > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > > > > Even if i915 driver is never loaded, the memory ranges still need to
> > > > > be fixed. They source of the problem is that the OEM BIOS which are
> > > > > not under our control get the programming wrong.
> > > > >
> > > > > We used to detect the memory region size again at i915 initialization
> > > > > but wanted to eliminate the code duplication and resulting subtle bugs
> > > > > that caused. Conclusion back then was that storing the struct resource
> > > > > in memory is the best trade-off.
> > > > >
> > > > > > How is this *supposed* to work?  Is there something we can do in E820
> > > > > > or other resource management that would make this easier?
> > > > >
> > > > > The code was added around Haswell (HSW) device generation to mitigate
> > > > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> > > > > BIOS when things work for Windows. It's only later years when some
> > > > > laptop models are intended to be sold with Linux.
> > > > >
> > > > > The alternative would be to get all the OEM to fix their BIOS for Linux,
> > > > > but that is not very realistic given past experiences. So it seems
> > > > > a better choice to to add new line per platform generation to make
> > > > > sure the users can boot to Linux.
> > > >
> > > > How does Windows do this?  Do they have to add similar code for each
> > > > new platform?
> > >
> > > Windows is chicken and doesn't move any mmio bar around on its own.
> > > Except if the bios explicitly told it somehow (e.g. for the 64bit bar
> > > stuff amd recently announced for windows, that linux supports since
> > > years by moving the bar). So except if you want to preemptively
> > > disable the pci code that does this anytime there's an intel gpu, this
> > > is what we have to do.
> >
> > I think Windows *does* move BARs (they use the more generic
> > terminology of "rebalancing PNP resources") in some cases [3,4].  Of
> > course, I'm pretty sure Windows will only assign PCI resources inside
> > the windows advertised in the host bridge _CRS.
> >
> > Linux *used* to ignore that host bridge _CRS and could set BARs to
> > addresses that appeared available but were in fact used by the
> > platform somehow.  But Linux has been paying attention to host bridge
> > _CRS for a long time now, so it should also only assign resources
> > inside those windows.
> 
> If this behaviour is newer than the addition of these quirks then yeah
> they're probably not needed anymore, and we can move all this back
> into the driver. Do you have the commit when pci core started
> observing _CRS on the host bridge?

I think the most relevant commit is this:

  2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by default on 2008 and newer machines")

but the earliest quirk I found is over three years later:

  2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving Intel graphics stolen memory v5")

So there must be something else going on.  814c5f1f52a4 mentions a
couple bug reports.  The dmesg from 66726 [5] shows that we *are*
observing the host bridge _CRS, but Linux just used the BIOS
configuration without changing anything:

  BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
  BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff] reserved
  PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
  ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
  pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
  pci 0000:00:1c.0: PCI bridge to [bus 01]
  pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
  pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
  pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit pref]
  pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
  pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
  [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7f80_0000 - 0x8000_0000]

So the BIOS programmed the 00:1c.0 bridge prefetchable window to
[mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.

On this system, there are no PCI BARs in that range.  01:00.0 looks
like a Ralink RT3090 Wireless 802.11n device that only has a
non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].

I don't know the details of the conflict.  IIUC, Joonas said the
stolen memory is accessible only by the integrated graphics, not by
the CPU.  The bridge window is CPU accessible, of course, and the
[mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the CPU
uses for programmed I/O to BARs below the bridge.

The graphics accesses sound like they would be DMA in the *bus*
address space, which is frequently, but not always, identical to the
CPU address space.

> > So I'm missing what the real problem is.  Joonas mentioned BIOS bugs
> > above, but I don't know what that means.
> >
> > Here's what I can figure out so far, tell me if I'm in the weeds:
> >
> >   - The intel_graphics_stolen() early quirk:
> >       1) Initializes global struct resource intel_graphics_stolen_res
> >       2) Adds the region to E820 map as E820_TYPE_RESERVED
> >
> >   - i915_driver_hw_probe() uses intel_graphics_stolen_res, but this
> >     happens after the PCI core is initialized, so early quirks
> >     wouldn't be required for this use.
> >
> >   - So I guess the E820 map update is what requires the early quirk?
> >     But I don't know exactly what depends on this update.
> >
> > I haven't found the connection to the early BIOS/UEFI code and the
> > initial framebuffer yet.
> >
> > > And given that this 64bit mmio bar support in windows still requires
> > > an explicit bios upgrade for the explicit opt in I don't think this
> > > will change anytime soon.
> > >
> > > We have a similar ugly problem with kvm, since you can't use these
> > > ranges freely (they're very special in hw), and the kvm maintainers
> > > are equally annoyed that they have to keep supporting RRMR to block
> > > that range, just because of intel integrated graphics. Apparently
> > > windows is again totally fine with this.
> > > -Daniel
> > >
> > >
> > > >
> > > > > > > ---
> > > > > > >  arch/x86/kernel/early-quirks.c | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > > > > > index a4b5af03dcc1..534cc3f78c6b 100644
> > > > > > > --- a/arch/x86/kernel/early-quirks.c
> > > > > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > > > > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> > > > > > >       INTEL_CNL_IDS(&gen9_early_ops),
> > > > > > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > > > > > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > > > > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > > > > > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > > > > > >       INTEL_RKL_IDS(&gen11_early_ops),
> > > > > > >  };
> > > > > >
> > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > > > > >
> > > > > > [2]
> > > > > >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> > > > > >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> > > > > >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
> > > > > >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
> > > > > >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> > > > > >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
> > > > > >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
> > > > > >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
> > > > > >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
> > > > > >   ...
> >
> > [3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/stopping-a-device-to-rebalance-resources
> > [4] http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/cpa070_wh06.ppt ("PCI Express In Depth For Windows Vista and Beyond")

[5] https://bugs.freedesktop.org/show_bug.cgi?id=66726

Daniel Vetter Nov. 19, 2020, 9:37 a.m. UTC | #9

On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas <[e-mail]> wrote:
>
> On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas <[e-mail]> wrote:
> > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > >
> > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > >
> > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > > > > > >
> > > > > > > I don't plan to do anything with this since previous similar patches
> > > > > > > have gone through some other tree, so this is just kibitzing.
> > > > > > >
> > > > > > > But the fact that we have this long list of Intel devices [1] that
> > > > > > > constantly needs updates [2] is a hint that something is wrong.
> > > > > >
> > > > > > We add an entry for every new integrated graphics platform. Once the
> > > > > > platform is added, there have not been changes lately.
> > > > > >
> > > > > > > IIUC the general idea is that we need to discover Intel gfx memory by
> > > > > > > looking at device-dependent config space and add it to the E820 map.
> > > > > > > Apparently the quirks discover this via PCI config registers like
> > > > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > > > > > > global "intel_graphics_stolen_res"?
> > > > > >
> > > > > > We discover what is called the graphics data stolen memory. It is regular
> > > > > > system memory range that is not CPU accessible. It is accessible by the
> > > > > > integrated graphics only.
> > > > > >
> > > > > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > >
> > > > > > > That's not the way this should work.  There should some generic, non
> > > > > > > device-dependent PCI or ACPI method to discover the memory used, or at
> > > > > > > least some way to do it in the driver instead of early arch code.
> > > > > >
> > > > > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > > > > > Even if i915 driver is never loaded, the memory ranges still need to
> > > > > > be fixed. They source of the problem is that the OEM BIOS which are
> > > > > > not under our control get the programming wrong.
> > > > > >
> > > > > > We used to detect the memory region size again at i915 initialization
> > > > > > but wanted to eliminate the code duplication and resulting subtle bugs
> > > > > > that caused. Conclusion back then was that storing the struct resource
> > > > > > in memory is the best trade-off.
> > > > > >
> > > > > > > How is this *supposed* to work?  Is there something we can do in E820
> > > > > > > or other resource management that would make this easier?
> > > > > >
> > > > > > The code was added around Haswell (HSW) device generation to mitigate
> > > > > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> > > > > > BIOS when things work for Windows. It's only later years when some
> > > > > > laptop models are intended to be sold with Linux.
> > > > > >
> > > > > > The alternative would be to get all the OEM to fix their BIOS for Linux,
> > > > > > but that is not very realistic given past experiences. So it seems
> > > > > > a better choice to to add new line per platform generation to make
> > > > > > sure the users can boot to Linux.
> > > > >
> > > > > How does Windows do this?  Do they have to add similar code for each
> > > > > new platform?
> > > >
> > > > Windows is chicken and doesn't move any mmio bar around on its own.
> > > > Except if the bios explicitly told it somehow (e.g. for the 64bit bar
> > > > stuff amd recently announced for windows, that linux supports since
> > > > years by moving the bar). So except if you want to preemptively
> > > > disable the pci code that does this anytime there's an intel gpu, this
> > > > is what we have to do.
> > >
> > > I think Windows *does* move BARs (they use the more generic
> > > terminology of "rebalancing PNP resources") in some cases [3,4].  Of
> > > course, I'm pretty sure Windows will only assign PCI resources inside
> > > the windows advertised in the host bridge _CRS.
> > >
> > > Linux *used* to ignore that host bridge _CRS and could set BARs to
> > > addresses that appeared available but were in fact used by the
> > > platform somehow.  But Linux has been paying attention to host bridge
> > > _CRS for a long time now, so it should also only assign resources
> > > inside those windows.
> >
> > If this behaviour is newer than the addition of these quirks then yeah
> > they're probably not needed anymore, and we can move all this back
> > into the driver. Do you have the commit when pci core started
> > observing _CRS on the host bridge?
>
> I think the most relevant commit is this:
>
>   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by default on 2008 and newer machines")
>
> but the earliest quirk I found is over three years later:
>
>   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving Intel graphics stolen memory v5")
>
> So there must be something else going on.  814c5f1f52a4 mentions a
> couple bug reports.  The dmesg from 66726 [5] shows that we *are*
> observing the host bridge _CRS, but Linux just used the BIOS
> configuration without changing anything:
>
>   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
>   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff] reserved
>   PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
>   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
>   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
>   pci 0000:00:1c.0: PCI bridge to [bus 01]
>   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
>   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
>   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit pref]
>   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
>   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
>   [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7f80_0000 - 0x8000_0000]
>
> So the BIOS programmed the 00:1c.0 bridge prefetchable window to
> [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
>
> On this system, there are no PCI BARs in that range.  01:00.0 looks
> like a Ralink RT3090 Wireless 802.11n device that only has a
> non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
>
> I don't know the details of the conflict.  IIUC, Joonas said the
> stolen memory is accessible only by the integrated graphics, not by
> the CPU.  The bridge window is CPU accessible, of course, and the
> [mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the CPU
> uses for programmed I/O to BARs below the bridge.
>
> The graphics accesses sound like they would be DMA in the *bus*
> address space, which is frequently, but not always, identical to the
> CPU address space.

So apparently on some platforms the conflict is harmless because the
BIOS puts BARs and stuff over it from boot-up, and things work:
0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts on
gen3") But we also had conflict reports on other machines.

GPU does all its access with CPU address space (after the iommu, which
is entirely integrated). So I'm not sure whether we've seen something
go boom or whether reserving that resource was just precaution in
eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
conflict"), it's all a bit way back in history.

So really not sure what to do here or what the risks are.
-Daniel


>
> > > So I'm missing what the real problem is.  Joonas mentioned BIOS bugs
> > > above, but I don't know what that means.
> > >
> > > Here's what I can figure out so far, tell me if I'm in the weeds:
> > >
> > >   - The intel_graphics_stolen() early quirk:
> > >       1) Initializes global struct resource intel_graphics_stolen_res
> > >       2) Adds the region to E820 map as E820_TYPE_RESERVED
> > >
> > >   - i915_driver_hw_probe() uses intel_graphics_stolen_res, but this
> > >     happens after the PCI core is initialized, so early quirks
> > >     wouldn't be required for this use.
> > >
> > >   - So I guess the E820 map update is what requires the early quirk?
> > >     But I don't know exactly what depends on this update.
> > >
> > > I haven't found the connection to the early BIOS/UEFI code and the
> > > initial framebuffer yet.
> > >
> > > > And given that this 64bit mmio bar support in windows still requires
> > > > an explicit bios upgrade for the explicit opt in I don't think this
> > > > will change anytime soon.
> > > >
> > > > We have a similar ugly problem with kvm, since you can't use these
> > > > ranges freely (they're very special in hw), and the kvm maintainers
> > > > are equally annoyed that they have to keep supporting RRMR to block
> > > > that range, just because of intel integrated graphics. Apparently
> > > > windows is again totally fine with this.
> > > > -Daniel
> > > >
> > > >
> > > > >
> > > > > > > > ---
> > > > > > > >  arch/x86/kernel/early-quirks.c | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > > > > > > index a4b5af03dcc1..534cc3f78c6b 100644
> > > > > > > > --- a/arch/x86/kernel/early-quirks.c
> > > > > > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > > > > > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> > > > > > > >       INTEL_CNL_IDS(&gen9_early_ops),
> > > > > > > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > > > > > > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > > > > > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > > > > > > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > > > > > > >       INTEL_RKL_IDS(&gen11_early_ops),
> > > > > > > >  };
> > > > > > >
> > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > > > > > >
> > > > > > > [2]
> > > > > > >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> > > > > > >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> > > > > > >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
> > > > > > >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
> > > > > > >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> > > > > > >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
> > > > > > >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
> > > > > > >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
> > > > > > >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
> > > > > > >   ...
> > >
> > > [3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/stopping-a-device-to-rebalance-resources
> > > [4] http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/cpa070_wh06.ppt ("PCI Express In Depth For Windows Vista and Beyond")
>
> [5] https://bugs.freedesktop.org/show_bug.cgi?id=66726

Bjorn Helgaas Nov. 19, 2020, 7:19 p.m. UTC | #10

[+cc Jesse]

On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas <[e-mail]> wrote:
> > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas <[e-mail]> wrote:
> > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > >
> > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > >
> > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > > > > > > >
> > > > > > > > I don't plan to do anything with this since previous similar patches
> > > > > > > > have gone through some other tree, so this is just kibitzing.
> > > > > > > >
> > > > > > > > But the fact that we have this long list of Intel devices [1] that
> > > > > > > > constantly needs updates [2] is a hint that something is wrong.
> > > > > > >
> > > > > > > We add an entry for every new integrated graphics platform. Once the
> > > > > > > platform is added, there have not been changes lately.
> > > > > > >
> > > > > > > > IIUC the general idea is that we need to discover Intel gfx memory by
> > > > > > > > looking at device-dependent config space and add it to the E820 map.
> > > > > > > > Apparently the quirks discover this via PCI config registers like
> > > > > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > > > > > > > global "intel_graphics_stolen_res"?
> > > > > > >
> > > > > > > We discover what is called the graphics data stolen memory. It is regular
> > > > > > > system memory range that is not CPU accessible. It is accessible by the
> > > > > > > integrated graphics only.
> > > > > > >
> > > > > > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > >
> > > > > > > > That's not the way this should work.  There should some generic, non
> > > > > > > > device-dependent PCI or ACPI method to discover the memory used, or at
> > > > > > > > least some way to do it in the driver instead of early arch code.
> > > > > > >
> > > > > > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > > > > > > Even if i915 driver is never loaded, the memory ranges still need to
> > > > > > > be fixed. They source of the problem is that the OEM BIOS which are
> > > > > > > not under our control get the programming wrong.
> > > > > > >
> > > > > > > We used to detect the memory region size again at i915 initialization
> > > > > > > but wanted to eliminate the code duplication and resulting subtle bugs
> > > > > > > that caused. Conclusion back then was that storing the struct resource
> > > > > > > in memory is the best trade-off.
> > > > > > >
> > > > > > > > How is this *supposed* to work?  Is there something we can do in E820
> > > > > > > > or other resource management that would make this easier?
> > > > > > >
> > > > > > > The code was added around Haswell (HSW) device generation to mitigate
> > > > > > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> > > > > > > BIOS when things work for Windows. It's only later years when some
> > > > > > > laptop models are intended to be sold with Linux.
> > > > > > >
> > > > > > > The alternative would be to get all the OEM to fix their BIOS for Linux,
> > > > > > > but that is not very realistic given past experiences. So it seems
> > > > > > > a better choice to to add new line per platform generation to make
> > > > > > > sure the users can boot to Linux.
> > > > > >
> > > > > > How does Windows do this?  Do they have to add similar code for each
> > > > > > new platform?
> > > > >
> > > > > Windows is chicken and doesn't move any mmio bar around on its own.
> > > > > Except if the bios explicitly told it somehow (e.g. for the 64bit bar
> > > > > stuff amd recently announced for windows, that linux supports since
> > > > > years by moving the bar). So except if you want to preemptively
> > > > > disable the pci code that does this anytime there's an intel gpu, this
> > > > > is what we have to do.
> > > >
> > > > I think Windows *does* move BARs (they use the more generic
> > > > terminology of "rebalancing PNP resources") in some cases [3,4].  Of
> > > > course, I'm pretty sure Windows will only assign PCI resources inside
> > > > the windows advertised in the host bridge _CRS.
> > > >
> > > > Linux *used* to ignore that host bridge _CRS and could set BARs to
> > > > addresses that appeared available but were in fact used by the
> > > > platform somehow.  But Linux has been paying attention to host bridge
> > > > _CRS for a long time now, so it should also only assign resources
> > > > inside those windows.
> > >
> > > If this behaviour is newer than the addition of these quirks then yeah
> > > they're probably not needed anymore, and we can move all this back
> > > into the driver. Do you have the commit when pci core started
> > > observing _CRS on the host bridge?
> >
> > I think the most relevant commit is this:
> >
> >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by default on 2008 and newer machines")
> >
> > but the earliest quirk I found is over three years later:
> >
> >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving Intel graphics stolen memory v5")
> >
> > So there must be something else going on.  814c5f1f52a4 mentions a
> > couple bug reports.  The dmesg from 66726 [5] shows that we *are*
> > observing the host bridge _CRS, but Linux just used the BIOS
> > configuration without changing anything:
> >
> >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
> >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff] reserved
> >   PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
> >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit pref]
> >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7f80_0000 - 0x8000_0000]
> >
> > So the BIOS programmed the 00:1c.0 bridge prefetchable window to
> > [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> >
> > On this system, there are no PCI BARs in that range.  01:00.0 looks
> > like a Ralink RT3090 Wireless 802.11n device that only has a
> > non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> >
> > I don't know the details of the conflict.  IIUC, Joonas said the
> > stolen memory is accessible only by the integrated graphics, not by
> > the CPU.  The bridge window is CPU accessible, of course, and the
> > [mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the CPU
> > uses for programmed I/O to BARs below the bridge.
> >
> > The graphics accesses sound like they would be DMA in the *bus*
> > address space, which is frequently, but not always, identical to the
> > CPU address space.
> 
> So apparently on some platforms the conflict is harmless because the
> BIOS puts BARs and stuff over it from boot-up, and things work:
> 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts on
> gen3") But we also had conflict reports on other machines.

The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk for
reserving Intel graphics stolen memory v5") and 0b6d24c01932
("drm/i915: Don't complain about stolen conflicts on gen3") seem to be
basically complaints about the *message*, not anything that's actually
broken.  

Jesse's comment [6]:

  Given the decode priority on our GMCHs, it's fine if the regions
  overlap.  However it doesn't look like there's a nice way to detect
  it.  In this case, part of the range occupied by the stolen space is
  simply "reserved" per the E820, but the rest of it is under the bus
  0 range (which kind of makes sense too).

sounds relevant but I don't know enough to interpret it.  I added
Jesse in case he wants to comment.

> GPU does all its access with CPU address space (after the iommu, which
> is entirely integrated). So I'm not sure whether we've seen something
> go boom or whether reserving that resource was just precaution in
> eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> conflict"), it's all a bit way back in history.
> 
> So really not sure what to do here or what the risks are.

I'm not either.  Seems like we're not really converging on anything
useful we can do at this point.  The only thing I can think of would
be to collect data about actual failures (not just warning messages).
That might lead to something we could improve in the future.

> > > > So I'm missing what the real problem is.  Joonas mentioned BIOS bugs
> > > > above, but I don't know what that means.
> > > >
> > > > Here's what I can figure out so far, tell me if I'm in the weeds:
> > > >
> > > >   - The intel_graphics_stolen() early quirk:
> > > >       1) Initializes global struct resource intel_graphics_stolen_res
> > > >       2) Adds the region to E820 map as E820_TYPE_RESERVED
> > > >
> > > >   - i915_driver_hw_probe() uses intel_graphics_stolen_res, but this
> > > >     happens after the PCI core is initialized, so early quirks
> > > >     wouldn't be required for this use.
> > > >
> > > >   - So I guess the E820 map update is what requires the early quirk?
> > > >     But I don't know exactly what depends on this update.
> > > >
> > > > I haven't found the connection to the early BIOS/UEFI code and the
> > > > initial framebuffer yet.
> > > >
> > > > > And given that this 64bit mmio bar support in windows still requires
> > > > > an explicit bios upgrade for the explicit opt in I don't think this
> > > > > will change anytime soon.
> > > > >
> > > > > We have a similar ugly problem with kvm, since you can't use these
> > > > > ranges freely (they're very special in hw), and the kvm maintainers
> > > > > are equally annoyed that they have to keep supporting RRMR to block
> > > > > that range, just because of intel integrated graphics. Apparently
> > > > > windows is again totally fine with this.
> > > > > -Daniel
> > > > >
> > > > >
> > > > > >
> > > > > > > > > ---
> > > > > > > > >  arch/x86/kernel/early-quirks.c | 1 +
> > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > > > > > > > index a4b5af03dcc1..534cc3f78c6b 100644
> > > > > > > > > --- a/arch/x86/kernel/early-quirks.c
> > > > > > > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > > > > > > @@ -549,6 +549,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
> > > > > > > > >       INTEL_CNL_IDS(&gen9_early_ops),
> > > > > > > > >       INTEL_ICL_11_IDS(&gen11_early_ops),
> > > > > > > > >       INTEL_EHL_IDS(&gen11_early_ops),
> > > > > > > > > +     INTEL_JSL_IDS(&gen11_early_ops),
> > > > > > > > >       INTEL_TGL_12_IDS(&gen11_early_ops),
> > > > > > > > >       INTEL_RKL_IDS(&gen11_early_ops),
> > > > > > > > >  };
> > > > > > > >
> > > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v5.10-rc2#n518
> > > > > > > >
> > > > > > > > [2]
> > > > > > > >   May 2020 efbee021ad02 ("x86/gpu: add RKL stolen memory support")
> > > > > > > >   Jul 2019 6b2436aeb945 ("x86/gpu: add TGL stolen memory support")
> > > > > > > >   Mar 2019 d53fef0be4a5 ("x86/gpu: add ElkhartLake to gen11 early quirks")
> > > > > > > >   May 2018 db0c8d8b031d ("x86/gpu: reserve ICL's graphics stolen memory")
> > > > > > > >   Dec 2017 33aa69ed8aac ("x86/gpu: add CFL to early quirks")
> > > > > > > >   Jul 2017 2e1e9d48939e ("x86/gpu: CNL uses the same GMS values as SKL")
> > > > > > > >   Jan 2017 bc384c77e3bb ("x86/gpu: GLK uses the same GMS values as SKL")
> > > > > > > >   Oct 2015 00ce5c8a66fb ("drm/i915/kbl: Kabylake uses the same GMS values as Skylake")
> > > > > > > >   Mar 2015 31d4dcf705c3 ("drm/i915/bxt: Broxton uses the same GMS values as Skylake")
> > > > > > > >   ...
> > > >
> > > > [3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/stopping-a-device-to-rebalance-resources
> > > > [4] http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/cpa070_wh06.ppt ("PCI Express In Depth For Windows Vista and Beyond")
> >
> > [5] https://bugs.freedesktop.org/show_bug.cgi?id=66726

[6] https://bugs.freedesktop.org/show_bug.cgi?id=76983#c11

Jesse Barnes Nov. 19, 2020, 10:01 p.m. UTC | #11

On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]> wrote:
>
> [+cc Jesse]
>
> On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas <[e-mail]> wrote:
> > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas <[e-mail]> wrote:
> > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > >
> > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > >
> > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> > > > > > > > >
> > > > > > > > > I don't plan to do anything with this since previous similar patches
> > > > > > > > > have gone through some other tree, so this is just kibitzing.
> > > > > > > > >
> > > > > > > > > But the fact that we have this long list of Intel devices [1] that
> > > > > > > > > constantly needs updates [2] is a hint that something is wrong.
> > > > > > > >
> > > > > > > > We add an entry for every new integrated graphics platform. Once the
> > > > > > > > platform is added, there have not been changes lately.
> > > > > > > >
> > > > > > > > > IIUC the general idea is that we need to discover Intel gfx memory by
> > > > > > > > > looking at device-dependent config space and add it to the E820 map.
> > > > > > > > > Apparently the quirks discover this via PCI config registers like
> > > > > > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> > > > > > > > > global "intel_graphics_stolen_res"?
> > > > > > > >
> > > > > > > > We discover what is called the graphics data stolen memory. It is regular
> > > > > > > > system memory range that is not CPU accessible. It is accessible by the
> > > > > > > > integrated graphics only.
> > > > > > > >
> > > > > > > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > >
> > > > > > > > > That's not the way this should work.  There should some generic, non
> > > > > > > > > device-dependent PCI or ACPI method to discover the memory used, or at
> > > > > > > > > least some way to do it in the driver instead of early arch code.
> > > > > > > >
> > > > > > > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> > > > > > > > Even if i915 driver is never loaded, the memory ranges still need to
> > > > > > > > be fixed. They source of the problem is that the OEM BIOS which are
> > > > > > > > not under our control get the programming wrong.
> > > > > > > >
> > > > > > > > We used to detect the memory region size again at i915 initialization
> > > > > > > > but wanted to eliminate the code duplication and resulting subtle bugs
> > > > > > > > that caused. Conclusion back then was that storing the struct resource
> > > > > > > > in memory is the best trade-off.
> > > > > > > >
> > > > > > > > > How is this *supposed* to work?  Is there something we can do in E820
> > > > > > > > > or other resource management that would make this easier?
> > > > > > > >
> > > > > > > > The code was added around Haswell (HSW) device generation to mitigate
> > > > > > > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> > > > > > > > BIOS when things work for Windows. It's only later years when some
> > > > > > > > laptop models are intended to be sold with Linux.
> > > > > > > >
> > > > > > > > The alternative would be to get all the OEM to fix their BIOS for Linux,
> > > > > > > > but that is not very realistic given past experiences. So it seems
> > > > > > > > a better choice to to add new line per platform generation to make
> > > > > > > > sure the users can boot to Linux.
> > > > > > >
> > > > > > > How does Windows do this?  Do they have to add similar code for each
> > > > > > > new platform?
> > > > > >
> > > > > > Windows is chicken and doesn't move any mmio bar around on its own.
> > > > > > Except if the bios explicitly told it somehow (e.g. for the 64bit bar
> > > > > > stuff amd recently announced for windows, that linux supports since
> > > > > > years by moving the bar). So except if you want to preemptively
> > > > > > disable the pci code that does this anytime there's an intel gpu, this
> > > > > > is what we have to do.
> > > > >
> > > > > I think Windows *does* move BARs (they use the more generic
> > > > > terminology of "rebalancing PNP resources") in some cases [3,4].  Of
> > > > > course, I'm pretty sure Windows will only assign PCI resources inside
> > > > > the windows advertised in the host bridge _CRS.
> > > > >
> > > > > Linux *used* to ignore that host bridge _CRS and could set BARs to
> > > > > addresses that appeared available but were in fact used by the
> > > > > platform somehow.  But Linux has been paying attention to host bridge
> > > > > _CRS for a long time now, so it should also only assign resources
> > > > > inside those windows.
> > > >
> > > > If this behaviour is newer than the addition of these quirks then yeah
> > > > they're probably not needed anymore, and we can move all this back
> > > > into the driver. Do you have the commit when pci core started
> > > > observing _CRS on the host bridge?
> > >
> > > I think the most relevant commit is this:
> > >
> > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by default on 2008 and newer machines")
> > >
> > > but the earliest quirk I found is over three years later:
> > >
> > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving Intel graphics stolen memory v5")
> > >
> > > So there must be something else going on.  814c5f1f52a4 mentions a
> > > couple bug reports.  The dmesg from 66726 [5] shows that we *are*
> > > observing the host bridge _CRS, but Linux just used the BIOS
> > > configuration without changing anything:
> > >
> > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
> > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff] reserved
> > >   PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
> > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit pref]
> > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7f80_0000 - 0x8000_0000]
> > >
> > > So the BIOS programmed the 00:1c.0 bridge prefetchable window to
> > > [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> > >
> > > On this system, there are no PCI BARs in that range.  01:00.0 looks
> > > like a Ralink RT3090 Wireless 802.11n device that only has a
> > > non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> > >
> > > I don't know the details of the conflict.  IIUC, Joonas said the
> > > stolen memory is accessible only by the integrated graphics, not by
> > > the CPU.  The bridge window is CPU accessible, of course, and the
> > > [mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the CPU
> > > uses for programmed I/O to BARs below the bridge.
> > >
> > > The graphics accesses sound like they would be DMA in the *bus*
> > > address space, which is frequently, but not always, identical to the
> > > CPU address space.
> >
> > So apparently on some platforms the conflict is harmless because the
> > BIOS puts BARs and stuff over it from boot-up, and things work:
> > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts on
> > gen3") But we also had conflict reports on other machines.
>
> The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk for
> reserving Intel graphics stolen memory v5") and 0b6d24c01932
> ("drm/i915: Don't complain about stolen conflicts on gen3") seem to be
> basically complaints about the *message*, not anything that's actually
> broken.
>
> Jesse's comment [6]:
>
>   Given the decode priority on our GMCHs, it's fine if the regions
>   overlap.  However it doesn't look like there's a nice way to detect
>   it.  In this case, part of the range occupied by the stolen space is
>   simply "reserved" per the E820, but the rest of it is under the bus
>   0 range (which kind of makes sense too).
>
> sounds relevant but I don't know enough to interpret it.  I added
> Jesse in case he wants to comment.
>
> > GPU does all its access with CPU address space (after the iommu, which
> > is entirely integrated). So I'm not sure whether we've seen something
> > go boom or whether reserving that resource was just precaution in
> > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> > conflict"), it's all a bit way back in history.
> >
> > So really not sure what to do here or what the risks are.
>
> I'm not either.  Seems like we're not really converging on anything
> useful we can do at this point.  The only thing I can think of would
> be to collect data about actual failures (not just warning messages).
> That might lead to something we could improve in the future.

I don't have any brilliant ideas here unfortunately.  Maybe it's worth
talking to some of the Windows folks internally to see how these
ranges are handled these days and matching it?  Historically this has
been an area fraught with danger because getting things wrong can lead
to corruption of various kinds or boot hangs.

Jesse

Tejas Upadhyay Nov. 30, 2020, 10:44 a.m. UTC | #12

Hi All,

Are we merging this patch in?

Thanks,
Tejas

> -----Original Message-----
> From: Jesse Barnes <[e-mail]>
> Sent: 20 November 2020 03:32
> To: Bjorn Helgaas <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Surendrakumar Upadhyay, TejaskumarX
> <[e-mail]>; Linux PCI <linux-
> [e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> Matthew D <[e-mail]>; Pandey, Hariom
> <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> 
> On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]>
> wrote:
> >
> > [+cc Jesse]
> >
> > On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas <[e-mail]>
> wrote:
> > > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas
> <[e-mail]> wrote:
> > > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas
> <[e-mail]> wrote:
> > > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen
> wrote:
> > > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > > >
> > > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay
> wrote:
> > > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > > >
> > > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > > > > > > <[e-mail]>
> > > > > > > > > >
> > > > > > > > > > I don't plan to do anything with this since previous
> > > > > > > > > > similar patches have gone through some other tree, so this is
> just kibitzing.
> > > > > > > > > >
> > > > > > > > > > But the fact that we have this long list of Intel
> > > > > > > > > > devices [1] that constantly needs updates [2] is a hint that
> something is wrong.
> > > > > > > > >
> > > > > > > > > We add an entry for every new integrated graphics
> > > > > > > > > platform. Once the platform is added, there have not been
> changes lately.
> > > > > > > > >
> > > > > > > > > > IIUC the general idea is that we need to discover
> > > > > > > > > > Intel gfx memory by looking at device-dependent config
> space and add it to the E820 map.
> > > > > > > > > > Apparently the quirks discover this via PCI config
> > > > > > > > > > registers like I830_ESMRAMC, I845_ESMRAMC, etc, and
> > > > > > > > > > tell the driver about it via the global
> "intel_graphics_stolen_res"?
> > > > > > > > >
> > > > > > > > > We discover what is called the graphics data stolen
> > > > > > > > > memory. It is regular system memory range that is not
> > > > > > > > > CPU accessible. It is accessible by the integrated graphics only.
> > > > > > > > >
> > > > > > > > > See:
> > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds
> > > > > > > > > /linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10
> > > > > > > > > -rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > > >
> > > > > > > > > > That's not the way this should work.  There should
> > > > > > > > > > some generic, non device-dependent PCI or ACPI method
> > > > > > > > > > to discover the memory used, or at least some way to do it in
> the driver instead of early arch code.
> > > > > > > > >
> > > > > > > > > It's used by the early BIOS/UEFI code to set up initial
> framebuffer.
> > > > > > > > > Even if i915 driver is never loaded, the memory ranges
> > > > > > > > > still need to be fixed. They source of the problem is
> > > > > > > > > that the OEM BIOS which are not under our control get the
> programming wrong.
> > > > > > > > >
> > > > > > > > > We used to detect the memory region size again at i915
> > > > > > > > > initialization but wanted to eliminate the code
> > > > > > > > > duplication and resulting subtle bugs that caused.
> > > > > > > > > Conclusion back then was that storing the struct resource in
> memory is the best trade-off.
> > > > > > > > >
> > > > > > > > > > How is this *supposed* to work?  Is there something we
> > > > > > > > > > can do in E820 or other resource management that would
> make this easier?
> > > > > > > > >
> > > > > > > > > The code was added around Haswell (HSW) device
> > > > > > > > > generation to mitigate bugs in BIOS. It is traditionally
> > > > > > > > > hard to get all OEMs to fix their BIOS when things work
> > > > > > > > > for Windows. It's only later years when some laptop models
> are intended to be sold with Linux.
> > > > > > > > >
> > > > > > > > > The alternative would be to get all the OEM to fix their
> > > > > > > > > BIOS for Linux, but that is not very realistic given
> > > > > > > > > past experiences. So it seems a better choice to to add
> > > > > > > > > new line per platform generation to make sure the users can
> boot to Linux.
> > > > > > > >
> > > > > > > > How does Windows do this?  Do they have to add similar
> > > > > > > > code for each new platform?
> > > > > > >
> > > > > > > Windows is chicken and doesn't move any mmio bar around on its
> own.
> > > > > > > Except if the bios explicitly told it somehow (e.g. for the
> > > > > > > 64bit bar stuff amd recently announced for windows, that
> > > > > > > linux supports since years by moving the bar). So except if
> > > > > > > you want to preemptively disable the pci code that does this
> > > > > > > anytime there's an intel gpu, this is what we have to do.
> > > > > >
> > > > > > I think Windows *does* move BARs (they use the more generic
> > > > > > terminology of "rebalancing PNP resources") in some cases
> > > > > > [3,4].  Of course, I'm pretty sure Windows will only assign
> > > > > > PCI resources inside the windows advertised in the host bridge
> _CRS.
> > > > > >
> > > > > > Linux *used* to ignore that host bridge _CRS and could set
> > > > > > BARs to addresses that appeared available but were in fact
> > > > > > used by the platform somehow.  But Linux has been paying
> > > > > > attention to host bridge _CRS for a long time now, so it
> > > > > > should also only assign resources inside those windows.
> > > > >
> > > > > If this behaviour is newer than the addition of these quirks
> > > > > then yeah they're probably not needed anymore, and we can move
> > > > > all this back into the driver. Do you have the commit when pci
> > > > > core started observing _CRS on the host bridge?
> > > >
> > > > I think the most relevant commit is this:
> > > >
> > > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by
> > > > default on 2008 and newer machines")
> > > >
> > > > but the earliest quirk I found is over three years later:
> > > >
> > > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving
> > > > Intel graphics stolen memory v5")
> > > >
> > > > So there must be something else going on.  814c5f1f52a4 mentions a
> > > > couple bug reports.  The dmesg from 66726 [5] shows that we *are*
> > > > observing the host bridge _CRS, but Linux just used the BIOS
> > > > configuration without changing anything:
> > > >
> > > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
> > > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff]
> reserved
> > > >   PCI: Using host bridge windows from ACPI; if necessary, use
> "pci=nocrs" and report a bug
> > > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> > > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> > > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> > > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> > > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> > > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit
> pref]
> > > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> > > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> > > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with
> > > > stolen region: [0x7f80_0000 - 0x8000_0000]
> > > >
> > > > So the BIOS programmed the 00:1c.0 bridge prefetchable window to
> > > > [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> > > >
> > > > On this system, there are no PCI BARs in that range.  01:00.0
> > > > looks like a Ralink RT3090 Wireless 802.11n device that only has a
> > > > non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> > > >
> > > > I don't know the details of the conflict.  IIUC, Joonas said the
> > > > stolen memory is accessible only by the integrated graphics, not
> > > > by the CPU.  The bridge window is CPU accessible, of course, and
> > > > the [mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the
> > > > CPU uses for programmed I/O to BARs below the bridge.
> > > >
> > > > The graphics accesses sound like they would be DMA in the *bus*
> > > > address space, which is frequently, but not always, identical to
> > > > the CPU address space.
> > >
> > > So apparently on some platforms the conflict is harmless because the
> > > BIOS puts BARs and stuff over it from boot-up, and things work:
> > > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts on
> > > gen3") But we also had conflict reports on other machines.
> >
> > The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk for
> > reserving Intel graphics stolen memory v5") and 0b6d24c01932
> > ("drm/i915: Don't complain about stolen conflicts on gen3") seem to be
> > basically complaints about the *message*, not anything that's actually
> > broken.
> >
> > Jesse's comment [6]:
> >
> >   Given the decode priority on our GMCHs, it's fine if the regions
> >   overlap.  However it doesn't look like there's a nice way to detect
> >   it.  In this case, part of the range occupied by the stolen space is
> >   simply "reserved" per the E820, but the rest of it is under the bus
> >   0 range (which kind of makes sense too).
> >
> > sounds relevant but I don't know enough to interpret it.  I added
> > Jesse in case he wants to comment.
> >
> > > GPU does all its access with CPU address space (after the iommu,
> > > which is entirely integrated). So I'm not sure whether we've seen
> > > something go boom or whether reserving that resource was just
> > > precaution in
> > > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> > > conflict"), it's all a bit way back in history.
> > >
> > > So really not sure what to do here or what the risks are.
> >
> > I'm not either.  Seems like we're not really converging on anything
> > useful we can do at this point.  The only thing I can think of would
> > be to collect data about actual failures (not just warning messages).
> > That might lead to something we could improve in the future.
> 
> I don't have any brilliant ideas here unfortunately.  Maybe it's worth talking
> to some of the Windows folks internally to see how these ranges are handled
> these days and matching it?  Historically this has been an area fraught with
> danger because getting things wrong can lead to corruption of various kinds
> or boot hangs.
> 
> Jesse

Bjorn Helgaas Nov. 30, 2020, 4:51 p.m. UTC | #13

On Mon, Nov 30, 2020 at 10:44:14AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> Hi All,
> 
> Are we merging this patch in?

Does it fix something?  If something is broken without this patch, can
we collect information about exactly what is broken and how it fails?

But I don't object if somebody else wants to apply this.

> > -----Original Message-----
> > From: Jesse Barnes <[e-mail]>
> > Sent: 20 November 2020 03:32
> > To: Bjorn Helgaas <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>; Joonas Lahtinen
> > <[e-mail]>; Surendrakumar Upadhyay, TejaskumarX
> > <[e-mail]>; Linux PCI <linux-
> > [e-mail]>; Linux Kernel Mailing List <linux-
> > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > Matthew D <[e-mail]>; Pandey, Hariom
> > <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> > Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > 
> > On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]>
> > wrote:
> > >
> > > [+cc Jesse]
> > >
> > > On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas <[e-mail]>
> > wrote:
> > > > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas
> > <[e-mail]> wrote:
> > > > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas
> > <[e-mail]> wrote:
> > > > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen
> > wrote:
> > > > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay
> > wrote:
> > > > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > > > >
> > > > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > >
> > > > > > > > > > > I don't plan to do anything with this since previous
> > > > > > > > > > > similar patches have gone through some other tree, so this is
> > just kibitzing.
> > > > > > > > > > >
> > > > > > > > > > > But the fact that we have this long list of Intel
> > > > > > > > > > > devices [1] that constantly needs updates [2] is a hint that
> > something is wrong.
> > > > > > > > > >
> > > > > > > > > > We add an entry for every new integrated graphics
> > > > > > > > > > platform. Once the platform is added, there have not been
> > changes lately.
> > > > > > > > > >
> > > > > > > > > > > IIUC the general idea is that we need to discover
> > > > > > > > > > > Intel gfx memory by looking at device-dependent config
> > space and add it to the E820 map.
> > > > > > > > > > > Apparently the quirks discover this via PCI config
> > > > > > > > > > > registers like I830_ESMRAMC, I845_ESMRAMC, etc, and
> > > > > > > > > > > tell the driver about it via the global
> > "intel_graphics_stolen_res"?
> > > > > > > > > >
> > > > > > > > > > We discover what is called the graphics data stolen
> > > > > > > > > > memory. It is regular system memory range that is not
> > > > > > > > > > CPU accessible. It is accessible by the integrated graphics only.
> > > > > > > > > >
> > > > > > > > > > See:
> > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds
> > > > > > > > > > /linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10
> > > > > > > > > > -rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > > > >
> > > > > > > > > > > That's not the way this should work.  There should
> > > > > > > > > > > some generic, non device-dependent PCI or ACPI method
> > > > > > > > > > > to discover the memory used, or at least some way to do it in
> > the driver instead of early arch code.
> > > > > > > > > >
> > > > > > > > > > It's used by the early BIOS/UEFI code to set up initial
> > framebuffer.
> > > > > > > > > > Even if i915 driver is never loaded, the memory ranges
> > > > > > > > > > still need to be fixed. They source of the problem is
> > > > > > > > > > that the OEM BIOS which are not under our control get the
> > programming wrong.
> > > > > > > > > >
> > > > > > > > > > We used to detect the memory region size again at i915
> > > > > > > > > > initialization but wanted to eliminate the code
> > > > > > > > > > duplication and resulting subtle bugs that caused.
> > > > > > > > > > Conclusion back then was that storing the struct resource in
> > memory is the best trade-off.
> > > > > > > > > >
> > > > > > > > > > > How is this *supposed* to work?  Is there something we
> > > > > > > > > > > can do in E820 or other resource management that would
> > make this easier?
> > > > > > > > > >
> > > > > > > > > > The code was added around Haswell (HSW) device
> > > > > > > > > > generation to mitigate bugs in BIOS. It is traditionally
> > > > > > > > > > hard to get all OEMs to fix their BIOS when things work
> > > > > > > > > > for Windows. It's only later years when some laptop models
> > are intended to be sold with Linux.
> > > > > > > > > >
> > > > > > > > > > The alternative would be to get all the OEM to fix their
> > > > > > > > > > BIOS for Linux, but that is not very realistic given
> > > > > > > > > > past experiences. So it seems a better choice to to add
> > > > > > > > > > new line per platform generation to make sure the users can
> > boot to Linux.
> > > > > > > > >
> > > > > > > > > How does Windows do this?  Do they have to add similar
> > > > > > > > > code for each new platform?
> > > > > > > >
> > > > > > > > Windows is chicken and doesn't move any mmio bar around on its
> > own.
> > > > > > > > Except if the bios explicitly told it somehow (e.g. for the
> > > > > > > > 64bit bar stuff amd recently announced for windows, that
> > > > > > > > linux supports since years by moving the bar). So except if
> > > > > > > > you want to preemptively disable the pci code that does this
> > > > > > > > anytime there's an intel gpu, this is what we have to do.
> > > > > > >
> > > > > > > I think Windows *does* move BARs (they use the more generic
> > > > > > > terminology of "rebalancing PNP resources") in some cases
> > > > > > > [3,4].  Of course, I'm pretty sure Windows will only assign
> > > > > > > PCI resources inside the windows advertised in the host bridge
> > _CRS.
> > > > > > >
> > > > > > > Linux *used* to ignore that host bridge _CRS and could set
> > > > > > > BARs to addresses that appeared available but were in fact
> > > > > > > used by the platform somehow.  But Linux has been paying
> > > > > > > attention to host bridge _CRS for a long time now, so it
> > > > > > > should also only assign resources inside those windows.
> > > > > >
> > > > > > If this behaviour is newer than the addition of these quirks
> > > > > > then yeah they're probably not needed anymore, and we can move
> > > > > > all this back into the driver. Do you have the commit when pci
> > > > > > core started observing _CRS on the host bridge?
> > > > >
> > > > > I think the most relevant commit is this:
> > > > >
> > > > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by
> > > > > default on 2008 and newer machines")
> > > > >
> > > > > but the earliest quirk I found is over three years later:
> > > > >
> > > > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving
> > > > > Intel graphics stolen memory v5")
> > > > >
> > > > > So there must be something else going on.  814c5f1f52a4 mentions a
> > > > > couple bug reports.  The dmesg from 66726 [5] shows that we *are*
> > > > > observing the host bridge _CRS, but Linux just used the BIOS
> > > > > configuration without changing anything:
> > > > >
> > > > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
> > > > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff]
> > reserved
> > > > >   PCI: Using host bridge windows from ACPI; if necessary, use
> > "pci=nocrs" and report a bug
> > > > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> > > > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> > > > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> > > > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> > > > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> > > > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit
> > pref]
> > > > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> > > > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> > > > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with
> > > > > stolen region: [0x7f80_0000 - 0x8000_0000]
> > > > >
> > > > > So the BIOS programmed the 00:1c.0 bridge prefetchable window to
> > > > > [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> > > > >
> > > > > On this system, there are no PCI BARs in that range.  01:00.0
> > > > > looks like a Ralink RT3090 Wireless 802.11n device that only has a
> > > > > non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> > > > >
> > > > > I don't know the details of the conflict.  IIUC, Joonas said the
> > > > > stolen memory is accessible only by the integrated graphics, not
> > > > > by the CPU.  The bridge window is CPU accessible, of course, and
> > > > > the [mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the
> > > > > CPU uses for programmed I/O to BARs below the bridge.
> > > > >
> > > > > The graphics accesses sound like they would be DMA in the *bus*
> > > > > address space, which is frequently, but not always, identical to
> > > > > the CPU address space.
> > > >
> > > > So apparently on some platforms the conflict is harmless because the
> > > > BIOS puts BARs and stuff over it from boot-up, and things work:
> > > > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts on
> > > > gen3") But we also had conflict reports on other machines.
> > >
> > > The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk for
> > > reserving Intel graphics stolen memory v5") and 0b6d24c01932
> > > ("drm/i915: Don't complain about stolen conflicts on gen3") seem to be
> > > basically complaints about the *message*, not anything that's actually
> > > broken.
> > >
> > > Jesse's comment [6]:
> > >
> > >   Given the decode priority on our GMCHs, it's fine if the regions
> > >   overlap.  However it doesn't look like there's a nice way to detect
> > >   it.  In this case, part of the range occupied by the stolen space is
> > >   simply "reserved" per the E820, but the rest of it is under the bus
> > >   0 range (which kind of makes sense too).
> > >
> > > sounds relevant but I don't know enough to interpret it.  I added
> > > Jesse in case he wants to comment.
> > >
> > > > GPU does all its access with CPU address space (after the iommu,
> > > > which is entirely integrated). So I'm not sure whether we've seen
> > > > something go boom or whether reserving that resource was just
> > > > precaution in
> > > > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> > > > conflict"), it's all a bit way back in history.
> > > >
> > > > So really not sure what to do here or what the risks are.
> > >
> > > I'm not either.  Seems like we're not really converging on anything
> > > useful we can do at this point.  The only thing I can think of would
> > > be to collect data about actual failures (not just warning messages).
> > > That might lead to something we could improve in the future.
> > 
> > I don't have any brilliant ideas here unfortunately.  Maybe it's worth talking
> > to some of the Windows folks internally to see how these ranges are handled
> > these days and matching it?  Historically this has been an area fraught with
> > danger because getting things wrong can lead to corruption of various kinds
> > or boot hangs.
> > 
> > Jesse

Tejas Upadhyay Dec. 2, 2020, 5:21 a.m. UTC | #14

Yes it fails all the tests which are allocating from this stolen memory bunch. For example IGT tests like " igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* | igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to work on stolen memory.

Thanks,
Tejas

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: 30 November 2020 22:21
> To: Surendrakumar Upadhyay, TejaskumarX
> <[e-mail]>
> Cc: Jesse Barnes <[e-mail]>; Daniel Vetter <[e-mail]>;
> Joonas Lahtinen <[e-mail]>; Linux PCI <linux-
> [e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> Matthew D <[e-mail]>; Pandey, Hariom
> <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> 
> On Mon, Nov 30, 2020 at 10:44:14AM +0000, Surendrakumar Upadhyay,
> TejaskumarX wrote:
> > Hi All,
> >
> > Are we merging this patch in?
> 
> Does it fix something?  If something is broken without this patch, can we
> collect information about exactly what is broken and how it fails?
> 
> But I don't object if somebody else wants to apply this.
> 
> > > -----Original Message-----
> > > From: Jesse Barnes <[e-mail]>
> > > Sent: 20 November 2020 03:32
> > > To: Bjorn Helgaas <[e-mail]>
> > > Cc: Daniel Vetter <[e-mail]>; Joonas Lahtinen
> > > <[e-mail]>; Surendrakumar Upadhyay,
> > > TejaskumarX <[e-mail]>; Linux
> > > PCI <linux- [e-mail]>; Linux Kernel Mailing List <linux-
> > > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > > Matthew D <[e-mail]>; Pandey, Hariom
> > > <[e-mail]>; Jani Nikula
> > > <[e-mail]>; Vivi, Rodrigo
> > > <[e-mail]>; David Airlie <[e-mail]>
> > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > >
> > > On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]>
> > > wrote:
> > > >
> > > > [+cc Jesse]
> > > >
> > > > On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > > > > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas
> > > > > <[e-mail]>
> > > wrote:
> > > > > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas
> > > <[e-mail]> wrote:
> > > > > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas
> > > <[e-mail]> wrote:
> > > > > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas
> > > > > > > > > > Lahtinen
> > > wrote:
> > > > > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas
> > > > > > > > > > > > Upadhyay
> > > wrote:
> > > > > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > >
> > > > > > > > > > > > I don't plan to do anything with this since
> > > > > > > > > > > > previous similar patches have gone through some
> > > > > > > > > > > > other tree, so this is
> > > just kibitzing.
> > > > > > > > > > > >
> > > > > > > > > > > > But the fact that we have this long list of Intel
> > > > > > > > > > > > devices [1] that constantly needs updates [2] is a
> > > > > > > > > > > > hint that
> > > something is wrong.
> > > > > > > > > > >
> > > > > > > > > > > We add an entry for every new integrated graphics
> > > > > > > > > > > platform. Once the platform is added, there have not
> > > > > > > > > > > been
> > > changes lately.
> > > > > > > > > > >
> > > > > > > > > > > > IIUC the general idea is that we need to discover
> > > > > > > > > > > > Intel gfx memory by looking at device-dependent
> > > > > > > > > > > > config
> > > space and add it to the E820 map.
> > > > > > > > > > > > Apparently the quirks discover this via PCI config
> > > > > > > > > > > > registers like I830_ESMRAMC, I845_ESMRAMC, etc,
> > > > > > > > > > > > and tell the driver about it via the global
> > > "intel_graphics_stolen_res"?
> > > > > > > > > > >
> > > > > > > > > > > We discover what is called the graphics data stolen
> > > > > > > > > > > memory. It is regular system memory range that is
> > > > > > > > > > > not CPU accessible. It is accessible by the integrated
> graphics only.
> > > > > > > > > > >
> > > > > > > > > > > See:
> > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torv
> > > > > > > > > > > alds
> > > > > > > > > > > /linux.git/commit/arch/x86/kernel/early-quirks.c?h=v
> > > > > > > > > > > 5.10
> > > > > > > > > > > -rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > > > > >
> > > > > > > > > > > > That's not the way this should work.  There should
> > > > > > > > > > > > some generic, non device-dependent PCI or ACPI
> > > > > > > > > > > > method to discover the memory used, or at least
> > > > > > > > > > > > some way to do it in
> > > the driver instead of early arch code.
> > > > > > > > > > >
> > > > > > > > > > > It's used by the early BIOS/UEFI code to set up
> > > > > > > > > > > initial
> > > framebuffer.
> > > > > > > > > > > Even if i915 driver is never loaded, the memory
> > > > > > > > > > > ranges still need to be fixed. They source of the
> > > > > > > > > > > problem is that the OEM BIOS which are not under our
> > > > > > > > > > > control get the
> > > programming wrong.
> > > > > > > > > > >
> > > > > > > > > > > We used to detect the memory region size again at
> > > > > > > > > > > i915 initialization but wanted to eliminate the code
> > > > > > > > > > > duplication and resulting subtle bugs that caused.
> > > > > > > > > > > Conclusion back then was that storing the struct
> > > > > > > > > > > resource in
> > > memory is the best trade-off.
> > > > > > > > > > >
> > > > > > > > > > > > How is this *supposed* to work?  Is there
> > > > > > > > > > > > something we can do in E820 or other resource
> > > > > > > > > > > > management that would
> > > make this easier?
> > > > > > > > > > >
> > > > > > > > > > > The code was added around Haswell (HSW) device
> > > > > > > > > > > generation to mitigate bugs in BIOS. It is
> > > > > > > > > > > traditionally hard to get all OEMs to fix their BIOS
> > > > > > > > > > > when things work for Windows. It's only later years
> > > > > > > > > > > when some laptop models
> > > are intended to be sold with Linux.
> > > > > > > > > > >
> > > > > > > > > > > The alternative would be to get all the OEM to fix
> > > > > > > > > > > their BIOS for Linux, but that is not very realistic
> > > > > > > > > > > given past experiences. So it seems a better choice
> > > > > > > > > > > to to add new line per platform generation to make
> > > > > > > > > > > sure the users can
> > > boot to Linux.
> > > > > > > > > >
> > > > > > > > > > How does Windows do this?  Do they have to add similar
> > > > > > > > > > code for each new platform?
> > > > > > > > >
> > > > > > > > > Windows is chicken and doesn't move any mmio bar around
> > > > > > > > > on its
> > > own.
> > > > > > > > > Except if the bios explicitly told it somehow (e.g. for
> > > > > > > > > the 64bit bar stuff amd recently announced for windows,
> > > > > > > > > that linux supports since years by moving the bar). So
> > > > > > > > > except if you want to preemptively disable the pci code
> > > > > > > > > that does this anytime there's an intel gpu, this is what we
> have to do.
> > > > > > > >
> > > > > > > > I think Windows *does* move BARs (they use the more
> > > > > > > > generic terminology of "rebalancing PNP resources") in
> > > > > > > > some cases [3,4].  Of course, I'm pretty sure Windows will
> > > > > > > > only assign PCI resources inside the windows advertised in
> > > > > > > > the host bridge
> > > _CRS.
> > > > > > > >
> > > > > > > > Linux *used* to ignore that host bridge _CRS and could set
> > > > > > > > BARs to addresses that appeared available but were in fact
> > > > > > > > used by the platform somehow.  But Linux has been paying
> > > > > > > > attention to host bridge _CRS for a long time now, so it
> > > > > > > > should also only assign resources inside those windows.
> > > > > > >
> > > > > > > If this behaviour is newer than the addition of these quirks
> > > > > > > then yeah they're probably not needed anymore, and we can
> > > > > > > move all this back into the driver. Do you have the commit
> > > > > > > when pci core started observing _CRS on the host bridge?
> > > > > >
> > > > > > I think the most relevant commit is this:
> > > > > >
> > > > > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info
> > > > > > by default on 2008 and newer machines")
> > > > > >
> > > > > > but the earliest quirk I found is over three years later:
> > > > > >
> > > > > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving
> > > > > > Intel graphics stolen memory v5")
> > > > > >
> > > > > > So there must be something else going on.  814c5f1f52a4
> > > > > > mentions a couple bug reports.  The dmesg from 66726 [5] shows
> > > > > > that we *are* observing the host bridge _CRS, but Linux just
> > > > > > used the BIOS configuration without changing anything:
> > > > > >
> > > > > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff]
> usable
> > > > > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff]
> > > reserved
> > > > > >   PCI: Using host bridge windows from ACPI; if necessary, use
> > > "pci=nocrs" and report a bug
> > > > > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> > > > > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> > > > > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> > > > > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> > > > > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> > > > > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff
> 64bit
> > > pref]
> > > > > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> > > > > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> > > > > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with
> > > > > > stolen region: [0x7f80_0000 - 0x8000_0000]
> > > > > >
> > > > > > So the BIOS programmed the 00:1c.0 bridge prefetchable window
> > > > > > to [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> > > > > >
> > > > > > On this system, there are no PCI BARs in that range.  01:00.0
> > > > > > looks like a Ralink RT3090 Wireless 802.11n device that only
> > > > > > has a non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> > > > > >
> > > > > > I don't know the details of the conflict.  IIUC, Joonas said
> > > > > > the stolen memory is accessible only by the integrated
> > > > > > graphics, not by the CPU.  The bridge window is CPU
> > > > > > accessible, of course, and the [mem 0x7f70_0000-0x7f8f_ffff]
> > > > > > range contains the addresses the CPU uses for programmed I/O to
> BARs below the bridge.
> > > > > >
> > > > > > The graphics accesses sound like they would be DMA in the
> > > > > > *bus* address space, which is frequently, but not always,
> > > > > > identical to the CPU address space.
> > > > >
> > > > > So apparently on some platforms the conflict is harmless because
> > > > > the BIOS puts BARs and stuff over it from boot-up, and things work:
> > > > > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts
> > > > > on
> > > > > gen3") But we also had conflict reports on other machines.
> > > >
> > > > The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk
> > > > for reserving Intel graphics stolen memory v5") and 0b6d24c01932
> > > > ("drm/i915: Don't complain about stolen conflicts on gen3") seem
> > > > to be basically complaints about the *message*, not anything
> > > > that's actually broken.
> > > >
> > > > Jesse's comment [6]:
> > > >
> > > >   Given the decode priority on our GMCHs, it's fine if the regions
> > > >   overlap.  However it doesn't look like there's a nice way to detect
> > > >   it.  In this case, part of the range occupied by the stolen space is
> > > >   simply "reserved" per the E820, but the rest of it is under the bus
> > > >   0 range (which kind of makes sense too).
> > > >
> > > > sounds relevant but I don't know enough to interpret it.  I added
> > > > Jesse in case he wants to comment.
> > > >
> > > > > GPU does all its access with CPU address space (after the iommu,
> > > > > which is entirely integrated). So I'm not sure whether we've
> > > > > seen something go boom or whether reserving that resource was
> > > > > just precaution in
> > > > > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> > > > > conflict"), it's all a bit way back in history.
> > > > >
> > > > > So really not sure what to do here or what the risks are.
> > > >
> > > > I'm not either.  Seems like we're not really converging on
> > > > anything useful we can do at this point.  The only thing I can
> > > > think of would be to collect data about actual failures (not just warning
> messages).
> > > > That might lead to something we could improve in the future.
> > >
> > > I don't have any brilliant ideas here unfortunately.  Maybe it's
> > > worth talking to some of the Windows folks internally to see how
> > > these ranges are handled these days and matching it?  Historically
> > > this has been an area fraught with danger because getting things
> > > wrong can lead to corruption of various kinds or boot hangs.
> > >
> > > Jesse

Bjorn Helgaas Dec. 2, 2020, 8:22 p.m. UTC | #15

On Wed, Dec 02, 2020 at 05:21:58AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> Yes it fails all the tests which are allocating from this stolen
> memory bunch. For example IGT tests like "
> igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* |
> igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to work
> on stolen memory.

I'm sure that means something to graphics developers, but I have no
idea!  Do you have URLs for the test case source, outputs, dmesg log,
lspci info, bug reports, etc?

> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > Sent: 30 November 2020 22:21
> > To: Surendrakumar Upadhyay, TejaskumarX
> > <[e-mail]>
> > Cc: Jesse Barnes <[e-mail]>; Daniel Vetter <[e-mail]>;
> > Joonas Lahtinen <[e-mail]>; Linux PCI <linux-
> > [e-mail]>; Linux Kernel Mailing List <linux-
> > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > Matthew D <[e-mail]>; Pandey, Hariom
> > <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> > Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > 
> > On Mon, Nov 30, 2020 at 10:44:14AM +0000, Surendrakumar Upadhyay,
> > TejaskumarX wrote:
> > > Hi All,
> > >
> > > Are we merging this patch in?
> > 
> > Does it fix something?  If something is broken without this patch, can we
> > collect information about exactly what is broken and how it fails?
> > 
> > But I don't object if somebody else wants to apply this.
> > 
> > > > -----Original Message-----
> > > > From: Jesse Barnes <[e-mail]>
> > > > Sent: 20 November 2020 03:32
> > > > To: Bjorn Helgaas <[e-mail]>
> > > > Cc: Daniel Vetter <[e-mail]>; Joonas Lahtinen
> > > > <[e-mail]>; Surendrakumar Upadhyay,
> > > > TejaskumarX <[e-mail]>; Linux
> > > > PCI <linux- [e-mail]>; Linux Kernel Mailing List <linux-
> > > > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > > > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > > > Matthew D <[e-mail]>; Pandey, Hariom
> > > > <[e-mail]>; Jani Nikula
> > > > <[e-mail]>; Vivi, Rodrigo
> > > > <[e-mail]>; David Airlie <[e-mail]>
> > > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > > >
> > > > On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]>
> > > > wrote:
> > > > >
> > > > > [+cc Jesse]
> > > > >
> > > > > On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > > > > > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas
> > > > > > <[e-mail]>
> > > > wrote:
> > > > > > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > > > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas
> > > > <[e-mail]> wrote:
> > > > > > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas
> > > > <[e-mail]> wrote:
> > > > > > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas
> > > > > > > > > > > Lahtinen
> > > > wrote:
> > > > > > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas
> > > > > > > > > > > > > Upadhyay
> > > > wrote:
> > > > > > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > > >
> > > > > > > > > > > > > I don't plan to do anything with this since
> > > > > > > > > > > > > previous similar patches have gone through some
> > > > > > > > > > > > > other tree, so this is
> > > > just kibitzing.
> > > > > > > > > > > > >
> > > > > > > > > > > > > But the fact that we have this long list of Intel
> > > > > > > > > > > > > devices [1] that constantly needs updates [2] is a
> > > > > > > > > > > > > hint that
> > > > something is wrong.
> > > > > > > > > > > >
> > > > > > > > > > > > We add an entry for every new integrated graphics
> > > > > > > > > > > > platform. Once the platform is added, there have not
> > > > > > > > > > > > been
> > > > changes lately.
> > > > > > > > > > > >
> > > > > > > > > > > > > IIUC the general idea is that we need to discover
> > > > > > > > > > > > > Intel gfx memory by looking at device-dependent
> > > > > > > > > > > > > config
> > > > space and add it to the E820 map.
> > > > > > > > > > > > > Apparently the quirks discover this via PCI config
> > > > > > > > > > > > > registers like I830_ESMRAMC, I845_ESMRAMC, etc,
> > > > > > > > > > > > > and tell the driver about it via the global
> > > > "intel_graphics_stolen_res"?
> > > > > > > > > > > >
> > > > > > > > > > > > We discover what is called the graphics data stolen
> > > > > > > > > > > > memory. It is regular system memory range that is
> > > > > > > > > > > > not CPU accessible. It is accessible by the integrated
> > graphics only.
> > > > > > > > > > > >
> > > > > > > > > > > > See:
> > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torv
> > > > > > > > > > > > alds
> > > > > > > > > > > > /linux.git/commit/arch/x86/kernel/early-quirks.c?h=v
> > > > > > > > > > > > 5.10
> > > > > > > > > > > > -rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > > > > > >
> > > > > > > > > > > > > That's not the way this should work.  There should
> > > > > > > > > > > > > some generic, non device-dependent PCI or ACPI
> > > > > > > > > > > > > method to discover the memory used, or at least
> > > > > > > > > > > > > some way to do it in
> > > > the driver instead of early arch code.
> > > > > > > > > > > >
> > > > > > > > > > > > It's used by the early BIOS/UEFI code to set up
> > > > > > > > > > > > initial
> > > > framebuffer.
> > > > > > > > > > > > Even if i915 driver is never loaded, the memory
> > > > > > > > > > > > ranges still need to be fixed. They source of the
> > > > > > > > > > > > problem is that the OEM BIOS which are not under our
> > > > > > > > > > > > control get the
> > > > programming wrong.
> > > > > > > > > > > >
> > > > > > > > > > > > We used to detect the memory region size again at
> > > > > > > > > > > > i915 initialization but wanted to eliminate the code
> > > > > > > > > > > > duplication and resulting subtle bugs that caused.
> > > > > > > > > > > > Conclusion back then was that storing the struct
> > > > > > > > > > > > resource in
> > > > memory is the best trade-off.
> > > > > > > > > > > >
> > > > > > > > > > > > > How is this *supposed* to work?  Is there
> > > > > > > > > > > > > something we can do in E820 or other resource
> > > > > > > > > > > > > management that would
> > > > make this easier?
> > > > > > > > > > > >
> > > > > > > > > > > > The code was added around Haswell (HSW) device
> > > > > > > > > > > > generation to mitigate bugs in BIOS. It is
> > > > > > > > > > > > traditionally hard to get all OEMs to fix their BIOS
> > > > > > > > > > > > when things work for Windows. It's only later years
> > > > > > > > > > > > when some laptop models
> > > > are intended to be sold with Linux.
> > > > > > > > > > > >
> > > > > > > > > > > > The alternative would be to get all the OEM to fix
> > > > > > > > > > > > their BIOS for Linux, but that is not very realistic
> > > > > > > > > > > > given past experiences. So it seems a better choice
> > > > > > > > > > > > to to add new line per platform generation to make
> > > > > > > > > > > > sure the users can
> > > > boot to Linux.
> > > > > > > > > > >
> > > > > > > > > > > How does Windows do this?  Do they have to add similar
> > > > > > > > > > > code for each new platform?
> > > > > > > > > >
> > > > > > > > > > Windows is chicken and doesn't move any mmio bar around
> > > > > > > > > > on its
> > > > own.
> > > > > > > > > > Except if the bios explicitly told it somehow (e.g. for
> > > > > > > > > > the 64bit bar stuff amd recently announced for windows,
> > > > > > > > > > that linux supports since years by moving the bar). So
> > > > > > > > > > except if you want to preemptively disable the pci code
> > > > > > > > > > that does this anytime there's an intel gpu, this is what we
> > have to do.
> > > > > > > > >
> > > > > > > > > I think Windows *does* move BARs (they use the more
> > > > > > > > > generic terminology of "rebalancing PNP resources") in
> > > > > > > > > some cases [3,4].  Of course, I'm pretty sure Windows will
> > > > > > > > > only assign PCI resources inside the windows advertised in
> > > > > > > > > the host bridge
> > > > _CRS.
> > > > > > > > >
> > > > > > > > > Linux *used* to ignore that host bridge _CRS and could set
> > > > > > > > > BARs to addresses that appeared available but were in fact
> > > > > > > > > used by the platform somehow.  But Linux has been paying
> > > > > > > > > attention to host bridge _CRS for a long time now, so it
> > > > > > > > > should also only assign resources inside those windows.
> > > > > > > >
> > > > > > > > If this behaviour is newer than the addition of these quirks
> > > > > > > > then yeah they're probably not needed anymore, and we can
> > > > > > > > move all this back into the driver. Do you have the commit
> > > > > > > > when pci core started observing _CRS on the host bridge?
> > > > > > >
> > > > > > > I think the most relevant commit is this:
> > > > > > >
> > > > > > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info
> > > > > > > by default on 2008 and newer machines")
> > > > > > >
> > > > > > > but the earliest quirk I found is over three years later:
> > > > > > >
> > > > > > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving
> > > > > > > Intel graphics stolen memory v5")
> > > > > > >
> > > > > > > So there must be something else going on.  814c5f1f52a4
> > > > > > > mentions a couple bug reports.  The dmesg from 66726 [5] shows
> > > > > > > that we *are* observing the host bridge _CRS, but Linux just
> > > > > > > used the BIOS configuration without changing anything:
> > > > > > >
> > > > > > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff]
> > usable
> > > > > > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff]
> > > > reserved
> > > > > > >   PCI: Using host bridge windows from ACPI; if necessary, use
> > > > "pci=nocrs" and report a bug
> > > > > > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> > > > > > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> > > > > > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> > > > > > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> > > > > > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> > > > > > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff
> > 64bit
> > > > pref]
> > > > > > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> > > > > > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> > > > > > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with
> > > > > > > stolen region: [0x7f80_0000 - 0x8000_0000]
> > > > > > >
> > > > > > > So the BIOS programmed the 00:1c.0 bridge prefetchable window
> > > > > > > to [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> > > > > > >
> > > > > > > On this system, there are no PCI BARs in that range.  01:00.0
> > > > > > > looks like a Ralink RT3090 Wireless 802.11n device that only
> > > > > > > has a non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> > > > > > >
> > > > > > > I don't know the details of the conflict.  IIUC, Joonas said
> > > > > > > the stolen memory is accessible only by the integrated
> > > > > > > graphics, not by the CPU.  The bridge window is CPU
> > > > > > > accessible, of course, and the [mem 0x7f70_0000-0x7f8f_ffff]
> > > > > > > range contains the addresses the CPU uses for programmed I/O to
> > BARs below the bridge.
> > > > > > >
> > > > > > > The graphics accesses sound like they would be DMA in the
> > > > > > > *bus* address space, which is frequently, but not always,
> > > > > > > identical to the CPU address space.
> > > > > >
> > > > > > So apparently on some platforms the conflict is harmless because
> > > > > > the BIOS puts BARs and stuff over it from boot-up, and things work:
> > > > > > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts
> > > > > > on
> > > > > > gen3") But we also had conflict reports on other machines.
> > > > >
> > > > > The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk
> > > > > for reserving Intel graphics stolen memory v5") and 0b6d24c01932
> > > > > ("drm/i915: Don't complain about stolen conflicts on gen3") seem
> > > > > to be basically complaints about the *message*, not anything
> > > > > that's actually broken.
> > > > >
> > > > > Jesse's comment [6]:
> > > > >
> > > > >   Given the decode priority on our GMCHs, it's fine if the regions
> > > > >   overlap.  However it doesn't look like there's a nice way to detect
> > > > >   it.  In this case, part of the range occupied by the stolen space is
> > > > >   simply "reserved" per the E820, but the rest of it is under the bus
> > > > >   0 range (which kind of makes sense too).
> > > > >
> > > > > sounds relevant but I don't know enough to interpret it.  I added
> > > > > Jesse in case he wants to comment.
> > > > >
> > > > > > GPU does all its access with CPU address space (after the iommu,
> > > > > > which is entirely integrated). So I'm not sure whether we've
> > > > > > seen something go boom or whether reserving that resource was
> > > > > > just precaution in
> > > > > > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> > > > > > conflict"), it's all a bit way back in history.
> > > > > >
> > > > > > So really not sure what to do here or what the risks are.
> > > > >
> > > > > I'm not either.  Seems like we're not really converging on
> > > > > anything useful we can do at this point.  The only thing I can
> > > > > think of would be to collect data about actual failures (not just warning
> > messages).
> > > > > That might lead to something we could improve in the future.
> > > >
> > > > I don't have any brilliant ideas here unfortunately.  Maybe it's
> > > > worth talking to some of the Windows folks internally to see how
> > > > these ranges are handled these days and matching it?  Historically
> > > > this has been an area fraught with danger because getting things
> > > > wrong can lead to corruption of various kinds or boot hangs.
> > > >
> > > > Jesse

Tejas Upadhyay Dec. 3, 2020, 3:23 a.m. UTC | #16

I sent patch to fix https://gitlab.freedesktop.org/drm/intel/-/issues/2610 issue.

Thanks,
Tejas

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: 03 December 2020 01:53
> To: Surendrakumar Upadhyay, TejaskumarX
> <[e-mail]>
> Cc: Jesse Barnes <[e-mail]>; Daniel Vetter <[e-mail]>;
> Joonas Lahtinen <[e-mail]>; Linux PCI <linux-
> [e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> Matthew D <[e-mail]>; Pandey, Hariom
> <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> 
> On Wed, Dec 02, 2020 at 05:21:58AM +0000, Surendrakumar Upadhyay,
> TejaskumarX wrote:
> > Yes it fails all the tests which are allocating from this stolen
> > memory bunch. For example IGT tests like "
> > igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* |
> > igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to work
> > on stolen memory.
> 
> I'm sure that means something to graphics developers, but I have no idea!
> Do you have URLs for the test case source, outputs, dmesg log, lspci info, bug
> reports, etc?
> 
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]>
> > > Sent: 30 November 2020 22:21
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > <[e-mail]>
> > > Cc: Jesse Barnes <[e-mail]>; Daniel Vetter
> > > <[e-mail]>; Joonas Lahtinen
> > > <[e-mail]>; Linux PCI <linux-
> > > [e-mail]>; Linux Kernel Mailing List <linux-
> > > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > > Matthew D <[e-mail]>; Pandey, Hariom
> > > <[e-mail]>; Jani Nikula
> > > <[e-mail]>; Vivi, Rodrigo
> > > <[e-mail]>; David Airlie <[e-mail]>
> > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > >
> > > On Mon, Nov 30, 2020 at 10:44:14AM +0000, Surendrakumar Upadhyay,
> > > TejaskumarX wrote:
> > > > Hi All,
> > > >
> > > > Are we merging this patch in?
> > >
> > > Does it fix something?  If something is broken without this patch,
> > > can we collect information about exactly what is broken and how it fails?
> > >
> > > But I don't object if somebody else wants to apply this.
> > >
> > > > > -----Original Message-----
> > > > > From: Jesse Barnes <[e-mail]>
> > > > > Sent: 20 November 2020 03:32
> > > > > To: Bjorn Helgaas <[e-mail]>
> > > > > Cc: Daniel Vetter <[e-mail]>; Joonas Lahtinen
> > > > > <[e-mail]>; Surendrakumar Upadhyay,
> > > > > TejaskumarX <[e-mail]>;
> > > > > Linux PCI <linux- [e-mail]>; Linux Kernel Mailing
> > > > > List <linux- [e-mail]>; X86 ML <[e-mail]>;
> > > > > Borislav Petkov <[e-mail]>; De Marchi, Lucas
> > > > > <[e-mail]>; Roper, Matthew D
> > > > > <[e-mail]>; Pandey, Hariom
> > > > > <[e-mail]>; Jani Nikula
> > > > > <[e-mail]>; Vivi, Rodrigo
> > > > > <[e-mail]>; David Airlie <[e-mail]>
> > > > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > > > >
> > > > > On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas
> > > > > <[e-mail]>
> > > > > wrote:
> > > > > >
> > > > > > [+cc Jesse]
> > > > > >
> > > > > > On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > > > > > > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas
> > > > > > > <[e-mail]>
> > > > > wrote:
> > > > > > > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter
> wrote:
> > > > > > > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas
> > > > > <[e-mail]> wrote:
> > > > > > > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter
> wrote:
> > > > > > > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas
> > > > > <[e-mail]> wrote:
> > > > > > > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas
> > > > > > > > > > > > Lahtinen
> > > > > wrote:
> > > > > > > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530,
> > > > > > > > > > > > > > Tejas Upadhyay
> > > > > wrote:
> > > > > > > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Cc: Lucas De Marchi
> > > > > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > > > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > > > > > > > > > > <[e-mail]
> > > > > > > > > > > > > > > m>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I don't plan to do anything with this since
> > > > > > > > > > > > > > previous similar patches have gone through
> > > > > > > > > > > > > > some other tree, so this is
> > > > > just kibitzing.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > But the fact that we have this long list of
> > > > > > > > > > > > > > Intel devices [1] that constantly needs
> > > > > > > > > > > > > > updates [2] is a hint that
> > > > > something is wrong.
> > > > > > > > > > > > >
> > > > > > > > > > > > > We add an entry for every new integrated
> > > > > > > > > > > > > graphics platform. Once the platform is added,
> > > > > > > > > > > > > there have not been
> > > > > changes lately.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > IIUC the general idea is that we need to
> > > > > > > > > > > > > > discover Intel gfx memory by looking at
> > > > > > > > > > > > > > device-dependent config
> > > > > space and add it to the E820 map.
> > > > > > > > > > > > > > Apparently the quirks discover this via PCI
> > > > > > > > > > > > > > config registers like I830_ESMRAMC,
> > > > > > > > > > > > > > I845_ESMRAMC, etc, and tell the driver about
> > > > > > > > > > > > > > it via the global
> > > > > "intel_graphics_stolen_res"?
> > > > > > > > > > > > >
> > > > > > > > > > > > > We discover what is called the graphics data
> > > > > > > > > > > > > stolen memory. It is regular system memory range
> > > > > > > > > > > > > that is not CPU accessible. It is accessible by
> > > > > > > > > > > > > the integrated
> > > graphics only.
> > > > > > > > > > > > >
> > > > > > > > > > > > > See:
> > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/
> > > > > > > > > > > > > torv
> > > > > > > > > > > > > alds
> > > > > > > > > > > > > /linux.git/commit/arch/x86/kernel/early-quirks.c
> > > > > > > > > > > > > ?h=v
> > > > > > > > > > > > > 5.10
> > > > > > > > > > > > > -rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > > > > > > >
> > > > > > > > > > > > > > That's not the way this should work.  There
> > > > > > > > > > > > > > should some generic, non device-dependent PCI
> > > > > > > > > > > > > > or ACPI method to discover the memory used, or
> > > > > > > > > > > > > > at least some way to do it in
> > > > > the driver instead of early arch code.
> > > > > > > > > > > > >
> > > > > > > > > > > > > It's used by the early BIOS/UEFI code to set up
> > > > > > > > > > > > > initial
> > > > > framebuffer.
> > > > > > > > > > > > > Even if i915 driver is never loaded, the memory
> > > > > > > > > > > > > ranges still need to be fixed. They source of
> > > > > > > > > > > > > the problem is that the OEM BIOS which are not
> > > > > > > > > > > > > under our control get the
> > > > > programming wrong.
> > > > > > > > > > > > >
> > > > > > > > > > > > > We used to detect the memory region size again
> > > > > > > > > > > > > at
> > > > > > > > > > > > > i915 initialization but wanted to eliminate the
> > > > > > > > > > > > > code duplication and resulting subtle bugs that caused.
> > > > > > > > > > > > > Conclusion back then was that storing the struct
> > > > > > > > > > > > > resource in
> > > > > memory is the best trade-off.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > How is this *supposed* to work?  Is there
> > > > > > > > > > > > > > something we can do in E820 or other resource
> > > > > > > > > > > > > > management that would
> > > > > make this easier?
> > > > > > > > > > > > >
> > > > > > > > > > > > > The code was added around Haswell (HSW) device
> > > > > > > > > > > > > generation to mitigate bugs in BIOS. It is
> > > > > > > > > > > > > traditionally hard to get all OEMs to fix their
> > > > > > > > > > > > > BIOS when things work for Windows. It's only
> > > > > > > > > > > > > later years when some laptop models
> > > > > are intended to be sold with Linux.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The alternative would be to get all the OEM to
> > > > > > > > > > > > > fix their BIOS for Linux, but that is not very
> > > > > > > > > > > > > realistic given past experiences. So it seems a
> > > > > > > > > > > > > better choice to to add new line per platform
> > > > > > > > > > > > > generation to make sure the users can
> > > > > boot to Linux.
> > > > > > > > > > > >
> > > > > > > > > > > > How does Windows do this?  Do they have to add
> > > > > > > > > > > > similar code for each new platform?
> > > > > > > > > > >
> > > > > > > > > > > Windows is chicken and doesn't move any mmio bar
> > > > > > > > > > > around on its
> > > > > own.
> > > > > > > > > > > Except if the bios explicitly told it somehow (e.g.
> > > > > > > > > > > for the 64bit bar stuff amd recently announced for
> > > > > > > > > > > windows, that linux supports since years by moving
> > > > > > > > > > > the bar). So except if you want to preemptively
> > > > > > > > > > > disable the pci code that does this anytime there's
> > > > > > > > > > > an intel gpu, this is what we
> > > have to do.
> > > > > > > > > >
> > > > > > > > > > I think Windows *does* move BARs (they use the more
> > > > > > > > > > generic terminology of "rebalancing PNP resources") in
> > > > > > > > > > some cases [3,4].  Of course, I'm pretty sure Windows
> > > > > > > > > > will only assign PCI resources inside the windows
> > > > > > > > > > advertised in the host bridge
> > > > > _CRS.
> > > > > > > > > >
> > > > > > > > > > Linux *used* to ignore that host bridge _CRS and could
> > > > > > > > > > set BARs to addresses that appeared available but were
> > > > > > > > > > in fact used by the platform somehow.  But Linux has
> > > > > > > > > > been paying attention to host bridge _CRS for a long
> > > > > > > > > > time now, so it should also only assign resources inside those
> windows.
> > > > > > > > >
> > > > > > > > > If this behaviour is newer than the addition of these
> > > > > > > > > quirks then yeah they're probably not needed anymore,
> > > > > > > > > and we can move all this back into the driver. Do you
> > > > > > > > > have the commit when pci core started observing _CRS on the
> host bridge?
> > > > > > > >
> > > > > > > > I think the most relevant commit is this:
> > > > > > > >
> > > > > > > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS
> > > > > > > > info by default on 2008 and newer machines")
> > > > > > > >
> > > > > > > > but the earliest quirk I found is over three years later:
> > > > > > > >
> > > > > > > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for
> > > > > > > > reserving Intel graphics stolen memory v5")
> > > > > > > >
> > > > > > > > So there must be something else going on.  814c5f1f52a4
> > > > > > > > mentions a couple bug reports.  The dmesg from 66726 [5]
> > > > > > > > shows that we *are* observing the host bridge _CRS, but
> > > > > > > > Linux just used the BIOS configuration without changing
> anything:
> > > > > > > >
> > > > > > > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff]
> > > usable
> > > > > > > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff]
> > > > > reserved
> > > > > > > >   PCI: Using host bridge windows from ACPI; if necessary,
> > > > > > > > use
> > > > > "pci=nocrs" and report a bug
> > > > > > > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> > > > > > > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-
> 0xffff_ffff]
> > > > > > > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> > > > > > > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> > > > > > > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-
> 0xfe9f_ffff]
> > > > > > > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-
> 0x7f8f_ffff
> > > 64bit
> > > > > pref]
> > > > > > > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> > > > > > > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> > > > > > > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected
> > > > > > > > with stolen region: [0x7f80_0000 - 0x8000_0000]
> > > > > > > >
> > > > > > > > So the BIOS programmed the 00:1c.0 bridge prefetchable
> > > > > > > > window to [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's
> a conflict.
> > > > > > > >
> > > > > > > > On this system, there are no PCI BARs in that range.
> > > > > > > > 01:00.0 looks like a Ralink RT3090 Wireless 802.11n device
> > > > > > > > that only has a non-prefetchable BAR at [mem 0xfe90_0000-
> 0xfe90_ffff].
> > > > > > > >
> > > > > > > > I don't know the details of the conflict.  IIUC, Joonas
> > > > > > > > said the stolen memory is accessible only by the
> > > > > > > > integrated graphics, not by the CPU.  The bridge window is
> > > > > > > > CPU accessible, of course, and the [mem
> > > > > > > > 0x7f70_0000-0x7f8f_ffff] range contains the addresses the
> > > > > > > > CPU uses for programmed I/O to
> > > BARs below the bridge.
> > > > > > > >
> > > > > > > > The graphics accesses sound like they would be DMA in the
> > > > > > > > *bus* address space, which is frequently, but not always,
> > > > > > > > identical to the CPU address space.
> > > > > > >
> > > > > > > So apparently on some platforms the conflict is harmless
> > > > > > > because the BIOS puts BARs and stuff over it from boot-up, and
> things work:
> > > > > > > 0b6d24c01932 ("drm/i915: Don't complain about stolen
> > > > > > > conflicts on
> > > > > > > gen3") But we also had conflict reports on other machines.
> > > > > >
> > > > > > The bug reports mentioned in 814c5f1f52a4 ("x86: add early
> > > > > > quirk for reserving Intel graphics stolen memory v5") and
> > > > > > 0b6d24c01932
> > > > > > ("drm/i915: Don't complain about stolen conflicts on gen3")
> > > > > > seem to be basically complaints about the *message*, not
> > > > > > anything that's actually broken.
> > > > > >
> > > > > > Jesse's comment [6]:
> > > > > >
> > > > > >   Given the decode priority on our GMCHs, it's fine if the regions
> > > > > >   overlap.  However it doesn't look like there's a nice way to detect
> > > > > >   it.  In this case, part of the range occupied by the stolen space is
> > > > > >   simply "reserved" per the E820, but the rest of it is under the bus
> > > > > >   0 range (which kind of makes sense too).
> > > > > >
> > > > > > sounds relevant but I don't know enough to interpret it.  I
> > > > > > added Jesse in case he wants to comment.
> > > > > >
> > > > > > > GPU does all its access with CPU address space (after the
> > > > > > > iommu, which is entirely integrated). So I'm not sure
> > > > > > > whether we've seen something go boom or whether reserving
> > > > > > > that resource was just precaution in
> > > > > > > eaba1b8f3379 ("drm/i915: Verify that our stolen memory
> > > > > > > doesn't conflict"), it's all a bit way back in history.
> > > > > > >
> > > > > > > So really not sure what to do here or what the risks are.
> > > > > >
> > > > > > I'm not either.  Seems like we're not really converging on
> > > > > > anything useful we can do at this point.  The only thing I can
> > > > > > think of would be to collect data about actual failures (not
> > > > > > just warning
> > > messages).
> > > > > > That might lead to something we could improve in the future.
> > > > >
> > > > > I don't have any brilliant ideas here unfortunately.  Maybe it's
> > > > > worth talking to some of the Windows folks internally to see how
> > > > > these ranges are handled these days and matching it?
> > > > > Historically this has been an area fraught with danger because
> > > > > getting things wrong can lead to corruption of various kinds or boot
> hangs.
> > > > >
> > > > > Jesse

Joonas Lahtinen Dec. 3, 2020, 8:46 a.m. UTC | #17

Quoting Bjorn Helgaas (2020-12-02 22:22:53)
> On Wed, Dec 02, 2020 at 05:21:58AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> > Yes it fails all the tests which are allocating from this stolen
> > memory bunch. For example IGT tests like "
> > igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* |
> > igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to work
> > on stolen memory.

That's just because we have de-duped the stolen memory detection code.
If it's not detected at the early quirks, it's not detected by the
driver at all.

So if the patch is not merged to early quirks, we'd have to refactor the
code to add alternative detection path to i915. Before that is done, the
failures are expected.

> I'm sure that means something to graphics developers, but I have no
> idea!  Do you have URLs for the test case source, outputs, dmesg log,
> lspci info, bug reports, etc?

The thing is, the bug reports for stuff like this would only start to
flow after Jasperlake systems are shipping widely and the less common
OEMs start integrating it to into strangely behaving BIOSes. Or that
is the assumption.

If it's fine to merge this through i915 for now with an Acked-by, like
the previous patches, that'd be great. We can start a discussion on if
the new platforms are affected anymore. But I'd rather not drop it
before we have that understanding, as the previous problems have
included boot time memory corruption.

Would that work?

Regards, Joonas

> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]>
> > > Sent: 30 November 2020 22:21
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > <[e-mail]>
> > > Cc: Jesse Barnes <[e-mail]>; Daniel Vetter <[e-mail]>;
> > > Joonas Lahtinen <[e-mail]>; Linux PCI <linux-
> > > [e-mail]>; Linux Kernel Mailing List <linux-
> > > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > > Matthew D <[e-mail]>; Pandey, Hariom
> > > <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> > > Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > > 
> > > On Mon, Nov 30, 2020 at 10:44:14AM +0000, Surendrakumar Upadhyay,
> > > TejaskumarX wrote:
> > > > Hi All,
> > > >
> > > > Are we merging this patch in?
> > > 
> > > Does it fix something?  If something is broken without this patch, can we
> > > collect information about exactly what is broken and how it fails?
> > > 
> > > But I don't object if somebody else wants to apply this.
> > > 
> > > > > -----Original Message-----
> > > > > From: Jesse Barnes <[e-mail]>
> > > > > Sent: 20 November 2020 03:32
> > > > > To: Bjorn Helgaas <[e-mail]>
> > > > > Cc: Daniel Vetter <[e-mail]>; Joonas Lahtinen
> > > > > <[e-mail]>; Surendrakumar Upadhyay,
> > > > > TejaskumarX <[e-mail]>; Linux
> > > > > PCI <linux- [e-mail]>; Linux Kernel Mailing List <linux-
> > > > > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > > > > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > > > > Matthew D <[e-mail]>; Pandey, Hariom
> > > > > <[e-mail]>; Jani Nikula
> > > > > <[e-mail]>; Vivi, Rodrigo
> > > > > <[e-mail]>; David Airlie <[e-mail]>
> > > > > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> > > > >
> > > > > On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]>
> > > > > wrote:
> > > > > >
> > > > > > [+cc Jesse]
> > > > > >
> > > > > > On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> > > > > > > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas
> > > > > > > <[e-mail]>
> > > > > wrote:
> > > > > > > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> > > > > > > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas
> > > > > <[e-mail]> wrote:
> > > > > > > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> > > > > > > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas
> > > > > <[e-mail]> wrote:
> > > > > > > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas
> > > > > > > > > > > > Lahtinen
> > > > > wrote:
> > > > > > > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> > > > > > > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas
> > > > > > > > > > > > > > Upadhyay
> > > > > wrote:
> > > > > > > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> > > > > > > > > > > > > > > Cc: Matt Roper <[e-mail]>
> > > > > > > > > > > > > > > Signed-off-by: Tejas Upadhyay
> > > > > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I don't plan to do anything with this since
> > > > > > > > > > > > > > previous similar patches have gone through some
> > > > > > > > > > > > > > other tree, so this is
> > > > > just kibitzing.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > But the fact that we have this long list of Intel
> > > > > > > > > > > > > > devices [1] that constantly needs updates [2] is a
> > > > > > > > > > > > > > hint that
> > > > > something is wrong.
> > > > > > > > > > > > >
> > > > > > > > > > > > > We add an entry for every new integrated graphics
> > > > > > > > > > > > > platform. Once the platform is added, there have not
> > > > > > > > > > > > > been
> > > > > changes lately.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > IIUC the general idea is that we need to discover
> > > > > > > > > > > > > > Intel gfx memory by looking at device-dependent
> > > > > > > > > > > > > > config
> > > > > space and add it to the E820 map.
> > > > > > > > > > > > > > Apparently the quirks discover this via PCI config
> > > > > > > > > > > > > > registers like I830_ESMRAMC, I845_ESMRAMC, etc,
> > > > > > > > > > > > > > and tell the driver about it via the global
> > > > > "intel_graphics_stolen_res"?
> > > > > > > > > > > > >
> > > > > > > > > > > > > We discover what is called the graphics data stolen
> > > > > > > > > > > > > memory. It is regular system memory range that is
> > > > > > > > > > > > > not CPU accessible. It is accessible by the integrated
> > > graphics only.
> > > > > > > > > > > > >
> > > > > > > > > > > > > See:
> > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torv
> > > > > > > > > > > > > alds
> > > > > > > > > > > > > /linux.git/commit/arch/x86/kernel/early-quirks.c?h=v
> > > > > > > > > > > > > 5.10
> > > > > > > > > > > > > -rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> > > > > > > > > > > > >
> > > > > > > > > > > > > > That's not the way this should work.  There should
> > > > > > > > > > > > > > some generic, non device-dependent PCI or ACPI
> > > > > > > > > > > > > > method to discover the memory used, or at least
> > > > > > > > > > > > > > some way to do it in
> > > > > the driver instead of early arch code.
> > > > > > > > > > > > >
> > > > > > > > > > > > > It's used by the early BIOS/UEFI code to set up
> > > > > > > > > > > > > initial
> > > > > framebuffer.
> > > > > > > > > > > > > Even if i915 driver is never loaded, the memory
> > > > > > > > > > > > > ranges still need to be fixed. They source of the
> > > > > > > > > > > > > problem is that the OEM BIOS which are not under our
> > > > > > > > > > > > > control get the
> > > > > programming wrong.
> > > > > > > > > > > > >
> > > > > > > > > > > > > We used to detect the memory region size again at
> > > > > > > > > > > > > i915 initialization but wanted to eliminate the code
> > > > > > > > > > > > > duplication and resulting subtle bugs that caused.
> > > > > > > > > > > > > Conclusion back then was that storing the struct
> > > > > > > > > > > > > resource in
> > > > > memory is the best trade-off.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > How is this *supposed* to work?  Is there
> > > > > > > > > > > > > > something we can do in E820 or other resource
> > > > > > > > > > > > > > management that would
> > > > > make this easier?
> > > > > > > > > > > > >
> > > > > > > > > > > > > The code was added around Haswell (HSW) device
> > > > > > > > > > > > > generation to mitigate bugs in BIOS. It is
> > > > > > > > > > > > > traditionally hard to get all OEMs to fix their BIOS
> > > > > > > > > > > > > when things work for Windows. It's only later years
> > > > > > > > > > > > > when some laptop models
> > > > > are intended to be sold with Linux.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The alternative would be to get all the OEM to fix
> > > > > > > > > > > > > their BIOS for Linux, but that is not very realistic
> > > > > > > > > > > > > given past experiences. So it seems a better choice
> > > > > > > > > > > > > to to add new line per platform generation to make
> > > > > > > > > > > > > sure the users can
> > > > > boot to Linux.
> > > > > > > > > > > >
> > > > > > > > > > > > How does Windows do this?  Do they have to add similar
> > > > > > > > > > > > code for each new platform?
> > > > > > > > > > >
> > > > > > > > > > > Windows is chicken and doesn't move any mmio bar around
> > > > > > > > > > > on its
> > > > > own.
> > > > > > > > > > > Except if the bios explicitly told it somehow (e.g. for
> > > > > > > > > > > the 64bit bar stuff amd recently announced for windows,
> > > > > > > > > > > that linux supports since years by moving the bar). So
> > > > > > > > > > > except if you want to preemptively disable the pci code
> > > > > > > > > > > that does this anytime there's an intel gpu, this is what we
> > > have to do.
> > > > > > > > > >
> > > > > > > > > > I think Windows *does* move BARs (they use the more
> > > > > > > > > > generic terminology of "rebalancing PNP resources") in
> > > > > > > > > > some cases [3,4].  Of course, I'm pretty sure Windows will
> > > > > > > > > > only assign PCI resources inside the windows advertised in
> > > > > > > > > > the host bridge
> > > > > _CRS.
> > > > > > > > > >
> > > > > > > > > > Linux *used* to ignore that host bridge _CRS and could set
> > > > > > > > > > BARs to addresses that appeared available but were in fact
> > > > > > > > > > used by the platform somehow.  But Linux has been paying
> > > > > > > > > > attention to host bridge _CRS for a long time now, so it
> > > > > > > > > > should also only assign resources inside those windows.
> > > > > > > > >
> > > > > > > > > If this behaviour is newer than the addition of these quirks
> > > > > > > > > then yeah they're probably not needed anymore, and we can
> > > > > > > > > move all this back into the driver. Do you have the commit
> > > > > > > > > when pci core started observing _CRS on the host bridge?
> > > > > > > >
> > > > > > > > I think the most relevant commit is this:
> > > > > > > >
> > > > > > > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info
> > > > > > > > by default on 2008 and newer machines")
> > > > > > > >
> > > > > > > > but the earliest quirk I found is over three years later:
> > > > > > > >
> > > > > > > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving
> > > > > > > > Intel graphics stolen memory v5")
> > > > > > > >
> > > > > > > > So there must be something else going on.  814c5f1f52a4
> > > > > > > > mentions a couple bug reports.  The dmesg from 66726 [5] shows
> > > > > > > > that we *are* observing the host bridge _CRS, but Linux just
> > > > > > > > used the BIOS configuration without changing anything:
> > > > > > > >
> > > > > > > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff]
> > > usable
> > > > > > > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff]
> > > > > reserved
> > > > > > > >   PCI: Using host bridge windows from ACPI; if necessary, use
> > > > > "pci=nocrs" and report a bug
> > > > > > > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> > > > > > > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> > > > > > > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> > > > > > > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> > > > > > > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> > > > > > > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff
> > > 64bit
> > > > > pref]
> > > > > > > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> > > > > > > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> > > > > > > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with
> > > > > > > > stolen region: [0x7f80_0000 - 0x8000_0000]
> > > > > > > >
> > > > > > > > So the BIOS programmed the 00:1c.0 bridge prefetchable window
> > > > > > > > to [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> > > > > > > >
> > > > > > > > On this system, there are no PCI BARs in that range.  01:00.0
> > > > > > > > looks like a Ralink RT3090 Wireless 802.11n device that only
> > > > > > > > has a non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> > > > > > > >
> > > > > > > > I don't know the details of the conflict.  IIUC, Joonas said
> > > > > > > > the stolen memory is accessible only by the integrated
> > > > > > > > graphics, not by the CPU.  The bridge window is CPU
> > > > > > > > accessible, of course, and the [mem 0x7f70_0000-0x7f8f_ffff]
> > > > > > > > range contains the addresses the CPU uses for programmed I/O to
> > > BARs below the bridge.
> > > > > > > >
> > > > > > > > The graphics accesses sound like they would be DMA in the
> > > > > > > > *bus* address space, which is frequently, but not always,
> > > > > > > > identical to the CPU address space.
> > > > > > >
> > > > > > > So apparently on some platforms the conflict is harmless because
> > > > > > > the BIOS puts BARs and stuff over it from boot-up, and things work:
> > > > > > > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts
> > > > > > > on
> > > > > > > gen3") But we also had conflict reports on other machines.
> > > > > >
> > > > > > The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk
> > > > > > for reserving Intel graphics stolen memory v5") and 0b6d24c01932
> > > > > > ("drm/i915: Don't complain about stolen conflicts on gen3") seem
> > > > > > to be basically complaints about the *message*, not anything
> > > > > > that's actually broken.
> > > > > >
> > > > > > Jesse's comment [6]:
> > > > > >
> > > > > >   Given the decode priority on our GMCHs, it's fine if the regions
> > > > > >   overlap.  However it doesn't look like there's a nice way to detect
> > > > > >   it.  In this case, part of the range occupied by the stolen space is
> > > > > >   simply "reserved" per the E820, but the rest of it is under the bus
> > > > > >   0 range (which kind of makes sense too).
> > > > > >
> > > > > > sounds relevant but I don't know enough to interpret it.  I added
> > > > > > Jesse in case he wants to comment.
> > > > > >
> > > > > > > GPU does all its access with CPU address space (after the iommu,
> > > > > > > which is entirely integrated). So I'm not sure whether we've
> > > > > > > seen something go boom or whether reserving that resource was
> > > > > > > just precaution in
> > > > > > > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> > > > > > > conflict"), it's all a bit way back in history.
> > > > > > >
> > > > > > > So really not sure what to do here or what the risks are.
> > > > > >
> > > > > > I'm not either.  Seems like we're not really converging on
> > > > > > anything useful we can do at this point.  The only thing I can
> > > > > > think of would be to collect data about actual failures (not just warning
> > > messages).
> > > > > > That might lead to something we could improve in the future.
> > > > >
> > > > > I don't have any brilliant ideas here unfortunately.  Maybe it's
> > > > > worth talking to some of the Windows folks internally to see how
> > > > > these ranges are handled these days and matching it?  Historically
> > > > > this has been an area fraught with danger because getting things
> > > > > wrong can lead to corruption of various kinds or boot hangs.
> > > > >
> > > > > Jesse

Bjorn Helgaas Dec. 3, 2020, 3:25 p.m. UTC | #18

On Thu, Dec 03, 2020 at 10:46:29AM +0200, Joonas Lahtinen wrote:
> Quoting Bjorn Helgaas (2020-12-02 22:22:53)
> > On Wed, Dec 02, 2020 at 05:21:58AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> > > Yes it fails all the tests which are allocating from this stolen
> > > memory bunch. For example IGT tests like "
> > > igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* |
> > > igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to work
> > > on stolen memory.
> 
> That's just because we have de-duped the stolen memory detection code.
> If it's not detected at the early quirks, it's not detected by the
> driver at all.
> 
> So if the patch is not merged to early quirks, we'd have to refactor the
> code to add alternative detection path to i915. Before that is done, the
> failures are expected.
> 
> > I'm sure that means something to graphics developers, but I have no
> > idea!  Do you have URLs for the test case source, outputs, dmesg log,
> > lspci info, bug reports, etc?
> 
> The thing is, the bug reports for stuff like this would only start to
> flow after Jasperlake systems are shipping widely and the less common
> OEMs start integrating it to into strangely behaving BIOSes. Or that
> is the assumption.
> 
> If it's fine to merge this through i915 for now with an Acked-by, like
> the previous patches, that'd be great. We can start a discussion on if
> the new platforms are affected anymore. But I'd rather not drop it
> before we have that understanding, as the previous problems have
> included boot time memory corruption.
> 
> Would that work?

Like I said, I'm not objecting if somebody else wants to apply this.

I'm just pointing out that there's a little bit of voodoo here because
it's not clear what makes a BIOS strangely behaving or what causes
boot-time memory corruption, and that means we don't really have any
hope of resolving this stream of quirk updates.

Bjorn

Tejas Upadhyay Dec. 3, 2020, 4:09 p.m. UTC | #19

Okay then I will wait for someone to respond with "Reviewed-by". So this can be merged.

Thanks,
Tejas

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: 03 December 2020 20:55
> To: Joonas Lahtinen <[e-mail]>
> Cc: Surendrakumar Upadhyay, TejaskumarX
> <[e-mail]>; Jesse Barnes
> <[e-mail]>; Daniel Vetter <[e-mail]>; Linux PCI <linux-
> [e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> Matthew D <[e-mail]>; Pandey, Hariom
> <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> 
> On Thu, Dec 03, 2020 at 10:46:29AM +0200, Joonas Lahtinen wrote:
> > Quoting Bjorn Helgaas (2020-12-02 22:22:53)
> > > On Wed, Dec 02, 2020 at 05:21:58AM +0000, Surendrakumar Upadhyay,
> TejaskumarX wrote:
> > > > Yes it fails all the tests which are allocating from this stolen
> > > > memory bunch. For example IGT tests like "
> > > > igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* |
> > > > igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to
> > > > work on stolen memory.
> >
> > That's just because we have de-duped the stolen memory detection code.
> > If it's not detected at the early quirks, it's not detected by the
> > driver at all.
> >
> > So if the patch is not merged to early quirks, we'd have to refactor
> > the code to add alternative detection path to i915. Before that is
> > done, the failures are expected.
> >
> > > I'm sure that means something to graphics developers, but I have no
> > > idea!  Do you have URLs for the test case source, outputs, dmesg
> > > log, lspci info, bug reports, etc?
> >
> > The thing is, the bug reports for stuff like this would only start to
> > flow after Jasperlake systems are shipping widely and the less common
> > OEMs start integrating it to into strangely behaving BIOSes. Or that
> > is the assumption.
> >
> > If it's fine to merge this through i915 for now with an Acked-by, like
> > the previous patches, that'd be great. We can start a discussion on if
> > the new platforms are affected anymore. But I'd rather not drop it
> > before we have that understanding, as the previous problems have
> > included boot time memory corruption.
> >
> > Would that work?
> 
> Like I said, I'm not objecting if somebody else wants to apply this.
> 
> I'm just pointing out that there's a little bit of voodoo here because it's not
> clear what makes a BIOS strangely behaving or what causes boot-time
> memory corruption, and that means we don't really have any hope of
> resolving this stream of quirk updates.
> 
> Bjorn

Tejas Upadhyay Jan. 20, 2021, 10:04 a.m. UTC | #20

Hello,

Have we decided anything on this patch?

Thanks,
Tejas

> -----Original Message-----
> From: Surendrakumar Upadhyay, TejaskumarX
> Sent: 03 December 2020 21:40
> To: Bjorn Helgaas <[e-mail]>; Joonas Lahtinen
> <[e-mail]>
> Cc: Jesse Barnes <[e-mail]>; Daniel Vetter <[e-mail]>;
> Linux PCI <[e-mail]>; Linux Kernel Mailing List <linux-
> [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> Matthew D <[e-mail]>; Pandey, Hariom
> <[e-mail]>; Jani Nikula <[e-mail]>; Vivi,
> Rodrigo <[e-mail]>; David Airlie <[e-mail]>
> Subject: RE: [PATCH] x86/gpu: add JSL stolen memory support
> 
> Okay then I will wait for someone to respond with "Reviewed-by". So this can
> be merged.
> 
> Thanks,
> Tejas
> 
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > Sent: 03 December 2020 20:55
> > To: Joonas Lahtinen <[e-mail]>
> > Cc: Surendrakumar Upadhyay, TejaskumarX
> > <[e-mail]>; Jesse Barnes
> > <[e-mail]>; Daniel Vetter <[e-mail]>; Linux PCI
> > <linux- [e-mail]>; Linux Kernel Mailing List <linux-
> > [e-mail]>; X86 ML <[e-mail]>; Borislav Petkov
> > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Roper,
> > Matthew D <[e-mail]>; Pandey, Hariom
> > <[e-mail]>; Jani Nikula <[e-mail]>;
> > Vivi, Rodrigo <[e-mail]>; David Airlie
> > <[e-mail]>
> > Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> >
> > On Thu, Dec 03, 2020 at 10:46:29AM +0200, Joonas Lahtinen wrote:
> > > Quoting Bjorn Helgaas (2020-12-02 22:22:53)
> > > > On Wed, Dec 02, 2020 at 05:21:58AM +0000, Surendrakumar Upadhyay,
> > TejaskumarX wrote:
> > > > > Yes it fails all the tests which are allocating from this stolen
> > > > > memory bunch. For example IGT tests like "
> > > > > igt@kms_frontbuffer_tracking@-[fbc|fbcpsr].* |
> > > > > igt@kms_fbcon_fbt@fbc.* " are failing as they totally depend to
> > > > > work on stolen memory.
> > >
> > > That's just because we have de-duped the stolen memory detection code.
> > > If it's not detected at the early quirks, it's not detected by the
> > > driver at all.
> > >
> > > So if the patch is not merged to early quirks, we'd have to refactor
> > > the code to add alternative detection path to i915. Before that is
> > > done, the failures are expected.
> > >
> > > > I'm sure that means something to graphics developers, but I have
> > > > no idea!  Do you have URLs for the test case source, outputs,
> > > > dmesg log, lspci info, bug reports, etc?
> > >
> > > The thing is, the bug reports for stuff like this would only start
> > > to flow after Jasperlake systems are shipping widely and the less
> > > common OEMs start integrating it to into strangely behaving BIOSes.
> > > Or that is the assumption.
> > >
> > > If it's fine to merge this through i915 for now with an Acked-by,
> > > like the previous patches, that'd be great. We can start a
> > > discussion on if the new platforms are affected anymore. But I'd
> > > rather not drop it before we have that understanding, as the
> > > previous problems have included boot time memory corruption.
> > >
> > > Would that work?
> >
> > Like I said, I'm not objecting if somebody else wants to apply this.
> >
> > I'm just pointing out that there's a little bit of voodoo here because
> > it's not clear what makes a BIOS strangely behaving or what causes
> > boot-time memory corruption, and that means we don't really have any
> > hope of resolving this stream of quirk updates.
> >
> > Bjorn

Lucas De Marchi Jan. 20, 2022, 1:10 a.m. UTC | #21

Resurrecting this thread after the other discussion on stolen memory
for Intel integrated GPU when there are Intel discrete GPU present:
https://lore.kernel.org/linux-pci/20220118200145.GA887728@bhelgaas/

see below.

On Thu, Nov 19, 2020 at 02:01:50PM -0800, Jesse Barnes wrote:
>On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]> wrote:
>>
>> [+cc Jesse]
>>
>> On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
>> > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas <[e-mail]> wrote:
>> > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
>> > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas <[e-mail]> wrote:
>> > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
>> > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
>> > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
>> > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
>> > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
>> > > > > > > > >
>> > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
>> > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
>> > > > > > > > > >
>> > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
>> > > > > > > > > > Cc: Matt Roper <[e-mail]>
>> > > > > > > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
>> > > > > > > > >
>> > > > > > > > > I don't plan to do anything with this since previous similar patches
>> > > > > > > > > have gone through some other tree, so this is just kibitzing.
>> > > > > > > > >
>> > > > > > > > > But the fact that we have this long list of Intel devices [1] that
>> > > > > > > > > constantly needs updates [2] is a hint that something is wrong.
>> > > > > > > >
>> > > > > > > > We add an entry for every new integrated graphics platform. Once the
>> > > > > > > > platform is added, there have not been changes lately.
>> > > > > > > >
>> > > > > > > > > IIUC the general idea is that we need to discover Intel gfx memory by
>> > > > > > > > > looking at device-dependent config space and add it to the E820 map.
>> > > > > > > > > Apparently the quirks discover this via PCI config registers like
>> > > > > > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
>> > > > > > > > > global "intel_graphics_stolen_res"?
>> > > > > > > >
>> > > > > > > > We discover what is called the graphics data stolen memory. It is regular
>> > > > > > > > system memory range that is not CPU accessible. It is accessible by the
>> > > > > > > > integrated graphics only.
>> > > > > > > >
>> > > > > > > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
>> > > > > > > >
>> > > > > > > > > That's not the way this should work.  There should some generic, non
>> > > > > > > > > device-dependent PCI or ACPI method to discover the memory used, or at
>> > > > > > > > > least some way to do it in the driver instead of early arch code.
>> > > > > > > >
>> > > > > > > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
>> > > > > > > > Even if i915 driver is never loaded, the memory ranges still need to
>> > > > > > > > be fixed. They source of the problem is that the OEM BIOS which are
>> > > > > > > > not under our control get the programming wrong.
>> > > > > > > >
>> > > > > > > > We used to detect the memory region size again at i915 initialization
>> > > > > > > > but wanted to eliminate the code duplication and resulting subtle bugs
>> > > > > > > > that caused. Conclusion back then was that storing the struct resource
>> > > > > > > > in memory is the best trade-off.
>> > > > > > > >
>> > > > > > > > > How is this *supposed* to work?  Is there something we can do in E820
>> > > > > > > > > or other resource management that would make this easier?
>> > > > > > > >
>> > > > > > > > The code was added around Haswell (HSW) device generation to mitigate
>> > > > > > > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
>> > > > > > > > BIOS when things work for Windows. It's only later years when some
>> > > > > > > > laptop models are intended to be sold with Linux.
>> > > > > > > >
>> > > > > > > > The alternative would be to get all the OEM to fix their BIOS for Linux,
>> > > > > > > > but that is not very realistic given past experiences. So it seems
>> > > > > > > > a better choice to to add new line per platform generation to make
>> > > > > > > > sure the users can boot to Linux.
>> > > > > > >
>> > > > > > > How does Windows do this?  Do they have to add similar code for each
>> > > > > > > new platform?
>> > > > > >
>> > > > > > Windows is chicken and doesn't move any mmio bar around on its own.
>> > > > > > Except if the bios explicitly told it somehow (e.g. for the 64bit bar
>> > > > > > stuff amd recently announced for windows, that linux supports since
>> > > > > > years by moving the bar). So except if you want to preemptively
>> > > > > > disable the pci code that does this anytime there's an intel gpu, this
>> > > > > > is what we have to do.
>> > > > >
>> > > > > I think Windows *does* move BARs (they use the more generic
>> > > > > terminology of "rebalancing PNP resources") in some cases [3,4].  Of
>> > > > > course, I'm pretty sure Windows will only assign PCI resources inside
>> > > > > the windows advertised in the host bridge _CRS.
>> > > > >
>> > > > > Linux *used* to ignore that host bridge _CRS and could set BARs to
>> > > > > addresses that appeared available but were in fact used by the
>> > > > > platform somehow.  But Linux has been paying attention to host bridge
>> > > > > _CRS for a long time now, so it should also only assign resources
>> > > > > inside those windows.
>> > > >
>> > > > If this behaviour is newer than the addition of these quirks then yeah
>> > > > they're probably not needed anymore, and we can move all this back
>> > > > into the driver. Do you have the commit when pci core started
>> > > > observing _CRS on the host bridge?
>> > >
>> > > I think the most relevant commit is this:
>> > >
>> > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by default on 2008 and newer machines")
>> > >
>> > > but the earliest quirk I found is over three years later:
>> > >
>> > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving Intel graphics stolen memory v5")
>> > >
>> > > So there must be something else going on.  814c5f1f52a4 mentions a
>> > > couple bug reports.  The dmesg from 66726 [5] shows that we *are*
>> > > observing the host bridge _CRS, but Linux just used the BIOS
>> > > configuration without changing anything:
>> > >
>> > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
>> > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff] reserved
>> > >   PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
>> > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
>> > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
>> > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
>> > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
>> > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
>> > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit pref]
>> > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
>> > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
>> > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7f80_0000 - 0x8000_0000]
>> > >
>> > > So the BIOS programmed the 00:1c.0 bridge prefetchable window to
>> > > [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
>> > >
>> > > On this system, there are no PCI BARs in that range.  01:00.0 looks
>> > > like a Ralink RT3090 Wireless 802.11n device that only has a
>> > > non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
>> > >
>> > > I don't know the details of the conflict.  IIUC, Joonas said the
>> > > stolen memory is accessible only by the integrated graphics, not by
>> > > the CPU.  The bridge window is CPU accessible, of course, and the
>> > > [mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the CPU
>> > > uses for programmed I/O to BARs below the bridge.
>> > >
>> > > The graphics accesses sound like they would be DMA in the *bus*
>> > > address space, which is frequently, but not always, identical to the
>> > > CPU address space.
>> >
>> > So apparently on some platforms the conflict is harmless because the
>> > BIOS puts BARs and stuff over it from boot-up, and things work:
>> > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts on
>> > gen3") But we also had conflict reports on other machines.
>>
>> The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk for
>> reserving Intel graphics stolen memory v5") and 0b6d24c01932
>> ("drm/i915: Don't complain about stolen conflicts on gen3") seem to be
>> basically complaints about the *message*, not anything that's actually
>> broken.
>>
>> Jesse's comment [6]:
>>
>>   Given the decode priority on our GMCHs, it's fine if the regions
>>   overlap.  However it doesn't look like there's a nice way to detect
>>   it.  In this case, part of the range occupied by the stolen space is
>>   simply "reserved" per the E820, but the rest of it is under the bus
>>   0 range (which kind of makes sense too).
>>
>> sounds relevant but I don't know enough to interpret it.  I added
>> Jesse in case he wants to comment.
>>
>> > GPU does all its access with CPU address space (after the iommu, which
>> > is entirely integrated). So I'm not sure whether we've seen something
>> > go boom or whether reserving that resource was just precaution in
>> > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
>> > conflict"), it's all a bit way back in history.
>> >
>> > So really not sure what to do here or what the risks are.
>>
>> I'm not either.  Seems like we're not really converging on anything
>> useful we can do at this point.  The only thing I can think of would
>> be to collect data about actual failures (not just warning messages).
>> That might lead to something we could improve in the future.
>
>I don't have any brilliant ideas here unfortunately.  Maybe it's worth
>talking to some of the Windows folks internally to see how these
>ranges are handled these days and matching it?  Historically this has
>been an area fraught with danger because getting things wrong can lead
>to corruption of various kinds or boot hangs.

We could try something else, but if there are bios bugs for old systems
preventing us to do this entirely in i915, I'm not sure that would
solve it.

What if we phase out the quirks for new platforms? Idea would be to
revive eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't conflict")
adapted to the current code. Then we can move some o the latest
platforms and watch out for regressions. At least we would stop
additions to this early-quirk.c

Another idea: wouldn't DECLARE_PCI_FIXUP_EARLY work? AFAICS this is
early enough to reserve the memory.

Also We could add only those systems where we reproduce bugs
rather than preemptively adding them to the table - it would at least
allow to catch those bugs in bioses rather than hiding them.


Lucas De Marchi

>
>Jesse

Joonas Lahtinen Jan. 20, 2022, 7:35 a.m. UTC | #22

+ Tvrtko

Quoting Lucas De Marchi (2022-01-20 03:10:08)
> Resurrecting this thread after the other discussion on stolen memory
> for Intel integrated GPU when there are Intel discrete GPU present:
> https://lore.kernel.org/linux-pci/20220118200145.GA887728@bhelgaas/
> 
> see below.
> 
> On Thu, Nov 19, 2020 at 02:01:50PM -0800, Jesse Barnes wrote:
> >On Thu, Nov 19, 2020 at 11:19 AM Bjorn Helgaas <[e-mail]> wrote:
> >>
> >> [+cc Jesse]
> >>
> >> On Thu, Nov 19, 2020 at 10:37:10AM +0100, Daniel Vetter wrote:
> >> > On Thu, Nov 19, 2020 at 12:14 AM Bjorn Helgaas <[e-mail]> wrote:
> >> > > On Wed, Nov 18, 2020 at 10:57:26PM +0100, Daniel Vetter wrote:
> >> > > > On Wed, Nov 18, 2020 at 5:02 PM Bjorn Helgaas <[e-mail]> wrote:
> >> > > > > On Fri, Nov 06, 2020 at 10:39:16AM +0100, Daniel Vetter wrote:
> >> > > > > > On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <[e-mail]> wrote:
> >> > > > > > > On Thu, Nov 05, 2020 at 11:46:06AM +0200, Joonas Lahtinen wrote:
> >> > > > > > > > Quoting Bjorn Helgaas (2020-11-04 19:35:56)
> >> > > > > > > > > [+cc Jani, Joonas, Rodrigo, David, Daniel]
> >> > > > > > > > >
> >> > > > > > > > > On Wed, Nov 04, 2020 at 05:35:06PM +0530, Tejas Upadhyay wrote:
> >> > > > > > > > > > JSL re-uses the same stolen memory as ICL and EHL.
> >> > > > > > > > > >
> >> > > > > > > > > > Cc: Lucas De Marchi <[e-mail]>
> >> > > > > > > > > > Cc: Matt Roper <[e-mail]>
> >> > > > > > > > > > Signed-off-by: Tejas Upadhyay <[e-mail]>
> >> > > > > > > > >
> >> > > > > > > > > I don't plan to do anything with this since previous similar patches
> >> > > > > > > > > have gone through some other tree, so this is just kibitzing.
> >> > > > > > > > >
> >> > > > > > > > > But the fact that we have this long list of Intel devices [1] that
> >> > > > > > > > > constantly needs updates [2] is a hint that something is wrong.
> >> > > > > > > >
> >> > > > > > > > We add an entry for every new integrated graphics platform. Once the
> >> > > > > > > > platform is added, there have not been changes lately.
> >> > > > > > > >
> >> > > > > > > > > IIUC the general idea is that we need to discover Intel gfx memory by
> >> > > > > > > > > looking at device-dependent config space and add it to the E820 map.
> >> > > > > > > > > Apparently the quirks discover this via PCI config registers like
> >> > > > > > > > > I830_ESMRAMC, I845_ESMRAMC, etc, and tell the driver about it via the
> >> > > > > > > > > global "intel_graphics_stolen_res"?
> >> > > > > > > >
> >> > > > > > > > We discover what is called the graphics data stolen memory. It is regular
> >> > > > > > > > system memory range that is not CPU accessible. It is accessible by the
> >> > > > > > > > integrated graphics only.
> >> > > > > > > >
> >> > > > > > > > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/early-quirks.c?h=v5.10-rc2&id=814c5f1f52a4beb3710317022acd6ad34fc0b6b9
> >> > > > > > > >
> >> > > > > > > > > That's not the way this should work.  There should some generic, non
> >> > > > > > > > > device-dependent PCI or ACPI method to discover the memory used, or at
> >> > > > > > > > > least some way to do it in the driver instead of early arch code.
> >> > > > > > > >
> >> > > > > > > > It's used by the early BIOS/UEFI code to set up initial framebuffer.
> >> > > > > > > > Even if i915 driver is never loaded, the memory ranges still need to
> >> > > > > > > > be fixed. They source of the problem is that the OEM BIOS which are
> >> > > > > > > > not under our control get the programming wrong.
> >> > > > > > > >
> >> > > > > > > > We used to detect the memory region size again at i915 initialization
> >> > > > > > > > but wanted to eliminate the code duplication and resulting subtle bugs
> >> > > > > > > > that caused. Conclusion back then was that storing the struct resource
> >> > > > > > > > in memory is the best trade-off.
> >> > > > > > > >
> >> > > > > > > > > How is this *supposed* to work?  Is there something we can do in E820
> >> > > > > > > > > or other resource management that would make this easier?
> >> > > > > > > >
> >> > > > > > > > The code was added around Haswell (HSW) device generation to mitigate
> >> > > > > > > > bugs in BIOS. It is traditionally hard to get all OEMs to fix their
> >> > > > > > > > BIOS when things work for Windows. It's only later years when some
> >> > > > > > > > laptop models are intended to be sold with Linux.
> >> > > > > > > >
> >> > > > > > > > The alternative would be to get all the OEM to fix their BIOS for Linux,
> >> > > > > > > > but that is not very realistic given past experiences. So it seems
> >> > > > > > > > a better choice to to add new line per platform generation to make
> >> > > > > > > > sure the users can boot to Linux.
> >> > > > > > >
> >> > > > > > > How does Windows do this?  Do they have to add similar code for each
> >> > > > > > > new platform?
> >> > > > > >
> >> > > > > > Windows is chicken and doesn't move any mmio bar around on its own.
> >> > > > > > Except if the bios explicitly told it somehow (e.g. for the 64bit bar
> >> > > > > > stuff amd recently announced for windows, that linux supports since
> >> > > > > > years by moving the bar). So except if you want to preemptively
> >> > > > > > disable the pci code that does this anytime there's an intel gpu, this
> >> > > > > > is what we have to do.
> >> > > > >
> >> > > > > I think Windows *does* move BARs (they use the more generic
> >> > > > > terminology of "rebalancing PNP resources") in some cases [3,4].  Of
> >> > > > > course, I'm pretty sure Windows will only assign PCI resources inside
> >> > > > > the windows advertised in the host bridge _CRS.
> >> > > > >
> >> > > > > Linux *used* to ignore that host bridge _CRS and could set BARs to
> >> > > > > addresses that appeared available but were in fact used by the
> >> > > > > platform somehow.  But Linux has been paying attention to host bridge
> >> > > > > _CRS for a long time now, so it should also only assign resources
> >> > > > > inside those windows.
> >> > > >
> >> > > > If this behaviour is newer than the addition of these quirks then yeah
> >> > > > they're probably not needed anymore, and we can move all this back
> >> > > > into the driver. Do you have the commit when pci core started
> >> > > > observing _CRS on the host bridge?
> >> > >
> >> > > I think the most relevant commit is this:
> >> > >
> >> > >   2010-02-23 7bc5e3f2be32 ("x86/PCI: use host bridge _CRS info by default on 2008 and newer machines")
> >> > >
> >> > > but the earliest quirk I found is over three years later:
> >> > >
> >> > >   2013-07-26 814c5f1f52a4 ("x86: add early quirk for reserving Intel graphics stolen memory v5")
> >> > >
> >> > > So there must be something else going on.  814c5f1f52a4 mentions a
> >> > > couple bug reports.  The dmesg from 66726 [5] shows that we *are*
> >> > > observing the host bridge _CRS, but Linux just used the BIOS
> >> > > configuration without changing anything:
> >> > >
> >> > >   BIOS-e820: [mem 0x000000007f49_f000-0x000000007f5f_ffff] usable
> >> > >   BIOS-e820: [mem 0x00000000fec0_0000-0x00000000fec0_0fff] reserved
> >> > >   PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
> >> > >   ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
> >> > >   pci_bus 0000:00: root bus resource [mem 0x7f70_0000-0xffff_ffff]
> >> > >   pci 0000:00:1c.0: PCI bridge to [bus 01]
> >> > >   pci 0000:00:1c.0:   bridge window [io  0x1000-0x1fff]
> >> > >   pci 0000:00:1c.0:   bridge window [mem 0xfe90_0000-0xfe9f_ffff]
> >> > >   pci 0000:00:1c.0:   bridge window [mem 0x7f70_0000-0x7f8f_ffff 64bit pref]
> >> > >   pci 0000:01:00.0: [1814:3090] type 00 class 0x028000
> >> > >   pci 0000:01:00.0: reg 10: [mem 0xfe90_0000-0xfe90_ffff]
> >> > >   [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7f80_0000 - 0x8000_0000]
> >> > >
> >> > > So the BIOS programmed the 00:1c.0 bridge prefetchable window to
> >> > > [mem 0x7f70_0000-0x7f8f_ffff], and i915 thinks that's a conflict.
> >> > >
> >> > > On this system, there are no PCI BARs in that range.  01:00.0 looks
> >> > > like a Ralink RT3090 Wireless 802.11n device that only has a
> >> > > non-prefetchable BAR at [mem 0xfe90_0000-0xfe90_ffff].
> >> > >
> >> > > I don't know the details of the conflict.  IIUC, Joonas said the
> >> > > stolen memory is accessible only by the integrated graphics, not by
> >> > > the CPU.  The bridge window is CPU accessible, of course, and the
> >> > > [mem 0x7f70_0000-0x7f8f_ffff] range contains the addresses the CPU
> >> > > uses for programmed I/O to BARs below the bridge.
> >> > >
> >> > > The graphics accesses sound like they would be DMA in the *bus*
> >> > > address space, which is frequently, but not always, identical to the
> >> > > CPU address space.
> >> >
> >> > So apparently on some platforms the conflict is harmless because the
> >> > BIOS puts BARs and stuff over it from boot-up, and things work:
> >> > 0b6d24c01932 ("drm/i915: Don't complain about stolen conflicts on
> >> > gen3") But we also had conflict reports on other machines.
> >>
> >> The bug reports mentioned in 814c5f1f52a4 ("x86: add early quirk for
> >> reserving Intel graphics stolen memory v5") and 0b6d24c01932
> >> ("drm/i915: Don't complain about stolen conflicts on gen3") seem to be
> >> basically complaints about the *message*, not anything that's actually
> >> broken.
> >>
> >> Jesse's comment [6]:
> >>
> >>   Given the decode priority on our GMCHs, it's fine if the regions
> >>   overlap.  However it doesn't look like there's a nice way to detect
> >>   it.  In this case, part of the range occupied by the stolen space is
> >>   simply "reserved" per the E820, but the rest of it is under the bus
> >>   0 range (which kind of makes sense too).
> >>
> >> sounds relevant but I don't know enough to interpret it.  I added
> >> Jesse in case he wants to comment.
> >>
> >> > GPU does all its access with CPU address space (after the iommu, which
> >> > is entirely integrated). So I'm not sure whether we've seen something
> >> > go boom or whether reserving that resource was just precaution in
> >> > eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't
> >> > conflict"), it's all a bit way back in history.
> >> >
> >> > So really not sure what to do here or what the risks are.
> >>
> >> I'm not either.  Seems like we're not really converging on anything
> >> useful we can do at this point.  The only thing I can think of would
> >> be to collect data about actual failures (not just warning messages).
> >> That might lead to something we could improve in the future.
> >
> >I don't have any brilliant ideas here unfortunately.  Maybe it's worth
> >talking to some of the Windows folks internally to see how these
> >ranges are handled these days and matching it?  Historically this has
> >been an area fraught with danger because getting things wrong can lead
> >to corruption of various kinds or boot hangs.
> 
> We could try something else, but if there are bios bugs for old systems
> preventing us to do this entirely in i915, I'm not sure that would
> solve it.
> 
> What if we phase out the quirks for new platforms? Idea would be to
> revive eaba1b8f3379 ("drm/i915: Verify that our stolen memory doesn't conflict")
> adapted to the current code. Then we can move some o the latest
> platforms and watch out for regressions. At least we would stop
> additions to this early-quirk.c

I'm not really a big fan of doing that unless we follow with the
hardware and Windows folks to double-check that the problem really
shouldn't occur anymore in the BIOS.

Considering the trade-off here: we eliminate a few line additions and
risk making user systems non-bootable. I don't think doing that blindly
is very friendly to our users.

Regards, Joonas

> 
> Another idea: wouldn't DECLARE_PCI_FIXUP_EARLY work? AFAICS this is
> early enough to reserve the memory.
> 
> Also We could add only those systems where we reproduce bugs
> rather than preemptively adding them to the table - it would at least
> allow to catch those bugs in bioses rather than hiding them.
> 
> 
> Lucas De Marchi
> 
> >
> >Jesse

Patch

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index a4b5af03dcc1..534cc3f78c6b 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -549,6 +549,7 @@  static const struct pci_device_id intel_early_ids[] __initconst = {
 	INTEL_CNL_IDS(&gen9_early_ops),
 	INTEL_ICL_11_IDS(&gen11_early_ops),
 	INTEL_EHL_IDS(&gen11_early_ops),
+	INTEL_JSL_IDS(&gen11_early_ops),
 	INTEL_TGL_12_IDS(&gen11_early_ops),
 	INTEL_RKL_IDS(&gen11_early_ops),
 };