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?