It's just open coded and matches.
Note that Thomas said that his version apparently failed for some
reason, but hey maybe we should try again.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Dave Airlie <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: Javier Martinez Canillas <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: [email protected]
---
drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 420fc75c240e..3ac24a780f50 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
MODULE_DEVICE_TABLE(pci, ast_pciidlist);
-static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
-{
- bool primary = false;
- resource_size_t base, size;
-
- base = pci_resource_start(pdev, 0);
- size = pci_resource_len(pdev, 0);
-#ifdef CONFIG_X86
- primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
-#endif
-
- return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
-}
-
static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
struct ast_private *ast;
struct drm_device *dev;
int ret;
- ret = ast_remove_conflicting_framebuffers(pdev);
+ ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
if (ret)
return ret;
--
2.39.0
Again this just gets setting the primary parameter right, which might
help in some case (but then I guess the hyperv display isn't vga
compatible, I have no idea). It's more consistent for sure.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: [email protected]
---
drivers/video/fbdev/hyperv_fb.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index fdbf02b42723..1067a64bbdf3 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -1076,9 +1076,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
info->screen_size = dio_fb_size;
getmem_done:
- aperture_remove_conflicting_devices(info->apertures->ranges[0].base,
- info->apertures->ranges[0].size,
- false, KBUILD_MODNAME);
+ aperture_remove_conflicting_pci_devices(pdev, KBUILD_MODNAME);
if (gen2vm) {
/* framebuffer is reallocated, clear screen_info to avoid misuse from kexec */
--
2.39.0
Only really pci devices have a business setting this - it's for
figuring out whether the legacy vga stuff should be nuked too. And
with the preceeding two patches those are all using the pci version of
this.
Which means for all other callers primary == false and we can remove
it now.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: Javier Martinez Canillas <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Deepak Rawat <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Martin Blumenstingl <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Jonathan Hunter <[email protected]>
Cc: Emma Anholt <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/arm/hdlcd_drv.c | 2 +-
drivers/gpu/drm/armada/armada_drv.c | 2 +-
drivers/gpu/drm/drm_aperture.c | 11 +++--------
drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 1 -
drivers/gpu/drm/meson/meson_drv.c | 2 +-
drivers/gpu/drm/msm/msm_fbdev.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +-
drivers/gpu/drm/stm/drv.c | 2 +-
drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +-
drivers/gpu/drm/tegra/drm.c | 2 +-
drivers/gpu/drm/vc4/vc4_drv.c | 2 +-
include/drm/drm_aperture.h | 7 +++----
12 files changed, 15 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 7043d1c9ed8f..98267e355918 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -297,7 +297,7 @@ static int hdlcd_drm_bind(struct device *dev)
*/
if (hdlcd_read(hdlcd, HDLCD_REG_COMMAND)) {
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
- drm_aperture_remove_framebuffers(false, &hdlcd_driver);
+ drm_aperture_remove_framebuffers(&hdlcd_driver);
}
drm_mode_config_reset(drm);
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 0643887800b4..c99ec7078301 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -95,7 +95,7 @@ static int armada_drm_bind(struct device *dev)
}
/* Remove early framebuffers */
- ret = drm_aperture_remove_framebuffers(false, &armada_drm_driver);
+ ret = drm_aperture_remove_framebuffers(&armada_drm_driver);
if (ret) {
dev_err(dev, "[" DRM_NAME ":%s] can't kick out simple-fb: %d\n",
__func__, ret);
diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
index 3b8fdeeafd53..697cffbfd603 100644
--- a/drivers/gpu/drm/drm_aperture.c
+++ b/drivers/gpu/drm/drm_aperture.c
@@ -32,17 +32,13 @@
*
* static int remove_conflicting_framebuffers(struct pci_dev *pdev)
* {
- * bool primary = false;
* resource_size_t base, size;
* int ret;
*
* base = pci_resource_start(pdev, 0);
* size = pci_resource_len(pdev, 0);
- * #ifdef CONFIG_X86
- * primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
- * #endif
*
- * return drm_aperture_remove_conflicting_framebuffers(base, size, primary,
+ * return drm_aperture_remove_conflicting_framebuffers(base, size,
* &example_driver);
* }
*
@@ -161,7 +157,6 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
* drm_aperture_remove_conflicting_framebuffers - remove existing framebuffers in the given range
* @base: the aperture's base address in physical memory
* @size: aperture size in bytes
- * @primary: also kick vga16fb if present
* @req_driver: requesting DRM driver
*
* This function removes graphics device drivers which use the memory range described by
@@ -171,9 +166,9 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
* 0 on success, or a negative errno code otherwise
*/
int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
- bool primary, const struct drm_driver *req_driver)
+ const struct drm_driver *req_driver)
{
- return aperture_remove_conflicting_devices(base, size, primary, req_driver->name);
+ return aperture_remove_conflicting_devices(base, size, false, req_driver->name);
}
EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers);
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index 427c20ba3404..7e81d58c083f 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -74,7 +74,6 @@ static int hyperv_setup_vram(struct hyperv_drm_device *hv,
drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
screen_info.lfb_size,
- false,
&hyperv_driver);
hv->fb_size = (unsigned long)hv->mmio_megabytes * 1024 * 1024;
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 79bfe3938d3c..c8d39809d897 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -285,7 +285,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
* Remove early framebuffers (ie. simplefb). The framebuffer can be
* located anywhere in RAM
*/
- ret = drm_aperture_remove_framebuffers(false, &meson_driver);
+ ret = drm_aperture_remove_framebuffers(&meson_driver);
if (ret)
goto free_drm;
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 31e1e30cb52a..84dfbccb6912 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -155,7 +155,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
}
/* the fw fb could be anywhere in memory */
- ret = drm_aperture_remove_framebuffers(false, dev->driver);
+ ret = drm_aperture_remove_framebuffers(dev->driver);
if (ret)
goto fini;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 6e0788d14c10..d97f2edc646b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -140,7 +140,7 @@ static int rockchip_drm_bind(struct device *dev)
int ret;
/* Remove existing drivers that may own the framebuffer memory. */
- ret = drm_aperture_remove_framebuffers(false, &rockchip_drm_driver);
+ ret = drm_aperture_remove_framebuffers(&rockchip_drm_driver);
if (ret) {
DRM_DEV_ERROR(dev,
"Failed to remove existing framebuffers - %d.\n",
diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 50410bd99dfe..354349c6e085 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -185,7 +185,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
DRM_DEBUG("%s\n", __func__);
- ret = drm_aperture_remove_framebuffers(false, &drv_driver);
+ ret = drm_aperture_remove_framebuffers(&drv_driver);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index cc94efbbf2d4..6367b89cbab1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -98,7 +98,7 @@ static int sun4i_drv_bind(struct device *dev)
goto cleanup_mode_config;
/* Remove early framebuffers (ie. simplefb) */
- ret = drm_aperture_remove_framebuffers(false, &sun4i_drv_driver);
+ ret = drm_aperture_remove_framebuffers(&sun4i_drv_driver);
if (ret)
goto cleanup_mode_config;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 7bd2e65c2a16..d2ff527cf6d7 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1252,7 +1252,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
drm_mode_config_reset(drm);
- err = drm_aperture_remove_framebuffers(false, &tegra_drm_driver);
+ err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
if (err < 0)
goto hub;
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 0ccaee57fe9a..0a9e922636b1 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -350,7 +350,7 @@ static int vc4_drm_bind(struct device *dev)
return -EPROBE_DEFER;
}
- ret = drm_aperture_remove_framebuffers(false, driver);
+ ret = drm_aperture_remove_framebuffers(driver);
if (ret)
return ret;
diff --git a/include/drm/drm_aperture.h b/include/drm/drm_aperture.h
index 7096703c3949..cbe33b49fd5d 100644
--- a/include/drm/drm_aperture.h
+++ b/include/drm/drm_aperture.h
@@ -13,14 +13,13 @@ int devm_aperture_acquire_from_firmware(struct drm_device *dev, resource_size_t
resource_size_t size);
int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
- bool primary, const struct drm_driver *req_driver);
+ const struct drm_driver *req_driver);
int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
const struct drm_driver *req_driver);
/**
* drm_aperture_remove_framebuffers - remove all existing framebuffers
- * @primary: also kick vga16fb if present
* @req_driver: requesting DRM driver
*
* This function removes all graphics device drivers. Use this function on systems
@@ -30,9 +29,9 @@ int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
* 0 on success, or a negative errno code otherwise
*/
static inline int
-drm_aperture_remove_framebuffers(bool primary, const struct drm_driver *req_driver)
+drm_aperture_remove_framebuffers(const struct drm_driver *req_driver)
{
- return drm_aperture_remove_conflicting_framebuffers(0, (resource_size_t)-1, primary,
+ return drm_aperture_remove_conflicting_framebuffers(0, (resource_size_t)-1,
req_driver);
}
--
2.39.0
It exists! Note that since this is an exact copy, there shouldn't be
any functional difference here.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Sudip Mukherjee <[email protected]>
Cc: Teddy Wang <[email protected]>
Cc: [email protected]
---
drivers/staging/sm750fb/sm750.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index effc7fcc3703..22ace3168723 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -989,20 +989,6 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
return err;
}
-static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev)
-{
- resource_size_t base = pci_resource_start(pdev, 0);
- resource_size_t size = pci_resource_len(pdev, 0);
- bool primary = false;
-
-#ifdef CONFIG_X86
- primary = pdev->resource[PCI_ROM_RESOURCE].flags &
- IORESOURCE_ROM_SHADOW;
-#endif
-
- return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");
-}
-
static int lynxfb_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
@@ -1011,7 +997,7 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
int fbidx;
int err;
- err = lynxfb_kick_out_firmware_fb(pdev);
+ err = aperture_remove_conflicting_pci_devices(pdev, "sm750_fb1");
if (err)
return err;
--
2.39.0
This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.
Signed-off-by: Daniel Vetter <[email protected]>
---
drivers/gpu/drm/gma500/psb_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index cd9c73f5a64a..9b0daf90dc50 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
* TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
* might be able to read the framebuffer range from the device.
*/
- ret = drm_aperture_remove_framebuffers(true, &driver);
+ ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
if (ret)
return ret;
--
2.39.0
Since vgaarb has been promoted to be a core piece of the pci subsystem
we don't have to open code random guesses anymore, we actually know
this in a platform agnostic way, and there's no need for an x86
specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
drivers/pci")
This should not result in any functional change, and the non-x86
multi-gpu pci systems are probably rare enough to not matter (I don't
know of any tbh). But it's a nice cleanup, so let's do it.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: Javier Martinez Canillas <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: [email protected]
Cc: Bjorn Helgaas <[email protected]>
Cc: [email protected]
---
drivers/video/aperture.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 41e77de1ea82..3d8c925c7365 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
*/
int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
{
- bool primary = false;
+ bool primary;
resource_size_t base, size;
int bar, ret;
-#ifdef CONFIG_X86
- primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
-#endif
+ primary = pdev == vga_default_device();
for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
--
2.39.0
Otherwise it's bit silly, and we might throw out the driver for the
screen the user is actually looking at. I haven't found a bug report
for this case yet, but we did get bug reports for the analog case
where we're throwing out the efifb driver.
References: https://bugzilla.kernel.org/show_bug.cgi?id=216303
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: Javier Martinez Canillas <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: [email protected]
---
drivers/video/aperture.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 3d8c925c7365..6f351a58f6c6 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -341,6 +341,9 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
return ret;
}
+ if (!primary)
+ return 0;
+
/*
* WARNING: Apparently we must kick fbdev drivers before vgacon,
* otherwise the vga fbdev driver falls over.
--
2.39.0
A few reasons for this:
- It's really the only one where this matters. I tried looking around,
and I didn't find any non-pci vga-compatible controllers for x86
(since that's the only platform where we had this until a few
patches ago), where a driver participating in the aperture claim
dance would interfere.
- I also don't expect that any future bus anytime soon will
not just look like pci towards the OS, that's been the case for like
25+ years by now for practically everything (even non non-x86).
- Also it's a bit funny if we have one part of the vga removal in the
pci function, and the other in the generic one.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: Javier Martinez Canillas <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: [email protected]
---
drivers/video/aperture.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 6f351a58f6c6..03f8a5e95238 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -298,14 +298,6 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
aperture_detach_devices(base, size);
- /*
- * If this is the primary adapter, there could be a VGA device
- * that consumes the VGA framebuffer I/O range. Remove this device
- * as well.
- */
- if (primary)
- aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
-
return 0;
}
EXPORT_SYMBOL(aperture_remove_conflicting_devices);
@@ -344,6 +336,13 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
if (!primary)
return 0;
+ /*
+ * If this is the primary adapter, there could be a VGA device
+ * that consumes the VGA framebuffer I/O range. Remove this device
+ * as well.
+ */
+ aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
+
/*
* WARNING: Apparently we must kick fbdev drivers before vgacon,
* otherwise the vga fbdev driver falls over.
--
2.39.0
Hi
Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It exists! Note that since this is an exact copy, there shouldn't be
> any functional difference here.
>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Sudip Mukherjee <[email protected]>
> Cc: Teddy Wang <[email protected]>
> Cc: [email protected]
> ---
> drivers/staging/sm750fb/sm750.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index effc7fcc3703..22ace3168723 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -989,20 +989,6 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
> return err;
> }
>
> -static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev)
> -{
> - resource_size_t base = pci_resource_start(pdev, 0);
> - resource_size_t size = pci_resource_len(pdev, 0);
> - bool primary = false;
> -
> -#ifdef CONFIG_X86
> - primary = pdev->resource[PCI_ROM_RESOURCE].flags &
> - IORESOURCE_ROM_SHADOW;
> -#endif
> -
> - return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");
This still had the primary argument, it needs to be handled in an early
patch in some way.
> -}
> -
> static int lynxfb_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> {
> @@ -1011,7 +997,7 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
> int fbidx;
> int err;
>
> - err = lynxfb_kick_out_firmware_fb(pdev);
> + err = aperture_remove_conflicting_pci_devices(pdev, "sm750_fb1");
> if (err)
> return err;
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi
Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It's just open coded and matches.
>
> Note that Thomas said that his version apparently failed for some
> reason, but hey maybe we should try again.
I'll give this patch a test tomorrow.
Best regards
Thomas
>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: [email protected]
> ---
> drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 420fc75c240e..3ac24a780f50 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
>
> MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>
> -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> -{
> - bool primary = false;
> - resource_size_t base, size;
> -
> - base = pci_resource_start(pdev, 0);
> - size = pci_resource_len(pdev, 0);
> -#ifdef CONFIG_X86
> - primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> -
> - return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> -}
> -
> static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> struct ast_private *ast;
> struct drm_device *dev;
> int ret;
>
> - ret = ast_remove_conflicting_framebuffers(pdev);
> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
> if (ret)
> return ret;
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi
Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> Otherwise it's bit silly, and we might throw out the driver for the
> screen the user is actually looking at. I haven't found a bug report
> for this case yet, but we did get bug reports for the analog case
> where we're throwing out the efifb driver.
>
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: [email protected]
> ---
> drivers/video/aperture.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index 3d8c925c7365..6f351a58f6c6 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -341,6 +341,9 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
> return ret;
> }
>
> + if (!primary)
> + return 0;
> +
The original code from fbdev didn't do this, so this code didn't either.
It appears more to be a special case than an early-out branch. So can we
write it as
if (primary) {
// kick_vgacon
}
?
Best regards
Thomas
> /*
> * WARNING: Apparently we must kick fbdev drivers before vgacon,
> * otherwise the vga fbdev driver falls over.
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi
Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> Since vgaarb has been promoted to be a core piece of the pci subsystem
> we don't have to open code random guesses anymore, we actually know
> this in a platform agnostic way, and there's no need for an x86
> specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
> drivers/pci")
>
> This should not result in any functional change, and the non-x86
> multi-gpu pci systems are probably rare enough to not matter (I don't
> know of any tbh). But it's a nice cleanup, so let's do it.
>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: [email protected]
> Cc: Bjorn Helgaas <[email protected]>
> Cc: [email protected]
> ---
> drivers/video/aperture.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index 41e77de1ea82..3d8c925c7365 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
> */
> int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
> {
> - bool primary = false;
> + bool primary;
> resource_size_t base, size;
> int bar, ret;
>
> -#ifdef CONFIG_X86
> - primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> + primary = pdev == vga_default_device();
vga_default_device() is part of vgaarb and can return NULL. [1] That new
test is likely to be incorrect on many systems.
I suggest to implement a helper like fb_is_primary_device() on x86: it
uses the default VGA if set, or falls back to the original test. [2]
It's noteworthy that on most architectures, fb_is_primary_device()
returns 0. But at least on Sparc [3] and some Parisc [4] machines, it
does not.
I've long wanted to rework this helper anyway. So this is a good
opportunity.
Best regards
Thomas
[1]
https://elixir.bootlin.com/linux/latest/source/include/linux/vgaarb.h#L69
[2]
https://elixir.bootlin.com/linux/latest/source/arch/x86/video/fbdev.c#L14
[3]
https://elixir.bootlin.com/linux/latest/source/arch/sparc/include/asm/fb.h#L18
[4]
https://elixir.bootlin.com/linux/latest/source/drivers/video/console/sticore.c#L1153
>
> for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
> if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On Wed, Jan 11, 2023 at 05:03:02PM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > Otherwise it's bit silly, and we might throw out the driver for the
> > screen the user is actually looking at. I haven't found a bug report
> > for this case yet, but we did get bug reports for the analog case
> > where we're throwing out the efifb driver.
> >
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=216303
> > Signed-off-by: Daniel Vetter <[email protected]>
> > Cc: Thomas Zimmermann <[email protected]>
> > Cc: Javier Martinez Canillas <[email protected]>
> > Cc: Helge Deller <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/video/aperture.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> > index 3d8c925c7365..6f351a58f6c6 100644
> > --- a/drivers/video/aperture.c
> > +++ b/drivers/video/aperture.c
> > @@ -341,6 +341,9 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
> > return ret;
> > }
> > + if (!primary)
> > + return 0;
> > +
>
> The original code from fbdev didn't do this, so this code didn't either.
>
> It appears more to be a special case than an early-out branch. So can we
> write it as
Yeah I think this was a mistake going way back to when I added this to
i915 originally. It is a real change, but also I guess the people who have
machines without efifb or vesafb are ... really not many :-) Iirc you had
some very funny kernels going way back when vgacon was considered the only
safe choice to even hit this stuff.
> if (primary) {
> // kick_vgacon
> }
Yeah, but next patch adds the vga aperture, and then I think it makes a
bit more sense.
-Daniel
>
> ?
>
> Best regards
> Thomas
>
> > /*
> > * WARNING: Apparently we must kick fbdev drivers before vgacon,
> > * otherwise the vga fbdev driver falls over.
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N?rnberg, Germany
> (HRB 36809, AG N?rnberg)
> Gesch?ftsf?hrer: Ivo Totev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Jan 11, 2023 at 04:48:39PM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > It's just open coded and matches.
> >
> > Note that Thomas said that his version apparently failed for some
> > reason, but hey maybe we should try again.
>
> I'll give this patch a test tomorrow.
Thanks a lot!
-Daniel
>
> Best regards
> Thomas
>
> >
> > Signed-off-by: Daniel Vetter <[email protected]>
> > Cc: Dave Airlie <[email protected]>
> > Cc: Thomas Zimmermann <[email protected]>
> > Cc: Javier Martinez Canillas <[email protected]>
> > Cc: Helge Deller <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
> > 1 file changed, 1 insertion(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index 420fc75c240e..3ac24a780f50 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
> > MODULE_DEVICE_TABLE(pci, ast_pciidlist);
> > -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> > -{
> > - bool primary = false;
> > - resource_size_t base, size;
> > -
> > - base = pci_resource_start(pdev, 0);
> > - size = pci_resource_len(pdev, 0);
> > -#ifdef CONFIG_X86
> > - primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> > -#endif
> > -
> > - return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> > -}
> > -
> > static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > {
> > struct ast_private *ast;
> > struct drm_device *dev;
> > int ret;
> > - ret = ast_remove_conflicting_framebuffers(pdev);
> > + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
> > if (ret)
> > return ret;
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N?rnberg, Germany
> (HRB 36809, AG N?rnberg)
> Gesch?ftsf?hrer: Ivo Totev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Jan 11, 2023 at 04:59:30PM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > Since vgaarb has been promoted to be a core piece of the pci subsystem
> > we don't have to open code random guesses anymore, we actually know
> > this in a platform agnostic way, and there's no need for an x86
> > specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
> > drivers/pci")
> >
> > This should not result in any functional change, and the non-x86
> > multi-gpu pci systems are probably rare enough to not matter (I don't
> > know of any tbh). But it's a nice cleanup, so let's do it.
> >
> > Signed-off-by: Daniel Vetter <[email protected]>
> > Cc: Thomas Zimmermann <[email protected]>
> > Cc: Javier Martinez Canillas <[email protected]>
> > Cc: Helge Deller <[email protected]>
> > Cc: [email protected]
> > Cc: Bjorn Helgaas <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/video/aperture.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> > index 41e77de1ea82..3d8c925c7365 100644
> > --- a/drivers/video/aperture.c
> > +++ b/drivers/video/aperture.c
> > @@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
> > */
> > int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
> > {
> > - bool primary = false;
> > + bool primary;
> > resource_size_t base, size;
> > int bar, ret;
> > -#ifdef CONFIG_X86
> > - primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> > -#endif
> > + primary = pdev == vga_default_device();
>
> vga_default_device() is part of vgaarb and can return NULL. [1] That new
> test is likely to be incorrect on many systems.
>
> I suggest to implement a helper like fb_is_primary_device() on x86: it uses
> the default VGA if set, or falls back to the original test. [2]
>
> It's noteworthy that on most architectures, fb_is_primary_device() returns
> 0. But at least on Sparc [3] and some Parisc [4] machines, it does not.
Afaik these neither do legacy vga nor sysfb, so we should be fine. I'll
augment the commit message.
> I've long wanted to rework this helper anyway. So this is a good
> opportunity.
Hm yeah that sounds like a good thing to copy. I'm honestly not sure
whether it's needed, but at least it shouldn't hurt. I thought at least
that the boot default device should be set on all pci architectures.
I'll also reorder this with the previous patch to avoid the compile fail.
-Daniel
>
> Best regards
> Thomas
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/include/linux/vgaarb.h#L69
> [2]
> https://elixir.bootlin.com/linux/latest/source/arch/x86/video/fbdev.c#L14
> [3] https://elixir.bootlin.com/linux/latest/source/arch/sparc/include/asm/fb.h#L18
> [4] https://elixir.bootlin.com/linux/latest/source/drivers/video/console/sticore.c#L1153
>
>
> > for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
> > if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N?rnberg, Germany
> (HRB 36809, AG N?rnberg)
> Gesch?ftsf?hrer: Ivo Totev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi
Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> This one nukes all framebuffers, which is a bit much. In reality
> gma500 is igpu and never shipped with anything discrete, so there should
> not be any difference.
>
> Signed-off-by: Daniel Vetter <[email protected]>
> ---
> drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index cd9c73f5a64a..9b0daf90dc50 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> * might be able to read the framebuffer range from the device.
> */
> - ret = drm_aperture_remove_framebuffers(true, &driver);
> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
This does not work. The comment just above the changed line explains
why. The device uses shared memory similar to other integrated Intel
chips. The console is somewhere in a 16 MiB range, which has been stolen
by the BIOS from main memory. There's only a 1 MiB memory range on the
device to program the device. Unless you want to refactor as described,
this call has to cover the whole memory for now.
Best regards
Thomas
> if (ret)
> return ret;
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It's just open coded and matches.
>
> Note that Thomas said that his version apparently failed for some
> reason, but hey maybe we should try again.
It apparently worked this time. Tested on an AST2100 chip.
>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: [email protected]
Tested-by: Thomas Zimmmermann <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
> ---
> drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 420fc75c240e..3ac24a780f50 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
>
> MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>
> -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> -{
> - bool primary = false;
> - resource_size_t base, size;
> -
> - base = pci_resource_start(pdev, 0);
> - size = pci_resource_len(pdev, 0);
> -#ifdef CONFIG_X86
> - primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> -
> - return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> -}
> -
> static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> struct ast_private *ast;
> struct drm_device *dev;
> int ret;
>
> - ret = ast_remove_conflicting_framebuffers(pdev);
> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
> if (ret)
> return ret;
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > This one nukes all framebuffers, which is a bit much. In reality
> > gma500 is igpu and never shipped with anything discrete, so there should
> > not be any difference.
> >
> > Signed-off-by: Daniel Vetter <[email protected]>
> > ---
> > drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> > index cd9c73f5a64a..9b0daf90dc50 100644
> > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> > * might be able to read the framebuffer range from the device.
> > */
> > - ret = drm_aperture_remove_framebuffers(true, &driver);
> > + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>
> This does not work. The comment just above the changed line explains why.
> The device uses shared memory similar to other integrated Intel chips. The
> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
> from main memory. There's only a 1 MiB memory range on the device to program
> the device. Unless you want to refactor as described, this call has to cover
> the whole memory for now.
Uh. So it's maybe not so pretty, but what if I just call both functions?
That way we get the vga handling through the pci one, and the "make sure
there's no fb left" through the other one. Plus comment of course.
Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
and that just feels all kinds of wrong to me.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi
Am 12.01.23 um 10:59 schrieb Daniel Vetter:
> On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
>>> This one nukes all framebuffers, which is a bit much. In reality
>>> gma500 is igpu and never shipped with anything discrete, so there should
>>> not be any difference.
>>>
>>> Signed-off-by: Daniel Vetter <[email protected]>
>>> ---
>>> drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
>>> index cd9c73f5a64a..9b0daf90dc50 100644
>>> --- a/drivers/gpu/drm/gma500/psb_drv.c
>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c
>>> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>> * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
>>> * might be able to read the framebuffer range from the device.
>>> */
>>> - ret = drm_aperture_remove_framebuffers(true, &driver);
>>> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>>
>> This does not work. The comment just above the changed line explains why.
>> The device uses shared memory similar to other integrated Intel chips. The
>> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
>> from main memory. There's only a 1 MiB memory range on the device to program
>> the device. Unless you want to refactor as described, this call has to cover
>> the whole memory for now.
>
> Uh. So it's maybe not so pretty, but what if I just call both functions?
That's ways more ugly IMHO.
> That way we get the vga handling through the pci one, and the "make sure
> there's no fb left" through the other one. Plus comment of course.
>
> Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
> and that just feels all kinds of wrong to me.
With your patch applied, aperture_detach_devices() does all the work of
removing. I'd add the following internal functions:
static void aperture_detach_head(bool is_primary)
{
/*
* lengthy comment here
*/
if (is_primary)
sysfb_disable()
}
static void aperture_detach_tail(bool remove_vga)
{
if (remove_vga) {
aperture_detach_devices(VGA_PHYS_)
vga_remove_vgacon()
}
}
And call both of them at the beginning/end of
aperture_remove_conflicting_devices() and
aperture_remove_conflicting_pci_devices().
You'd still need to primary argument to
aperture_remove_conflicting_devices(), but there will be no code
duplication with the aperture helpers and the purpose of each code
fragment will be clearer.
Best regards
Thomas
> -Daniel
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On Thu, Jan 12, 2023 at 11:24:13AM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 12.01.23 um 10:59 schrieb Daniel Vetter:
> > On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > > > This one nukes all framebuffers, which is a bit much. In reality
> > > > gma500 is igpu and never shipped with anything discrete, so there should
> > > > not be any difference.
> > > >
> > > > Signed-off-by: Daniel Vetter <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> > > > index cd9c73f5a64a..9b0daf90dc50 100644
> > > > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > > > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > > > @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > > > * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> > > > * might be able to read the framebuffer range from the device.
> > > > */
> > > > - ret = drm_aperture_remove_framebuffers(true, &driver);
> > > > + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> > >
> > > This does not work. The comment just above the changed line explains why.
> > > The device uses shared memory similar to other integrated Intel chips. The
> > > console is somewhere in a 16 MiB range, which has been stolen by the BIOS
> > > from main memory. There's only a 1 MiB memory range on the device to program
> > > the device. Unless you want to refactor as described, this call has to cover
> > > the whole memory for now.
> >
> > Uh. So it's maybe not so pretty, but what if I just call both functions?
>
> That's ways more ugly IMHO.
>
> > That way we get the vga handling through the pci one, and the "make sure
> > there's no fb left" through the other one. Plus comment of course.
> >
> > Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
> > and that just feels all kinds of wrong to me.
>
> With your patch applied, aperture_detach_devices() does all the work of
> removing. I'd add the following internal functions:
>
> static void aperture_detach_head(bool is_primary)
> {
> /*
> * lengthy comment here
> */
> if (is_primary)
> sysfb_disable()
> }
>
> static void aperture_detach_tail(bool remove_vga)
> {
> if (remove_vga) {
> aperture_detach_devices(VGA_PHYS_)
> vga_remove_vgacon()
> }
> }
>
> And call both of them at the beginning/end of
> aperture_remove_conflicting_devices() and
> aperture_remove_conflicting_pci_devices().
>
> You'd still need to primary argument to
> aperture_remove_conflicting_devices(), but there will be no code duplication
> with the aperture helpers and the purpose of each code fragment will be
> clearer.
Yeah I don't want the primary argument. Aside from this one case here it's
not needed. Also by pushing this special case into the one driver that
needs it we keep it contained, instead of spreading it all around.
Inflicting a parameter on every (and in total we have a lot of callers of
this stuff) just because of gma500 does not seem like a good idea to me.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi
Am 12.01.23 um 11:45 schrieb Daniel Vetter:
> On Thu, Jan 12, 2023 at 11:24:13AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.01.23 um 10:59 schrieb Daniel Vetter:
>>> On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
>>>>> This one nukes all framebuffers, which is a bit much. In reality
>>>>> gma500 is igpu and never shipped with anything discrete, so there should
>>>>> not be any difference.
>>>>>
>>>>> Signed-off-by: Daniel Vetter <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
>>>>> index cd9c73f5a64a..9b0daf90dc50 100644
>>>>> --- a/drivers/gpu/drm/gma500/psb_drv.c
>>>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c
>>>>> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>> * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
>>>>> * might be able to read the framebuffer range from the device.
>>>>> */
>>>>> - ret = drm_aperture_remove_framebuffers(true, &driver);
>>>>> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>>>>
>>>> This does not work. The comment just above the changed line explains why.
>>>> The device uses shared memory similar to other integrated Intel chips. The
>>>> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
>>>> from main memory. There's only a 1 MiB memory range on the device to program
>>>> the device. Unless you want to refactor as described, this call has to cover
>>>> the whole memory for now.
>>>
>>> Uh. So it's maybe not so pretty, but what if I just call both functions?
>>
>> That's ways more ugly IMHO.
>>
>>> That way we get the vga handling through the pci one, and the "make sure
>>> there's no fb left" through the other one. Plus comment of course.
>>>
>>> Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
>>> and that just feels all kinds of wrong to me.
>>
>> With your patch applied, aperture_detach_devices() does all the work of
>> removing. I'd add the following internal functions:
>>
>> static void aperture_detach_head(bool is_primary)
>> {
>> /*
>> * lengthy comment here
>> */
>> if (is_primary)
>> sysfb_disable()
>> }
>>
>> static void aperture_detach_tail(bool remove_vga)
>> {
>> if (remove_vga) {
>> aperture_detach_devices(VGA_PHYS_)
>> vga_remove_vgacon()
>> }
>> }
>>
>> And call both of them at the beginning/end of
>> aperture_remove_conflicting_devices() and
>> aperture_remove_conflicting_pci_devices().
>>
>> You'd still need to primary argument to
>> aperture_remove_conflicting_devices(), but there will be no code duplication
>> with the aperture helpers and the purpose of each code fragment will be
>> clearer.
>
> Yeah I don't want the primary argument. Aside from this one case here it's
> not needed. Also by pushing this special case into the one driver that
> needs it we keep it contained, instead of spreading it all around.
> Inflicting a parameter on every (and in total we have a lot of callers of
> this stuff) just because of gma500 does not seem like a good idea to me.
Unfortunately, vgacon and vgaarb require a PCI device. I don't like the
proposal, but maybe it's the best for now. So go ahead if you like. I do
expect that this needs additional work in future, however.
Just some more thoughts.
Grep for drm_aperture_remove_framebuffers(). Within DRM there are really
just 10 drivers calling this function (vs. 12 callers of
drm_aperture_remove_conflicting_pci_framebuffers()). In fbdev, there are
many callers of the PCI helper (~40) and apparently only 3 for the
non-PCI one. The other drivers are panels, USB, MIPI, etc and don't
interact with the system framebuffer. Compared to the overall number of
drivers, we have surprisingly few 'traditional graphics cards' in DRM.
Another thing is that gma500 and the other 9 drivers simply don't bother
to get the framebuffer range. They should be reworked to fetch the
configured framebuffer from the device and release that region only. The
practical impact is close to zero, so it hasn't happened.
Best regards
Thomas
> -Daniel
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On Thu, 12 Jan 2023 at 13:15, Thomas Zimmermann <[email protected]> wrote:
>
> Hi
>
> Am 12.01.23 um 11:45 schrieb Daniel Vetter:
> > On Thu, Jan 12, 2023 at 11:24:13AM +0100, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 12.01.23 um 10:59 schrieb Daniel Vetter:
> >>> On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
> >>>> Hi
> >>>>
> >>>> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> >>>>> This one nukes all framebuffers, which is a bit much. In reality
> >>>>> gma500 is igpu and never shipped with anything discrete, so there should
> >>>>> not be any difference.
> >>>>>
> >>>>> Signed-off-by: Daniel Vetter <[email protected]>
> >>>>> ---
> >>>>> drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> >>>>> index cd9c73f5a64a..9b0daf90dc50 100644
> >>>>> --- a/drivers/gpu/drm/gma500/psb_drv.c
> >>>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> >>>>> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>>>> * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> >>>>> * might be able to read the framebuffer range from the device.
> >>>>> */
> >>>>> - ret = drm_aperture_remove_framebuffers(true, &driver);
> >>>>> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> >>>>
> >>>> This does not work. The comment just above the changed line explains why.
> >>>> The device uses shared memory similar to other integrated Intel chips. The
> >>>> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
> >>>> from main memory. There's only a 1 MiB memory range on the device to program
> >>>> the device. Unless you want to refactor as described, this call has to cover
> >>>> the whole memory for now.
> >>>
> >>> Uh. So it's maybe not so pretty, but what if I just call both functions?
> >>
> >> That's ways more ugly IMHO.
> >>
> >>> That way we get the vga handling through the pci one, and the "make sure
> >>> there's no fb left" through the other one. Plus comment of course.
> >>>
> >>> Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
> >>> and that just feels all kinds of wrong to me.
> >>
> >> With your patch applied, aperture_detach_devices() does all the work of
> >> removing. I'd add the following internal functions:
> >>
> >> static void aperture_detach_head(bool is_primary)
> >> {
> >> /*
> >> * lengthy comment here
> >> */
> >> if (is_primary)
> >> sysfb_disable()
> >> }
> >>
> >> static void aperture_detach_tail(bool remove_vga)
> >> {
> >> if (remove_vga) {
> >> aperture_detach_devices(VGA_PHYS_)
> >> vga_remove_vgacon()
> >> }
> >> }
> >>
> >> And call both of them at the beginning/end of
> >> aperture_remove_conflicting_devices() and
> >> aperture_remove_conflicting_pci_devices().
> >>
> >> You'd still need to primary argument to
> >> aperture_remove_conflicting_devices(), but there will be no code duplication
> >> with the aperture helpers and the purpose of each code fragment will be
> >> clearer.
> >
> > Yeah I don't want the primary argument. Aside from this one case here it's
> > not needed. Also by pushing this special case into the one driver that
> > needs it we keep it contained, instead of spreading it all around.
> > Inflicting a parameter on every (and in total we have a lot of callers of
> > this stuff) just because of gma500 does not seem like a good idea to me.
>
> Unfortunately, vgacon and vgaarb require a PCI device. I don't like the
> proposal, but maybe it's the best for now. So go ahead if you like. I do
> expect that this needs additional work in future, however.
>
> Just some more thoughts.
>
> Grep for drm_aperture_remove_framebuffers(). Within DRM there are really
> just 10 drivers calling this function (vs. 12 callers of
> drm_aperture_remove_conflicting_pci_framebuffers()). In fbdev, there are
> many callers of the PCI helper (~40) and apparently only 3 for the
> non-PCI one. The other drivers are panels, USB, MIPI, etc and don't
> interact with the system framebuffer. Compared to the overall number of
> drivers, we have surprisingly few 'traditional graphics cards' in DRM.
This is largely historical. fbdev is from the 90s, when we had the
huge explosion in largely pci graphics cards, because that was the
place where all the growth and hence drivers were. 80% of these
companies/chipe all died within a short few years.
kms otoh had the huge growth in the 2010s, where there was the tail
end of the SoC mobile explosion, so that's where we have tons of
drivers. If you look at staging there's a pile more fbdev drivers for
SoC, but many of these never got beyond the "vendor hacked some stuff
together and shipped it" stage. So that's probably why they lack
polish like fw -> driver handover (most of these just booted directly
to the driver in real products).
> Another thing is that gma500 and the other 9 drivers simply don't bother
> to get the framebuffer range. They should be reworked to fetch the
> configured framebuffer from the device and release that region only. The
> practical impact is close to zero, so it hasn't happened.
I think that's ok, because trying to figure out the real fb means you
need fairly complete hw state readout (otherwise there's no
motivation), and i915 is the only driver that ever really did that.
Just for fw driver removal "nuke everything" gets the job done.
-Daniel
> Best regards
> Thomas
>
>
> > -Daniel
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
> From: Daniel Vetter <[email protected]>
> Sent: Wednesday, January 11, 2023 7:41 AM
> [...]
> diff --git a/drivers/video/fbdev/hyperv_fb.c
> b/drivers/video/fbdev/hyperv_fb.c
> @@ -1076,9 +1076,7 @@ static int hvfb_getmem(struct hv_device *hdev,
> struct fb_info *info)
> info->screen_size = dio_fb_size;
>
> getmem_done:
> -
> aperture_remove_conflicting_devices(info->apertures->ranges[0].base,
> -
> info->apertures->ranges[0].size,
> - false,
> KBUILD_MODNAME);
> + aperture_remove_conflicting_pci_devices(pdev,
> KBUILD_MODNAME);
NACK. I think the patch breaks Gen-2 VMs, because 'pdev' is NULL in a
Gen-2 VM.
A VM running on Hyper-V can be Gen-1 or Gen-2: see
https://learn.microsoft.com/en-us/windows-server/virtualization/hyper-v/plan/should-i-create-a-generation-1-or-2-virtual-machine-in-hyper-v
A Gen-2 VM doesn't have the legacy PCI Bus, and doesn't have a legacy
PCI VGA adapter device, so the 'pdev' is NULL here.
>
> if (gen2vm) {
> /* framebuffer is reallocated, clear screen_info to avoid
> misuse from kexec */
Hi,
FYI I have merged patches 1, 6 and 7 of this patchset. They look fine
and are worthwhile fixes on their own.
Best regards
Thomas
Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It's just open coded and matches.
>
> Note that Thomas said that his version apparently failed for some
> reason, but hey maybe we should try again.
>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: [email protected]
> ---
> drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 420fc75c240e..3ac24a780f50 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
>
> MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>
> -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> -{
> - bool primary = false;
> - resource_size_t base, size;
> -
> - base = pci_resource_start(pdev, 0);
> - size = pci_resource_len(pdev, 0);
> -#ifdef CONFIG_X86
> - primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> -
> - return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> -}
> -
> static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> struct ast_private *ast;
> struct drm_device *dev;
> int ret;
>
> - ret = ast_remove_conflicting_framebuffers(pdev);
> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
> if (ret)
> return ret;
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi Thomas,
The Intel(x86) CPUs have a separate address space for "IO", but the ARM
architecture only has "memory", so all IO devices are accessed as if
they were memory. Which means ARM does not support isolated IO. Here is
a related discussion on ARM's forum.
https://community.arm.com/support-forums/f/architectures-and-processors-forum/52046/how-to-read-write-an-i-o-port-in-aarch64
Thus, we adapt MMIO only after this patch.
On 2023/4/4 下午 10:45, Thomas Zimmermann wrote:
> Hi,
>
> FYI I have merged patches 1, 6 and 7 of this patchset. They look fine
> and are worthwhile fixes on their own.
>
> Best regards
> Thomas
>
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
>> It's just open coded and matches.
>>
>> Note that Thomas said that his version apparently failed for some
>> reason, but hey maybe we should try again.
>>
>> Signed-off-by: Daniel Vetter <[email protected]>
>> Cc: Dave Airlie <[email protected]>
>> Cc: Thomas Zimmermann <[email protected]>
>> Cc: Javier Martinez Canillas <[email protected]>
>> Cc: Helge Deller <[email protected]>
>> Cc: [email protected]
>> ---
>> drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
>> 1 file changed, 1 insertion(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.c
>> b/drivers/gpu/drm/ast/ast_drv.c
>> index 420fc75c240e..3ac24a780f50 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.c
>> +++ b/drivers/gpu/drm/ast/ast_drv.c
>> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[]
>> = {
>> MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>> -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
>> -{
>> - bool primary = false;
>> - resource_size_t base, size;
>> -
>> - base = pci_resource_start(pdev, 0);
>> - size = pci_resource_len(pdev, 0);
>> -#ifdef CONFIG_X86
>> - primary = pdev->resource[PCI_ROM_RESOURCE].flags &
>> IORESOURCE_ROM_SHADOW;
>> -#endif
>> -
>> - return drm_aperture_remove_conflicting_framebuffers(base, size,
>> primary, &ast_driver);
>> -}
>> -
>> static int ast_pci_probe(struct pci_dev *pdev, const struct
>> pci_device_id *ent)
>> {
>> struct ast_private *ast;
>> struct drm_device *dev;
>> int ret;
>> - ret = ast_remove_conflicting_framebuffers(pdev);
>> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
>> &ast_driver);
>> if (ret)
>> return ret;
>
--
Best Regards
Jammy