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

[Thomas Zimmermann]

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 at 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.
> 	 */

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20230405/340974ee/attachment.sig>