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

Hello Thomas,

On 6/21/22 13:29, Thomas Zimmermann wrote:

[...]

quoted

quoted

+
+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.

Yes, that works too. Probably then we should just keep as is and then as a follow
up we can add another helper to include/linux/ioport.h to avoid having something
that's only for the aperture helpers.

quoted

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?

I don't know to be honest.

[...]

quoted

quoted

+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.

Ok, I was wondering what was the value of the indirection level other than
making the code more complex and supporting an hypothetical case of a FW
driver that would not bind against a platform device.

But if the goal is to move this at some point to a more generic place (i.e:
the Linux device model itself) then I agree that we can just keep it as is.

When you re-spin this patch, feel free to add my R-b since looks good to me.

And thanks again for working on this!

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat