Hi Zack Am 19.01.22 um 10:13 schrieb Thomas Zimmermann:
Hi Am 19.01.22 um 03:15 schrieb Zack Rusin:On Tue, 2022-01-18 at 20:00 +0100, Javier Martinez Canillas wrote:Hello Zack, On 1/17/22 19:03, Zack Rusin wrote:From: Zack Rusin <zackr@xxxxxxxxxx> When sysfb_simple is enabled loading vmwgfx fails because the regions are held by the platform. In that case remove_conflicting*_framebuffers only removes the simplefb but not the regions held by sysfb.Indeed, that's an issue. I wonder if we should drop the IORESOURCE_BUSY flag from the memory resource added to the "simple-framebuffer" device ?I think this is one of those cases where it depends on what we plan to do after that. Sementically it makes sense to have it in there - the framebuffer memory is claimed by the "simple-framebuffer" and it's busy, it's just that it creates issues for drivers after unloading. I think removing it, while making things easier for drivers, would be confusing for people reading the code later, unless there's some kind of cleanup that would happen with it (e.g. removing IORESOURCE_BUSY altogether and making the drm drivers properly request their resources). At least by itself it doesn't seem to be much better solution than having the drm drivers not call pci_request_region[s], which apart from hyperv and cirrus (iirc bochs does it for resources other than fb which wouldn't have been claimed by "simple-framebuffer") is already the case. I do think we should do one of them to make the codebase coherent: either remove IORESOURCE_BUSY from "simple-framebuffer" or remove pci_request_region[s] from hyperv and cirrus.I just discussed this a bit with Javier. It's a problem with the simple-framebuffer code, rather then vmwgfx.IMHO the best solution is to drop IORESOURCE_BUSY from sysfb and have drivers register/release the range with _BUSY. That would signal the memory belongs to the sysfb device but is not busy unless a driver has been bound. After simplefb released the range, it should be 'non-busy' again and available for vmwgfx. Simpledrm does a hot-unplug of the sysfb device, so the memory range gets released entirely. If you want, I'll prepare some patches for this scenario.
Attached is a patch that implements this. Doing
cat /proc/iomem
...
e0000000-efffffff : 0000:00:02.0
e0000000-e07e8fff : BOOTFB
e0000000-e07e8fff : simplefb
...
shows the memory. 'BOOTFB' is the simple-framebuffer device and
'simplefb' is the driver. Only the latter uses _BUSY. Same for
simpledrm. Once the drivers have been unloaded, the BUSY flag is gone
and the memory canbe acquired by vmwgfx.
Zack, please test this patch. If it works, I'll send out the real patchset. Best regards Thomas
If this doesn't work, pushing all request/release pairs into drivers would be my next option.If none of this is feasible, we can still remove pci_request_region() from vmwgfx.Best regards ThomasLike the other drm drivers we need to stop requesting all the pci regions to let the driver load with platform code enabled. This allows vmwgfx to load correctly on systems with sysfb_simple enabled.I read this very interesting thread from two years ago: https://lkml.org/lkml/2020/11/5/248 Maybe is worth mentioning in the commit message what Daniel said there, that is that only a few DRM drivers request explicitly the PCI regions and the only reliable approach is for bus drivers to claim these.Ah, great point. I'll update the commit log with that.Signed-off-by: Zack Rusin <zackr@xxxxxxxxxx> Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64") Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx Cc: <stable@xxxxxxxxxxxxxxx> Reviewed-by: Martin Krastev <krastevm@xxxxxxxxxx> ---The patch looks good to me, thanks a lot for fixing this: Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>Thanks for taking a look at this, I appreciate it! z
-- 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
From f5da496296f7dcaf8754e1b0446660b100539a73 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@xxxxxxx>
Date: Wed, 19 Jan 2022 10:23:07 +0100
Subject: [PATCH] [RFC] drop IORESOURCE_BUSY from sysfb code and request memory
in drivers
Drop the IORESOURCE_BUSY flag when creating a simple-framebuffer
device. Instead acquire the memory in drivers.
---
drivers/firmware/sysfb_simplefb.c | 2 +-
drivers/gpu/drm/tiny/simpledrm.c | 17 ++++++---
drivers/video/fbdev/simplefb.c | 57 ++++++++++++++++++++++---------
3 files changed, 53 insertions(+), 23 deletions(-)
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index b86761904949..179e9d0ef3e9 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -99,7 +99,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si,
/* setup IORESOURCE_MEM as framebuffer memory */
memset(&res, 0, sizeof(res));
- res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+ res.flags = IORESOURCE_MEM;
res.name = simplefb_resname;
res.start = base;
res.end = res.start + length - 1;
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 04146da2d1d8..32550697b1a7 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -526,21 +526,28 @@ static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
{
struct drm_device *dev = &sdev->dev;
struct platform_device *pdev = sdev->pdev;
- struct resource *mem;
+ struct resource *res, *mem;
void __iomem *screen_base;
int ret;
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!mem)
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
return -EINVAL;
- ret = devm_aperture_acquire_from_firmware(dev, mem->start, resource_size(mem));
+ ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
if (ret) {
drm_err(dev, "could not acquire memory range %pr: error %d\n",
- mem, ret);
+ res, ret);
return ret;
}
+ mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
+ sdev->dev.driver->name);
+ if (!mem) {
+ drm_err(dev, "could not acquire memory region %pr\n", res);
+ return -EBUSY;
+ }
+
screen_base = devm_ioremap_wc(&pdev->dev, mem->start,
resource_size(mem));
if (!screen_base)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 57541887188b..7b9072f07337 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -66,16 +66,36 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
return 0;
}
-struct simplefb_par;
+struct simplefb_par {
+ u32 palette[PSEUDO_PALETTE_SIZE];
+#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
+ bool clks_enabled;
+ unsigned int clk_count;
+ struct clk **clks;
+#endif
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+ bool regulators_enabled;
+ u32 regulator_count;
+ struct regulator **regulators;
+#endif
+ bool release_mem_region;
+};
+
static void simplefb_clocks_destroy(struct simplefb_par *par);
static void simplefb_regulators_destroy(struct simplefb_par *par);
static void simplefb_destroy(struct fb_info *info)
{
+ struct simplefb_par *par = info->par;
+
simplefb_regulators_destroy(info->par);
simplefb_clocks_destroy(info->par);
if (info->screen_base)
iounmap(info->screen_base);
+
+ if (par->release_mem_region)
+ release_mem_region(info->apertures->ranges[0].base,
+ info->apertures->ranges[0].size);
}
static const struct fb_ops simplefb_ops = {
@@ -169,20 +189,6 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return 0;
}
-struct simplefb_par {
- u32 palette[PSEUDO_PALETTE_SIZE];
-#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
- bool clks_enabled;
- unsigned int clk_count;
- struct clk **clks;
-#endif
-#if defined CONFIG_OF && defined CONFIG_REGULATOR
- bool regulators_enabled;
- u32 regulator_count;
- struct regulator **regulators;
-#endif
-};
-
#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
/*
* Clock handling code.
@@ -401,6 +407,7 @@ static void simplefb_regulators_destroy(struct simplefb_par *par) { }
static int simplefb_probe(struct platform_device *pdev)
{
+ bool request_mem_succeeded = false;
int ret;
struct simplefb_params params;
struct fb_info *info;
@@ -436,9 +443,20 @@ static int simplefb_probe(struct platform_device *pdev)
return -EINVAL;
}
+ if (request_mem_region(mem->start, resource_size(mem), "simplefb")) {
+ request_mem_succeeded = true;
+ } else {
+ /* We cannot make this fatal. Sometimes this comes from magic
+ spaces our resource handlers simply don't know about */
+ dev_warn(&pdev->dev, "simplefb: cannot reserve video memory at %pR\n",
+ mem);
+ }
+
info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
- if (!info)
- return -ENOMEM;
+ if (!info) {
+ ret = -ENOMEM;
+ goto error_release_mem_region;
+ }
platform_set_drvdata(pdev, info);
par = info->par;
@@ -495,6 +513,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->var.xres, info->var.yres,
info->var.bits_per_pixel, info->fix.line_length);
+ par->release_mem_region = request_mem_succeeded;
+
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
@@ -513,6 +533,9 @@ static int simplefb_probe(struct platform_device *pdev)
iounmap(info->screen_base);
error_fb_release:
framebuffer_release(info);
+error_release_mem_region:
+ if (request_mem_succeeded)
+ release_mem_region(mem->start, resource_size(mem));
return ret;
}
--
2.34.1
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
