Thomas Zimmermann <tzimmermann@xxxxxxx> writes: [...] > Am 04.04.23 um 22:18 schrieb Daniel Vetter: > Gma500 therefore calls both helpers to catch all cases. It's confusing > as it implies that there's something about the PCI device that requires > ownership management. The relationship between the PCI device and the > VGA devices is non-obvious. At worst, readers might assume that calling > two functions for aperture clearing ownership is a bug in the driver. > Yeah, or someone looking as the driver for reference may wrongly think that calling both aperture helpers are needed for PCI drivers and that is not the case. > Hence, move the PCI removal helper's code for VGA functionality into > a separate function and call this function from gma500. Documents the > purpose of each call to aperture helpers. The change contains comments > and example code form the discussion at [1]. > > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> > Link: https://patchwork.kernel.org/project/dri-devel/patch/20230404201842.567344-1-daniel.vetter@xxxxxxxx/ # 1 > --- Looks good to me and I agree that it makes the code easier to understand. Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> I've a couple of comments below though: [...] > + * Hardware for gma500 is a 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. > + * Is "have" the correct word here or "do" ? Or maybe "are implemented" ? > + * To catch all cases we need to remove conflicting firmware devices > + * for the stolen system memory and for the VGA functionality. As we > + * currently cannot easily find the framebuffer's location in stolen > + * memory, we 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. > + */ > +static int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, > + const struct drm_driver *req_driver) > { > - struct drm_psb_private *dev_priv; > - struct drm_device *dev; > + resource_size_t base = 0; > + resource_size_t size = PHYS_ADDR_MAX; > + const char *name = req_driver->name; > int ret; > > - /* > - * We cannot yet easily find the framebuffer's location in memory. So > - * remove all framebuffers here. Note that we still want the pci special > - * handling to kick out vgacon. > - * > - * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we > - * might be able to read the framebuffer range from the device. > - */ > - ret = drm_aperture_remove_framebuffers(&driver); > + ret = aperture_remove_conflicting_devices(base, size, name); > if (ret) > return ret; > > - ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver); > + return __aperture_remove_legacy_vga_devices(pdev); I don't like the __ prefix for this function name, as it usually implies that is a function that is only local to the compilation unit. But it is an exported symbol from the aperture infrastructure. [...] > +/** > + * __aperture_remove_legacy_vga_devices - remove legacy VGA devices of a PCI devices > + * @pdev: PCI device > + * > + * This function removes VGA devices provided by @pdev, such as a VGA > + * framebuffer or a console. This is useful if you have a VGA-compatible > + * PCI graphics device with framebuffers in non-BAR locations. Drivers > + * should acquire ownership of those memory areas and afterwards call > + * this helper to release remaining VGA devices. > + * > + * If your hardware has its framebuffers accessible via PCI BARS, use > + * aperture_remove_conflicting_pci_devices() instead. The function will > + * release any VGA devices automatically. > + * > + * WARNING: Apparently we must remove graphics drivers before calling > + * this helper. Otherwise the vga fbdev driver falls over if > + * we have vgacon configured. > + * > + * Returns: > + * 0 on success, or a negative errno code otherwise > + */ > +int __aperture_remove_legacy_vga_devices(struct pci_dev *pdev) > +{ > + /* VGA framebuffer */ > + aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE); > + > + /* VGA textmode console */ > + return vga_remove_vgacon(pdev); > +} > +EXPORT_SYMBOL(__aperture_remove_legacy_vga_devices); I would just call this symbol aperture_remove_legacy_vga_devices() as mentioned, the fact that aperture_remove_conflicting_pci_devices() use it internally is an implementation detail IMO. But it's an exported symbol so the naming should be consistent. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
