Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Thomas Zimmermann
Hi Am 05.11.21 um 13:00 schrieb Javier Martinez Canillas: On 11/5/21 11:04, Jani Nikula wrote: On Fri, 05 Nov 2021, Javier Martinez Canillas wrote: [snip] Do you envision other condition that could be added later to disable a DRM driver ? Or do you think that just from a code readability

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Javier Martinez Canillas
On 11/5/21 11:04, Jani Nikula wrote: > On Fri, 05 Nov 2021, Javier Martinez Canillas wrote: [snip] >> >> Do you envision other condition that could be added later to disable a >> DRM driver ? Or do you think that just from a code readability point of >> view makes worth it ? > > Taking a step

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Jani Nikula
On Fri, 05 Nov 2021, Javier Martinez Canillas wrote: > Hello Thomas, > > On 11/5/21 09:43, Thomas Zimmermann wrote: >> Hi >> >> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas: >>> Hello Jani, >>> >>> On 11/4/21 20:57, Jani Nikula wrote: On Thu, 04 Nov 2021, Javier Martinez Canillas

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Thomas Zimmermann
Hi Am 05.11.21 um 10:48 schrieb Javier Martinez Canillas: Hello Thomas, On 11/5/21 09:43, Thomas Zimmermann wrote: Hi Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas: Hello Jani, On 11/4/21 20:57, Jani Nikula wrote: On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: +/** + *

Re: [Spice-devel] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

2021-11-05 Thread Javier Martinez Canillas
On 11/5/21 10:39, Thomas Zimmermann wrote: [snip] +obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o + >>> >>> This now depends on the VGA textmode console. Even if you have no VGA >>> console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can >>> provide graphics.

Re: [Spice-devel] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

2021-11-05 Thread Javier Martinez Canillas
On 11/5/21 10:00, Thomas Zimmermann wrote: [snip] >> + >> +static int __init disable_modeset(char *str) >> +{ >> +drm_nomodeset = true; >> + >> +pr_warn("You have booted with nomodeset. This means your GPU drivers >> are DISABLED\n"); >> +pr_warn("Any video related functionality

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Javier Martinez Canillas
Hello Thomas, On 11/5/21 09:43, Thomas Zimmermann wrote: > Hi > > Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas: >> Hello Jani, >> >> On 11/4/21 20:57, Jani Nikula wrote: >>> On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: +/** + * drm_drv_enabled - Checks if a DRM driver

Re: [Spice-devel] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

2021-11-05 Thread Jani Nikula
On Fri, 05 Nov 2021, Thomas Zimmermann wrote: > Hi > > Am 04.11.21 um 17:07 schrieb Javier Martinez Canillas: >> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver >> but the exported vgacon_text_force() symbol is only used by DRM drivers. >> >> It makes much more sense for

Re: [Spice-devel] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

2021-11-05 Thread Thomas Zimmermann
Hi Am 05.11.21 um 10:22 schrieb Jani Nikula: On Fri, 05 Nov 2021, Thomas Zimmermann wrote: Hi Am 04.11.21 um 17:07 schrieb Javier Martinez Canillas: The "nomodeset" kernel cmdline parameter is handled by the vgacon driver but the exported vgacon_text_force() symbol is only used by DRM

Re: [Spice-devel] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

2021-11-05 Thread Thomas Zimmermann
Hi Am 04.11.21 um 17:07 schrieb Javier Martinez Canillas: The "nomodeset" kernel cmdline parameter is handled by the vgacon driver but the exported vgacon_text_force() symbol is only used by DRM drivers. It makes much more sense for the parameter logic to be in the subsystem of the drivers

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Thomas Zimmermann
Hi Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas: Hello Jani, On 11/4/21 20:57, Jani Nikula wrote: On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: +/** + * drm_drv_enabled - Checks if a DRM driver can be enabled + * @driver: DRM driver to check + * + * Checks whether a DRM

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Jani Nikula
On Thu, 04 Nov 2021, Sam Ravnborg wrote: > Hi Javier, > >> >> >>> >> >>> -if (vgacon_text_force() && i915_modparams.modeset == -1) >> >>> +ret = drm_drv_enabled(); >> >> >> >> You pass the local driver variable here - which looks wrong as this is >> >> not the same as the

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Javier Martinez Canillas
Hello Jani, On 11/4/21 20:57, Jani Nikula wrote: > On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: >> +/** >> + * 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

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Jani Nikula
On Thu, 04 Nov 2021, 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

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

2021-11-05 Thread Javier Martinez Canillas
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

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Javier Martinez Canillas
Hello Sam, On 11/4/21 18:57, Jani Nikula wrote: > On Thu, 04 Nov 2021, Sam Ravnborg 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

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Jani Nikula
On Thu, 04 Nov 2021, Sam Ravnborg 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

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Javier Martinez Canillas
On 11/4/21 17:24, Jani Nikula wrote: [snip] >> 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

[Spice-devel] [PATCH v2 0/2] Cleanups for the nomodeset kernel command line parameter logic

2021-11-05 Thread Javier Martinez Canillas
There is a lot of historical baggage on this parameter. It is defined in the vgacon driver as nomodeset, but its set function is called text_mode() and the value queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in

[Spice-devel] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

2021-11-05 Thread Javier Martinez Canillas
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver but the exported vgacon_text_force() symbol is only used by DRM drivers. It makes much more sense for the parameter logic to be in the subsystem of the drivers that are making use of it. Let's move the vgacon_text_force()

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Sam Ravnborg
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

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Jani Nikula
On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: > +/** > + * 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

Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

2021-11-05 Thread Sam Ravnborg
Hi Javier, > > >>> > >>> - if (vgacon_text_force() && i915_modparams.modeset == -1) > >>> + ret = drm_drv_enabled(); > >> > >> 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