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

[Zack Rusin <zackr@vmware.com>]

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