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

[suijingfeng]

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