Re: [Intel-gfx] [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 05, 2023 at 01:16:27PM +0200, Javier Martinez Canillas wrote:
> Thomas Zimmermann <tzimmermann@suse.de> 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.
	 */
> >
> > 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?

> > 	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.

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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch