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 04:32:19PM +0200, Thomas Zimmermann wrote:
> 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@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.

But you still pass a pci_dev to that helper. Which just doesn't make any
sense to me (assuming your entire point is that this isn't just a normal
pci device but some special legacy vga thing), but if we go with (void)
then there's more refactoring to do because the vga_remove_vgacon also
wants a pdev.

All so that we don't call aperture_detach_devices() on a bunch of pci
bars, which apparently is not problem for any other driver, but absolutely
is a huge problem for gma500 somehow.

I don't understand why.

Consider this me throwing in the towel. If you&Javier are convinced this
makes sense please type it up and merge it, but I'm not going to type
something that just doesn't make sense to me.
-Daniel

> 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




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch