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

[Thomas Zimmermann]

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 at 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 at vmware.com>
>>>> Fixes: 523375c943e5 ("drm/vmwgfx: Port vmwgfx to arm64")
>>>> Cc: dri-devel at lists.freedesktop.org
>>>> Cc: <stable at vger.kernel.org>
>>>> Reviewed-by: Martin Krastev <krastevm at vmware.com>
>>>> ---
>>>
>>> The patch looks good to me, thanks a lot for fixing this:
>>>
>>> Reviewed-by: Javier Martinez Canillas <javierm at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-RFC-drop-IORESOURCE_BUSY-from-sysfb-code-and-request.patch
Type: text/x-patch
Size: 5678 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220119/31133457/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220119/31133457/attachment.sig>