Re: [PATCH v2 1/2] drm: Implement DRM aperture helpers under video/

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux