[PATCH] drm/vmwgfx: Stop requesting the pci regions

[PATCH] drm/vmwgfx: Stop requesting the pci regions

[Zack Rusin]

From: Zack Rusin <zackr@vmware.com>

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.

Like 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.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64")
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org>
Reviewed-by: Martin Krastev <krastevm@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index fe36efdb7ff5..27feb19f3324 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -724,10 +724,6 @@ static int vmw_setup_pci_resources(struct vmw_private *dev,
 
 	pci_set_master(pdev);
 
-	ret = pci_request_regions(pdev, "vmwgfx probe");
-	if (ret)
-		return ret;
-
 	dev->pci_id = pci_id;
 	if (pci_id == VMWGFX_PCI_ID_SVGA3) {
 		rmmio_start = pci_resource_start(pdev, 0);
-- 
2.32.0

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Javier Martinez Canillas]

Hello Zack,

On 1/17/22 19:03, Zack Rusin wrote:
> From: Zack Rusin <zackr@vmware.com>
> 
> 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 ?

In fact, maybe in sysfb_create_simplefb() shouldn't even attempt to claim
a memory resource and just register the platform device with no resources ?
 
> Like 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.

In other words, removing the pci_request_regions() seems to have merit
on its own.

> Signed-off-by: Zack Rusin <zackr@vmware.com>
> Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64")
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Martin Krastev <krastevm@vmware.com>
> ---

The patch looks good to me, thanks a lot for fixing this:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Javier Martinez Canillas]

On 1/18/22 20:00, Javier Martinez Canillas wrote:
> Hello Zack,
> 
> On 1/17/22 19:03, Zack Rusin wrote:
>> From: Zack Rusin <zackr@vmware.com>
>>
>> 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 ?
>
> In fact, maybe in sysfb_create_simplefb() shouldn't even attempt to claim
> a memory resource and just register the platform device with no resources ?
>  

Answering my own question, this would mean adding that platform specific
logic to the drivers matching the "simple-framebuffer" device so it should
remain in sysfb_create_simplefb().

But I still wonder if dropping the IORESOURCE_BUSY flag is something that
will be reasonable to prevent other drivers having the the issue reported
in this patch.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[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@vmware.com>
> > 
> > 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.



> > Like 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@vmware.com>
> > Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64")
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org>
> > Reviewed-by: Martin Krastev <krastevm@vmware.com>
> > ---
> 
> The patch looks good to me, thanks a lot for fixing this:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks for taking a look at this, I appreciate it!

z

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3777 bytes --]

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@vmware.com>
>>>
>>> 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.

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
Thomas

> 
> 
> 
>>> Like 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@vmware.com>
>>> Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64")
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: <stable@vger.kernel.org>
>>> Reviewed-by: Martin Krastev <krastevm@vmware.com>
>>> ---
>>
>> The patch looks good to me, thanks a lot for fixing this:
>>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4500 bytes --]

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@vmware.com>
>>>>
>>>> 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
> Thomas
> 
>>
>>
>>
>>>> Like 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@vmware.com>
>>>> Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64")
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: <stable@vger.kernel.org>
>>>> Reviewed-by: Martin Krastev <krastevm@vmware.com>
>>>> ---
>>>
>>> The patch looks good to me, thanks a lot for fixing this:
>>>
>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> 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

[-- Attachment #1.1.2: 0001-RFC-drop-IORESOURCE_BUSY-from-sysfb-code-and-request.patch --]
[-- Type: text/x-patch, Size: 5678 bytes --]

From f5da496296f7dcaf8754e1b0446660b100539a73 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@suse.de>
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Zack Rusin]

On Wed, 2022-01-19 at 15:00 +0100, Thomas Zimmermann wrote:
> 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@vmware.com>
> > > > > 
> > > > > 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 
> and the memory canbe acquired by vmwgfx.
> 
> Zack, please test this patch. If it works, I'll send out the real
> patchset.

Hmm, the patch looks good but it doesn't work. After boot: /proc/iomem
50000000-7fffffff : pcie@0x40000000
  78000000-7fffffff : 0000:00:0f.0
    78000000-782fffff : BOOTFB

and vmwgfx fails on pci_request_regions:

kernel: fb0: switching to vmwgfx from simple
kernel: Console: switching to colour dummy device 80x25
kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
0x7fffffff 64bit pref]
kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16

leaving the system without a fb driver.

z

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4419 bytes --]

Hi

Am 19.01.22 um 16:12 schrieb Zack Rusin:
> On Wed, 2022-01-19 at 15:00 +0100, Thomas Zimmermann wrote:
>> 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@vmware.com>
>>>>>>
>>>>>> 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
>> and the memory canbe acquired by vmwgfx.
>>
>> Zack, please test this patch. If it works, I'll send out the real
>> patchset.
> 
> Hmm, the patch looks good but it doesn't work. After boot: /proc/iomem
> 50000000-7fffffff : pcie@0x40000000
>    78000000-7fffffff : 0000:00:0f.0
>      78000000-782fffff : BOOTFB
> 
> and vmwgfx fails on pci_request_regions:
> 
> kernel: fb0: switching to vmwgfx from simple
> kernel: Console: switching to colour dummy device 80x25
> kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
> 0x7fffffff 64bit pref]
> kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16
> 
> leaving the system without a fb driver.

OK, I suspect that it would work if you use simpledrm instead of 
simplefb. Could you try please? You'd have to build DRM and simpledrm 
into the kernel binary.

If that works, would you consider protecting pci_request_region() with
  #if not defined(CONFIG_FB_SIMPLE)
  #endif

with a FIXME comment?

simpledrm hot-unplugs the sysfb device entirely. Maybe simplefb can do 
the same. I'm not sure, but possibly.

Best regards
Thomas

> 
> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Javier Martinez Canillas]

On 1/19/22 16:50, Thomas Zimmermann wrote:

[snip]

>>>> 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
>>> and the memory canbe acquired by vmwgfx.
>>>
>>> Zack, please test this patch. If it works, I'll send out the real
>>> patchset.
>>
>> Hmm, the patch looks good but it doesn't work. After boot: /proc/iomem
>> 50000000-7fffffff : pcie@0x40000000
>>    78000000-7fffffff : 0000:00:0f.0
>>      78000000-782fffff : BOOTFB
>>
>> and vmwgfx fails on pci_request_regions:
>>
>> kernel: fb0: switching to vmwgfx from simple
>> kernel: Console: switching to colour dummy device 80x25
>> kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
>> 0x7fffffff 64bit pref]
>> kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16
>>
>> leaving the system without a fb driver.
> 
> OK, I suspect that it would work if you use simpledrm instead of 
> simplefb. Could you try please? You'd have to build DRM and simpledrm 
> into the kernel binary.
>

Yes, I believe that should work.
 Zack, could you please try if just the following [0] make it works ?

That is, dropping the IORESOURCE_BUSY but not doing the memory region
request / release in simplefb and keeping it in the vmwgfx driver.

[0]:
From da6de1430b9dc252eccf2d6fee0446d33375fa6d Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Wed, 19 Jan 2022 14:41:25 +0100
Subject: [PATCH] drivers/firmware: Don't mark as busy the simple-framebuffer
 IO resource

The sysfb_create_simplefb() function requests a IO memory resource for the
simple-framebuffer platform device, but it also marks it as busy which led
to drivers requesting the same memory resource to fail.

Let's drop the IORESOURCE_BUSY flag and let drivers to request it as busy
instead.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb_simplefb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 303a491e520d..76c4abc42a30 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;
-- 
2.33.1

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Zack Rusin]

On Wed, 2022-01-19 at 17:36 +0100, Javier Martinez Canillas wrote:
> On 1/19/22 16:50, Thomas Zimmermann wrote:
> 
> [snip]
> 
> > > > > 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
> > > > and the memory canbe acquired by vmwgfx.
> > > > 
> > > > Zack, please test this patch. If it works, I'll send out the real
> > > > patchset.
> > > 
> > > Hmm, the patch looks good but it doesn't work. After boot:
> > > /proc/iomem
> > > 50000000-7fffffff : pcie@0x40000000
> > >    78000000-7fffffff : 0000:00:0f.0
> > >      78000000-782fffff : BOOTFB
> > > 
> > > and vmwgfx fails on pci_request_regions:
> > > 
> > > kernel: fb0: switching to vmwgfx from simple
> > > kernel: Console: switching to colour dummy device 80x25
> > > kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
> > > 0x7fffffff 64bit pref]
> > > kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16
> > > 
> > > leaving the system without a fb driver.
> > 
> > OK, I suspect that it would work if you use simpledrm instead of 
> > simplefb. Could you try please? You'd have to build DRM and simpledrm
> > into the kernel binary.
> > 
> 
> Yes, I believe that should work.
>  Zack, could you please try if just the following [0] make it works ?

Unfortunately that still fails with the same:
kernel: fb0: switching to vmwgfx from simple
kernel: Console: switching to colour dummy device 80x25
kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
0x7fffffff 64bit pref]
kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16

z

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Zack Rusin]

On Wed, 2022-01-19 at 16:50 +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 19.01.22 um 16:12 schrieb Zack Rusin:
> > On Wed, 2022-01-19 at 15:00 +0100, Thomas Zimmermann wrote:
> > > 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@vmware.com>
> > > > > > > 
> > > > > > > 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
> > > and the memory canbe acquired by vmwgfx.
> > > 
> > > Zack, please test this patch. If it works, I'll send out the real
> > > patchset.
> > 
> > Hmm, the patch looks good but it doesn't work. After boot:
> > /proc/iomem
> > 50000000-7fffffff : pcie@0x40000000
> >    78000000-7fffffff : 0000:00:0f.0
> >      78000000-782fffff : BOOTFB
> > 
> > and vmwgfx fails on pci_request_regions:
> > 
> > kernel: fb0: switching to vmwgfx from simple
> > kernel: Console: switching to colour dummy device 80x25
> > kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
> > 0x7fffffff 64bit pref]
> > kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16
> > 
> > leaving the system without a fb driver.
> 
> OK, I suspect that it would work if you use simpledrm instead of 
> into the kernel binary.

Yes, simpledrm works fine. BTW, is there any remaining work before
distros can enable it by default?

> 
> If that works, would you consider protecting pci_request_region()
> with
>   #if not defined(CONFIG_FB_SIMPLE)
>   #endif
> 
> with a FIXME comment?

Yes, I think that's a good compromise. I'll respin the patch with that.

z

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Javier Martinez Canillas]

Hello Zack,

On 1/20/22 05:06, Zack Rusin wrote:

[snip]

>>>
>>> Hmm, the patch looks good but it doesn't work. After boot:
>>> /proc/iomem
>>> 50000000-7fffffff : pcie@0x40000000
>>>    78000000-7fffffff : 0000:00:0f.0
>>>      78000000-782fffff : BOOTFB
>>>
>>> and vmwgfx fails on pci_request_regions:
>>>
>>> kernel: fb0: switching to vmwgfx from simple
>>> kernel: Console: switching to colour dummy device 80x25
>>> kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
>>> 0x7fffffff 64bit pref]
>>> kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16
>>>
>>> leaving the system without a fb driver.
>>
>> OK, I suspect that it would work if you use simpledrm instead of 
>> into the kernel binary.
> 
> Yes, simpledrm works fine. BTW, is there any remaining work before
> distros can enable it by default?
> 

Alpine already did AFAIK, OpenSUSE and Fedora are doing it soon

I don't know about the others distros but I guess they will follow.

>>
>> If that works, would you consider protecting pci_request_region()
>> with
>>   #if not defined(CONFIG_FB_SIMPLE)
>>   #endif
>>
>> with a FIXME comment?
> 
> Yes, I think that's a good compromise. I'll respin the patch with that.
> 

Agreed. Thanks a lot for testing the other patches anyways.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1711 bytes --]

Hi

Am 20.01.22 um 05:06 schrieb Zack Rusin:
[...]
>>>
>>> kernel: fb0: switching to vmwgfx from simple
>>> kernel: Console: switching to colour dummy device 80x25
>>> kernel: vmwgfx 0000:00:0f.0: BAR 2: can't reserve [mem 0x78000000-
>>> 0x7fffffff 64bit pref]
>>> kernel: vmwgfx: probe of 0000:00:0f.0 failed with error -16
>>>
>>> leaving the system without a fb driver.
>>
>> OK, I suspect that it would work if you use simpledrm instead of
>> into the kernel binary.
> 
> Yes, simpledrm works fine. BTW, is there any remaining work before
> distros can enable it by default?

It's here and ready for use.

Simpledrm requires DRM to be linked in the kernel. We're modularizing 
DRM helpers to reduce the binary size.

On the distro side, Javier and me are sorting out issue. But it's fairly 
quite and not many problems show up.

> 
>>
>> If that works, would you consider protecting pci_request_region()
>> with
>>    #if not defined(CONFIG_FB_SIMPLE)
>>    #endif
>>
>> with a FIXME comment?
> 
> Yes, I think that's a good compromise. I'll respin the patch with that.

Before you do that, I have one more patch for you to try. It's all the 
changes as before, but now fbdev hot-unplugs the underlying platform 
device from the device hierarchy. The BOOTFB will not consume parts of 
vmwgfx's display memory range any longer. It's now the same behavior as 
with simpledrm.

This works for me with simplefb and efifb on i915 hardware.

Best regards
Thomas

> 
> 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

[-- Attachment #1.1.2: v2-0001-RFC-drop-IORESOURCE_BUSY-from-sysfb-code-and-requ.patch --]
[-- Type: text/x-patch, Size: 8443 bytes --]

From dfd482bfc143b9ec0d8b103a56f6576acef8a860 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@suse.de>
Date: Wed, 19 Jan 2022 10:23:07 +0100
Subject: [PATCH v2] [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. Also hot-unplug the
platform device.
---
 drivers/firmware/sysfb_simplefb.c |  2 +-
 drivers/gpu/drm/tiny/simpledrm.c  | 17 ++++++---
 drivers/video/fbdev/core/fbmem.c  | 29 ++++++++++++++--
 drivers/video/fbdev/simplefb.c    | 57 ++++++++++++++++++++++---------
 include/linux/fb.h                |  1 +
 5 files changed, 80 insertions(+), 26 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/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0fa7ede94fa6..2db31ea7cbb4 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -25,6 +25,7 @@
 #include <linux/init.h>
 #include <linux/linux_logo.h>
 #include <linux/proc_fs.h>
+#include <linux/platform_device.h>
 #include <linux/seq_file.h>
 #include <linux/console.h>
 #include <linux/kmod.h>
@@ -1557,18 +1558,36 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	/* check all firmware fbs and kick off if the base addr overlaps */
 	for_each_registered_fb(i) {
 		struct apertures_struct *gen_aper;
+		struct device *dev;
 
 		if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE))
 			continue;
 
 		gen_aper = registered_fb[i]->apertures;
+		dev = registered_fb[i]->device;
 		if (fb_do_apertures_overlap(gen_aper, a) ||
 			(primary && gen_aper && gen_aper->count &&
 			 gen_aper->ranges[0].base == VGA_FB_PHYS)) {
 
 			printk(KERN_INFO "fb%d: switching to %s from %s\n",
 			       i, name, registered_fb[i]->fix.id);
-			do_unregister_framebuffer(registered_fb[i]);
+
+			/*
+			 * If we kick-out a firmware driver, we also want to remove
+			 * the underlying platform device, such as simple-framebuffer,
+			 * VESA, EFI, etc. A native driver will then be able to
+			 * allocate the memory range.
+			 *
+			 * If it's not a platform device, at least print a warning. A
+			 * fix would add code to remove the device from the system.
+			 */
+			if (dev_is_platform(dev)) {
+				registered_fb[i]->forced_out = true;
+				platform_device_unregister(to_platform_device(dev));
+			} else {
+				printk(KERN_WARNING "fb%d: cannot remove device\n", i);
+				do_unregister_framebuffer(registered_fb[i]);
+			}
 		}
 	}
 }
@@ -1898,9 +1917,13 @@ EXPORT_SYMBOL(register_framebuffer);
 void
 unregister_framebuffer(struct fb_info *fb_info)
 {
-	mutex_lock(&registration_lock);
+	bool forced_out = fb_info->forced_out;
+
+	if (!forced_out)
+		mutex_lock(&registration_lock);
 	do_unregister_framebuffer(fb_info);
-	mutex_unlock(&registration_lock);
+	if (!forced_out)
+		mutex_unlock(&registration_lock);
 }
 EXPORT_SYMBOL(unregister_framebuffer);
 
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;
 }
 
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 3da95842b207..9a14f3f8a329 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -502,6 +502,7 @@ struct fb_info {
 	} *apertures;
 
 	bool skip_vt_switch; /* no VT switch on suspend/resume required */
+	bool forced_out; /* set when being removed by another driver */
 };
 
 static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
-- 
2.34.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Zack Rusin]

On Thu, 2022-01-20 at 11:00 +0100, Thomas Zimmermann wrote:
> > > 
> > > If that works, would you consider protecting pci_request_region()
> > > with
> > >    #if not defined(CONFIG_FB_SIMPLE)
> > >    #endif
> > > 
> > > with a FIXME comment?
> > 
> > Yes, I think that's a good compromise. I'll respin the patch with
> > that.
> 
> Before you do that, I have one more patch for you to try. It's all
> the 
> changes as before, but now fbdev hot-unplugs the underlying platform 
> device from the device hierarchy. The BOOTFB will not consume parts
> of 
> vmwgfx's display memory range any longer. It's now the same behavior
> as 
> with simpledrm.
> 
> This works for me with simplefb and efifb on i915 hardware.

Yea, that works for me too. Both with simpledrm and simplefb. The patch
looks good too.

Do you think you'll be able to get this in stable kernels? If not I'll
still need something for vmwgfx to make kernels between 5.15 and
whenever this patch gets in boot with fb.

z

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1588 bytes --]

Hi

Am 20.01.22 um 22:28 schrieb Zack Rusin:
> On Thu, 2022-01-20 at 11:00 +0100, Thomas Zimmermann wrote:
>>>>
>>>> If that works, would you consider protecting pci_request_region()
>>>> with
>>>>     #if not defined(CONFIG_FB_SIMPLE)
>>>>     #endif
>>>>
>>>> with a FIXME comment?
>>>
>>> Yes, I think that's a good compromise. I'll respin the patch with
>>> that.
>>
>> Before you do that, I have one more patch for you to try. It's all
>> the
>> changes as before, but now fbdev hot-unplugs the underlying platform
>> device from the device hierarchy. The BOOTFB will not consume parts
>> of
>> vmwgfx's display memory range any longer. It's now the same behavior
>> as
>> with simpledrm.
>>
>> This works for me with simplefb and efifb on i915 hardware.
> 
> Yea, that works for me too. Both with simpledrm and simplefb. The patch
> looks good too.
> 
> Do you think you'll be able to get this in stable kernels? If not I'll
> still need something for vmwgfx to make kernels between 5.15 and
> whenever this patch gets in boot with fb.

I'll prepare a patchset with these changes. The actual fix is the change 
to fbmem.c. This one should go to stable and should be easy to backport. 
All the other patches are mostly for correctness and can go to 
drm-misc-next only.

Can I add your Tested-by tag to the patches?

Best regards
Thomas

> 
> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Zack Rusin]

On Wed, 2022-01-19 at 10:13 +0100, Thomas Zimmermann wrote:
> 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@vmware.com>
> > > > 
> > > > 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.
> 
> 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.


I think that's orthogonal to the fix because having pci_request_region
makes vmwgfx behave differently from majority of DRM drivers, e.g. on
systems with sysfb enabled with 5.15 vmwgfx fails to boot and leaves
the system broken without any fb driver (because while we have
*remove_conflicting*_framebuffers we don't have drm_restore_system_fb
or such to load back the boot fb after drm driver load fails) but since
it's one of the few drivers which does request regions it took a bit
for us to notice.

So in this case I'd much rather be like the other drivers rather than
correct because it lowers the odds of vmwgfx breaking in the future.

z

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4331 bytes --]

Hi

Am 19.01.22 um 15:24 schrieb Zack Rusin:
> On Wed, 2022-01-19 at 10:13 +0100, Thomas Zimmermann wrote:
>> 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@vmware.com>
>>>>>
>>>>> 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.
>>
>> 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.
> 
> 
> I think that's orthogonal to the fix because having pci_request_region
> makes vmwgfx behave differently from majority of DRM drivers, e.g. on
> systems with sysfb enabled with 5.15 vmwgfx fails to boot and leaves
> the system broken without any fb driver (because while we have
> *remove_conflicting*_framebuffers we don't have drm_restore_system_fb
> or such to load back the boot fb after drm driver load fails) but since
> it's one of the few drivers which does request regions it took a bit
> for us to notice.
> 
> So in this case I'd much rather be like the other drivers rather than
> correct because it lowers the odds of vmwgfx breaking in the future.

Well, if you want to remove the calls then do so, of course.

I'd rather add the request_memory() calls to all the other drivers that 
are missing them. Javier suggested to make this an official TODO item. 
Having the BUSY flag set when a driver is active, still is the correct 
thing to do.

I'm not sure i understand your comment about drm_restore_system_fb. 
There is no way of atomically switching drivers and that's always been a 
problem. Failing at pci_request_memroy() is just one of many possible 
reasons for the switch to fail. The best you could do is to rearrange 
the code to do 'remove_conflicting*()' at the latest point possible.

Best regards
Thomas

> 
> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/vmwgfx: Stop requesting the pci regions

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1959 bytes --]

Hi Zack

Am 17.01.22 um 19:03 schrieb Zack Rusin:
> From: Zack Rusin <zackr@vmware.com>
> 
> 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.

I don't understand this sentence. There's only one memory resource 
claimed by the sysfb code. What else would block vmwgfx?

I appears to me as if simplefb should release the region (or the 
simple-framebuffer device).

simpledrm does a hot-unplug of the simple-framebuffer, so the region 
should be released afterwards.

Best regard
Thomas

> 
> Like 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.
> 
> Signed-off-by: Zack Rusin <zackr@vmware.com>
> Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64")
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Martin Krastev <krastevm@vmware.com>
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index fe36efdb7ff5..27feb19f3324 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -724,10 +724,6 @@ static int vmw_setup_pci_resources(struct vmw_private *dev,
>   
>   	pci_set_master(pdev);
>   
> -	ret = pci_request_regions(pdev, "vmwgfx probe");
> -	if (ret)
> -		return ret;
> -
>   	dev->pci_id = pci_id;
>   	if (pci_id == VMWGFX_PCI_ID_SVGA3) {
>   		rmmio_start = pci_resource_start(pdev, 0);

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]