Details

Reviewers

bwidawsk
kib
mmacy

Commits

rS339979: Add pci_early function to detect Intel stolen memory.

Summary

On some Intel devices BIOS firmware does not properly reserve memory (called "stolen memory") for the GPU. If the stolen memory is claimed by the OS, functions that depend on stolen memory (like frame buffer compression) can't be used.

A function called pci_early_quirks that is called before the virtual memory system is started was added. In Linux, this PCI early quirks function iterates through all PCI slots to check for any device that require quirks. While this more generic solution is preferable I only ported the Intel graphics specific parts because I think my implementation would be too similar to Linux GPL'd solution after looking at the Linux code too much.

The code regarding Intel graphics stolen memory was ported from Linux. In the case of Intel graphics stolen memory this pci_early_quirks will read the stolen memory base and size from registers and reserve it in phys_avail (if not already reserved). The values are also stored in global variables that is later read by linuxkpi_gplv2. Linuxkpi stores these values in a Linux-specific structure that is read by the drm driver.

Relevant linuxkpi code is here:
https://github.com/FreeBSDDesktop/kms-drm/blob/drm-v4.16/linuxkpi/gplv2/src/linux_compat.c#L37

Notes:

For now, only amd64 arch is supported since that is the only arch supported by the new drm drivers.
Not sure if I put declarations in the correct headers so please feel free to suggest if there's a better alternative (same goes for all code).
I was told that Intel GPUs are always located on 0:2:0 so these values are hard coded for now.

UPDATE: phys_avail code moved to separate review.

Test Plan

The algorithm is tested separately:
https://onlinegdb.com/BJiL_9bIQ
I'm sure there are improvements to be made but the current implementation should be solid.

Kernel + updated drm drivers have been tested on a Broadwell laptop (where BIOS does reserve memory properly)
From pci_early_quirks() I did some calls to phys_avail_reserve() with various base and size parameters and confirmed in verbose boot that the ranges had been excluded from the phys avail list.

A patched kernel can be built from here: https://github.com/FreeBSDDesktop/freebsd-base/tree/intelstolen-physavail
drm drivers must be rebuilt against the patched kernel source: https://github.com/FreeBSDDesktop/kms-drm/tree/drm-v4.16

It will add a log entry similar to this
[drm] Got stolen memory base 0xcd800000, size 0x2000000

Without these bits in the kernel, stolen memory is set to 0 and this causes panics during kldload on certain Intel hardware. We have not been able to reproduce the panic when using this patch (together with https://reviews.freebsd.org/D17431)

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

johalun0_gmail.com created this revision.Aug 15 2018, 12:45 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 15 2018, 12:45 PM

This strikes me as two different changes. One for the phys_avail_reserve() and another to use it for the pci early stuff. I'm a little uneasy with the pci early name, but the code is clearly PCI specific.
Would there ever be a need to do things like this when running the 32-bit kernel? I know we don't support graphics cards there, but when we're bringing in new interfaces, it's a question that suggests itself.
RPi has this sort of thing too, but it's dealt with in the bowels of the FDT code...

sys/x86/include/x86_var.h
152 ↗	(On Diff #46699)	Is this a MI interface we want all our MD code to use to cope with similar issues that we're currently coping with in other, ad-hoc ways?

In D16719#355599, @imp wrote:

This strikes me as two different changes. One for the phys_avail_reserve() and another to use it for the pci early stuff. I'm a little uneasy with the pci early name, but the code is clearly PCI specific.
Would there ever be a need to do things like this when running the 32-bit kernel? I know we don't support graphics cards there, but when we're bringing in new interfaces, it's a question that suggests itself.
RPi has this sort of thing too, but it's dealt with in the bowels of the FDT code...

Yes, they are two separate things and once we agree on the best solution I can split them up in two reviews. I thought it was easier to get the whole picture if it's all in one place to start with. As for graphics, I'm uncertain about Intel stolen for i386 support. Time will tell. Maybe other devs have ideas?

I couldn't find any port pci read functions to reuse. If there really aren't any, maybe these pci_early_read_* should be made public (with a nicer name)?

In D16719#355614, @johalun0_gmail.com wrote:

In D16719#355599, @imp wrote:

This strikes me as two different changes. One for the phys_avail_reserve() and another to use it for the pci early stuff. I'm a little uneasy with the pci early name, but the code is clearly PCI specific.
Would there ever be a need to do things like this when running the 32-bit kernel? I know we don't support graphics cards there, but when we're bringing in new interfaces, it's a question that suggests itself.
RPi has this sort of thing too, but it's dealt with in the bowels of the FDT code...

Yes, they are two separate things and once we agree on the best solution I can split them up in two reviews. I thought it was easier to get the whole picture if it's all in one place to start with. As for graphics, I'm uncertain about Intel stolen for i386 support. Time will tell. Maybe other devs have ideas?

I couldn't find any port pci read functions to reuse. If there really aren't any, maybe these pci_early_read_* should be made public (with a nicer name)?

I think if you want to make a generic function, you would want to support memory mapped and BIOS pci access as well. For supporting GEN, it's pretty safe to assume cf8/cfc is there.

Note: I did not read the phys_avail[] editing code. I think pci config is somewhat more interesting for start.

sys/amd64/amd64/machdep.c
2699	No init at the declaration location.
2706	binary ops require spaces around the op symbol.
2707	What is end ? Is it the address to reserve, or the address right after the reserve ?
sys/dev/pci/pci_early.c
61 ↗	(On Diff #46699)	oxcf8 is spelled as CONF1_ADDR_PORT, 0x80000000 is CONF1_ENABLE. See x86/include/pci_cfgreg.h.
62 ↗	(On Diff #46699)	Both PCI config access method and port accesses are x86 MD. I do not believe that this code belongs to dev/pci. That said, why did not you used pci_cfregopen() and pci_cfgregread() ? I think that there is no dependencies so normal PCI (not PCIe) config access would work very early.
127 ↗	(On Diff #46699)	return (1 * MB);
218 ↗	(On Diff #46699)	return (0);
289 ↗	(On Diff #46699)	intel_graphics_stolen(void)
314 ↗	(On Diff #46699)	Look at style(9) how spaces should be used.
319 ↗	(On Diff #46699)	Too many blank lines.
sys/x86/include/x86_var.h
152 ↗	(On Diff #46699)	I believe this is highly MD code and interface. Early manipulations with phys_avail[] cannot be anything else.

kib mentioned this in D16907: Several bug fixes and robustness improvements for the AP boot page table allocation..Aug 26 2018, 1:04 PM

johalun0_gmail.com added inline comments.Aug 27 2018, 7:56 AM

sys/amd64/amd64/machdep.c
2706	Do you mean [(i+2)] ?
2707	End is the address right after the reserve. Maybe having the function take base and size as arguments would make things more clear?
sys/x86/include/x86_var.h
152 ↗	(On Diff #46699)	Yes. The question is, do we want this function for 32bit as well (I left this open for discussion)? For graphics I can't think of a future we're will need it for i386. If we keep it amd64 only, what would be the correct header for it?

kib added inline comments.Aug 27 2018, 10:04 AM

sys/amd64/amd64/machdep.c
2706	I mean spaces, not braces. phys_avail[i + 2]
2707	No, it is common to take the range end as the address after the range. But then you have off by one in the check.
sys/x86/include/x86_var.h
152 ↗	(On Diff #46699)	We have amd64/include/md_var.h and i386/include/md_var.h. x86_var.h is just a common extract. But I do not see why i386 should be excluded.

johalun0_gmail.com added inline comments.Aug 27 2018, 2:45 PM

sys/x86/include/x86_var.h
152 ↗	(On Diff #46699)	Is there a common source file for i386/amd64 that make sense to put phys_avail() in (since the implementations would be the same) or should we duplicate it in machdep.c?

kib added inline comments.Aug 27 2018, 2:56 PM

sys/x86/include/x86_var.h
152 ↗	(On Diff #46699)	I believe a new source file in x86/x86 is the best.

zeising added a subscriber: zeising.Oct 4 2018, 6:10 PM

johalun0_gmail.com edited the summary of this revision. (Show Details)Oct 5 2018, 12:57 PM

johalun0_gmail.com added a reviewer: mmacy.

phys_avail_reserve has been moved to a separate diff https://reviews.freebsd.org/D17431 and this diff should be applied after D17431.

This code still depends on the phys_avail_reserve function and we'd like to have both of these diffs in the tree before 12.0 so we can use drm drivers beyond Linux 4.15 versions.

johalun0_gmail.com edited the test plan for this revision. (Show Details)Oct 5 2018, 1:11 PM

physmem.h has moved from machine/ to x86/

Changes: Disable phys_avail until KPI is in.

On commit: Bump __FreeBSD_version so we can check against that in driver code when reading the global variables.

kib added inline comments.Oct 26 2018, 6:26 PM

sys/dev/pci/pci_early_quirks.c
44	Look at sys/amd64/amd64/mp_machdep.c:87, where the GiB(x) macro is defined. I like this way much more than manual multiplication by a constant.
49	See inline comment in the loop iterating over the array.
69	This blank line is not needed.
84	return (); there and in all returns down.
122	Extra blank line, there and in all functions down.
173	'else' is not needed.
175	The return is dead.
251	Well, style recommends avoiding initialization in declaration. There you really provide constants, so perhaps either add const qualifiers to b/s/f names, or use #defines with more telling name, like NB_BUS etc.
274	Why do you do it this way ? Make the type of data member 'constr struct intel_stolen_ops ' directly. Also, style: add space before '', remove space after ')'.
278	Extra blank line.
291	Add blank line after '{'.
sys/dev/pci/pci_early_quirks.h
3	Don't you need to give the credit to the Intel' rights ? What is the copyright on the equivalent Linux code ?
sys/dev/pci/pcivar.h
730	I remember we discussed that the extern declarations should be moved to sys/amd64/include/md_var.h. I may be mis-remembered, but pcivar.h is a weird place for MD vars.
sys/i386/i386/machdep.c
2478	I am not sure that this makes sense to add, since you do not implemented anything for i386.

johalun0_gmail.com marked 15 inline comments as done.Oct 29 2018, 12:39 PM

johalun0_gmail.com added inline comments.

sys/dev/pci/pci_early_quirks.c
49	The plan was to make a function that could be used for many other things, other than only stolen memory, similar to what Linux does. Hence the generic pointer.
274	So that data can be a generic pointer for to be used by other early quirk functions.
sys/dev/pci/pci_early_quirks.h
3	In Linux this file is a generic early quirks function that handles many different things including stolen memory. There are no copyright names at all.

Fix comments.

bwidawsk accepted this revision.Oct 30 2018, 8:30 PM

bwidawsk added inline comments.

sys/dev/pci/pci_early_quirks.c
276	I'd suggest a warning here if size == 0. Graphics will not work properly if size is 0. (Although maybe i915 code already does this, in which case ignore).

This revision is now accepted and ready to land.Oct 30 2018, 8:30 PM

I will commit this after some local tests.

See D17775.

Closed by commit rS339979: Add pci_early function to detect Intel stolen memory. (authored by kib). · Explain WhyOct 31 2018, 11:17 PM

This revision was automatically updated to reflect the committed changes.

Add pci_early function for Intel stolen memory (and possibly more)
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 48781

sys/amd64/amd64/machdep.c

sys/conf/files

sys/dev/pci/pci_early_quirks.h

sys/dev/pci/pci_early_quirks.c

sys/dev/pci/pcivar.h

sys/i386/i386/machdep.c

Add pci_early function for Intel stolen memory (and possibly more)ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 48781

sys/amd64/amd64/machdep.c

sys/conf/files

sys/dev/pci/pci_early_quirks.h

sys/dev/pci/pci_early_quirks.c

sys/dev/pci/pcivar.h

sys/i386/i386/machdep.c

Add pci_early function for Intel stolen memory (and possibly more)
ClosedPublic
Actions

Revision Contents
Changeset List