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

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

 



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


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux