Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <015855d9-62f3-be81-a4c1-b8439534ec06@redhat.com>
Date:   Sun, 24 Oct 2021 22:40:25 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Subject: Re: [RFC PATCH] drm/aperture: Add param to disable conflicting
 framebuffers removal
Content-Language: en-US
To:     Thomas Zimmermann <tzimmermann@suse.de>,
        linux-kernel@vger.kernel.org
Cc:     Peter Robinson <pbrobinson@gmail.com>,
        Neal Gompa <ngompa13@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        dri-devel@lists.freedesktop.org
References: <20211022144040.3418284-1-javierm@redhat.com>
 <14a41bd8-cd70-b9d0-ce1c-869cfde8bdcb@suse.de>
From:   Javier Martinez Canillas <javierm@redhat.com>
In-Reply-To: <14a41bd8-cd70-b9d0-ce1c-869cfde8bdcb@suse.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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