Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31502
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
[WIP]device/pci_rom: Add ATRM method
Add ATRM method for AMD GPUs, which is similar to _ROM. As both functions seem to have identical arguments and return values, redirect ATRM to _ROM.
Don't depend on VGA devices, but display devices, as some AMD dGPUs identify themself as PCI_CLASS_DISPLAY_OTHER.
Needs test on platforms with AMD display devices.
Change-Id: Id2212f29f0de051f6cbd59f8332e86696c1166ab Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/device/pci_rom.c 1 file changed, 15 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/31502/1
diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 82d9a30..86c839c 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -272,8 +272,8 @@ { static size_t ngfx;
- /* Only handle VGA devices */ - if ((device->class >> 8) != PCI_CLASS_DISPLAY_VGA) + /* Only handle display devices */ + if ((device->class >> 16) != PCI_BASE_CLASS_DISPLAY) return;
/* Only handle enabled devices */ @@ -319,9 +319,21 @@
memcpy(cbrom, rom, cbrom_length);
- /* write _ROM method */ + /* write ACPI methods */ acpigen_write_scope(scope); + + /* Nvidia uses _ROM */ acpigen_write_rom(cbrom, cbrom_length); + + /* AMD uses ATRM. Redirect ATRM to _ROM */ + acpigen_write_method_serialized("ATRM", 2); + acpigen_write_to_integer(ARG1_OP, LOCAL0_OP); + acpigen_emit_byte(RETURN_OP); + acpigen_emit_namestring("_ROM"); + acpigen_emit_byte(ARG0_OP); + acpigen_emit_byte(LOCAL0_OP); + acpigen_pop_len(); /* pop method */ + acpigen_pop_len(); /* pop scope */ } #endif
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c@326 PS1, Line 326: acpigen_write_rom(cbrom, cbrom_length); Do we still need _ROM in AMD case? If not, how about passing the method name here? Then we wouldn't need the additional, redirecting method.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c@326 PS1, Line 326: acpigen_write_rom(cbrom, cbrom_length);
Do we still need _ROM in AMD case? If not, how about passing the method […]
At least the AMD driver in linux doesn't need _ROM. But _ROM is defined in ACPI spec, and it costs us nothing. It looks like the kernel uses the _ROM method to identify ACPI compatible "video devices".
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
Please see my recent messages at CB:31448 . In short, this patch above, in its' current revision: is not fixing the things but is not breaking them either ( more details at this message - https://review.coreboot.org/c/coreboot/+/31448#message-8f77411787cf2f98295a7... ) so I am not voting on it yet but will be following.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c@326 PS1, Line 326: acpigen_write_rom(cbrom, cbrom_length);
At least the AMD driver in linux doesn't need _ROM. […]
Ack, I thought it might be worth to keep _ROM for VGA devs only, but the spec really doesn't say what devices exactly.
I noticed, though, that it only applies to devices without a real ROM. But pci_rom_probe() would also return that? Should we bail out on non-onboard devices maybe?
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c@328 PS1, Line 328: /* AMD uses ATRM. Redirect ATRM to _ROM */ This is run, too, on the primary VGA device. Intended?
I don't look through the whole thing yet. Is it allowed/ possible to make the AMD dGPU the primary VGA device?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
Patch Set 1:
Please see my recent messages at CB:31448 . In short, this patch above, in its' current revision: is not fixing the things but is not breaking them either ( more details at this message - https://review.coreboot.org/c/coreboot/+/31448#message-8f77411787cf2f98295a7... ) so I am not voting on it yet but will be following.
Please provide a coreboot log. Do the ATRM entries show up in ssdt ? Does Linux attempt to use them ? Are the PCIe slot's ACPI path correct ?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31502/1/src/device/pci_rom.c@328 PS1, Line 328: /* AMD uses ATRM. Redirect ATRM to _ROM */
This is run, too, on the primary VGA device. Intended? […]
No, if VFCT is in place. IDK for the second question.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Please see my recent messages at CB:31448 . In short, this patch above, in its' current revision: is not fixing the things but is not breaking them either ( more details at this message - https://review.coreboot.org/c/coreboot/+/31448#message-8f77411787cf2f98295a7... ) so I am not voting on it yet but will be following.
Please provide a coreboot log. Do the ATRM entries show up in ssdt ? Does Linux attempt to use them ?
Here is a coreboot cbmem log - https://pastebin.com/QUmNfTPH . Compared to my earlier cbmem log, the only difference observed with the addition of your patch is the insertion of 462 - 466 lines: coreboot tried to access pci1002,6665.rom an extra time while doing ACPI but no ATRM appeared.
Also compared the Linux kernel logs (please let me know if I need to upload because I'd need to clean from serial numbers) - and there are no changes at all, no ATRM either.
Please note that this comparison has been done for "CB:31448 / CB:31449 / CB:31450 applied" to get dGPU working (with VFCT done for dGPU instead of iGPU) vs " ---/the same set/--- + your patch ". I also checked the logs for a build where I tried doing VFCT the way you suggested (for iGPU instead of dGPU, with dGPU failing to initialize later) and there are no ATRM as well.
Are the PCIe slot's ACPI path correct ?
Sorry don't know how to check it, but hopefully it could be visible from cbmem log above. If you'd like you could insert some debug prints to your patch or somehow improve it and I could re-test it.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Please see my recent messages at CB:31448 . In short, this patch above, in its' current revision: is not fixing the things but is not breaking them either ( more details at this message - https://review.coreboot.org/c/coreboot/+/31448#message-8f77411787cf2f98295a7... ) so I am not voting on it yet but will be following.
Please provide a coreboot log. Do the ATRM entries show up in ssdt ? Does Linux attempt to use them ?
Here is a coreboot cbmem log - https://pastebin.com/QUmNfTPH . Compared to my earlier cbmem log, the only difference observed with the addition of your patch is the insertion of 462 - 466 lines: coreboot tried to access pci1002,6665.rom an extra time while doing ACPI but no ATRM appeared.
Also compared the Linux kernel logs (please let me know if I need to upload because I'd need to clean from serial numbers) - and there are no changes at all, no ATRM either.
Please note that this comparison has been done for "CB:31448 / CB:31449 / CB:31450 applied" to get dGPU working (with VFCT done for dGPU instead of iGPU) vs " ---/the same set/--- + your patch ". I also checked the logs for a build where I tried doing VFCT the way you suggested (for iGPU instead of dGPU, with dGPU failing to initialize later) and there are no ATRM as well.
Are the PCIe slot's ACPI path correct ?
Sorry don't know how to check it, but hopefully it could be visible from cbmem log above. If you'd like you could insert some debug prints to your patch or somehow improve it and I could re-test it.
It prints "PCI: 00:01.0: Missing ACPI scope" That was fixed on Sandybridge using: https://review.coreboot.org/c/coreboot/+/22337 You might need a similar implementation that returns a valid "acpi name" for PCI: 00:01.0 and PCI: 01:00.0.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
Patch Set 1:
It prints "PCI: 00:01.0: Missing ACPI scope" That was fixed on Sandybridge using: https://review.coreboot.org/c/coreboot/+/22337 You might need a similar implementation that returns a valid "acpi name" for PCI: 00:01.0 and PCI: 01:00.0.
Thank you for reply, although this change above seems too hard for me to execute properly - and to be honest it is hard for me to justify working on it since my dGPU is already working, unless these ATRMs could be somehow helpful later for Crossfire.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
Thank you for reply, although this change above seems too hard for me to execute properly
We can help you with that. It's not a reason to give up. Either we fix that AMD code properly, or there'll be no further upstreaming.
- and to be honest it is hard for me to justify working on it since my dGPU is already working, unless these ATRMs could be somehow helpful later for Crossfire.
If you want it upstream, it is not only about your dGPU and we'll have to find a maintainable solution (some trial-by-error code that already nobody understands when merging is not what I'd consider maintainable).
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
Patch Set 1:
and we'll have to find a maintainable solution (some trial-by-error code that already nobody understands when merging is not what I'd consider maintainable).
Nico, could you please clarify what exact parts of my/HJK patches are still unclear? If "running two OptionROMs" part is raising the questions, I will happily change it to "run for iGPU only" - actually HJK original patches were already done this way. If "already existing code for MULTIPLE_VGA_ADAPTERS" part - I have fulfilled your testing requests at this message https://review.coreboot.org/c/coreboot/+/31448#message-86a7c4f33a958847d16f9... . And I already replied why "VFCT for dGPU instead of iGPU" and many other things. It just seems to me that nobody read my new messages under CB:31448 (sorry if "TLDR"), and all this is really disappointing - got more hardware working but nobody is happy :-(
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31502 )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Patch Set 1:
and we'll have to find a maintainable solution (some trial-by-error code that already nobody understands when merging is not what I'd consider maintainable).
Nico, could you please clarify what exact parts of my/HJK patches are still unclear? If "running two OptionROMs" part is raising the questions, I will happily change it to "run for iGPU only" - actually HJK original patches were already done this way. If "already existing code for MULTIPLE_VGA_ADAPTERS" part - I have fulfilled your testing requests at this message https://review.coreboot.org/c/coreboot/+/31448#message-86a7c4f33a958847d16f9... . And I already replied why "VFCT for dGPU instead of iGPU" and many other things. It just seems to me that nobody read my new messages under CB:31448 (sorry if "TLDR"), and all this is really disappointing - got more hardware working but nobody is happy :-(
I don't recall mentioning anything to be unclear. In fact, I still didn't look at most of the patches. IMHO, it's not worth to review before the ground work it done. And that is to figure out what is supposed to work and not what does work. Your/HJK's whole solution seems to be based on trial- by-error, which means it might just work by chance and with the next kernel update it may break. To prevent that, we need to figure out how exactly AMD intended it to work. And if there is no public spec, then the OS driver's source is the only spec and we have to read that. This is how core- boot development works. I'm sorry that it's not all fun.
I have read your reply on the other thread and considered to investigate further today. But with already the second angree message that I write today, I have to postpone this to next week (better ping me, I might forget about it). So above all you'll need patience to get your changes upstream. And please stop arguing that it already works. We know that. Upstream development is not only about having something work but also about getting new things to work without dragging too much maintenance burden. Your device works, that's nice. But will you help everytime somebody is confused about code in `device/pci_rom.c`? Probably not. If we'd just merge every patch that "works", soon nothing would work anymore, because things affect each other upstream and nobody would still want to fix things in confusing, unreadable code.
I have no idea what makes you think that we'll all work 24/7 on your review. It takes time. When we don't reply to a comment, then most likely because we didn't have the time to do so. Speaking of work, Patrick just did the right thing and looked into the Linux driver. I assume, he wasn't paid for it. He probably just worked for you for free and your reaction seemed to be disappointment with the results. So, to sum it up, you are not willing to look into things for yourself, you don't pay people, you don't inspire them, you get upset when people don't do what you expect them to do. It seems pretty clear to me why your patches "have not been received well".
My personal roadmap on this issue is (and yes it may take weeks): * Understand the existing MULTIPLE_VGA_ADAPTERS code. * Get rid of it if possible. * Understand the intention behind VFCT and ATRM. * Make sure they can be added to ACPI correctly (what Patrick says that needs to be done here).
One question that would make this easier: Is the AMD dGPU supposed to work alone? i.e. are any display connectors attached (or muxable) to it? or does it only work as a computation accelerator for the iGPU?
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31502?usp=email )
Change subject: [WIP]device/pci_rom: Add ATRM method ......................................................................
Abandoned
-
mikeb mikeb (Code Review) -
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Stefan Reinauer (Code Review)