Message-ID: <4f6e9b63-f955-d263-e69b-6396fbe48868@suse.de>
Date:   Tue, 21 Jun 2022 13:29:55 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>, corbet@....net,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        airlied@...ux.ie, daniel@...ll.ch, deller@....de,
        gregkh@...uxfoundation.org
Cc:     dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 1/2] drm: Implement DRM aperture helpers under video/

Hi

Am 21.06.22 um 02:14 schrieb Javier Martinez Canillas:
[...]
> 
> Since we are talking about remove_conflicting_devices() here, a better code
> example could be for a platform device instead of a PCI device, like this:
> 
> *	static const struct platform_driver example_driver = {
> *		.name = "example",
> *		...
> *	};
> *
> *	static int probe(struct platform_device *pdev)
> *	{
> *		struct resource *mem;
> *		resource_size_t base, size;
> *
> *		mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> *		if (!mem)
> *			return -EINVAL;
> *		base = mem->start;
> *		size = resource_size(mem);
> *
> *		ret = remove_conflicting_devices(base, size, false, &example_driver->name);
> *		if (ret)
> *			return ret;
> *
> *		// ... and initialize the hardware.
> *		...
> *
> *		return 0;
> *	}
> 
>> + *	static int probe(struct pci_dev *pdev)
>> + *	{
>> + *		int ret;
>> + *
>> + *		// Remove any generic drivers...
>> + *		ret = remove_conflicting_framebuffers(pdev);
> 
> And here we can just use remove_conflicting_pci_devices(pdev) without the
> unnecessary level of indirection. It makes the example more clear IMO and
> it could be moved as an example for the remove_conflicting_pci_devices().

I'll see if I can simplify the examples.

> 
> Another option is to have here an example for platform devices instead of
> a PCI device (and move this example when talking about remove
> 
> [...]
> 
>> + * PCI device drivers can also call remove_conflicting_pci_devices() and let the
>> + * function detect the apertures automatically. Device drivers without knowledge of
>> + * the framebuffer's location shall call remove_all_conflicting_devices(),
>> + * which removes all known devices.
>> + *
> 
> Can we get all the public aperture functions be in the aperture namespace? i.e:
> aperture_remove_conflicting_devices(), aperture_remove_all_conflicting_devices()
> and so on. That makes easier to grep, ftrace and also read the drivers' code.

Ok.

> 
>> + * Drivers that are susceptible to being removed by other drivers, such as
>> + * generic EFI or VESA drivers, have to register themselves as owners of their
>> + * framebuffer apertures. Ownership of the framebuffer memory is achieved
>> + * by calling devm_acquire_aperture_of_platform_device(). On success, the driver
> 
> AFAICT the aperture infrastructure only allows to remove platform devices, even
> when it can check if the requested I/O resource overlaps with a PCI BAR range,
> so maybe all functions also should use _platform_device() as suffix instead of
> just _device() ? Or maybe the _platform is just verbose but I think that the
> functions should be named consistently and only use either _device or _platform.

That nameing makes sense. Firmware drivers register a platform device. 
But native drivers unregister any device. If we ever have a generic 
driver that does not use platform devices, we'd need another variant of 
devm_acquire_aperture_of_*_device().


> 
> [...]
> 
>> + *	static int acquire_framebuffers(struct drm_device *dev, struct platform_device *pdev)
>> + *	{
>> + *		resource_size_t base, size;
>> + *
> 
> This example is missing a struct resource *mem declaration.
> 
>> + * The generic driver is now subject to forced removal by other drivers. This
>> + * only works for platform drivers that support hot unplugging.
>> + * When a driver calls remove_conflicting_devices() et al
>> + * for the registered framebuffer range, the aperture helpers call
>> + * platform_device_unregister() and the generic driver unloads itself. It
>> + * may not access the device's registers, framebuffer memory, ROM, etc
>> + * afterwards.
>> + */
>> +
>> +struct dev_aperture {
>> +	struct device *dev;
> 
> And here we could just use a struct platform_device *pdev since currently we
> only support platform devices. It seems to me that this is a DRM-ism that we
> are carrying since for DRM drivers made sense to use struct device.
> 
> Doing that would also allow get rid of indirections like the need of both a
> devm_acquire_aperture_of_platform_device() and devm_aperture_acquire() just
> to do a &pdev->dev.
> 
> And also some to_platform_device() in drm_aperture_detach_firmware() and
> detach_platform_device().
> 
> If we ever support non-platform devices then we can refactor it, but I don't
> think that is worth to complicate just in case we ever support struct device.
> 
>> +	resource_size_t base;
>> +	resource_size_t size;
>> +	struct list_head lh;
>> +	void (*detach)(struct device *dev);
> 
> Same here, just	void (*detach)(struct platform_device *pdev) if you agree with
> that I mentioned above.

I took that code from DRM. No need to change it for less flexibility.

> 
>> +};
>> +
>> +static LIST_HEAD(apertures);
>> +static DEFINE_MUTEX(apertures_lock);
>> +
>> +static bool overlap(resource_size_t base1, resource_size_t end1,
>> +		    resource_size_t base2, resource_size_t end2)
>> +{
>> +	return (base1 < end2) && (end1 > base2);
>> +}
> 
> There's a resource_overlaps() helper in include/linux/ioport.h, I wonder if it
> could just be used, maybe declaring and filling a struct resource just to call
> that helper. Later as an optimization a resource_range_overlap() or something
> could be proposed for include/linux/ioport.h.

Bu then we'd have to declare struct resource-es for using an interface. 
This helper is trivial. If anything, resource_overlaps() should be 
generalized.

> 
> Also, I noticed that resource_overlaps() uses <= and >= but this helper uses
> < and >. It seems there's an off-by-one error here but maybe I'm wrong on this.

struct resource stores the final byte of the resource. In our case 'end' 
is the byte after that. So the code is correct.

Do we ever have resources that end at the top-most byte of the address 
space?

> 
> [...]
> 
>> +static void detach_platform_device(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +
>> +	/*
>> +	 * Remove the device from the device hierarchy. This is the right thing
>> +	 * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
>> +	 * the new driver takes over the hardware, the firmware device's state
>> +	 * will be lost.
>> +	 *
>> +	 * For non-platform devices, a new callback would be required.
>> +	 *
> 
> I wonder if we ever are going to need this. AFAICT the problem only happens for
> platform devices. Or do you envision a case when some a bus could need this and
> the aperture unregister the device instead of the Linux kernel device model ?
> 

In the current code, we clearly distinguish between the device and the 
platform device. The latter is only used in a few places where it's 
absolutely necessary, because there's no generic equivalent to 
platform_device_unregister(). (device_unregister() is something else 
AFAICT.)  At some point, I'd like to see the aperture code being handled 
in a more prominent place within resource management. That would need it 
to use struct device.

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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists