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

On Fri, 05 Nov 2021, Javier Martinez Canillas [off-list ref] wrote:

Hello Thomas,

On 11/5/21 09:43, Thomas Zimmermann wrote:

quoted

Hi

Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:

quoted

Hello Jani,

On 11/4/21 20:57, Jani Nikula wrote:

quoted

On Thu, 04 Nov 2021, Javier Martinez Canillas [off-list ref] wrote:

quoted

+/**
+ * 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)

Jani mentioned that i915 absolutely wants this to run from the 
module_init function. Best is to drop the parameter.

Ok. I now wonder though how much value would add this function since
it will just be a wrapper around the nomodeset check.

We talked about adding a new DRIVER_GENERIC feature flag and check for
this, but as danvet mentioned that is not really needed. We just need
to avoid testing for nomodeset in the simpledrm driver.

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 back for perspective.

I think there's broad consensus in moving the parameter to drm, naming
the check function to drm_something_something(), and breaking the ties
to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that
effect.

I think everything beyond that is still a bit vague and/or
contentious. So how about making the first 2-3 patches just that?
Something we can all agree on, makes good progress, improves the kernel,
and gives us something to build on?

BR,
Jani.

quoted

quoted

quoted

quoted

+{
+	if (vgacon_text_force()) {
+		DRM_INFO("%s driver is disabled\n", driver->name);
+		return -ENODEV;
+	}

If we run this from within a module_init function, we'd get plenty of 
these warnings if drivers are compiled into the kernel. Maybe simply 
remove the message. There's already a warning printed by the nomodeset 
handler.

Indeed. I'll just drop it.

quoted

quoted

quoted

quoted

+
+	return 0;
+}
+EXPORT_SYMBOL(drm_drv_enabled);

The name implies a bool return, but it's not.

	if (drm_drv_enabled(...)) {
		/* surprise, it's disabled! */
	}

It used to return a bool in v2 but Thomas suggested an int instead to
have consistency on the errno code that was returned by the callers.

I should probably name that function differently to avoid confusion.

Yes, please.

drm_driver_check() maybe ?
 
Best regards,

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization