Hello Sam,
On 11/4/21 18:57, Jani Nikula wrote:
On Thu, 04 Nov 2021, Sam Ravnborg sam@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@suse.de Signed-off-by: Javier Martinez Canillas javierm@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,