Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proper AMD Integrated Graphics detection. #97

Merged
merged 12 commits into from
Aug 27, 2024

Conversation

Zormeister
Copy link
Contributor

@Zormeister Zormeister commented Aug 8, 2024

As the title implies, it's a proper implementation for detecting the iGPU device.

This targets more "recent" iGPUs.

Notably those of the:

  • Trinity and Richland family,
  • Kaveri/Kabini/Mullins Family,
  • Carrizo and Stoney family,
  • Mobile Vega ASIC family

Copy link
Collaborator

@vit9696 vit9696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the current code break that you added hardcoded device identifiers, which are going to break far more often as new hardware revisions are released?

Lilu/Sources/kern_devinfo.cpp Outdated Show resolved Hide resolved
Lilu/Sources/kern_devinfo.cpp Outdated Show resolved Hide resolved
Lilu/Sources/kern_devinfo.cpp Outdated Show resolved Hide resolved
@Zormeister
Copy link
Contributor Author

Zormeister commented Aug 8, 2024

Where does the current code break that you added hardcoded device identifiers, which are going to break far more often as new hardware revisions are released?

The old HDA detection code breaks for legacy systems somewhere down the line, AMD’s Device IDs and GC versions are pretty hardcoded in of themselves, you have to know what versions (GC) map to a device or get the APU flag set under AMDGPU

There’s a handy reference list for GPUs on Techpowerup, other than that, it’s a guessing game of what IP versions exist on a certain GPU.

@Zormeister Zormeister requested a review from vit9696 August 8, 2024 22:20
@vit9696
Copy link
Collaborator

vit9696 commented Aug 9, 2024

I.e. this is entirely impossible to distinguish discrete PCI GPU from APU based on topology on AMD?

uint32_t dev = 0;
WIOKit::getOSDataValue(obj, "device-id", dev);
dev = (dev & 0xFF00);
if ((dev == 0x1600 || dev == 0x1500) || (dev == 0x9800 || dev == 0x1300) || (dev == 0x9900 || dev == 0x9600)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up having this code, these numbers must become named constants, so that it is clear which id belongs to which family.

Copy link
Contributor Author

@Zormeister Zormeister Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for this, the &ing makes the Device ID range more generic so there isn’t a million cases.

0x1500 is commonly associated with AMD’s Raven line, 0x1600 catches devices from Renoir, Van Gogh and Rembrandt

0x1300 catches Kaveri (Spectre) and a certain Granite Ridge iGPU (0x13C0)

0x9800 catches Carrizo, Mullins (Godavari), Kabini (Kalindi) and Stoney

0x9600 and 0x9900 catch some of the legacy TeraScale iGPUs such as Trinity and Richland iirc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information must be in the code, not in the comments to the review. Unnamed numeric literals are plain bad, make them named constants.

Copy link
Contributor Author

@Zormeister Zormeister Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was going to do this already, got caught up in other things
pushed out the changes now

lmk if the names need changing at all

@Zormeister
Copy link
Contributor Author

Zormeister commented Aug 9, 2024

I.e. this is entirely impossible to distinguish discrete PCI GPU from APU based on topology on AMD?

AFAIK, The only way to do it on the PCI device is by doing what I did.

There is no ‘isAPU’ anywhere without having to use hardcoded values to my knowledge

@jalavoui
Copy link

i lke the idea but why not use linux constants ? this would help in future updates.

from amd_asic_type.h
enum amd_asic_type {
CHIP_TAHITI = 0,
CHIP_PITCAIRN, /* 1 /
CHIP_VERDE, /
2 /
CHIP_OLAND, /
3 */
(...)

and from amdgpu_drv.c
/* Navi14 */
{0x1002, 0x7340, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14},
{0x1002, 0x7341, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14},
{0x1002, 0x7347, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14},
{0x1002, 0x734F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14},

/* Renoir */
{0x1002, 0x15E7, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},
{0x1002, 0x1636, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},
{0x1002, 0x1638, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},
{0x1002, 0x164C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},

(...)

note that the flags also help cause we got AMD_IS_APU for proper devices

@Zormeister
Copy link
Contributor Author

Zormeister commented Aug 20, 2024

i lke the idea but why not use linux constants ? this would help in future updates.

from amd_asic_type.h enum amd_asic_type { CHIP_TAHITI = 0, CHIP_PITCAIRN, /* 1 / CHIP_VERDE, / 2 / CHIP_OLAND, / 3 */ (...)

and from amdgpu_drv.c /* Navi14 */ {0x1002, 0x7340, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14}, {0x1002, 0x7341, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14}, {0x1002, 0x7347, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14}, {0x1002, 0x734F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_NAVI14},

/* Renoir */
{0x1002, 0x15E7, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},
{0x1002, 0x1636, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},
{0x1002, 0x1638, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},
{0x1002, 0x164C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_RENOIR|AMD_IS_APU},

(...)

note that the flags also help cause we got AMD_IS_APU for proper devices

the generic constants help to reduce complexity, but also Linux isn't a reliable source (for newer devices) since APU detection shifted to using the IP Discovery binary and marking the device as an APU from the GC version, see here

devices such as Granite Ridge and Raphael aren't in the AMDGPU driver itself, leading to whoever wanting to find the Device ID to have to use external places, such as pcilookup.com, pciids, GPUOpen-Tools/device_info (and even then, this specific one hasn't gotten an update for a while), etc.

@Zormeister Zormeister requested a review from vit9696 August 27, 2024 08:15
@vit9696 vit9696 merged commit 8ae3167 into acidanthera:master Aug 27, 2024
vit9696 added a commit that referenced this pull request Aug 27, 2024
This reverts commit 8ae3167.
The change breaks GCN 5 IGPU detection at least.
references acidanthera/bugtracker#2424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants