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@xxxxxxxxxx> 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:
OpenPGP_signature
Description: OpenPGP digital signature
