drm/modes: Skip invalid cmdline mode
Subsystems (3)
PATCH 1/1 drm/modes: Skip invalid cmdline mode
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.
--- drivers/gpu/drm/drm_client_modeset.c | 3 ++- drivers/gpu/drm/drm_modes.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_client_modeset.c +2 -1
@@ -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; }
drivers/gpu/drm/drm_modes.c +6 -0
@@ -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 ;
Signature · 1 line
2.22.0
Quoted 7 lines · Dmitry Osipenko
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.
Quoted 11 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/drm/drm_client_modeset.c | 3 ++- drivers/gpu/drm/drm_modes.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_client_modeset.c +0
-0
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 184
Quoted 6 lines
drivers/gpu/drm/drm_client_modeset.c +2
-1@@ -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.
Quoted 18 lines
drivers/gpu/drm/drm_client_modeset.c +0
-0return mode; }drivers/gpu/drm/drm_modes.c +3
-0@@ -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 */
drivers/gpu/drm/drm_modes.c drm_gtf_mode_complex(struct drm_device *dev, int hdisplay, int vdisplay, · line 400
Quoted 6 lines
drivers/gpu/drm/drm_modes.c +3
-0@@ -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
Quoted 10 lines
drivers/gpu/drm/drm_modes.c +0
-0drm_mode =drm_mode_create (dev);if (!drm_mode)return NULL ;Signature · 6 lines
2.22.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 6 lines
Ville Syrjälä Intel _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
10.07.2019 13:00, Ville Syrjälä пишет:
Quoted 10 lines · Dmitry Osipenko
Quoted 7 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.
Quoted 44 lines
Quoted 11 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/drm/drm_client_modeset.c | 3 ++- drivers/gpu/drm/drm_modes.c | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_client_modeset.c +0
-0drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 184
Quoted 6 lines
drivers/gpu/drm/drm_client_modeset.c +2
-1@@ -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=1But I'd have to rebase that so let's just go with your patch.
Quoted 21 lines
drivers/gpu/drm/drm_client_modeset.c +0
-0return mode; }drivers/gpu/drm/drm_modes.c +3
-0@@ -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;
Quoted 8 lines
drivers/gpu/drm/drm_modes.c struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay, · line 163
Quoted 3 lines
drivers/gpu/drm/drm_modes.c +3
-0+if (!hdisplay || !vdisplay) +return NULL ; +These lgtm
Patch is
Reviewed-by Ville Syrjälä <[email protected]>
Thanks!
Maxime Ripard
AppliedQuoted 8 lines · Dmitry Osipenko
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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 <[email protected]>
Applied to drm-misc-next-fixes
Thanks for figuring this out!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
10.07.2019 13:12, Maxime Ripard пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.
Quoted 6 lines
Quoted 2 lines
Fixes 3aeeb13d8996 ("drm/modes: Support modes names on the command line")Signed-off-by Dmitry Osipenko <[email protected]>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.
Quoted 15 lines · Dmitry Osipenko
10.07.2019 13:12, Maxime Ripard пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.Quoted 6 lines
Quoted 2 lines
Fixes 3aeeb13d8996 ("drm/modes: Support modes names on the command line")Signed-off-by Dmitry Osipenko <[email protected]>Applied to drm-misc-next-fixes
Thanks for figuring this out!
Quoted 7 lines
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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
10.07.2019 15:55, Maxime Ripard пишет:
Quoted 24 lines · Dmitry Osipenko
Quoted 15 lines
10.07.2019 13:12, Maxime Ripard пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.Quoted 6 lines
Quoted 2 lines
Fixes 3aeeb13d8996 ("drm/modes: Support modes names on the command line")Signed-off-by Dmitry Osipenko <[email protected]>Applied to drm-misc-next-fixes
Thanks for figuring this out!
Quoted 7 lines
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.
Quoted 1 line
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
Quoted 25 lines · Dmitry Osipenko
10.07.2019 15:55, Maxime Ripard пишет:
Quoted 24 lines · Dmitry Osipenko
Quoted 22 lines
10.07.2019 13:12, Maxime Ripard пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.Quoted 6 lines
Quoted 2 lines
Fixes 3aeeb13d8996 ("drm/modes: Support modes names on the command line")Signed-off-by Dmitry Osipenko <[email protected]>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.
Quoted 5 lines
Quoted 1 line
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
10.07.2019 16:06, Maxime Ripard пишет:
Quoted 32 lines · Dmitry Osipenko
Quoted 25 lines
10.07.2019 15:55, Maxime Ripard пишет:
Quoted 24 lines · Dmitry Osipenko
Quoted 22 lines
10.07.2019 13:12, Maxime Ripard пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.Quoted 6 lines
Quoted 2 lines
Fixes 3aeeb13d8996 ("drm/modes: Support modes names on the command line")Signed-off-by Dmitry Osipenko <[email protected]>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.
Quoted 5 lines
Quoted 1 line
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
Quoted 7 lines
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.
Quoted 2 lines
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)
10.07.2019 16:11, Dmitry Osipenko пишет:
Quoted 103 lines
10.07.2019 16:06, Maxime Ripard пишет:
Quoted 32 lines · Dmitry Osipenko
Quoted 25 lines
10.07.2019 15:55, Maxime Ripard пишет:
Quoted 24 lines · Dmitry Osipenko
Quoted 22 lines
10.07.2019 13:12, Maxime Ripard пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.Quoted 6 lines
Quoted 2 lines
Fixes 3aeeb13d8996 ("drm/modes: Support modes names on the command line")Signed-off-by Dmitry Osipenko <[email protected]>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.
Quoted 5 lines
Quoted 1 line
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
Quoted 7 lines
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.
Quoted 2 lines
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 :
Quoted 9 lines
[ 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:
drivers/gpu/drm/drm_client_modeset.c +2 -0
@@ -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.
10.07.2019 16:29, Dmitry Osipenko пишет:
Quoted 130 lines
10.07.2019 16:11, Dmitry Osipenko пишет:
Quoted 103 lines
10.07.2019 16:06, Maxime Ripard пишет:
Quoted 32 lines · Dmitry Osipenko
Quoted 25 lines
10.07.2019 15:55, Maxime Ripard пишет:
Quoted 24 lines · Dmitry Osipenko
Quoted 22 lines
10.07.2019 13:12, Maxime Ripard пишет:
Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines
The named mode could be invalid and then cmdline parser misses to validate mode's dimensions, happily adding
0x0mode 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.Quoted 6 lines
Quoted 2 lines
Fixes 3aeeb13d8996 ("drm/modes: Support modes names on the command line")Signed-off-by Dmitry Osipenko <[email protected]>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.
Quoted 5 lines
Quoted 1 line
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
Quoted 7 lines
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.
Quoted 2 lines
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 :Quoted 9 lines
[ 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:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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; }
Quoted 2 lines
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.
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines · Dmitry Osipenko
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 28 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.
Quoted 8 lines
I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_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:
(quoted diff) +0 -0
b/drivers/gpu/drm/drm_client_modeset.c
index 56d36779d213..e5b3be9ed689 100644
drivers/gpu/drm/drm_client_modeset.c +0 -0
@@ -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;10.07.2019 18:05, Dmitry Osipenko пишет:
Quoted 66 lines
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 28 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.Quoted 8 lines
I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_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_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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:drivers/gpu/drm/drm_client_modeset.c +1
-0@@ -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;drivers/gpu/drm/drm_client_modeset.c +1
-1@@ -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;
Quoted 2 lines
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.
10.07.2019 18:45, Dmitry Osipenko пишет:
Quoted 85 lines
10.07.2019 18:05, Dmitry Osipenko пишет:
Quoted 66 lines
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 28 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.Quoted 8 lines
I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_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_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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:drivers/gpu/drm/drm_client_modeset.c +1
-0@@ -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;drivers/gpu/drm/drm_client_modeset.c +1
-1@@ -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;Quoted 2 lines
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:
(quoted diff) +1
-1@@ -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;
Quoted 4 lines
And looks like there is another problem here.. the cmdline's rotation overrides *all* modes while the
doc/fb/modedb.rstclaims 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.
Quoted 36 lines · Dmitry Osipenko
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 35 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_connector. That would allow us to only change the content on success, which is what I would expect from such a function?
Quoted 6 lines
Quoted 1 line
How does that sound?
I now see that there is
DRM_MODE_TYPE_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
11.07.2019 12:03, Maxime Ripard пишет:
Quoted 44 lines · Dmitry Osipenko
Quoted 36 lines
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 35 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_connector. That would allow us to only change the content on success, which is what I would expect from such a function?Quoted 6 lines
Quoted 1 line
How does that sound?
I now see that there is
DRM_MODE_TYPE_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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:
Quoted 7 lines
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()
Quoted 2 lines
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.
Quoted 53 lines · Dmitry Osipenko
11.07.2019 12:03, Maxime Ripard пишет:
Quoted 44 lines · Dmitry Osipenko
Quoted 36 lines
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 35 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_connector. That would allow us to only change the content on success, which is what I would expect from such a function?Quoted 6 lines
Quoted 1 line
How does that sound?
I now see that there is
DRM_MODE_TYPE_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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:Quoted 7 lines
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.
Quoted 5 lines
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.
Quoted 7 lines
Quoted 2 lines
Can you try the followintg patch?
http://code.bulix.org/8cwk4c-794565?rawThis doesn't help because the problem with the
rotation_reflectionis 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
12.07.2019 11:10, Maxime Ripard пишет:
Quoted 54 lines · Dmitry Osipenko
Quoted 53 lines
11.07.2019 12:03, Maxime Ripard пишет:
Quoted 51 lines · Dmitry Osipenko
Quoted 36 lines
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 35 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_connector. That would allow us to only change the content on success, which is what I would expect from such a function?Quoted 6 lines
Quoted 1 line
How does that sound?
I now see that there is
DRM_MODE_TYPE_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
Can you try the followintg patch?
http://code.bulix.org/8cwk4c-794565?rawThis doesn't help because the problem with the
rotation_reflectionis 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 ;)
Quoted 59 lines · Dmitry Osipenko
12.07.2019 11:10, Maxime Ripard пишет:
Quoted 58 lines · Dmitry Osipenko
Quoted 57 lines
11.07.2019 12:03, Maxime Ripard пишет:
Quoted 44 lines · Dmitry Osipenko
Quoted 36 lines
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 35 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_connector. That would allow us to only change the content on success, which is what I would expect from such a function?Quoted 6 lines
Quoted 1 line
How does that sound?
I now see that there is
DRM_MODE_TYPE_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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:Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
[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.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Can you try the followintg patch?
http://code.bulix.org/8cwk4c-794565?rawThis doesn't help because the problem with the
rotation_reflectionis 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.
Quoted 8 lines
Quoted 1 line
So we would have the same issue with rotate=0 then?
No, we won't. Rotation mode is parsed into the
DRM_MODEbitmask and rotate=0 corresponds toDRM_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 seedrm_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
12.07.2019 22:53, Maxime Ripard пишет:
Quoted 62 lines · Dmitry Osipenko
Quoted 61 lines
12.07.2019 11:10, Maxime Ripard пишет:
Quoted 60 lines · Dmitry Osipenko
Quoted 58 lines
11.07.2019 12:03, Maxime Ripard пишет:
Quoted 44 lines · Dmitry Osipenko
Quoted 36 lines
10.07.2019 17:05, Maxime Ripard пишет:
Quoted 35 lines · Dmitry Osipenko
drivers/gpu/drm/drm_client_modeset.c drm_connector_pick_cmdline_mode(struct drm_connector *connector) · line 186
Quoted 12 lines
This works:
drivers/gpu/drm/drm_client_modeset.c +2
-0@@ -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_modebuffer from the connector pointer, that will probably have been parsed a first time usingdrm_mode_create_from_cmdline_modeindrm_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 setmode->specified.And we then move over to do other checks, and that's probably what fails and returns, but our
drm_cmdline_modewill have been modified.I'm not entirely sure how to deal with that though.
I guess we could allocate a
drm_cmdline_modestructure on the stack, fill that, and if successful copy over its content to the one indrm_connector. That would allow us to only change the content on success, which is what I would expect from such a function?Quoted 6 lines
Quoted 1 line
How does that sound?
I now see that there is
DRM_MODE_TYPE_USERDEFflag that is assigned only for the "cmdline" mode anddrm_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:Quoted 7 lines
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()
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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_optionsshould 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.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Can you try the followintg patch?
http://code.bulix.org/8cwk4c-794565?rawThis doesn't help because the problem with the
rotation_reflectionis 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.Quoted 8 lines
Quoted 1 line
So we would have the same issue with rotate=0 then?
No, we won't. Rotation mode is parsed into the
DRM_MODEbitmask and rotate=0 corresponds toDRM_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 seedrm_mode_parse_cmdline_options()which was written by yourself ;)
Quoted 3 lines
Sigh... You're right :)
Sorry for that, I'll reply to the other patch
Thank you very much.