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

[Javier Martinez Canillas <javierm@redhat.com>]

Thomas Zimmermann <tzimmermann@suse.de> 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@suse.de>
> Link: https://patchwork.kernel.org/project/dri-devel/patch/20230404201842.567344-1-daniel.vetter@ffwll.ch/ # 1
> ---

Looks good to me and I agree that it makes the code easier to understand.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

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