Hi Am 06.04.23 um 10:38 schrieb Javier Martinez Canillas:
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>
Thanks for the review.
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" ?
Right. I'll update this.
+ * 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.
That prefix __ is supposed to indicate that it's not a all-in-one solution. Most of all, it doesn't do sysfb_disable(). The helper is meant to be used as part of a larger function. I tried to outline this in the comment, where I say that drivers should first aquire framebuffer ownership and then call this helper. If naming isn't a showstopper, I'd like to keep the underscores.
Best regards Thomas
-- 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
