| Message ID | 20220117180359.18114-1-zack@kde.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/vmwgfx: Stop requesting the pci regions | expand |
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,
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,
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
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);
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
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 >
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
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 >
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
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
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]:
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
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
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,
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
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
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
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);