Change in ...coreboot[master]: [WIP]device/pci_rom: Add ATRM method

[Patrick Rudolph (Code Review)]

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 
---
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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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?

[S] Change in coreboot[master]: [WIP]device/pci_rom: Add ATRM method

[Stefan Reinauer (Code Review)]

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