Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal

[Javier Martinez Canillas <javierm@redhat.com>]

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