Hi Am 05.04.23 um 11:38 schrieb Daniel Vetter:
On Wed, Apr 05, 2023 at 11:26:51AM +0200, Thomas Zimmermann wrote: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 havingIt'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.I don't see why it can't be a pci vga card. There is no requirement that a pci vga card must be also a non-vga card with real non-vga framebuffer. We don't have a hole lot of them really.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.The thing is, pci is self-describing. We don't need to open code every variant in every driver, the pci code can figure out in a generic way whether vga needs to be nuked or not. That's the entire point of this refactoring. Also note that we nuke all bars, and on most pci cards that will include a bunch of mmio bars which will never ever hold a framebuffer. And the old per-driver open-coded version ensured that we only nuked the pci bar that could potentially contain the framebuffer.
I never talked about about PCI. I'm saying that the current code is not easy to understand.
Why is gma500 special and it needs to be the only pci driver where we intentionally filter out all the bars that wont ever contain a framebuffer? If this is your argument, the entire series is toast, not just the gma500 part.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.I didn't know about the dual-nature of gma500. Which is why I added a comment to explain what's going on, since at first glance it just looked like someone didn't bother to implement the open-coded pci conflicting driver removal correctly. It's by far not the only driver that was sloppy,
I know. And I think you've added the same problem again in a different look. I expect that the next guy who looks at this code will again think that someone messed up the open coded PCI handling.
Your comment says that it calls a PCI function to clean up to vgacon. That comment explains what is happening, not why. And how the PCI and vgacon code work together is non-obvious.
Again, here's my proposal for gma500:
// call this from psb_pci_probe()
int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
struct drm_driver *req_driver)
{
resource_size_t base = 0;
resource_size_t size = (resource_size_t)-1;
const char *name = req_driver->name;
int ret;
/*
* We cannot yet easily find the framebuffer's location in
* memory. So remove all framebuffers here.
*
* TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
* we might be able to read the framebuffer range from the
* device.
*/
ret = aperture_remove_conflicting_devices(base, size, name);
if (ret)
return ret;
/*
* WARNING: Apparently we must kick fbdev drivers before vgacon,
* otherwise the vga fbdev driver falls over.
*/
ret = vga_remove_vgacon(pdev);
if (ret)
return ret;
return 0;
}
Best regards
Thomas
a bunch did not compute the primary flag correctly at least. Maybe I overlooked some really funny special case there too? Also I think my goal fits, because if we'd have had the newly proposed helpers, then gma500 would have needed to have the two calls + comments from the start. Which would have helped me to realize that this is indeed intentionally special and not accidentally buggy. As-is, without the obvious special case in code or some comment or digging around elsewhere it just looks buggy. -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
