Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <79c3b66a-61b0-aafb-0935-975b0ce1ecb3@loongson.cn>
Date:   Tue, 22 Aug 2023 10:37:56 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less
 arch-dependent
Content-Language: en-US
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        loongson-kernel@lists.loongnix.cn, linux-pci@vger.kernel.org,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Emil Velikov <emil.velikov@collabora.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        "Deucher, Alexander" <Alexander.Deucher@amd.com>,
        "Limonciello, Mario" <Mario.Limonciello@amd.com>
References: <20230821173819.GA362570@bhelgaas>
From:   suijingfeng <suijingfeng@loongson.cn>
In-Reply-To: <20230821173819.GA362570@bhelgaas>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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