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

[Thomas Zimmermann]

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 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.
>>
>> 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
-------------- 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/18630a77/attachment.sig>