From: Huacai Chen <chenhuacai@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Huacai Chen <chenhuacai@loongson.cn>,
Bjorn Helgaas <bhelgaas@google.com>,
linux-pci <linux-pci@vger.kernel.org>,
Jiaxun Yang <jiaxun.yang@flygoat.com>,
Jingfeng Sui <suijingfeng@loongson.cn>
Subject: Re: [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge
Date: Tue, 25 May 2021 19:03:05 +0800 [thread overview]
Message-ID: <CAAhV-H7D-drrEaDskQhVx0c8_VAy--n3mbsQN_ijfWrRQGVQ=A@mail.gmail.com> (raw)
In-Reply-To: <20210519193327.GA248667@bjorn-Precision-5520>
Hi, Bjorn,
On Thu, May 20, 2021 at 3:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, May 19, 2021 at 10:17:14AM +0800, Huacai Chen wrote:
> > On Wed, May 19, 2021 at 3:35 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Tue, May 18, 2021 at 03:13:43PM +0800, Huacai Chen wrote:
> > > > On Tue, May 18, 2021 at 2:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Mon, May 17, 2021 at 08:53:43PM +0800, Huacai Chen wrote:
> > > > > > On Sat, May 15, 2021 at 5:09 PM Huacai Chen <chenhuacai@gmail.com> wrote:
> > > > > > > On Fri, May 14, 2021 at 11:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > On Fri, May 14, 2021 at 04:00:25PM +0800, Huacai Chen wrote:
> > > > > > > > > According to PCI-to-PCI bridge spec, bit 3 of Bridge Control Register is
> > > > > > > > > VGA Enable bit which modifies the response to VGA compatible addresses.
> > > > > > > >
> > > > > > > > The bridge spec is pretty old, and most of the content has been
> > > > > > > > incorporated into the PCIe spec. I think you can cite "PCIe r5.0, sec
> > > > > > > > 7.5.1.3.13" here instead.
> > > > > > > >
> > > > > > > > > If the VGA Enable bit is set, the bridge will decode and forward the
> > > > > > > > > following accesses on the primary interface to the secondary interface.
> > > > > > > >
> > > > > > > > *Which* following accesses? The structure of English requires that if
> > > > > > > > you say "the following accesses," you must continue by *listing* the
> > > > > > > > accesses.
> > > > > > > >
> > > > > > > > > The ASpeed AST2500 hardward does not set the VGA Enable bit on its
> > > > > > > > > bridge control register, which causes vgaarb subsystem don't think the
> > > > > > > > > VGA card behind the bridge as a valid boot vga device.
> > > > > > > >
> > > > > > > > s/hardward/bridge/
> > > > > > > > s/vga/VGA/ (also in code comments and dmesg strings below)
> > > > > > > >
> > > > > > > > From the code, it looks like AST2500 ([1a03:2000]) is a VGA device,
> > > > > > > > since it apparently has a VGA class code. But here you say the
> > > > > > > > AST2500 has a Bridge Control register, which suggests that it's a
> > > > > > > > bridge. If AST2500 is some sort of combination that includes both a
> > > > > > > > bridge and a VGA device, please outline that topology.
> > > > > > > >
> > > > > > > > But the hardware defect is that some bridges forward VGA accesses even
> > > > > > > > though their VGA Enable bit is not set? The quirk should be attached
> > > > > > > > to broken *bridges*, not to VGA devices.
> > > > > > > >
> > > > > > > > If a bridge forwards VGA accesses regardless of how its VGA Enable bit
> > > > > > > > is set, that means VGA arbitration (in vgaarb.c) cannot work
> > > > > > > > correctly, so merely setting the default VGA device once in a quirk is
> > > > > > > > not sufficient. You would have to somehow disable any future attempts
> > > > > > > > to use other VGA devices. Only the VGA device below this defective
> > > > > > > > bridge is usable. Any other VGA devices in the system would be
> > > > > > > > useless.
> > > > > > > >
> > > > > > > > > So we provide a quirk to fix Xorg auto-detection.
> > > > > > > > >
> > > > > > > > > See similar bug:
> > > > > > > > >
> > > > > > > > > https://patchwork.kernel.org/project/linux-pci/patch/20170619023528.11532-1-dja@axtens.net/
> > > > > > > >
> > > > > > > > This patch was never merged. If we merged a revised version, please
> > > > > > > > cite the SHA1 instead.
> > > > > > >
> > > > > > > This patch has never merged, and I found that it is unnecessary after
> > > > > > > commit a37c0f48950b56f6ef2ee637 ("vgaarb: Select a default VGA device
> > > > > > > even if there's no legacy VGA"). Maybe this ASpeed patch is also
> > > > > > > unnecessary. If it is still needed, I'll investigate the root cause.
> > > > > >
> > > > > > I found that vga_arb_device_init() and pcibios_init() are both wrapped
> > > > > > by subsys_initcall(), which means their sequence is unpredictable. And
> > > > > > unfortunately, in our platform vga_arb_device_init() is called before
> > > > > > pcibios_init(), which makes vga_arb_device_init() fail to set a
> > > > > > default vga device. This is the root cause why we thought that we
> > > > > > still need a quirk for AST2500.
> > > > >
> > > > > Does this mean there is no hardware defect here? The VGA Enable bit
> > > > > works correctly?
> > > > >
> > > > No, VGA Enable bit still doesn't set, but with commit
> > > > a37c0f48950b56f6ef2ee637 ("vgaarb: Select a default VGA device even if
> > > > there's no legacy VGA") we no longer depend on VGA Enable.
> > >
> > > Correct me if I'm wrong:
> > >
> > > - On the AST2500 bridge [1a03:1150], the VGA Enable bit is
> > > read-only 0.
> > >
> > > - The AST2500 bridge never forwards VGA accesses ([mem
> > > 0xa0000-0xbffff], [io 0x3b0-0x3bb], [io 0x3c0-0x3df]) to its
> > > secondary bus.
> > >
> > > The VGA Enable bit is optional, and if both the above are true, the
> > > bridge is working correctly per spec, and the quirk below is not the
> > > right solution, and whatever solution we come up with should not
> > > claim that the bridge is misbehaving.
> > Yes, you are right, the bridge is working correctly, which is similar
> > to HiSilicon D05.
> >
> >
> > >
> > > > > > I think the best solution is make vga_arb_device_init() be wrapped by
> > > > > > subsys_initcall_sync(), do you think so?
> > > > >
> > > > > Hmm. Unfortunately the semantics of subsys_initcall_sync() are not
> > > > > documented, so I'm not sure exactly *why* such a change would work and
> > > > > whether we could rely on it to continue working.
> > > > >
> > > > > pcibios_init() isn't very consistent across arches. On some,
> > > > > including alpha, microblaze, some MIPS platforms, powerpc, and sh, it
> > > > > enumerates PCI devices. On others (ia64, parisc, sparc, x86), it does
> > > > > basically nothing. That makes life a little difficult.
> > > >
> > > > subsys_initcall_sync() is ensured after all subsys_initcall()
> > > > functions, so at least it can solve the problem on platforms which use
> > > > pcibios_init() to enumerate PCI devices (x86 and other ACPI-based
> > > > platforms are also OK, because they use acpi_init()
> > > > -->acpi_scan_init() -->pci_acpi_scan_root() to enumerate devices).
> > >
> > > More details in my response to suijingfeng:
> > > https://lore.kernel.org/r/20210518193100.GA148462@bjorn-Precision-5520
> > >
> > > I'd rather not fiddle with the initcall ordering. That mechanism is
> > > fragile and I'd prefer something more robust.
> > >
> > > I'm wondering whether it's practical to do something in the normal PCI
> > > enumeration path, e.g., in pci_init_capabilities(). Maybe we can
> > > detect the default VGA device as we enumerate it. Then we wouldn't
> > > have this weird process of "find all PCI devices first, then scan for
> > > the default VGA device, and oh, by the way, also check for VGA devices
> > > hot-added later."
> >
> > If we don't want to rely on initcall order, and want to solve the
> > hot-added case, then can we add vga_arb_select_default_device() in
> > pci_notify() when (action == BUS_NOTIFY_ADD_DEVICE &&
> > !vga_default_device())?
>
> I think I would see if it's possible to call
> vga_arb_select_default_device() from vga_arbiter_add_pci_device()
> instead of from vga_arb_device_init().
>
> I would also (as a separate patch) try to get rid of this loop in
> vga_arb_device_init():
>
> list_for_each_entry(vgadev, &vga_list, list) {
> struct device *dev = &vgadev->pdev->dev;
>
> if (vgadev->bridge_has_one_vga)
> vgaarb_info(dev, "bridge control possible\n");
> else
> vgaarb_info(dev, "no bridge control possible\n");
> }
>
> and do the vgaarb_info() in vga_arbiter_check_bridge_sharing(), where
> the loop would not be needed.
Any updates?
Huacai
>
> Bjorn
next prev parent reply other threads:[~2021-05-25 11:03 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-14 8:00 [PATCH 0/5] PCI: Loongson-related pci quirks Huacai Chen
2021-05-14 8:00 ` [PATCH 1/5] PCI/portdrv: Don't disable pci device during shutdown Huacai Chen
2021-05-14 14:20 ` Krzysztof Wilczyński
2021-05-14 16:09 ` Bjorn Helgaas
2021-05-15 3:38 ` Huacai Chen
2021-05-14 8:00 ` [PATCH 2/5] PCI: Move loongson pci quirks to quirks.c Huacai Chen
2021-05-14 8:00 ` [PATCH 3/5] PCI: Improve the mrrs quirk for LS7A Huacai Chen
2021-05-14 14:03 ` Krzysztof Wilczyński
2021-05-14 15:39 ` Bjorn Helgaas
2021-05-15 3:49 ` Huacai Chen
2021-05-15 6:22 ` Jiaxun Yang
2021-05-14 8:00 ` [PATCH 4/5] PCI: Add quirk for multifunction devices of LS7A Huacai Chen
2021-05-14 13:22 ` Krzysztof Wilczyński
2021-05-14 14:52 ` Jiaxun Yang
2021-05-15 3:52 ` Huacai Chen
2021-05-18 13:59 ` Bjorn Helgaas
2021-05-19 2:26 ` Huacai Chen
2021-05-19 3:05 ` Jiaxun Yang
2021-05-14 15:51 ` Bjorn Helgaas
2021-05-15 3:56 ` Huacai Chen
2021-05-14 8:00 ` [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge Huacai Chen
2021-05-14 13:56 ` Krzysztof Wilczyński
2021-05-14 15:10 ` Bjorn Helgaas
2021-05-15 9:09 ` Huacai Chen
2021-05-17 12:53 ` Huacai Chen
2021-05-17 18:28 ` Bjorn Helgaas
2021-05-18 2:31 ` 隋景峰
2021-05-18 3:09 ` Bjorn Helgaas
2021-05-18 9:30 ` suijingfeng
2021-05-18 19:31 ` Bjorn Helgaas
2021-05-18 7:13 ` Huacai Chen
2021-05-18 19:35 ` Bjorn Helgaas
2021-05-19 2:17 ` Huacai Chen
2021-05-19 19:33 ` Bjorn Helgaas
2021-05-25 11:03 ` Huacai Chen [this message]
2021-05-25 13:55 ` Bjorn Helgaas
2021-05-26 2:33 ` Huacai Chen
2021-05-26 3:00 ` Dave Airlie
2021-05-26 18:29 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAhV-H7D-drrEaDskQhVx0c8_VAy--n3mbsQN_ijfWrRQGVQ=A@mail.gmail.com' \
--to=chenhuacai@kernel.org \
--cc=bhelgaas@google.com \
--cc=chenhuacai@loongson.cn \
--cc=helgaas@kernel.org \
--cc=jiaxun.yang@flygoat.com \
--cc=linux-pci@vger.kernel.org \
--cc=suijingfeng@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).