| Message ID | 20240821135750.102117-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe: Support 'nomodeset' kernel command-line option | expand |
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? 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 >
On Wed, Aug 21, 2024 at 03:56:59PM GMT, Thomas Zimmermann wrote: >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. I see some drivers like i915 and radeon using an additional 'modeset' parameter... probably to be able to avoid modeset for that specific driver while still allowing for others? > >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; nit: being the first, without an .exit may be equivalent, but probably better for parity with i915 to use a xe_check_modeset() and add it as the first one in the table. We'd need to check for exit != NULL, though. Anyway, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> thanks Lucas De Marchi >+ > for (i = 0; i < ARRAY_SIZE(init_funcs); i++) { > err = init_funcs[i].init(); > if (err) { >-- >2.46.0 >
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? See what i915 does in i915_module.c. 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 >>
Hi Am 21.08.24 um 16:16 schrieb Gustavo Sousa: > 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 semantics of 'nomodeset' is a bit vague in this case. The option is supposed to disable all of the driver in case there's a significant problem with booting up. Other drivers do that. And users should be able to specify 'nomodeset' and reliably get some display output; whether the problem is in KMS or rendering. I suggest to stick with that behavior, even if that disables other functionality as well. Up to you. Best regards Thomas > > 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 >>
Hi Am 21.08.24 um 16:24 schrieb Lucas De Marchi: > On Wed, Aug 21, 2024 at 03:56:59PM GMT, Thomas Zimmermann wrote: >> 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. > > I see some drivers like i915 and radeon using an additional 'modeset' > parameter... probably to be able to avoid modeset for that specific > driver while still allowing for others? These old per-module parameters are deprecated. We decided to keep them around as there's external blogs and documentation that refer to them. > >> >> 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; > > nit: being the first, without an .exit may be equivalent, but probably > better for parity with i915 to use a xe_check_modeset() and add it as > the first one in the table. We'd need to check for exit != NULL, though. OK, sure. That's also how i915 works. Best regards Thomas > > Anyway, > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > thanks > Lucas De Marchi > >> + >> for (i = 0; i < ARRAY_SIZE(init_funcs); i++) { >> err = init_funcs[i].init(); >> if (err) { >> -- >> 2.46.0 >>
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. > > 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 >>>
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? 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) >
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) >>
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; + for (i = 0; i < ARRAY_SIZE(init_funcs); i++) { err = init_funcs[i].init(); if (err) {
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(+)