Sam Ravnborg
2021-Nov-04 17:37 UTC
[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
Jani Nikula
2021-Nov-04 17:57 UTC
[PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
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 worseproblem for us than it solves. BR, Jani.> > > Sam-- Jani Nikula, Intel Open Source Graphics Center
Javier Martinez Canillas
2021-Nov-04 18:20 UTC
[Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
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