Hi Am 05.04.23 um 15:18 schrieb Daniel Vetter:
On Wed, Apr 05, 2023 at 01:16:27PM +0200, Javier Martinez Canillas wrote:Thomas Zimmermann <tzimmermann@xxxxxxx> writes: [...]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.Would a better comment help then: /* * gma500 is a strange hybrid device, which both acts as a pci * device (for legacy vga functionality) but also more like an * integrated display on a SoC where the framebuffer simply * resides in main memory and not in a special pci bar (that * internally redirects to a stolen range of main memory) like all * other integrated pci display devices have. * * To catch all cases we need to both remove conflicting fw * drivers for the pci device and main memory. */
Together with the existing comment, this should be the comment to describe gma_remove_conflicting_framebuffers().
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);Why can't this be a call to drm_aperture_remove_framebuffers? At least as long as we don't implement the "read out actual fb base and size" code, which also none of the other soc drivers bother with?
It can. Feel free to use it.But I have to say that those DRM helpers are somewhat empty and obsolete after the aperture code has been moved to drivers/video/. They exist mostly for convenience. As with other DRM helpers, if a driver needs something special, it can ignore them.
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);This isn't enough, we also nuke stuff that's mapping the vga fb range. Which is really the reason I don't want to open code random stuff, pci is self-describing, if it's decoding legacy vga it can figure this out and we only have to implement the "how do I nuke legacy vga fw drivers from a pci driver" once.
Sure, but it's really just one additional line: aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);as you mention below, this and vgacon can be exported in a single VGA aperture helper.
Not twice like this would result in, with the gma500 version being only half the thing. If it absolutely has to be a separate function for the gma500 pci legacy vga (I still don't get why, it's just a pci vga device, there's absolutely nothing special about that part at all) then I think it needs to be at least a common "nuke a legacy vga device for me pls" function, which shares the implementation with the pci one.
Sure
/**
* kerneldoc goes here
*
* WARNING: Apparently we must remove graphics drivers before calling
* this helper. Otherwise the vga fbdev driver falls over if
* we have vgacon configured.
*/
int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
{
aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
return vga_remove_vgacon(pdev);
}
And that can be called from gma500 and the pci aperture helper.
Best regards
Thomas
But not open-coding just half of it only.if (ret) return ret; return 0; }If this is enough I agree that is much more easier code to understand.It's still two calls and more code with more bugs? I'm not seeing the point. -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
