* [PATCH v1] drm/modes: Skip invalid cmdline mode
@ 2019-07-09 14:51 Dmitry Osipenko
2019-07-10 10:00 ` Ville Syrjälä
2019-07-10 10:12 ` Maxime Ripard
0 siblings, 2 replies; 21+ messages in thread
From: Dmitry Osipenko @ 2019-07-09 14:51 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, Daniel Vetter, David Airlie
Cc: dri-devel, linux-tegra, linux-kernel
The named mode could be invalid and then cmdline parser misses to validate
mode's dimensions, happily adding 0x0 mode as a valid mode. One case where
this happens is NVIDIA Tegra devices that are using downstream bootloader
which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra
DRM driver fails to probe because of the invalid mode.
Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line")
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/gpu/drm/drm_client_modeset.c | 3 ++-
drivers/gpu/drm/drm_modes.c | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index e95fceac8f8b..56d36779d213 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -180,7 +180,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
create_mode:
mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode);
- list_add(&mode->head, &connector->modes);
+ if (mode)
+ list_add(&mode->head, &connector->modes);
return mode;
}
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 910561d4f071..74a5739df506 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -158,6 +158,9 @@ struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay,
int interlace;
u64 tmp;
+ if (!hdisplay || !vdisplay)
+ return NULL;
+
/* allocate the drm_display_mode structure. If failure, we will
* return directly
*/
@@ -392,6 +395,9 @@ drm_gtf_mode_complex(struct drm_device *dev, int hdisplay, int vdisplay,
int hsync, hfront_porch, vodd_front_porch_lines;
unsigned int tmp1, tmp2;
+ if (!hdisplay || !vdisplay)
+ return NULL;
+
drm_mode = drm_mode_create(dev);
if (!drm_mode)
return NULL;
--
2.22.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-09 14:51 [PATCH v1] drm/modes: Skip invalid cmdline mode Dmitry Osipenko @ 2019-07-10 10:00 ` Ville Syrjälä 2019-07-10 14:03 ` Dmitry Osipenko 2019-07-10 10:12 ` Maxime Ripard 1 sibling, 1 reply; 21+ messages in thread From: Ville Syrjälä @ 2019-07-10 10:00 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Maxime Ripard, Sean Paul, Daniel Vetter, David Airlie, linux-tegra, linux-kernel, dri-devel On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: > The named mode could be invalid and then cmdline parser misses to validate > mode's dimensions, happily adding 0x0 mode as a valid mode. One case where > this happens is NVIDIA Tegra devices that are using downstream bootloader > which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra > DRM driver fails to probe because of the invalid mode. > > Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") Is that actually true? This problem has been in the code since forever AFAICS. > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/drm/drm_client_modeset.c | 3 ++- > drivers/gpu/drm/drm_modes.c | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index e95fceac8f8b..56d36779d213 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -180,7 +180,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) > > create_mode: > mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); > - list_add(&mode->head, &connector->modes); > + if (mode) > + list_add(&mode->head, &connector->modes); That's the same as what I did here https://patchwork.freedesktop.org/patch/309223/?series=61781&rev=1 But I'd have to rebase that so let's just go with your patch. > > return mode; > } > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 910561d4f071..74a5739df506 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -158,6 +158,9 @@ struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay, > int interlace; > u64 tmp; > > + if (!hdisplay || !vdisplay) > + return NULL; > + > /* allocate the drm_display_mode structure. If failure, we will > * return directly > */ > @@ -392,6 +395,9 @@ drm_gtf_mode_complex(struct drm_device *dev, int hdisplay, int vdisplay, > int hsync, hfront_porch, vodd_front_porch_lines; > unsigned int tmp1, tmp2; > > + if (!hdisplay || !vdisplay) > + return NULL; > + These lgtm Patch is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > drm_mode = drm_mode_create(dev); > if (!drm_mode) > return NULL; > -- > 2.22.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 10:00 ` Ville Syrjälä @ 2019-07-10 14:03 ` Dmitry Osipenko 0 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 14:03 UTC (permalink / raw) To: Ville Syrjälä Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Maxime Ripard, Sean Paul, Daniel Vetter, David Airlie, linux-tegra, linux-kernel, dri-devel 10.07.2019 13:00, Ville Syrjälä пишет: > On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: >> The named mode could be invalid and then cmdline parser misses to validate >> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where >> this happens is NVIDIA Tegra devices that are using downstream bootloader >> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra >> DRM driver fails to probe because of the invalid mode. >> >> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") > > Is that actually true? This problem has been in the code since forever AFAICS. Yes, it's a problem now because named mode is marked as specified and everything that do not match to a non-named mode is treated as named. >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/gpu/drm/drm_client_modeset.c | 3 ++- >> drivers/gpu/drm/drm_modes.c | 6 ++++++ >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >> index e95fceac8f8b..56d36779d213 100644 >> --- a/drivers/gpu/drm/drm_client_modeset.c >> +++ b/drivers/gpu/drm/drm_client_modeset.c >> @@ -180,7 +180,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) >> >> create_mode: >> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); >> - list_add(&mode->head, &connector->modes); >> + if (mode) >> + list_add(&mode->head, &connector->modes); > > That's the same as what I did here > https://patchwork.freedesktop.org/patch/309223/?series=61781&rev=1 > > But I'd have to rebase that so let's just go with your patch. > >> >> return mode; >> } >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >> index 910561d4f071..74a5739df506 100644 >> --- a/drivers/gpu/drm/drm_modes.c >> +++ b/drivers/gpu/drm/drm_modes.c >> @@ -158,6 +158,9 @@ struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay, >> int interlace; >> u64 tmp; >> >> + if (!hdisplay || !vdisplay) >> + return NULL; >> + >> /* allocate the drm_display_mode structure. If failure, we will >> * return directly >> */ >> @@ -392,6 +395,9 @@ drm_gtf_mode_complex(struct drm_device *dev, int hdisplay, int vdisplay, >> int hsync, hfront_porch, vodd_front_porch_lines; >> unsigned int tmp1, tmp2; >> >> + if (!hdisplay || !vdisplay) >> + return NULL; >> + > > These lgtm > > Patch is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks! ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-09 14:51 [PATCH v1] drm/modes: Skip invalid cmdline mode Dmitry Osipenko 2019-07-10 10:00 ` Ville Syrjälä @ 2019-07-10 10:12 ` Maxime Ripard 2019-07-10 12:42 ` Dmitry Osipenko 1 sibling, 1 reply; 21+ messages in thread From: Maxime Ripard @ 2019-07-10 10:12 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 716 bytes --] On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: > The named mode could be invalid and then cmdline parser misses to validate > mode's dimensions, happily adding 0x0 mode as a valid mode. One case where > this happens is NVIDIA Tegra devices that are using downstream bootloader > which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra > DRM driver fails to probe because of the invalid mode. > > Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> Applied to drm-misc-next-fixes Thanks for figuring this out! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 10:12 ` Maxime Ripard @ 2019-07-10 12:42 ` Dmitry Osipenko 2019-07-10 12:55 ` Maxime Ripard 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 12:42 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 13:12, Maxime Ripard пишет: > On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: >> The named mode could be invalid and then cmdline parser misses to validate >> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where >> this happens is NVIDIA Tegra devices that are using downstream bootloader >> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra >> DRM driver fails to probe because of the invalid mode. >> >> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > Applied to drm-misc-next-fixes > > Thanks for figuring this out! Thank you very much! So the driver now doesn't fail to probe because of the cmdline, but what else I noticed is that the framebuffer console is now rotated by 90° on a 800x1280 panel, while display in Xorg is vertical as it was before. Seems something else is still missing, reverting "drm/modes: Rewrite the command line parser" returns the framebuffer's console orientation into the original state. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 12:42 ` Dmitry Osipenko @ 2019-07-10 12:55 ` Maxime Ripard 2019-07-10 12:59 ` Dmitry Osipenko 0 siblings, 1 reply; 21+ messages in thread From: Maxime Ripard @ 2019-07-10 12:55 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1378 bytes --] On Wed, Jul 10, 2019 at 03:42:28PM +0300, Dmitry Osipenko wrote: > 10.07.2019 13:12, Maxime Ripard пишет: > > On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: > >> The named mode could be invalid and then cmdline parser misses to validate > >> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where > >> this happens is NVIDIA Tegra devices that are using downstream bootloader > >> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra > >> DRM driver fails to probe because of the invalid mode. > >> > >> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > > > Applied to drm-misc-next-fixes > > > > Thanks for figuring this out! > > Thank you very much! So the driver now doesn't fail to probe because > of the cmdline, but what else I noticed is that the framebuffer > console is now rotated by 90° on a 800x1280 panel, while display in > Xorg is vertical as it was before. Seems something else is still > missing, reverting "drm/modes: Rewrite the command line parser" > returns the framebuffer's console orientation into the original > state. What is the whole command line passed by the bootloader ? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 12:55 ` Maxime Ripard @ 2019-07-10 12:59 ` Dmitry Osipenko 2019-07-10 13:06 ` Maxime Ripard 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 12:59 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 15:55, Maxime Ripard пишет: > On Wed, Jul 10, 2019 at 03:42:28PM +0300, Dmitry Osipenko wrote: >> 10.07.2019 13:12, Maxime Ripard пишет: >>> On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: >>>> The named mode could be invalid and then cmdline parser misses to validate >>>> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where >>>> this happens is NVIDIA Tegra devices that are using downstream bootloader >>>> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra >>>> DRM driver fails to probe because of the invalid mode. >>>> >>>> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> >>> Applied to drm-misc-next-fixes >>> >>> Thanks for figuring this out! >> >> Thank you very much! So the driver now doesn't fail to probe because >> of the cmdline, but what else I noticed is that the framebuffer >> console is now rotated by 90° on a 800x1280 panel, while display in >> Xorg is vertical as it was before. Seems something else is still >> missing, reverting "drm/modes: Rewrite the command line parser" >> returns the framebuffer's console orientation into the original >> state. > > What is the whole command line passed by the bootloader ? tegraid=30.1.3.0.0 mem=1022M@2048M android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 video=tegrafb no_console_suspend=1 console=none debug_uartport=hsport usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 board_info=f41:a00:1:44:2 root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt gpt_sector=61079551 androidboot.bootloader=4.23 androidboot.baseband=1231_0.18.0_0409 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 12:59 ` Dmitry Osipenko @ 2019-07-10 13:06 ` Maxime Ripard 2019-07-10 13:11 ` Dmitry Osipenko 0 siblings, 1 reply; 21+ messages in thread From: Maxime Ripard @ 2019-07-10 13:06 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2348 bytes --] On Wed, Jul 10, 2019 at 03:59:55PM +0300, Dmitry Osipenko wrote: > 10.07.2019 15:55, Maxime Ripard пишет: > > On Wed, Jul 10, 2019 at 03:42:28PM +0300, Dmitry Osipenko wrote: > >> 10.07.2019 13:12, Maxime Ripard пишет: > >>> On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: > >>>> The named mode could be invalid and then cmdline parser misses to validate > >>>> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where > >>>> this happens is NVIDIA Tegra devices that are using downstream bootloader > >>>> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra > >>>> DRM driver fails to probe because of the invalid mode. > >>>> > >>>> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") > >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >>> > >>> Applied to drm-misc-next-fixes > >>> > >>> Thanks for figuring this out! > >> > >> Thank you very much! So the driver now doesn't fail to probe because > >> of the cmdline, but what else I noticed is that the framebuffer > >> console is now rotated by 90° on a 800x1280 panel, while display in > >> Xorg is vertical as it was before. Seems something else is still > >> missing, reverting "drm/modes: Rewrite the command line parser" > >> returns the framebuffer's console orientation into the original > >> state. > > > > What is the whole command line passed by the bootloader ? > > tegraid=30.1.3.0.0 mem=1022M@2048M android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 video=tegrafb no_console_suspend=1 console=none > debug_uartport=hsport usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 board_info=f41:a00:1:44:2 > root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt gpt_sector=61079551 androidboot.bootloader=4.23 androidboot.baseband=1231_0.18.0_0409 Thanks. It still doesn't really make sense to me why that video=tegrafb should be considered valid. However, I don't see anything rotation related in the commit you list. Are you sure it's really the offending one and not another one? Also, do you have the option to recompile a kernel so that we can add some debug? maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 13:06 ` Maxime Ripard @ 2019-07-10 13:11 ` Dmitry Osipenko 2019-07-10 13:29 ` Dmitry Osipenko 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 13:11 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 16:06, Maxime Ripard пишет: > On Wed, Jul 10, 2019 at 03:59:55PM +0300, Dmitry Osipenko wrote: >> 10.07.2019 15:55, Maxime Ripard пишет: >>> On Wed, Jul 10, 2019 at 03:42:28PM +0300, Dmitry Osipenko wrote: >>>> 10.07.2019 13:12, Maxime Ripard пишет: >>>>> On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: >>>>>> The named mode could be invalid and then cmdline parser misses to validate >>>>>> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where >>>>>> this happens is NVIDIA Tegra devices that are using downstream bootloader >>>>>> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra >>>>>> DRM driver fails to probe because of the invalid mode. >>>>>> >>>>>> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") >>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>> >>>>> Applied to drm-misc-next-fixes >>>>> >>>>> Thanks for figuring this out! >>>> >>>> Thank you very much! So the driver now doesn't fail to probe because >>>> of the cmdline, but what else I noticed is that the framebuffer >>>> console is now rotated by 90° on a 800x1280 panel, while display in >>>> Xorg is vertical as it was before. Seems something else is still >>>> missing, reverting "drm/modes: Rewrite the command line parser" >>>> returns the framebuffer's console orientation into the original >>>> state. >>> >>> What is the whole command line passed by the bootloader ? >> >> tegraid=30.1.3.0.0 mem=1022M@2048M android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 video=tegrafb no_console_suspend=1 console=none >> debug_uartport=hsport usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 board_info=f41:a00:1:44:2 >> root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt gpt_sector=61079551 androidboot.bootloader=4.23 androidboot.baseband=1231_0.18.0_0409 > > Thanks. > > It still doesn't really make sense to me why that video=tegrafb should > be considered valid. > > However, I don't see anything rotation related in the commit you > list. Are you sure it's really the offending one and not another one? Yes. > Also, do you have the option to recompile a kernel so that we can add > some debug? Recompiling kernel is not a problem at all. Before "drm/modes: Rewrite the command line parser": [ 1.256454] [drm] parse error at position 6 in video mode 'tegrafb' [ 1.256654] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 1.256664] [drm] No driver support for vblank timestamp query. [ 1.256703] [drm:drm_client_modeset_probe] [ 1.256719] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] [ 1.256731] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] status updated from unknown to connected [ 1.256828] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : [ 1.256842] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa [ 1.256849] [drm:drm_client_modeset_probe] connector 95 enabled? yes [ 1.256859] [drm:drm_client_modeset_probe] Not using firmware configuration [ 1.256867] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 [ 1.256874] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 [ 1.256880] [drm:drm_client_modeset_probe] found mode 800x1280 [ 1.256886] [drm:drm_client_modeset_probe] picking CRTCs for 4096x4096 config [ 1.256896] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) [ 1.279069] [drm:tegra_crtc_atomic_enable] rate: 408000000, div: 10 [ 1.279077] [drm:tegra_crtc_atomic_enable] pclk: 0 [ 1.296744] [drm:drm_fb_helper_hotplug_event.part.0] [ 1.296760] [drm:drm_client_modeset_probe] [ 1.296792] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] [ 1.296987] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : [ 1.297010] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa [ 1.297022] [drm:drm_client_modeset_probe] connector 95 enabled? yes [ 1.297040] [drm:drm_client_modeset_probe] Not using firmware configuration [ 1.297054] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 [ 1.297065] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 [ 1.297073] [drm:drm_client_modeset_probe] found mode 800x1280 [ 1.297083] [drm:drm_client_modeset_probe] picking CRTCs for 800x1280 config [ 1.297102] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) After: [ 1.225000] [drm:drm_connector_init] cmdline mode for connector LVDS-1 tegrafb 0x0@60Hz [ 1.225143] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 1.225154] [drm] No driver support for vblank timestamp query. [ 1.225182] [drm:drm_client_modeset_probe] [ 1.225195] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] [ 1.225203] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] status updated from unknown to connected [ 1.225283] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : [ 1.225294] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa [ 1.225299] [drm:drm_client_modeset_probe] connector 95 enabled? yes [ 1.225307] [drm:drm_client_modeset_probe] Not using firmware configuration [ 1.225314] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 [ 1.225319] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 [ 1.225323] [drm:drm_client_modeset_probe] found mode 800x1280 [ 1.225328] [drm:drm_client_modeset_probe] picking CRTCs for 4096x4096 config [ 1.225336] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) [ 1.249051] [drm:tegra_crtc_atomic_enable] rate: 408000000, div: 10 [ 1.249058] [drm:tegra_crtc_atomic_enable] pclk: 0 [ 1.266748] [drm:drm_fb_helper_hotplug_event.part.0] [ 1.266768] [drm:drm_client_modeset_probe] [ 1.266805] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] [ 1.267045] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : [ 1.267074] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa [ 1.267091] [drm:drm_client_modeset_probe] connector 95 enabled? yes [ 1.267113] [drm:drm_client_modeset_probe] Not using firmware configuration [ 1.267129] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 [ 1.267143] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 [ 1.267155] [drm:drm_client_modeset_probe] found mode 800x1280 [ 1.267168] [drm:drm_client_modeset_probe] picking CRTCs for 800x1280 config [ 1.267191] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 13:11 ` Dmitry Osipenko @ 2019-07-10 13:29 ` Dmitry Osipenko 2019-07-10 13:36 ` Dmitry Osipenko 2019-07-10 14:05 ` Maxime Ripard 0 siblings, 2 replies; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 13:29 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 16:11, Dmitry Osipenko пишет: > 10.07.2019 16:06, Maxime Ripard пишет: >> On Wed, Jul 10, 2019 at 03:59:55PM +0300, Dmitry Osipenko wrote: >>> 10.07.2019 15:55, Maxime Ripard пишет: >>>> On Wed, Jul 10, 2019 at 03:42:28PM +0300, Dmitry Osipenko wrote: >>>>> 10.07.2019 13:12, Maxime Ripard пишет: >>>>>> On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: >>>>>>> The named mode could be invalid and then cmdline parser misses to validate >>>>>>> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where >>>>>>> this happens is NVIDIA Tegra devices that are using downstream bootloader >>>>>>> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra >>>>>>> DRM driver fails to probe because of the invalid mode. >>>>>>> >>>>>>> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") >>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>>> >>>>>> Applied to drm-misc-next-fixes >>>>>> >>>>>> Thanks for figuring this out! >>>>> >>>>> Thank you very much! So the driver now doesn't fail to probe because >>>>> of the cmdline, but what else I noticed is that the framebuffer >>>>> console is now rotated by 90° on a 800x1280 panel, while display in >>>>> Xorg is vertical as it was before. Seems something else is still >>>>> missing, reverting "drm/modes: Rewrite the command line parser" >>>>> returns the framebuffer's console orientation into the original >>>>> state. >>>> >>>> What is the whole command line passed by the bootloader ? >>> >>> tegraid=30.1.3.0.0 mem=1022M@2048M android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 video=tegrafb no_console_suspend=1 console=none >>> debug_uartport=hsport usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 board_info=f41:a00:1:44:2 >>> root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt gpt_sector=61079551 androidboot.bootloader=4.23 androidboot.baseband=1231_0.18.0_0409 >> >> Thanks. >> >> It still doesn't really make sense to me why that video=tegrafb should >> be considered valid. >> >> However, I don't see anything rotation related in the commit you >> list. Are you sure it's really the offending one and not another one? > > Yes. > >> Also, do you have the option to recompile a kernel so that we can add >> some debug? > > Recompiling kernel is not a problem at all. > > Before "drm/modes: Rewrite the command line parser": > > [ 1.256454] [drm] parse error at position 6 in video mode 'tegrafb' > [ 1.256654] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [ 1.256664] [drm] No driver support for vblank timestamp query. > [ 1.256703] [drm:drm_client_modeset_probe] > [ 1.256719] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] > [ 1.256731] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] status updated from unknown to connected > [ 1.256828] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : > [ 1.256842] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa > [ 1.256849] [drm:drm_client_modeset_probe] connector 95 enabled? yes > [ 1.256859] [drm:drm_client_modeset_probe] Not using firmware configuration > [ 1.256867] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 > [ 1.256874] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 > [ 1.256880] [drm:drm_client_modeset_probe] found mode 800x1280 > [ 1.256886] [drm:drm_client_modeset_probe] picking CRTCs for 4096x4096 config > [ 1.256896] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) > [ 1.279069] [drm:tegra_crtc_atomic_enable] rate: 408000000, div: 10 > [ 1.279077] [drm:tegra_crtc_atomic_enable] pclk: 0 > [ 1.296744] [drm:drm_fb_helper_hotplug_event.part.0] > [ 1.296760] [drm:drm_client_modeset_probe] > [ 1.296792] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] > [ 1.296987] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : > [ 1.297010] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa > [ 1.297022] [drm:drm_client_modeset_probe] connector 95 enabled? yes > [ 1.297040] [drm:drm_client_modeset_probe] Not using firmware configuration > [ 1.297054] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 > [ 1.297065] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 > [ 1.297073] [drm:drm_client_modeset_probe] found mode 800x1280 > [ 1.297083] [drm:drm_client_modeset_probe] picking CRTCs for 800x1280 config > [ 1.297102] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) > > After: > > [ 1.225000] [drm:drm_connector_init] cmdline mode for connector LVDS-1 tegrafb 0x0@60Hz > [ 1.225143] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [ 1.225154] [drm] No driver support for vblank timestamp query. > [ 1.225182] [drm:drm_client_modeset_probe] > [ 1.225195] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] > [ 1.225203] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] status updated from unknown to connected > [ 1.225283] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : > [ 1.225294] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa > [ 1.225299] [drm:drm_client_modeset_probe] connector 95 enabled? yes > [ 1.225307] [drm:drm_client_modeset_probe] Not using firmware configuration > [ 1.225314] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 > [ 1.225319] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 > [ 1.225323] [drm:drm_client_modeset_probe] found mode 800x1280 > [ 1.225328] [drm:drm_client_modeset_probe] picking CRTCs for 4096x4096 config > [ 1.225336] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) > [ 1.249051] [drm:tegra_crtc_atomic_enable] rate: 408000000, div: 10 > [ 1.249058] [drm:tegra_crtc_atomic_enable] pclk: 0 > [ 1.266748] [drm:drm_fb_helper_hotplug_event.part.0] > [ 1.266768] [drm:drm_client_modeset_probe] > [ 1.266805] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] > [ 1.267045] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : > [ 1.267074] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa > [ 1.267091] [drm:drm_client_modeset_probe] connector 95 enabled? yes > [ 1.267113] [drm:drm_client_modeset_probe] Not using firmware configuration > [ 1.267129] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 > [ 1.267143] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 > [ 1.267155] [drm:drm_client_modeset_probe] found mode 800x1280 > [ 1.267168] [drm:drm_client_modeset_probe] picking CRTCs for 800x1280 config > [ 1.267191] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) > This works: diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 56d36779d213..e5a2f9c8f404 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); if (mode) list_add(&mode->head, &connector->modes); + else + cmdline_mode->specified = false; return mode; } I'll make a proper patch later today if there are no objections. ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 13:29 ` Dmitry Osipenko @ 2019-07-10 13:36 ` Dmitry Osipenko 2019-07-10 14:05 ` Maxime Ripard 1 sibling, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 13:36 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 16:29, Dmitry Osipenko пишет: > 10.07.2019 16:11, Dmitry Osipenko пишет: >> 10.07.2019 16:06, Maxime Ripard пишет: >>> On Wed, Jul 10, 2019 at 03:59:55PM +0300, Dmitry Osipenko wrote: >>>> 10.07.2019 15:55, Maxime Ripard пишет: >>>>> On Wed, Jul 10, 2019 at 03:42:28PM +0300, Dmitry Osipenko wrote: >>>>>> 10.07.2019 13:12, Maxime Ripard пишет: >>>>>>> On Tue, Jul 09, 2019 at 05:51:51PM +0300, Dmitry Osipenko wrote: >>>>>>>> The named mode could be invalid and then cmdline parser misses to validate >>>>>>>> mode's dimensions, happily adding 0x0 mode as a valid mode. One case where >>>>>>>> this happens is NVIDIA Tegra devices that are using downstream bootloader >>>>>>>> which adds "video=tegrafb" to the kernel's cmdline and thus upstream Tegra >>>>>>>> DRM driver fails to probe because of the invalid mode. >>>>>>>> >>>>>>>> Fixes: 3aeeb13d8996 ("drm/modes: Support modes names on the command line") >>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>>>> >>>>>>> Applied to drm-misc-next-fixes >>>>>>> >>>>>>> Thanks for figuring this out! >>>>>> >>>>>> Thank you very much! So the driver now doesn't fail to probe because >>>>>> of the cmdline, but what else I noticed is that the framebuffer >>>>>> console is now rotated by 90° on a 800x1280 panel, while display in >>>>>> Xorg is vertical as it was before. Seems something else is still >>>>>> missing, reverting "drm/modes: Rewrite the command line parser" >>>>>> returns the framebuffer's console orientation into the original >>>>>> state. >>>>> >>>>> What is the whole command line passed by the bootloader ? >>>> >>>> tegraid=30.1.3.0.0 mem=1022M@2048M android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 video=tegrafb no_console_suspend=1 console=none >>>> debug_uartport=hsport usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 board_info=f41:a00:1:44:2 >>>> root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt gpt_sector=61079551 androidboot.bootloader=4.23 androidboot.baseband=1231_0.18.0_0409 >>> >>> Thanks. >>> >>> It still doesn't really make sense to me why that video=tegrafb should >>> be considered valid. >>> >>> However, I don't see anything rotation related in the commit you >>> list. Are you sure it's really the offending one and not another one? >> >> Yes. >> >>> Also, do you have the option to recompile a kernel so that we can add >>> some debug? >> >> Recompiling kernel is not a problem at all. >> >> Before "drm/modes: Rewrite the command line parser": >> >> [ 1.256454] [drm] parse error at position 6 in video mode 'tegrafb' >> [ 1.256654] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >> [ 1.256664] [drm] No driver support for vblank timestamp query. >> [ 1.256703] [drm:drm_client_modeset_probe] >> [ 1.256719] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] >> [ 1.256731] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] status updated from unknown to connected >> [ 1.256828] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : >> [ 1.256842] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa >> [ 1.256849] [drm:drm_client_modeset_probe] connector 95 enabled? yes >> [ 1.256859] [drm:drm_client_modeset_probe] Not using firmware configuration >> [ 1.256867] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 >> [ 1.256874] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 >> [ 1.256880] [drm:drm_client_modeset_probe] found mode 800x1280 >> [ 1.256886] [drm:drm_client_modeset_probe] picking CRTCs for 4096x4096 config >> [ 1.256896] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) >> [ 1.279069] [drm:tegra_crtc_atomic_enable] rate: 408000000, div: 10 >> [ 1.279077] [drm:tegra_crtc_atomic_enable] pclk: 0 >> [ 1.296744] [drm:drm_fb_helper_hotplug_event.part.0] >> [ 1.296760] [drm:drm_client_modeset_probe] >> [ 1.296792] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] >> [ 1.296987] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : >> [ 1.297010] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa >> [ 1.297022] [drm:drm_client_modeset_probe] connector 95 enabled? yes >> [ 1.297040] [drm:drm_client_modeset_probe] Not using firmware configuration >> [ 1.297054] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 >> [ 1.297065] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 >> [ 1.297073] [drm:drm_client_modeset_probe] found mode 800x1280 >> [ 1.297083] [drm:drm_client_modeset_probe] picking CRTCs for 800x1280 config >> [ 1.297102] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) >> >> After: >> >> [ 1.225000] [drm:drm_connector_init] cmdline mode for connector LVDS-1 tegrafb 0x0@60Hz >> [ 1.225143] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >> [ 1.225154] [drm] No driver support for vblank timestamp query. >> [ 1.225182] [drm:drm_client_modeset_probe] >> [ 1.225195] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] >> [ 1.225203] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] status updated from unknown to connected >> [ 1.225283] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : >> [ 1.225294] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa >> [ 1.225299] [drm:drm_client_modeset_probe] connector 95 enabled? yes >> [ 1.225307] [drm:drm_client_modeset_probe] Not using firmware configuration >> [ 1.225314] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 >> [ 1.225319] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 >> [ 1.225323] [drm:drm_client_modeset_probe] found mode 800x1280 >> [ 1.225328] [drm:drm_client_modeset_probe] picking CRTCs for 4096x4096 config >> [ 1.225336] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) >> [ 1.249051] [drm:tegra_crtc_atomic_enable] rate: 408000000, div: 10 >> [ 1.249058] [drm:tegra_crtc_atomic_enable] pclk: 0 >> [ 1.266748] [drm:drm_fb_helper_hotplug_event.part.0] >> [ 1.266768] [drm:drm_client_modeset_probe] >> [ 1.266805] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] >> [ 1.267045] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:95:LVDS-1] probed modes : >> [ 1.267074] [drm:drm_mode_debug_printmodeline] Modeline "800x1280": 60 66770 800 849 882 899 1280 1281 1288 1303 0x48 0xa >> [ 1.267091] [drm:drm_client_modeset_probe] connector 95 enabled? yes >> [ 1.267113] [drm:drm_client_modeset_probe] Not using firmware configuration >> [ 1.267129] [drm:drm_client_modeset_probe] looking for cmdline mode on connector 95 >> [ 1.267143] [drm:drm_client_modeset_probe] looking for preferred mode on connector 95 0 >> [ 1.267155] [drm:drm_client_modeset_probe] found mode 800x1280 >> [ 1.267168] [drm:drm_client_modeset_probe] picking CRTCs for 800x1280 config >> [ 1.267191] [drm:drm_client_modeset_probe] desired mode 800x1280 set on crtc 94 (0,0) >> > > This works: > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index 56d36779d213..e5a2f9c8f404 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) > mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); > if (mode) > list_add(&mode->head, &connector->modes); > + else > + cmdline_mode->specified = false; > > return mode; > } > > I'll make a proper patch later today if there are no objections. > Pleas note that drm_client_rotation() takes into account whether cmdline->specified. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 13:29 ` Dmitry Osipenko 2019-07-10 13:36 ` Dmitry Osipenko @ 2019-07-10 14:05 ` Maxime Ripard 2019-07-10 15:05 ` Dmitry Osipenko 1 sibling, 1 reply; 21+ messages in thread From: Maxime Ripard @ 2019-07-10 14:05 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1651 bytes --] On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: > This works: > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index 56d36779d213..e5a2f9c8f404 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) > mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); > if (mode) > list_add(&mode->head, &connector->modes); > + else > + cmdline_mode->specified = false; Hmmm, it's not clear to me why that wouldn't be the case. If we come back to the beginning of that function, we retrieve the cmdline_mode buffer from the connector pointer, that will probably have been parsed a first time using drm_mode_create_from_cmdline_mode in drm_helper_probe_add_cmdline_mode. Now, I'm guessing that the issue is that in drm_mode_parse_command_line_for_connector, if we have a named mode, we just copy the mode over and set mode->specified. And we then move over to do other checks, and that's probably what fails and returns, but our drm_cmdline_mode will have been modified. I'm not entirely sure how to deal with that though. I guess we could allocate a drm_cmdline_mode structure on the stack, fill that, and if successful copy over its content to the one in drm_connector. That would allow us to only change the content on success, which is what I would expect from such a function? How does that sound? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 14:05 ` Maxime Ripard @ 2019-07-10 15:05 ` Dmitry Osipenko 2019-07-10 15:45 ` Dmitry Osipenko 2019-07-11 9:03 ` Maxime Ripard 0 siblings, 2 replies; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 15:05 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 17:05, Maxime Ripard пишет: > On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: >> This works: >> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >> index 56d36779d213..e5a2f9c8f404 100644 >> --- a/drivers/gpu/drm/drm_client_modeset.c >> +++ b/drivers/gpu/drm/drm_client_modeset.c >> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) >> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); >> if (mode) >> list_add(&mode->head, &connector->modes); >> + else >> + cmdline_mode->specified = false; > > Hmmm, it's not clear to me why that wouldn't be the case. > > If we come back to the beginning of that function, we retrieve the > cmdline_mode buffer from the connector pointer, that will probably > have been parsed a first time using drm_mode_create_from_cmdline_mode > in drm_helper_probe_add_cmdline_mode. > > Now, I'm guessing that the issue is that in > drm_mode_parse_command_line_for_connector, if we have a named mode, we > just copy the mode over and set mode->specified. > > And we then move over to do other checks, and that's probably what > fails and returns, but our drm_cmdline_mode will have been modified. > > I'm not entirely sure how to deal with that though. > > I guess we could allocate a drm_cmdline_mode structure on the stack, > fill that, and if successful copy over its content to the one in > drm_connector. That would allow us to only change the content on > success, which is what I would expect from such a function? > > How does that sound? I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only for the "cmdline" mode and drm_client_rotation() is the only place in DRM code that cares about whether mode is from cmdline, hence looks like it will be more correct to do the following: diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 56d36779d213..e5b3be9ed689 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -825,6 +825,7 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation) { struct drm_connector *connector = modeset->connectors[0]; struct drm_plane *plane = modeset->crtc->primary; + struct drm_display_mode *mode = modeset->mode; struct drm_cmdline_mode *cmdline; u64 valid_mask = 0; unsigned int i; @@ -859,7 +860,7 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation) * simple XOR between the two handle the addition nicely. */ cmdline = &connector->cmdline_mode; - if (cmdline->specified) { + if (mode->flags & DRM_MODE_TYPE_USERDEF) { unsigned int cmdline_rest, panel_rest; unsigned int cmdline_rot, panel_rot; unsigned int sum_rot, sum_rest; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 15:05 ` Dmitry Osipenko @ 2019-07-10 15:45 ` Dmitry Osipenko 2019-07-10 16:03 ` Dmitry Osipenko 2019-07-11 9:03 ` Maxime Ripard 1 sibling, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 15:45 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 18:05, Dmitry Osipenko пишет: > 10.07.2019 17:05, Maxime Ripard пишет: >> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: >>> This works: >>> >>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >>> index 56d36779d213..e5a2f9c8f404 100644 >>> --- a/drivers/gpu/drm/drm_client_modeset.c >>> +++ b/drivers/gpu/drm/drm_client_modeset.c >>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) >>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); >>> if (mode) >>> list_add(&mode->head, &connector->modes); >>> + else >>> + cmdline_mode->specified = false; >> >> Hmmm, it's not clear to me why that wouldn't be the case. >> >> If we come back to the beginning of that function, we retrieve the >> cmdline_mode buffer from the connector pointer, that will probably >> have been parsed a first time using drm_mode_create_from_cmdline_mode >> in drm_helper_probe_add_cmdline_mode. >> >> Now, I'm guessing that the issue is that in >> drm_mode_parse_command_line_for_connector, if we have a named mode, we >> just copy the mode over and set mode->specified. >> >> And we then move over to do other checks, and that's probably what >> fails and returns, but our drm_cmdline_mode will have been modified. >> >> I'm not entirely sure how to deal with that though. >> >> I guess we could allocate a drm_cmdline_mode structure on the stack, >> fill that, and if successful copy over its content to the one in >> drm_connector. That would allow us to only change the content on >> success, which is what I would expect from such a function? >> >> How does that sound? > > I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only > for the "cmdline" mode and drm_client_rotation() is the only place in > DRM code that cares about whether mode is from cmdline, hence looks like > it will be more correct to do the following: > > diff --git a/drivers/gpu/drm/drm_client_modeset.c > b/drivers/gpu/drm/drm_client_modeset.c > index 56d36779d213..e5b3be9ed689 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -825,6 +825,7 @@ bool drm_client_rotation(struct drm_mode_set > *modeset, unsigned int *rotation) > { > struct drm_connector *connector = modeset->connectors[0]; > struct drm_plane *plane = modeset->crtc->primary; > + struct drm_display_mode *mode = modeset->mode; > struct drm_cmdline_mode *cmdline; > u64 valid_mask = 0; > unsigned int i; > @@ -859,7 +860,7 @@ bool drm_client_rotation(struct drm_mode_set > *modeset, unsigned int *rotation) > * simple XOR between the two handle the addition nicely. > */ > cmdline = &connector->cmdline_mode; > - if (cmdline->specified) { > + if (mode->flags & DRM_MODE_TYPE_USERDEF) { > unsigned int cmdline_rest, panel_rest; > unsigned int cmdline_rot, panel_rot; > unsigned int sum_rot, sum_rest; > Although, then rotation won't be applied to the named mode in that case. Seems the fix could be even simpler: @@ -859,7 +859,7 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation) * simple XOR between the two handle the addition nicely. */ cmdline = &connector->cmdline_mode; - if (cmdline->specified) { + if (cmdline->specified && cmdline->rotation_reflection) { unsigned int cmdline_rest, panel_rest; unsigned int cmdline_rot, panel_rot; unsigned int sum_rot, sum_rest; And looks like there is another problem here.. the cmdline's rotation overrides *all* modes while the doc/fb/modedb.rst claims that rotation is applied only to the *initial* mode. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 15:45 ` Dmitry Osipenko @ 2019-07-10 16:03 ` Dmitry Osipenko 0 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-10 16:03 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 10.07.2019 18:45, Dmitry Osipenko пишет: > 10.07.2019 18:05, Dmitry Osipenko пишет: >> 10.07.2019 17:05, Maxime Ripard пишет: >>> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: >>>> This works: >>>> >>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >>>> index 56d36779d213..e5a2f9c8f404 100644 >>>> --- a/drivers/gpu/drm/drm_client_modeset.c >>>> +++ b/drivers/gpu/drm/drm_client_modeset.c >>>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) >>>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); >>>> if (mode) >>>> list_add(&mode->head, &connector->modes); >>>> + else >>>> + cmdline_mode->specified = false; >>> >>> Hmmm, it's not clear to me why that wouldn't be the case. >>> >>> If we come back to the beginning of that function, we retrieve the >>> cmdline_mode buffer from the connector pointer, that will probably >>> have been parsed a first time using drm_mode_create_from_cmdline_mode >>> in drm_helper_probe_add_cmdline_mode. >>> >>> Now, I'm guessing that the issue is that in >>> drm_mode_parse_command_line_for_connector, if we have a named mode, we >>> just copy the mode over and set mode->specified. >>> >>> And we then move over to do other checks, and that's probably what >>> fails and returns, but our drm_cmdline_mode will have been modified. >>> >>> I'm not entirely sure how to deal with that though. >>> >>> I guess we could allocate a drm_cmdline_mode structure on the stack, >>> fill that, and if successful copy over its content to the one in >>> drm_connector. That would allow us to only change the content on >>> success, which is what I would expect from such a function? >>> >>> How does that sound? >> >> I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only >> for the "cmdline" mode and drm_client_rotation() is the only place in >> DRM code that cares about whether mode is from cmdline, hence looks like >> it will be more correct to do the following: >> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c >> b/drivers/gpu/drm/drm_client_modeset.c >> index 56d36779d213..e5b3be9ed689 100644 >> --- a/drivers/gpu/drm/drm_client_modeset.c >> +++ b/drivers/gpu/drm/drm_client_modeset.c >> @@ -825,6 +825,7 @@ bool drm_client_rotation(struct drm_mode_set >> *modeset, unsigned int *rotation) >> { >> struct drm_connector *connector = modeset->connectors[0]; >> struct drm_plane *plane = modeset->crtc->primary; >> + struct drm_display_mode *mode = modeset->mode; >> struct drm_cmdline_mode *cmdline; >> u64 valid_mask = 0; >> unsigned int i; >> @@ -859,7 +860,7 @@ bool drm_client_rotation(struct drm_mode_set >> *modeset, unsigned int *rotation) >> * simple XOR between the two handle the addition nicely. >> */ >> cmdline = &connector->cmdline_mode; >> - if (cmdline->specified) { >> + if (mode->flags & DRM_MODE_TYPE_USERDEF) { >> unsigned int cmdline_rest, panel_rest; >> unsigned int cmdline_rot, panel_rot; >> unsigned int sum_rot, sum_rest; >> > > Although, then rotation won't be applied to the named mode in that case. > > Seems the fix could be even simpler: > > @@ -859,7 +859,7 @@ bool drm_client_rotation(struct drm_mode_set > *modeset, unsigned int *rotation) > * simple XOR between the two handle the addition nicely. > */ > cmdline = &connector->cmdline_mode; > - if (cmdline->specified) { > + if (cmdline->specified && cmdline->rotation_reflection) { > unsigned int cmdline_rest, panel_rest; > unsigned int cmdline_rot, panel_rot; > unsigned int sum_rot, sum_rest; > > And looks like there is another problem here.. the cmdline's rotation > overrides *all* modes while the doc/fb/modedb.rst claims that rotation > is applied only to the *initial* mode. > Actually, I was wrong here. This rotation is getting applied only to the framebuffer's console, so looks fine. I guess it's also okay to just keep applying cmdline's rotation even if the mode is invalid. Please let me know if you have any objections. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-10 15:05 ` Dmitry Osipenko 2019-07-10 15:45 ` Dmitry Osipenko @ 2019-07-11 9:03 ` Maxime Ripard 2019-07-11 15:55 ` Dmitry Osipenko 1 sibling, 1 reply; 21+ messages in thread From: Maxime Ripard @ 2019-07-11 9:03 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2628 bytes --] On Wed, Jul 10, 2019 at 06:05:18PM +0300, Dmitry Osipenko wrote: > 10.07.2019 17:05, Maxime Ripard пишет: > > On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: > >> This works: > >> > >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > >> index 56d36779d213..e5a2f9c8f404 100644 > >> --- a/drivers/gpu/drm/drm_client_modeset.c > >> +++ b/drivers/gpu/drm/drm_client_modeset.c > >> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) > >> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); > >> if (mode) > >> list_add(&mode->head, &connector->modes); > >> + else > >> + cmdline_mode->specified = false; > > > > Hmmm, it's not clear to me why that wouldn't be the case. > > > > If we come back to the beginning of that function, we retrieve the > > cmdline_mode buffer from the connector pointer, that will probably > > have been parsed a first time using drm_mode_create_from_cmdline_mode > > in drm_helper_probe_add_cmdline_mode. > > > > Now, I'm guessing that the issue is that in > > drm_mode_parse_command_line_for_connector, if we have a named mode, we > > just copy the mode over and set mode->specified. > > > > And we then move over to do other checks, and that's probably what > > fails and returns, but our drm_cmdline_mode will have been modified. > > > > I'm not entirely sure how to deal with that though. > > > > I guess we could allocate a drm_cmdline_mode structure on the stack, > > fill that, and if successful copy over its content to the one in > > drm_connector. That would allow us to only change the content on > > success, which is what I would expect from such a function? > > > > How does that sound? > > I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only > for the "cmdline" mode and drm_client_rotation() is the only place in > DRM code that cares about whether mode is from cmdline, hence looks like > it will be more correct to do the following: I'm still under the impression that we're dealing with workarounds of a more central issue, which is that we shouldn't return a partially modified drm_cmdline_mode. You said it yourself, the breakage is in the commit changing the command line parsing logic, while you're fixing here some code that was introduced later on. Can you try the followintg patch? http://code.bulix.org/8cwk4c-794565?raw Thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-11 9:03 ` Maxime Ripard @ 2019-07-11 15:55 ` Dmitry Osipenko 2019-07-12 8:10 ` Maxime Ripard 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-11 15:55 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 11.07.2019 12:03, Maxime Ripard пишет: > On Wed, Jul 10, 2019 at 06:05:18PM +0300, Dmitry Osipenko wrote: >> 10.07.2019 17:05, Maxime Ripard пишет: >>> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: >>>> This works: >>>> >>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >>>> index 56d36779d213..e5a2f9c8f404 100644 >>>> --- a/drivers/gpu/drm/drm_client_modeset.c >>>> +++ b/drivers/gpu/drm/drm_client_modeset.c >>>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) >>>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); >>>> if (mode) >>>> list_add(&mode->head, &connector->modes); >>>> + else >>>> + cmdline_mode->specified = false; >>> >>> Hmmm, it's not clear to me why that wouldn't be the case. >>> >>> If we come back to the beginning of that function, we retrieve the >>> cmdline_mode buffer from the connector pointer, that will probably >>> have been parsed a first time using drm_mode_create_from_cmdline_mode >>> in drm_helper_probe_add_cmdline_mode. >>> >>> Now, I'm guessing that the issue is that in >>> drm_mode_parse_command_line_for_connector, if we have a named mode, we >>> just copy the mode over and set mode->specified. >>> >>> And we then move over to do other checks, and that's probably what >>> fails and returns, but our drm_cmdline_mode will have been modified. >>> >>> I'm not entirely sure how to deal with that though. >>> >>> I guess we could allocate a drm_cmdline_mode structure on the stack, >>> fill that, and if successful copy over its content to the one in >>> drm_connector. That would allow us to only change the content on >>> success, which is what I would expect from such a function? >>> >>> How does that sound? >> >> I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only >> for the "cmdline" mode and drm_client_rotation() is the only place in >> DRM code that cares about whether mode is from cmdline, hence looks like >> it will be more correct to do the following: > > I'm still under the impression that we're dealing with workarounds of > a more central issue, which is that we shouldn't return a partially > modified drm_cmdline_mode. > > You said it yourself, the breakage is in the commit changing the > command line parsing logic, while you're fixing here some code that > was introduced later on. The problem stems from assumption that *any* named mode is valid. It looks to me that the ultimate solution would be to move the mode's name comparison into the [1], if that's possible. [1] drm_mode_parse_command_line_for_connector() > Can you try the followintg patch? > http://code.bulix.org/8cwk4c-794565?raw This doesn't help because the problem with the rotation_reflection is that it's 0 if "rotation" not present in the cmdline and then ilog2(0) returns -1. So the patch "drm/modes: Don't apply cmdline's rotation if it wasn't specified" should be correct in any case. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-11 15:55 ` Dmitry Osipenko @ 2019-07-12 8:10 ` Maxime Ripard 2019-07-12 8:30 ` Dmitry Osipenko 0 siblings, 1 reply; 21+ messages in thread From: Maxime Ripard @ 2019-07-12 8:10 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3570 bytes --] On Thu, Jul 11, 2019 at 06:55:03PM +0300, Dmitry Osipenko wrote: > 11.07.2019 12:03, Maxime Ripard пишет: > > On Wed, Jul 10, 2019 at 06:05:18PM +0300, Dmitry Osipenko wrote: > >> 10.07.2019 17:05, Maxime Ripard пишет: > >>> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: > >>>> This works: > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > >>>> index 56d36779d213..e5a2f9c8f404 100644 > >>>> --- a/drivers/gpu/drm/drm_client_modeset.c > >>>> +++ b/drivers/gpu/drm/drm_client_modeset.c > >>>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) > >>>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); > >>>> if (mode) > >>>> list_add(&mode->head, &connector->modes); > >>>> + else > >>>> + cmdline_mode->specified = false; > >>> > >>> Hmmm, it's not clear to me why that wouldn't be the case. > >>> > >>> If we come back to the beginning of that function, we retrieve the > >>> cmdline_mode buffer from the connector pointer, that will probably > >>> have been parsed a first time using drm_mode_create_from_cmdline_mode > >>> in drm_helper_probe_add_cmdline_mode. > >>> > >>> Now, I'm guessing that the issue is that in > >>> drm_mode_parse_command_line_for_connector, if we have a named mode, we > >>> just copy the mode over and set mode->specified. > >>> > >>> And we then move over to do other checks, and that's probably what > >>> fails and returns, but our drm_cmdline_mode will have been modified. > >>> > >>> I'm not entirely sure how to deal with that though. > >>> > >>> I guess we could allocate a drm_cmdline_mode structure on the stack, > >>> fill that, and if successful copy over its content to the one in > >>> drm_connector. That would allow us to only change the content on > >>> success, which is what I would expect from such a function? > >>> > >>> How does that sound? > >> > >> I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only > >> for the "cmdline" mode and drm_client_rotation() is the only place in > >> DRM code that cares about whether mode is from cmdline, hence looks like > >> it will be more correct to do the following: > > > > I'm still under the impression that we're dealing with workarounds of > > a more central issue, which is that we shouldn't return a partially > > modified drm_cmdline_mode. > > > > You said it yourself, the breakage is in the commit changing the > > command line parsing logic, while you're fixing here some code that > > was introduced later on. > > The problem stems from assumption that *any* named mode is valid. It > looks to me that the ultimate solution would be to move the mode's name > comparison into the [1], if that's possible. > > [1] drm_mode_parse_command_line_for_connector() Well, one could argue that video=tegrafb is invalid and should be rejected as well, but we haven't cleared that up. > > Can you try the followintg patch? > > http://code.bulix.org/8cwk4c-794565?raw > > This doesn't help because the problem with the rotation_reflection is > that it's 0 if "rotation" not present in the cmdline and then ilog2(0) > returns -1. So the patch "drm/modes: Don't apply cmdline's rotation if > it wasn't specified" should be correct in any case. So we would have the same issue with rotate=0 then? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-12 8:10 ` Maxime Ripard @ 2019-07-12 8:30 ` Dmitry Osipenko 2019-07-12 19:53 ` Maxime Ripard 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-12 8:30 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 12.07.2019 11:10, Maxime Ripard пишет: > On Thu, Jul 11, 2019 at 06:55:03PM +0300, Dmitry Osipenko wrote: >> 11.07.2019 12:03, Maxime Ripard пишет: >>> On Wed, Jul 10, 2019 at 06:05:18PM +0300, Dmitry Osipenko wrote: >>>> 10.07.2019 17:05, Maxime Ripard пишет: >>>>> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: >>>>>> This works: >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >>>>>> index 56d36779d213..e5a2f9c8f404 100644 >>>>>> --- a/drivers/gpu/drm/drm_client_modeset.c >>>>>> +++ b/drivers/gpu/drm/drm_client_modeset.c >>>>>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) >>>>>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); >>>>>> if (mode) >>>>>> list_add(&mode->head, &connector->modes); >>>>>> + else >>>>>> + cmdline_mode->specified = false; >>>>> >>>>> Hmmm, it's not clear to me why that wouldn't be the case. >>>>> >>>>> If we come back to the beginning of that function, we retrieve the >>>>> cmdline_mode buffer from the connector pointer, that will probably >>>>> have been parsed a first time using drm_mode_create_from_cmdline_mode >>>>> in drm_helper_probe_add_cmdline_mode. >>>>> >>>>> Now, I'm guessing that the issue is that in >>>>> drm_mode_parse_command_line_for_connector, if we have a named mode, we >>>>> just copy the mode over and set mode->specified. >>>>> >>>>> And we then move over to do other checks, and that's probably what >>>>> fails and returns, but our drm_cmdline_mode will have been modified. >>>>> >>>>> I'm not entirely sure how to deal with that though. >>>>> >>>>> I guess we could allocate a drm_cmdline_mode structure on the stack, >>>>> fill that, and if successful copy over its content to the one in >>>>> drm_connector. That would allow us to only change the content on >>>>> success, which is what I would expect from such a function? >>>>> >>>>> How does that sound? >>>> >>>> I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only >>>> for the "cmdline" mode and drm_client_rotation() is the only place in >>>> DRM code that cares about whether mode is from cmdline, hence looks like >>>> it will be more correct to do the following: >>> >>> I'm still under the impression that we're dealing with workarounds of >>> a more central issue, which is that we shouldn't return a partially >>> modified drm_cmdline_mode. >>> >>> You said it yourself, the breakage is in the commit changing the >>> command line parsing logic, while you're fixing here some code that >>> was introduced later on. >> >> The problem stems from assumption that *any* named mode is valid. It >> looks to me that the ultimate solution would be to move the mode's name >> comparison into the [1], if that's possible. >> >> [1] drm_mode_parse_command_line_for_connector() > > Well, one could argue that video=tegrafb is invalid and should be > rejected as well, but we haven't cleared that up. The video=tegrafb is invalid mode, there is nothing to argue here. And the problem is that invalid modes and not rejected for the very beginning. >>> Can you try the followintg patch? >>> http://code.bulix.org/8cwk4c-794565?raw >> >> This doesn't help because the problem with the rotation_reflection is >> that it's 0 if "rotation" not present in the cmdline and then ilog2(0) >> returns -1. So the patch "drm/modes: Don't apply cmdline's rotation if >> it wasn't specified" should be correct in any case. > > So we would have the same issue with rotate=0 then? No, we won't. Rotation mode is parsed into the DRM_MODE bitmask and rotate=0 corresponds to DRM_MODE_ROTATE_0, which is BIT(0) as you may notice. Hence rotation_reflection=0 is always an invalid value, meaning that "rotate" option does not present in the cmdline. Please consult the code, in particular see drm_mode_parse_cmdline_options() which was written by yourself ;) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-12 8:30 ` Dmitry Osipenko @ 2019-07-12 19:53 ` Maxime Ripard 2019-07-13 16:41 ` Dmitry Osipenko 0 siblings, 1 reply; 21+ messages in thread From: Maxime Ripard @ 2019-07-12 19:53 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel On Fri, Jul 12, 2019 at 11:30:01AM +0300, Dmitry Osipenko wrote: > 12.07.2019 11:10, Maxime Ripard пишет: > > On Thu, Jul 11, 2019 at 06:55:03PM +0300, Dmitry Osipenko wrote: > >> 11.07.2019 12:03, Maxime Ripard пишет: > >>> On Wed, Jul 10, 2019 at 06:05:18PM +0300, Dmitry Osipenko wrote: > >>>> 10.07.2019 17:05, Maxime Ripard пишет: > >>>>> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: > >>>>>> This works: > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > >>>>>> index 56d36779d213..e5a2f9c8f404 100644 > >>>>>> --- a/drivers/gpu/drm/drm_client_modeset.c > >>>>>> +++ b/drivers/gpu/drm/drm_client_modeset.c > >>>>>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) > >>>>>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); > >>>>>> if (mode) > >>>>>> list_add(&mode->head, &connector->modes); > >>>>>> + else > >>>>>> + cmdline_mode->specified = false; > >>>>> > >>>>> Hmmm, it's not clear to me why that wouldn't be the case. > >>>>> > >>>>> If we come back to the beginning of that function, we retrieve the > >>>>> cmdline_mode buffer from the connector pointer, that will probably > >>>>> have been parsed a first time using drm_mode_create_from_cmdline_mode > >>>>> in drm_helper_probe_add_cmdline_mode. > >>>>> > >>>>> Now, I'm guessing that the issue is that in > >>>>> drm_mode_parse_command_line_for_connector, if we have a named mode, we > >>>>> just copy the mode over and set mode->specified. > >>>>> > >>>>> And we then move over to do other checks, and that's probably what > >>>>> fails and returns, but our drm_cmdline_mode will have been modified. > >>>>> > >>>>> I'm not entirely sure how to deal with that though. > >>>>> > >>>>> I guess we could allocate a drm_cmdline_mode structure on the stack, > >>>>> fill that, and if successful copy over its content to the one in > >>>>> drm_connector. That would allow us to only change the content on > >>>>> success, which is what I would expect from such a function? > >>>>> > >>>>> How does that sound? > >>>> > >>>> I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only > >>>> for the "cmdline" mode and drm_client_rotation() is the only place in > >>>> DRM code that cares about whether mode is from cmdline, hence looks like > >>>> it will be more correct to do the following: > >>> > >>> I'm still under the impression that we're dealing with workarounds of > >>> a more central issue, which is that we shouldn't return a partially > >>> modified drm_cmdline_mode. > >>> > >>> You said it yourself, the breakage is in the commit changing the > >>> command line parsing logic, while you're fixing here some code that > >>> was introduced later on. > >> > >> The problem stems from assumption that *any* named mode is valid. It > >> looks to me that the ultimate solution would be to move the mode's name > >> comparison into the [1], if that's possible. > >> > >> [1] drm_mode_parse_command_line_for_connector() > > > > Well, one could argue that video=tegrafb is invalid and should be > > rejected as well, but we haven't cleared that up. > > The video=tegrafb is invalid mode, there is nothing to argue here. And > the problem is that invalid modes and not rejected for the very beginning. Yeah, I guess fb_get_options should also return an error in such a case, but I'm a bit worried about the side effects here. > >>> Can you try the followintg patch? > >>> http://code.bulix.org/8cwk4c-794565?raw > >> > >> This doesn't help because the problem with the rotation_reflection is > >> that it's 0 if "rotation" not present in the cmdline and then ilog2(0) > >> returns -1. So the patch "drm/modes: Don't apply cmdline's rotation if > >> it wasn't specified" should be correct in any case. > > > > So we would have the same issue with rotate=0 then? > > No, we won't. Rotation mode is parsed into the DRM_MODE bitmask and > rotate=0 corresponds to DRM_MODE_ROTATE_0, which is BIT(0) as you may > notice. Hence rotation_reflection=0 is always an invalid value, meaning > that "rotate" option does not present in the cmdline. Please consult the > code, in particular see drm_mode_parse_cmdline_options() which was > written by yourself ;) Sigh... You're right :) Sorry for that, I'll reply to the other patch Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1] drm/modes: Skip invalid cmdline mode 2019-07-12 19:53 ` Maxime Ripard @ 2019-07-13 16:41 ` Dmitry Osipenko 0 siblings, 0 replies; 21+ messages in thread From: Dmitry Osipenko @ 2019-07-13 16:41 UTC (permalink / raw) To: Maxime Ripard Cc: Thierry Reding, Jonathan Hunter, Maarten Lankhorst, Sean Paul, Daniel Vetter, David Airlie, dri-devel, linux-tegra, linux-kernel 12.07.2019 22:53, Maxime Ripard пишет: > On Fri, Jul 12, 2019 at 11:30:01AM +0300, Dmitry Osipenko wrote: >> 12.07.2019 11:10, Maxime Ripard пишет: >>> On Thu, Jul 11, 2019 at 06:55:03PM +0300, Dmitry Osipenko wrote: >>>> 11.07.2019 12:03, Maxime Ripard пишет: >>>>> On Wed, Jul 10, 2019 at 06:05:18PM +0300, Dmitry Osipenko wrote: >>>>>> 10.07.2019 17:05, Maxime Ripard пишет: >>>>>>> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote: >>>>>>>> This works: >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >>>>>>>> index 56d36779d213..e5a2f9c8f404 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_client_modeset.c >>>>>>>> +++ b/drivers/gpu/drm/drm_client_modeset.c >>>>>>>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector) >>>>>>>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode); >>>>>>>> if (mode) >>>>>>>> list_add(&mode->head, &connector->modes); >>>>>>>> + else >>>>>>>> + cmdline_mode->specified = false; >>>>>>> >>>>>>> Hmmm, it's not clear to me why that wouldn't be the case. >>>>>>> >>>>>>> If we come back to the beginning of that function, we retrieve the >>>>>>> cmdline_mode buffer from the connector pointer, that will probably >>>>>>> have been parsed a first time using drm_mode_create_from_cmdline_mode >>>>>>> in drm_helper_probe_add_cmdline_mode. >>>>>>> >>>>>>> Now, I'm guessing that the issue is that in >>>>>>> drm_mode_parse_command_line_for_connector, if we have a named mode, we >>>>>>> just copy the mode over and set mode->specified. >>>>>>> >>>>>>> And we then move over to do other checks, and that's probably what >>>>>>> fails and returns, but our drm_cmdline_mode will have been modified. >>>>>>> >>>>>>> I'm not entirely sure how to deal with that though. >>>>>>> >>>>>>> I guess we could allocate a drm_cmdline_mode structure on the stack, >>>>>>> fill that, and if successful copy over its content to the one in >>>>>>> drm_connector. That would allow us to only change the content on >>>>>>> success, which is what I would expect from such a function? >>>>>>> >>>>>>> How does that sound? >>>>>> >>>>>> I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only >>>>>> for the "cmdline" mode and drm_client_rotation() is the only place in >>>>>> DRM code that cares about whether mode is from cmdline, hence looks like >>>>>> it will be more correct to do the following: >>>>> >>>>> I'm still under the impression that we're dealing with workarounds of >>>>> a more central issue, which is that we shouldn't return a partially >>>>> modified drm_cmdline_mode. >>>>> >>>>> You said it yourself, the breakage is in the commit changing the >>>>> command line parsing logic, while you're fixing here some code that >>>>> was introduced later on. >>>> >>>> The problem stems from assumption that *any* named mode is valid. It >>>> looks to me that the ultimate solution would be to move the mode's name >>>> comparison into the [1], if that's possible. >>>> >>>> [1] drm_mode_parse_command_line_for_connector() >>> >>> Well, one could argue that video=tegrafb is invalid and should be >>> rejected as well, but we haven't cleared that up. >> >> The video=tegrafb is invalid mode, there is nothing to argue here. And >> the problem is that invalid modes and not rejected for the very beginning. > > Yeah, I guess fb_get_options should also return an error in such a > case, but I'm a bit worried about the side effects here. At least the showstopper regression is fixed now. Everything else could be worked out later on. >>>>> Can you try the followintg patch? >>>>> http://code.bulix.org/8cwk4c-794565?raw >>>> >>>> This doesn't help because the problem with the rotation_reflection is >>>> that it's 0 if "rotation" not present in the cmdline and then ilog2(0) >>>> returns -1. So the patch "drm/modes: Don't apply cmdline's rotation if >>>> it wasn't specified" should be correct in any case. >>> >>> So we would have the same issue with rotate=0 then? >> >> No, we won't. Rotation mode is parsed into the DRM_MODE bitmask and >> rotate=0 corresponds to DRM_MODE_ROTATE_0, which is BIT(0) as you may >> notice. Hence rotation_reflection=0 is always an invalid value, meaning >> that "rotate" option does not present in the cmdline. Please consult the >> code, in particular see drm_mode_parse_cmdline_options() which was >> written by yourself ;) > > Sigh... You're right :) > > Sorry for that, I'll reply to the other patch Thank you very much. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-07-13 16:41 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-09 14:51 [PATCH v1] drm/modes: Skip invalid cmdline mode Dmitry Osipenko 2019-07-10 10:00 ` Ville Syrjälä 2019-07-10 14:03 ` Dmitry Osipenko 2019-07-10 10:12 ` Maxime Ripard 2019-07-10 12:42 ` Dmitry Osipenko 2019-07-10 12:55 ` Maxime Ripard 2019-07-10 12:59 ` Dmitry Osipenko 2019-07-10 13:06 ` Maxime Ripard 2019-07-10 13:11 ` Dmitry Osipenko 2019-07-10 13:29 ` Dmitry Osipenko 2019-07-10 13:36 ` Dmitry Osipenko 2019-07-10 14:05 ` Maxime Ripard 2019-07-10 15:05 ` Dmitry Osipenko 2019-07-10 15:45 ` Dmitry Osipenko 2019-07-10 16:03 ` Dmitry Osipenko 2019-07-11 9:03 ` Maxime Ripard 2019-07-11 15:55 ` Dmitry Osipenko 2019-07-12 8:10 ` Maxime Ripard 2019-07-12 8:30 ` Dmitry Osipenko 2019-07-12 19:53 ` Maxime Ripard 2019-07-13 16:41 ` Dmitry Osipenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).