[PATCH v1] drm/modes: Skip invalid cmdline mode

[PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[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.

> 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

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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.

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[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)

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[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.

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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.

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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!

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[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;

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[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.

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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.

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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.

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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 ;)

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[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.

> >>> 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

Re: [PATCH v1] drm/modes: Skip invalid cmdline mode

[Dmitry Osipenko]

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.