Messages in this thread | | | Date | Sun, 24 Oct 2021 22:40:25 +0200 | Subject | Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal | From | Javier Martinez Canillas <> |
| |
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, -- Javier Martinez Canillas Linux Engineering Red Hat
| |