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

[Bjorn Helgaas <helgaas@kernel.org>]

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.

Bjorn