Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent

[suijingfeng]

Hi,


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*


Again, since the global screen_info is arch-dependent.
The vga_is_firmware_default() mess up the arch-dependent part and arch-independent part.
It's a mess and it's a bit harder to make the cleanup on the top of it.

While my version is my version split the arch-dependent part and arch-independent part clearly.
Since we decide to make it less arch-dependent, we have to bear the pain.
Despite all other arches should always export the screen_info like the X86 and IA64 arch does,
or at least a arch should give a Kconfig token (for example, CONFIG_ARCH_HAS_SCREEN_INFO) to
demonstrate that an arch has the support for it.
While currently, the fact is that the dependence just populated to everywhere.
I think this is the hard part, you have to investigate how various arches defines and set up
the screen_info. And then process dependency and the linkage problem across arch properly.


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.


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.