Hi Am 05.04.23 um 10:59 schrieb Daniel Vetter:
On Wed, Apr 05, 2023 at 10:07:54AM +0200, Thomas Zimmermann wrote:Hi Am 05.04.23 um 09:49 schrieb Thomas Zimmermann:Hi Am 04.04.23 um 22:18 schrieb Daniel Vetter:This one nukes all framebuffers, which is a bit much. In reality gma500 is igpu and never shipped with anything discrete, so there should not be any difference. v2: Unfortunately the framebuffer sits outside of the pci bars for gma500, and so only using the pci helpers won't be enough. Otoh if we only use non-pci helper, then we don't get the vga handling, and subsequent refactoring to untangle these special cases won't work. It's not pretty, but the simplest fix (since gma500 really is the only quirky pci driver like this we have) is to just have both calls. Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> Cc: Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> Cc: Javier Martinez Canillas <javierm@xxxxxxxxxx> --- drivers/gpu/drm/gma500/psb_drv.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c index 2ce96b1b9c74..f1e0eed8fea4 100644 --- a/drivers/gpu/drm/gma500/psb_drv.c +++ b/drivers/gpu/drm/gma500/psb_drv.c @@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) /* * We cannot yet easily find the framebuffer's location in memory. So - * remove all framebuffers here. + * remove all framebuffers here. Note that we still want the pci special + * handling to kick out vgacon. * * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we * might be able to read the framebuffer range from the device. */ - ret = drm_aperture_remove_framebuffers(true, &driver); + ret = drm_aperture_remove_framebuffers(false, &driver); + if (ret) + return ret; + + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);This simply isn't it. If you have to work around your own API, it's time to rethink the API.Here's a proposal: 1) As you're changing aperture_remove_conflicting_devices() anyway, rename it to aperture_remove_conflicting_devices_at(), so it's clear that it takes a memory range. 2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes a PCI device and a memory range. It should do the is_primary and vgacon stuff, but kick out the range. 3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with the full range. Even if we can restructure gma500 to detect the firmware framebuffer from its registers (there's this TODO item), we'd still need aperture_remove_conflicting_pci_devices_at() to do something useful with it. 4) With that, aperture_remove_conflicting_devices_at() can drop the primary argument. Of course, the DRM-related interface should be adapted as well. There's a bit of overlap in the implementation of both PCI aperture helpers, but the overall interface is clear.This essentially just gives us a helper which does the above two open-coded steps but all wrapped up. For gma500 only. Also I really don't think I'm working around the api here, it's gma500 which is special: - Normal pci devices all have their fw framebuffer within the memory bars, never outside. So for those the pci version is the right interface. - If the framebuffer is outside of the pci bar then it's just not really a pci vga device anymore, but looks a lot more like a SoC design. gma500 is somehow both at the same time, so it gets two calls. And having
It's not "both at the same time." It like an SoC that can act as VGA. But it's not really a PCI graphics card on its own. Maybe that's just nitpicking, though.
both calls explicitly I think is better, because it highlights the dual nature of gma500 of being both a pci vga devices and a SoC embedded device. Trying to make a wrapper to hide this dual nature just so we have a single call still seems worse to me. Aside from it's just boilerplate for no gain. Frankly at that point I think it would be clearer to call the gma500 function remove_conflicting_gma500 or something like that. Or at least remove_conflicting_pci_and_separate_range_at.
Yes. If you don't want a new _pci_devices_at() aperture helper, please duplicate the _pci_devices() helper within gma500 (with its sysfb and vgacon handling). Then let it take the gma500 memory range where the generic _pci() helper iterates over PCI BARs.
This would mark gma500 as special, while clearly communicating what's going on.
This is imo similar to the hypothetical case of a SoC chip which also happens to decode legacy vga, without being a pci device. We could add a new interface function which just nukes the vga stuff (without the pci device tie-in, maybe with some code sharing internally in aperture.c), and then that driver does 2 calls: 1. nuke aperture range 2. nuke vga stuff. And sure if you have a lot of those maybe you could make a helper to safe a few lines of code, but semantically it's still two different things your're removing. Or another case: A pci device with 2 subfunctions, each a gpu device. This happened with dual-head gpus 20 years ago because windows 2000 insisted that each crtc needs its own pci function. You'd just call the pci removal twice for that too (except not relevant because bios fw never figured out how to enable both heads, so just nuking the first one is good enough). iow, I don't understand why you think this is the wrong api. There's no rule that a driver must be able remove all conflicting fw drivers in a single call, if it's two things we need to nuke it can be two calls.
Your stated goal is to simplify the aperture interface and make it harder to misuse. But the reasoning behind the new code in gma500 is not understandable without following the discussion closely or without knowing the hardware. Remember that your first iteration of this patch actually got it wrong, because gma500 is different. So there should be one aperture call here that does the right thing for gma500.
Best regards Thomas
-Daniel
-- 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
