[+cc linux-pci; I don't apply or ack PCI patches unless they appear there]
On Wed, Jul 12, 2023 at 12:43:04AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
>
> The observation behind this is that we should avoid accessing the global
> screen_info directly. Call the aperture_contain_firmware_fb_nonreloc()
> function to implement the detection of whether an aperture contains the
> firmware FB.
Because it's better to access the global screen_info from
aperture_contain_firmware_fb_nonreloc()? The reasoning here is not
super clear to me.
> This patch helps to decouple the determination from the implementation.
> Or, in other words, we intend to make the determination opaque to the
> caller. The determination may choose to be arch-dependent or
> arch-independent. But vgaarb, as a consumer of the determination,
> shouldn't care how the does determination is implemented.
"how the determination ..." (drop the "does")
Are you saying that aperture_contain_firmware_fb_nonreloc() might be
arch-dependent? Are there multiple callers? Or does this just move
code from one place to a more appropriate place?
> Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> ---
> drivers/pci/vgaarb.c | 19 ++++---------------
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index bf96e085751d..953daf731b2c 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -14,6 +14,7 @@
> #define vgaarb_info(dev, fmt, arg...) dev_info(dev, "vgaarb: " fmt, ##arg)
> #define vgaarb_err(dev, fmt, arg...) dev_err(dev, "vgaarb: " fmt, ##arg)
>
> +#include <linux/aperture.h>
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/pci.h>
> @@ -26,7 +27,6 @@
> #include <linux/poll.h>
> #include <linux/miscdevice.h>
> #include <linux/slab.h>
> -#include <linux/screen_info.h>
> #include <linux/vt.h>
> #include <linux/console.h>
> #include <linux/acpi.h>
> @@ -558,20 +558,11 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> }
> EXPORT_SYMBOL(vga_put);
>
> +/* Select the device owning the boot framebuffer if there is one */
> static bool vga_is_firmware_default(struct pci_dev *pdev)
> {
> #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - u64 base = screen_info.lfb_base;
> - u64 size = screen_info.lfb_size;
> struct resource *r;
> - u64 limit;
> -
> - /* Select the device owning the boot framebuffer if there is one */
> -
> - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> - base |= (u64)screen_info.ext_lfb_base << 32;
> -
> - limit = base + size;
>
> /* Does firmware framebuffer belong to us? */
> pci_dev_for_each_resource(pdev, r) {
> @@ -581,10 +572,8 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
> if (!r->start || !r->end)
> continue;
>
> - if (base < r->start || limit >= r->end)
> - continue;
> -
> - return true;
> + if (aperture_contain_firmware_fb_nonreloc(r->start, r->end))
> + return true;
> }
> #endif
> return false;
> --
> 2.25.1
>
