Hello Thomas,
Thanks a lot for your feedback.
On 10/22/21 21:05, Thomas Zimmermann wrote:
[snip]
+static bool drm_aperture_remove_fb = true;
Global variables should default to zero if somehow possible. This way, they can all be stored in the BSS segment and backed by a single shared zero-filled page. Otherwise they require actual memory. In the worst case, you'd allocate a full page to hold a single boolean.
+module_param_named(remove_fb, drm_aperture_remove_fb, bool, 0600); +MODULE_PARM_DESC(remove_fb,
"Allow conflicting framebuffers removal [default=true]");And with variables set to zero, a command-line parameter enables non-default behavior (i.e., "drm-param=1"). That more logical than the other way around IMHO.
Agreed. I'll change that.
/**
- DOC: overview
@@ -283,6 +288,9 @@ static void drm_aperture_detach_drivers(resource_size_t base, resource_size_t si
- This function removes graphics device drivers which use memory range described by
- @base and @size.
- The conflicting framebuffers removal can be disabled by setting the drm.remove_fb=0 kernel
- command line option. When this is disabled, the function will return an -EINVAL errno code.
Please use -EBUSY for the error. That's what the acquire function returns in case of a conflict.
Sure, makes sense. I was pondering between -EINVAL, -EBUSY and -EPERM.
*/
- Returns:
- 0 on success, or a negative errno code otherwise
@@ -292,7 +300,12 @@ int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_ #if IS_REACHABLE(CONFIG_FB)
Rather not split up this block. It's better style to put the fbdev-related code into a helper and call it unconditionally.
static drm_aperture_remove_conflicting_fbdev_framebuffers() { #if (FB) ... #endif return 0; }
Ok.
struct apertures_struct *a; int ret; +#endif
- if (!drm_aperture_remove_fb)
return -EINVAL;There's still the question of the semantics of this parameter. It's a bit fuzzy.
If you use 'disable_handover' (as you mentioned in another mail), it would mean that only the handover itself is disabled. So if simpledrm is not bound to the device, then a native driver should load. That would be hard to implement with the current code base, where we have to take old fbdev drivers into account.
(And to be pedantic, we don't really do a handover of the device. We hot-unplug the generic platform device, so that the driver for the native device can operate the HW without interference.)
Simpledrm only acquires an aperture, but never removes a driver. If there is a driver already, simpledrm would fail. Only native drivers try > to remove drivers and would trigger the test. So your patch is more something like 'disable_native_drivers'.
I'd go with 'disable_native_drivers', or maybe 'disable_device_handover'
That works for me and "drm.disable_native_drivers" is also what Neal meant with his "drm.noplatformdrv", but yours is much easier to remember / type.
Best regards,