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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 5827 bytes --]

Hi

Am 06.04.23 um 10:38 schrieb Javier Martinez Canillas:
> 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>

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]