Re: [PATCH] drm/xe: Support 'nomodeset' kernel command-line option

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 27.08.24 um 05:59 schrieb Lucas De Marchi:
> On Wed, Aug 21, 2024 at 04:48:23PM GMT, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 21.08.24 um 16:29 schrieb Jani Nikula:
>>> On Wed, 21 Aug 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>>> Quoting Thomas Zimmermann (2024-08-21 10:56:59-03:00)
>>>>> Setting 'nomodeset' on the kernel command line disables all graphics
>>>>> drivers with modesetting capabilities; leaving only firmware drivers,
>>>>> such as simpledrm or efifb.
>>>>>
>>>>> Most DRM drivers automatically support 'nomodeset' via DRM's module
>>>>> helper macros. In xe, which uses regular module_init(), manually call
>>>>> drm_firmware_drivers_only() to test for 'nomodeset'. Do not register
>>>>> the driver if set.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>> drivers/gpu/drm/xe/xe_module.c | 5 +++++
>>>>> 1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_module.c 
>>>>> b/drivers/gpu/drm/xe/xe_module.c
>>>>> index 923460119cec..60fb7dd26903 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_module.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_module.c
>>>>> @@ -8,6 +8,8 @@
>>>>> #include <linux/init.h>
>>>>> #include <linux/module.h>
>>>>>
>>>>> +#include <drm/drm_module.h>
>>>>> +
>>>>> #include "xe_drv.h"
>>>>> #include "xe_hw_fence.h"
>>>>> #include "xe_pci.h"
>>>>> @@ -92,6 +94,9 @@ static int __init xe_init(void)
>>>>> {
>>>>>         int err, i;
>>>>>
>>>>> +        if (drm_firmware_drivers_only())
>>>>> +                return -ENODEV;
>>>>> +
>>>> Hm... But what if xe is to be used only for compute or render? 
>>>> Shouldn't
>>>> we handle this somewhere else?
>>> The question becomes, what does "nomodeset" really mean here?
>>
>> That function's name 'firmware drivers only' says it better than the 
>> option's name. We used 'nomodeset', because it was there already and 
>> had the correct semantics.
>
> agreed this should be on a module-level to maintain the behavior already
> used. If we were not maintaining that behavior, then we should probably
> not use "nomodeset" and choose something else :).
>
> Also we already have the other 2 as module params:  probe_display and
> disable_display, with driver still registering as a drm driver, but
> leaving the display part out.
>
> Thomas, are you going to send a v2 to use the init table?

Sure, in a bit.

Best regards
Thomas

>
> thanks
> Lucas De Marchi
>
>>
>>>
>>> See what i915 does in i915_module.c.
>>
>> i915 and the other drivers for PCI-based hardware don't load at all. 
>> Drivers for external displays (e.g., SPI, USB) ignore nomodeset, as 
>> these displays are not initialized by firmware.
>>
>> Best regards
>> Thomas
>>
>>>
>>> Cc: Sima.
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>
>>>> Taking a quick look, xe_display_probe() might be a good candidate?
>>>>
>>>> -- 
>>>> Gustavo Sousa
>>>>
>>>>>         for (i = 0; i < ARRAY_SIZE(init_funcs); i++) {
>>>>>                 err = init_funcs[i].init();
>>>>>                 if (err) {
>>>>> -- 
>>>>> 2.46.0
>>>>>
>>
>> -- 
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)