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
