Nouveau - Nov 2021 - [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

Hi Javier,

On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas
wrote:
> Some DRM drivers check the vgacon_text_force() function return value as an
> indication on whether they should be allowed to be enabled or not.
> 
> This function returns true if the nomodeset kernel command line parameter
> was set. But there may be other conditions besides this to determine if a
> driver should be enabled.
> 
> Let's add a drm_drv_enabled() helper function to encapsulate that logic
so
> can be later extended if needed, without having to modify all the drivers.
> 
> Also, while being there do some cleanup. The vgacon_text_force() function
> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do
it.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann at suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
> ---
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..3fb567d62881 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const
char *name)
>  }
>  EXPORT_SYMBOL(drm_dev_set_unique);
>  
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_drv_enabled(const struct drm_driver *driver)
> +{
> +	if (vgacon_text_force()) {
> +		DRM_INFO("%s driver is disabled\n", driver->name);
DRM_INFO is deprecated, please do not use it in new code.
Also other users had an error message and not just info - is info
enough?

> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);
> +
>  /*
>   * DRM Core
>   * The DRM core module initializes all global DRM objects and makes them
> diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
> index ab2295dd4500..45cb3e540eff 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -18,9 +18,12 @@
>  #include "i915_selftest.h"
>  #include "i915_vma.h"
>  
> +static const struct drm_driver driver;
Hmmm...
> +
>  static int i915_check_nomodeset(void)
>  {
>  	bool use_kms = true;
> +	int ret;
>  
>  	/*
>  	 * Enable KMS by default, unless explicitly overriden by
> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
>  	if (i915_modparams.modeset == 0)
>  		use_kms = false;
>  
> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
> +	ret = drm_drv_enabled(&driver);
You pass the local driver variable here - which looks wrong as this is
not the same as the driver variable declared in another file.

Maybe move the check to new function you can add to init_funcs,
and locate the new function in i915_drv - so it has access to driver?


	Sam

On Thu, 04 Nov 2021, Sam Ravnborg <sam at ravnborg.org>
wrote:
> Hi Javier,
>
> On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote:
>> Some DRM drivers check the vgacon_text_force() function return value as
an
>> indication on whether they should be allowed to be enabled or not.
>> 
>> This function returns true if the nomodeset kernel command line
parameter
>> was set. But there may be other conditions besides this to determine if
a
>> driver should be enabled.
>> 
>> Let's add a drm_drv_enabled() helper function to encapsulate that
logic so
>> can be later extended if needed, without having to modify all the
drivers.
>> 
>> Also, while being there do some cleanup. The vgacon_text_force()
function
>> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to
do it.
>> 
>> Suggested-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
>> ---
>> 
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 8214a0b1ab7f..3fb567d62881 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev,
const char *name)
>>  }
>>  EXPORT_SYMBOL(drm_dev_set_unique);
>>  
>> +/**
>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>> + * @driver: DRM driver to check
>> + *
>> + * Checks whether a DRM driver can be enabled or not. This may be the
case
>> + * if the "nomodeset" kernel command line parameter is used.
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int drm_drv_enabled(const struct drm_driver *driver)
>> +{
>> +	if (vgacon_text_force()) {
>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>
> DRM_INFO is deprecated, please do not use it in new code.
> Also other users had an error message and not just info - is info
> enough?
>
>
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_drv_enabled);
>> +
>>  /*
>>   * DRM Core
>>   * The DRM core module initializes all global DRM objects and makes
them
>> diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
>> index ab2295dd4500..45cb3e540eff 100644
>> --- a/drivers/gpu/drm/i915/i915_module.c
>> +++ b/drivers/gpu/drm/i915/i915_module.c
>> @@ -18,9 +18,12 @@
>>  #include "i915_selftest.h"
>>  #include "i915_vma.h"
>>  
>> +static const struct drm_driver driver;
> Hmmm...
>
>> +
>>  static int i915_check_nomodeset(void)
>>  {
>>  	bool use_kms = true;
>> +	int ret;
>>  
>>  	/*
>>  	 * Enable KMS by default, unless explicitly overriden by
>> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
>>  	if (i915_modparams.modeset == 0)
>>  		use_kms = false;
>>  
>> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
>> +	ret = drm_drv_enabled(&driver);
>
> You pass the local driver variable here - which looks wrong as this is
> not the same as the driver variable declared in another file.
Indeed.
> Maybe move the check to new function you can add to init_funcs,
> and locate the new function in i915_drv - so it has access to driver?
We don't really want that, though. This check is pretty much as early as
it can be, and there's a ton of useless initialization that would happen
if we waited until drm_driver is available.
>From my POV, drm_drv_enabled() is a solution that creates a worse
problem for us than it solves.


BR,
Jani.

>
>
> 	Sam
-- 
Jani Nikula, Intel Open Source Graphics Center

Hello Sam,

On 11/4/21 18:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Sam Ravnborg <sam at ravnborg.org> wrote:
>> Hi Javier,
>>
>> On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas
wrote:
>>> Some DRM drivers check the vgacon_text_force() function return
value as an
>>> indication on whether they should be allowed to be enabled or not.
>>>
>>> This function returns true if the nomodeset kernel command line
parameter
>>> was set. But there may be other conditions besides this to
determine if a
>>> driver should be enabled.
>>>
>>> Let's add a drm_drv_enabled() helper function to encapsulate
that logic so
>>> can be later extended if needed, without having to modify all the
drivers.
>>>
>>> Also, while being there do some cleanup. The vgacon_text_force()
function
>>> is guarded by CONFIG_VGA_CONSOLE and there's no need for
callers to do it.
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> Signed-off-by: Javier Martinez Canillas <javierm at
redhat.com>
>>> ---
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 8214a0b1ab7f..3fb567d62881 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev,
const char *name)
>>>  }
>>>  EXPORT_SYMBOL(drm_dev_set_unique);
>>>  
>>> +/**
>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>> + * @driver: DRM driver to check
>>> + *
>>> + * Checks whether a DRM driver can be enabled or not. This may be
the case
>>> + * if the "nomodeset" kernel command line parameter is
used.
>>> + *
>>> + * Return: 0 on success or a negative error code on failure.
>>> + */
>>> +int drm_drv_enabled(const struct drm_driver *driver)
>>> +{
>>> +	if (vgacon_text_force()) {
>>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>>
>> DRM_INFO is deprecated, please do not use it in new code.
>> Also other users had an error message and not just info - is info
>> enough?
>>
Thanks, I didn't know that. Right, they had an error but I do wonder
if that was correct though. After all isn't an error but an explicit
disable due "nomodeset" being set in the kernel command line.

[snip]
>>>  
>>> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
>>> +	ret = drm_drv_enabled(&driver);
>>
>> You pass the local driver variable here - which looks wrong as this is
>> not the same as the driver variable declared in another file.
>
Yes, Jani mentioned it already. I got confused and thought that the driver
variable was also defined in the same compilation unit...

Maybe I could squash the following change?

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b18a250e5d2e..b8f399b76363 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -89,7 +89,7 @@
 #include "intel_region_ttm.h"
 #include "vlv_suspend.h"
 
-static const struct drm_driver driver;
+const struct drm_driver driver;
 
 static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
 {
@@ -1777,7 +1777,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
        DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
DRM_RENDER_ALLOW),
 };
 
-static const struct drm_driver driver = {
+const struct drm_driver driver = {
        /* Don't use MTRRs here; the Xserver or userspace app should
         * deal with them for Intel hardware.
         */
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index c890c1ca20c4..88f770920324 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -16,7 +16,7 @@
 #include "i915_selftest.h"
 #include "i915_vma.h"
 
-static const struct drm_driver driver;
+extern const struct drm_driver driver;
 
 static int i915_check_nomodeset(void)
 {

That should work and I actually got a laptop with an i915 and tested the change
both when CONFIG_DRM_I915=m and CONFIG_DRM_I915=y is set.

Another option is to declare it in i915_drv.h and not make the definition
static.
 
> Indeed.
> 
>> Maybe move the check to new function you can add to init_funcs,
>> and locate the new function in i915_drv - so it has access to driver?
> 
> We don't really want that, though. This check is pretty much as early
as
> it can be, and there's a ton of useless initialization that would
happen
> if we waited until drm_driver is available.
>
Agreed.
 
> From my POV, drm_drv_enabled() is a solution that creates a worse
> problem for us than it solves.
>
I don't have a strong opinion on this. I could just do patch #2 without
adding
a level of indirection through drm_drv_enabled(). But Thomas and Daniel Vetter
suggested that we should do this change before.

That is, the drivers could just check if should be enabled by calling to the
drm_check_modeset() function directly if people agree that encapsulating that
logic in a drm_drv_enabled() is not needed.
> 
> BR,
> Jani.
> 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat