Hi,
On 2023/8/22 01:38, Bjorn Helgaas wrote:
On Fri, Aug 18, 2023 at 12:09:29PM +0800, suijingfeng wrote:
On 2023/8/18 06:08, Bjorn Helgaas wrote:
On Wed, Aug 16, 2023 at 06:05:27AM +0800, Sui Jingfeng wrote:
Currently, the vga_is_firmware_default() function only works on x86 and
ia64, it is a no-op on ARM, ARM64, PPC, RISC-V, etc. This patch completes
the implementation for the rest of the architectures. The added code tries
to identify the PCI(e) VGA device that owns the firmware framebuffer
before PCI resource reallocation happens.
As far as I can tell, this is basically identical to the existing
vga_is_firmware_default(), except that this patch funs that code as a
header fixup, so it happens before any PCI BAR reallocations happen.
Yes, what you said is right in overall.
But I think I should mention a few tiny points that make a difference.
1) My version is *less arch-dependent*
Of course. If we make the patch simple and the commit log simple by
removing extraneous details, this will all be obvious.
2) My version focus on the address in ranges, weaken the size parameter.
Which make the code easy to read and follow the canonical convention to
express the address range. while the vga_is_firmware_default() is not.
Whether it's start/size or start/end is a trivial question. We don't
need to waste time on it now.
3) A tiny change make a big difference.
The original vga_is_firmware_default() only works with the assumption
that the PCI resource reallocation won't happens. While I see no clue
that why this is true even on X86 and IA64. The original patch[1] not
mention this assumption explicitly.
[1] 86fd887b7fe3 ('vgaarb: Don't default exclusively to first video device with mem+io')
That sounds like a good idea, because this is all based on the
framebuffer in screen_info, and screen_info was initialized before PCI
enumeration, and it certainly doesn't account for any BAR changes done
by the PCI core.
Yes.
So why would we keep vga_is_firmware_default() at all? If the header
fixup has already identified the firmware framebuffer, it seems
pointless to look again later.
It need another patch to do the cleanup work, while my patch just
add code to solve the real problem. It focus on provide a solution
for the architectures which have a decent way set up the
screen_info. Other things except that is secondary.
I don't want both mechanisms when only one of them is useful. PCI BAR
reassignment is completely fine, and keeping the assumption in
vga_is_firmware_default() that we can compare reassigned BAR values to
the pre-reassignment screen_info range is a trap that we should
remove.
OK,it's clear now. I know what to do next.
Thanks.
Bjorn
