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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux