RE: [PATCH] x86/gpu: add JSL stolen memory support

[Surendrakumar Upadhyay, TejaskumarX]

Just checking, can somebody review/merge the patch? 

Thanks,
Tejas

> -----Original Message-----
> From: Daniel Vetter <daniel@xxxxxxxx>
> Sent: 06 November 2020 15:09
> To: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>; Surendrakumar
> Upadhyay, TejaskumarX
> <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>; Linux PCI <linux-
> pci@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>; X86 ML <x86@xxxxxxxxxx>; Borislav Petkov
> <bp@xxxxxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>; Roper,
> Matthew D <matthew.d.roper@xxxxxxxxx>; Pandey, Hariom
> <hariom.pandey@xxxxxxxxx>; Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>; Vivi,
> Rodrigo <rodrigo.vivi@xxxxxxxxx>; David Airlie <airlied@xxxxxxxx>
> Subject: Re: [PATCH] x86/gpu: add JSL stolen memory support
> 
> On Thu, Nov 5, 2020 at 3:17 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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 <lucas.demarchi@xxxxxxxxx>
> > > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > > > Signed-off-by: Tejas Upadhyay
> > > > > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>
> > > >
> > > > 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