drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
[ 4.480976] ------------[ cut here ]------------ [ 4.485603] WARNING: CPU: 5 PID: 369 at drivers/gpu/drm/drm_mode_object.c:45 __drm_mode_object_add+0xb4/0xbc <snip> [ 4.609772] Call trace: [ 4.612208] __drm_mode_object_add+0xb4/0xbc [ 4.616466] drm_mode_object_add+0x20/0x2c [ 4.620552] drm_property_create+0xdc/0x174 [ 4.624723] drm_property_create_enum+0x34/0x98 [ 4.629241] drm_connector_set_panel_orientation+0x64/0xa0 [ 4.634716] boe_panel_get_modes+0x88/0xd8 [ 4.638802] drm_panel_get_modes+0x2c/0x48 [ 4.642887] panel_bridge_get_modes+0x1c/0x28 [ 4.647233] drm_bridge_connector_get_modes+0xa0/0xd4 [ 4.652273] drm_helper_probe_single_connector_modes+0x218/0x700 [ 4.658266] drm_mode_getconnector+0x1b4/0x45c [ 4.662699] drm_ioctl_kernel+0xac/0x128 [ 4.666611] drm_ioctl+0x268/0x410 [ 4.670002] drm_compat_ioctl+0xdc/0xf0 [ 4.673829] __arm64_compat_sys_ioctl+0xc8/0x100 [ 4.678436] el0_svc_common+0xf4/0x1c0 [ 4.682174] do_el0_svc_compat+0x28/0x3c [ 4.686088] el0_svc_compat+0x10/0x1c [ 4.689738] el0_sync_compat_handler+0xa8/0xcc [ 4.694171] el0_sync_compat+0x178/0x180 [ 4.698082] ---[ end trace b4f2db9d9c88610b ]--- [ 4.702721] ------------[ cut here ]------------ [ 4.707329] WARNING: CPU: 5 PID: 369 at drivers/gpu/drm/drm_mode_object.c:243 drm_object_attach_property+0x48/0xb8 <snip> [ 4.833830] Call trace: [ 4.836266] drm_object_attach_property+0x48/0xb8 [ 4.840958] drm_connector_set_panel_orientation+0x84/0xa0 [ 4.846432] boe_panel_get_modes+0x88/0xd8 [ 4.850516] drm_panel_get_modes+0x2c/0x48 [ 4.854600] panel_bridge_get_modes+0x1c/0x28 [ 4.858946] drm_bridge_connector_get_modes+0xa0/0xd4 [ 4.863984] drm_helper_probe_single_connector_modes+0x218/0x700 [ 4.869978] drm_mode_getconnector+0x1b4/0x45c [ 4.874410] drm_ioctl_kernel+0xac/0x128 [ 4.878320] drm_ioctl+0x268/0x410 [ 4.881711] drm_compat_ioctl+0xdc/0xf0 [ 4.885536] __arm64_compat_sys_ioctl+0xc8/0x100 [ 4.890142] el0_svc_common+0xf4/0x1c0 [ 4.893879] do_el0_svc_compat+0x28/0x3c [ 4.897791] el0_svc_compat+0x10/0x1c [ 4.901441] el0_sync_compat_handler+0xa8/0xcc [ 4.905873] el0_sync_compat+0x178/0x180 [ 4.909783] ---[ end trace b4f2db9d9c88610c ]---
Signed-off-by: Hsin-Yi Wang hsinyi@chromium.org Reviewed-by: Sean Paul seanpaul@chromium.org --- v7->v8: - check if the prop is created to avoid leak issue when called multiple times. - attempt to create prop in drm_connector_set_panel_orientation if prop is not created before, so driver don't need to call drm_connector_init_panel_orientation_property if they don't need to set the property earlier. --- drivers/gpu/drm/drm_connector.c | 62 ++++++++++++++++++++++++--------- include/drm/drm_connector.h | 2 ++ 2 files changed, 48 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2fec..572ead7ac10690 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] = { * INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel * coordinates, so if userspace rotates the picture to adjust for * the orientation it must also apply the same transformation to the - * touchscreen input coordinates. This property is initialized by calling + * touchscreen input coordinates. This property value is set by calling * drm_connector_set_panel_orientation() or * drm_connector_set_panel_orientation_with_quirk() * @@ -2341,8 +2341,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property); * @connector: connector for which to set the panel-orientation property. * @panel_orientation: drm_panel_orientation value to set * - * This function sets the connector's panel_orientation and attaches - * a "panel orientation" property to the connector. + * This function sets the connector's panel_orientation value. If the property + * doesn't exist, it will try to create one. * * Calling this function on a connector where the panel_orientation has * already been set is a no-op (e.g. the orientation has been overridden with @@ -2373,19 +2373,12 @@ int drm_connector_set_panel_orientation( info->panel_orientation = panel_orientation;
prop = dev->mode_config.panel_orientation_property; - if (!prop) { - prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, - "panel orientation", - drm_panel_orientation_enum_list, - ARRAY_SIZE(drm_panel_orientation_enum_list)); - if (!prop) - return -ENOMEM; - - dev->mode_config.panel_orientation_property = prop; - } + if (!prop && + drm_connector_init_panel_orientation_property(connector) < 0) + return -ENOMEM;
- drm_object_attach_property(&connector->base, prop, - info->panel_orientation); + drm_object_property_set_value(&connector->base, prop, + info->panel_orientation); return 0; } EXPORT_SYMBOL(drm_connector_set_panel_orientation); @@ -2393,7 +2386,7 @@ EXPORT_SYMBOL(drm_connector_set_panel_orientation); /** * drm_connector_set_panel_orientation_with_quirk - set the * connector's panel_orientation after checking for quirks - * @connector: connector for which to init the panel-orientation property. + * @connector: connector for which to set the panel-orientation property. * @panel_orientation: drm_panel_orientation value to set * @width: width in pixels of the panel, used for panel quirk detection * @height: height in pixels of the panel, used for panel quirk detection @@ -2420,6 +2413,43 @@ int drm_connector_set_panel_orientation_with_quirk( } EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
+/** + * drm_connector_init_panel_orientation_property - + * create the connector's panel orientation property + * + * This function attaches a "panel orientation" property to the connector + * and initializes its value to DRM_MODE_PANEL_ORIENTATION_UNKNOWN. + * + * The value of the property can be set by drm_connector_set_panel_orientation() + * or drm_connector_set_panel_orientation_with_quirk() later. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_connector_init_panel_orientation_property( + struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + struct drm_property *prop; + + if(dev->mode_config.panel_orientation_property) + return 0; + + prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, + "panel orientation", + drm_panel_orientation_enum_list, + ARRAY_SIZE(drm_panel_orientation_enum_list)); + if (!prop) + return -ENOMEM; + + dev->mode_config.panel_orientation_property = prop; + drm_object_attach_property(&connector->base, prop, + DRM_MODE_PANEL_ORIENTATION_UNKNOWN); + + return 0; +} +EXPORT_SYMBOL(drm_connector_init_panel_orientation_property); + static const struct drm_prop_enum_list privacy_screen_enum[] = { { PRIVACY_SCREEN_DISABLED, "Disabled" }, { PRIVACY_SCREEN_ENABLED, "Enabled" }, diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 64cf5f88c05b6a..d3448a71bb4d85 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1798,6 +1798,8 @@ int drm_connector_set_panel_orientation_with_quirk( struct drm_connector *connector, enum drm_panel_orientation panel_orientation, int width, int height); +int drm_connector_init_panel_orientation_property( + struct drm_connector *connector); int drm_connector_attach_max_bpc_property(struct drm_connector *connector, int min, int max); void drm_connector_create_privacy_screen_properties(struct drm_connector *conn);
Init panel orientation property after connector is initialized. Let the panel driver decides the orientation value later.
Signed-off-by: Hsin-Yi Wang hsinyi@chromium.org Acked-by: Chun-Kuang Hu chunkuang.hu@kernel.org --- drivers/gpu/drm/mediatek/mtk_dsi.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 5d90d2eb001935..491bf5b0a2b984 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -965,6 +965,13 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) ret = PTR_ERR(dsi->connector); goto err_cleanup_encoder; } + + ret = drm_connector_init_panel_orientation_property(dsi->connector); + if (ret) { + DRM_ERROR("Unable to init panel orientation\n"); + goto err_cleanup_encoder; + } + drm_connector_attach_encoder(dsi->connector, &dsi->encoder);
return 0;
krane, kakadu, and kodama boards have a default panel rotation.
Signed-off-by: Hsin-Yi Wang hsinyi@chromium.org Reviewed-by: Enric Balletbo i Serra enric.balletbo@collabora.com Tested-by: Enric Balletbo i Serra enric.balletbo@collabora.com --- arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi index b42d81d26d7211..d29d4378170971 100644 --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi @@ -276,6 +276,7 @@ panel: panel@0 { avee-supply = <&ppvarp_lcd>; pp1800-supply = <&pp1800_lcd>; backlight = <&backlight_lcd0>; + rotation = <270>; port { panel_in: endpoint { remote-endpoint = <&dsi_out>;
Hsin-Yi Wang hsinyi@chromium.org writes:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Hi,
I stumbled upon this when investigating the same WARN_ON on amdgpu. Thanks for the patch :)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2fec..572ead7ac10690 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
- INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
- coordinates, so if userspace rotates the picture to adjust for
- the orientation it must also apply the same transformation to the
- touchscreen input coordinates. This property is initialized by calling
- touchscreen input coordinates. This property value is set by calling
- drm_connector_set_panel_orientation() or
- drm_connector_set_panel_orientation_with_quirk()
@@ -2341,8 +2341,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
- @connector: connector for which to set the panel-orientation property.
- @panel_orientation: drm_panel_orientation value to set
- This function sets the connector's panel_orientation and attaches
- a "panel orientation" property to the connector.
- This function sets the connector's panel_orientation value. If the property
- doesn't exist, it will try to create one.
- Calling this function on a connector where the panel_orientation has
- already been set is a no-op (e.g. the orientation has been overridden with
@@ -2373,19 +2373,12 @@ int drm_connector_set_panel_orientation( info->panel_orientation = panel_orientation;
prop = dev->mode_config.panel_orientation_property;
- if (!prop) {
prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,"panel orientation",drm_panel_orientation_enum_list,ARRAY_SIZE(drm_panel_orientation_enum_list));if (!prop)return -ENOMEM;dev->mode_config.panel_orientation_property = prop;- }
- if (!prop &&
drm_connector_init_panel_orientation_property(connector) < 0)return -ENOMEM;
In the case where the property has not been created beforehand, you forgot to reinitialize prop here, after calling drm_connector_init_panel_orientation_property(). This means drm_object_property_set_value() will be called with a NULL second argument and Oops the kernel.
- drm_object_attach_property(&connector->base, prop,
info->panel_orientation);
- drm_object_property_set_value(&connector->base, prop,
info->panel_orientation);
On Tue, Feb 15, 2022 at 9:17 AM Gabriel Krisman Bertazi krisman@collabora.com wrote:
Hsin-Yi Wang hsinyi@chromium.org writes:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Hi,
I stumbled upon this when investigating the same WARN_ON on amdgpu. Thanks for the patch :)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2fec..572ead7ac10690 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
- INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
- coordinates, so if userspace rotates the picture to adjust for
- the orientation it must also apply the same transformation to the
- touchscreen input coordinates. This property is initialized by calling
- touchscreen input coordinates. This property value is set by calling
- drm_connector_set_panel_orientation() or
- drm_connector_set_panel_orientation_with_quirk()
@@ -2341,8 +2341,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
- @connector: connector for which to set the panel-orientation property.
- @panel_orientation: drm_panel_orientation value to set
- This function sets the connector's panel_orientation and attaches
- a "panel orientation" property to the connector.
- This function sets the connector's panel_orientation value. If the property
- doesn't exist, it will try to create one.
- Calling this function on a connector where the panel_orientation has
- already been set is a no-op (e.g. the orientation has been overridden with
@@ -2373,19 +2373,12 @@ int drm_connector_set_panel_orientation( info->panel_orientation = panel_orientation;
prop = dev->mode_config.panel_orientation_property;
if (!prop) {prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,"panel orientation",drm_panel_orientation_enum_list,ARRAY_SIZE(drm_panel_orientation_enum_list));if (!prop)return -ENOMEM;dev->mode_config.panel_orientation_property = prop;}
if (!prop &&drm_connector_init_panel_orientation_property(connector) < 0)return -ENOMEM;In the case where the property has not been created beforehand, you forgot to reinitialize prop here, after calling drm_connector_init_panel_orientation_property(). This means
hi Gabriel,
drm_connector_init_panel_orientation_property() will create prop if it's null. If prop fails to be created there, it will return -ENOMEM.
drm_object_property_set_value() will be called with a NULL second argument and Oops the kernel.
drm_object_attach_property(&connector->base, prop,info->panel_orientation);
drm_object_property_set_value(&connector->base, prop,info->panel_orientation);-- Gabriel Krisman Bertazi
Hsin-Yi Wang hsinyi@chromium.org writes:
On Tue, Feb 15, 2022 at 9:17 AM Gabriel Krisman Bertazi krisman@collabora.com wrote:
Hsin-Yi Wang hsinyi@chromium.org writes:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Hi,
I stumbled upon this when investigating the same WARN_ON on amdgpu. Thanks for the patch :)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2fec..572ead7ac10690 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
- INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
- coordinates, so if userspace rotates the picture to adjust for
- the orientation it must also apply the same transformation to the
- touchscreen input coordinates. This property is initialized by calling
- touchscreen input coordinates. This property value is set by calling
- drm_connector_set_panel_orientation() or
- drm_connector_set_panel_orientation_with_quirk()
@@ -2341,8 +2341,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
- @connector: connector for which to set the panel-orientation property.
- @panel_orientation: drm_panel_orientation value to set
- This function sets the connector's panel_orientation and attaches
- a "panel orientation" property to the connector.
- This function sets the connector's panel_orientation value. If the property
- doesn't exist, it will try to create one.
- Calling this function on a connector where the panel_orientation has
- already been set is a no-op (e.g. the orientation has been overridden with
@@ -2373,19 +2373,12 @@ int drm_connector_set_panel_orientation( info->panel_orientation = panel_orientation;
prop = dev->mode_config.panel_orientation_property;
if (!prop) {prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,"panel orientation",drm_panel_orientation_enum_list,ARRAY_SIZE(drm_panel_orientation_enum_list));if (!prop)return -ENOMEM;dev->mode_config.panel_orientation_property = prop;}
if (!prop &&drm_connector_init_panel_orientation_property(connector) < 0)return -ENOMEM;In the case where the property has not been created beforehand, you forgot to reinitialize prop here, after calling drm_connector_init_panel_orientation_property(). This means
hi Gabriel,
drm_connector_init_panel_orientation_property() will create prop if it's null. If prop fails to be created there, it will return -ENOMEM.
Yes. But *after the property is successfully created*, the prop variable is still NULL. And then you call the following, using prop, which is still NULL:
drm_object_property_set_value(&connector->base, prop,info->panel_orientation);
This will do property->dev right on the first line of code, and dereference the null prop pointer.
You must do
prop = dev->mode_config.panel_orientation_property;
again after drm_connector_init_panel_orientation_property successfully returns, or call drm_object_property_set_value using dev->mode_config.panel_orientation_property directly:
drm_object_property_set_value(&connector->base, dev->mode_config.panel_orientation_property info->panel_orientation);
On Tue, Feb 15, 2022 at 12:03 PM Gabriel Krisman Bertazi krisman@collabora.com wrote:
Hsin-Yi Wang hsinyi@chromium.org writes:
On Tue, Feb 15, 2022 at 9:17 AM Gabriel Krisman Bertazi krisman@collabora.com wrote:
Hsin-Yi Wang hsinyi@chromium.org writes:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Hi,
I stumbled upon this when investigating the same WARN_ON on amdgpu. Thanks for the patch :)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2fec..572ead7ac10690 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1252,7 +1252,7 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
- INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
- coordinates, so if userspace rotates the picture to adjust for
- the orientation it must also apply the same transformation to the
- touchscreen input coordinates. This property is initialized by calling
- touchscreen input coordinates. This property value is set by calling
- drm_connector_set_panel_orientation() or
- drm_connector_set_panel_orientation_with_quirk()
@@ -2341,8 +2341,8 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
- @connector: connector for which to set the panel-orientation property.
- @panel_orientation: drm_panel_orientation value to set
- This function sets the connector's panel_orientation and attaches
- a "panel orientation" property to the connector.
- This function sets the connector's panel_orientation value. If the property
- doesn't exist, it will try to create one.
- Calling this function on a connector where the panel_orientation has
- already been set is a no-op (e.g. the orientation has been overridden with
@@ -2373,19 +2373,12 @@ int drm_connector_set_panel_orientation( info->panel_orientation = panel_orientation;
prop = dev->mode_config.panel_orientation_property;
if (!prop) {prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,"panel orientation",drm_panel_orientation_enum_list,ARRAY_SIZE(drm_panel_orientation_enum_list));if (!prop)return -ENOMEM;dev->mode_config.panel_orientation_property = prop;}
if (!prop &&drm_connector_init_panel_orientation_property(connector) < 0)return -ENOMEM;In the case where the property has not been created beforehand, you forgot to reinitialize prop here, after calling drm_connector_init_panel_orientation_property(). This means
hi Gabriel,
drm_connector_init_panel_orientation_property() will create prop if it's null. If prop fails to be created there, it will return -ENOMEM.
Yes. But *after the property is successfully created*, the prop variable is still NULL. And then you call the following, using prop, which is still NULL:
drm_object_property_set_value(&connector->base, prop,info->panel_orientation);This will do property->dev right on the first line of code, and dereference the null prop pointer.
Ah, right. Sorry that I totally missed this. I'll fix it in the next version if the idea of this patch is accepted. There might be another preferred solution for this.
You must do
prop = dev->mode_config.panel_orientation_property;
again after drm_connector_init_panel_orientation_property successfully returns, or call drm_object_property_set_value using dev->mode_config.panel_orientation_property directly:
drm_object_property_set_value(&connector->base, dev->mode_config.panel_orientation_property info->panel_orientation);
-- Gabriel Krisman Bertazi
Greetings everyone,
Padron for joining in so late o/
On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang hsinyi@chromium.org wrote:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
The warning illustrates a genuine race condition, where userspace will read the old/invalid property value/state. So this patch masks away the WARNING without addressing the actual issue. Instead can we fix the respective drivers, so that no properties are created after drm_dev_register()?
Longer version: As we look into drm_dev_register() it's in charge of creating the dev/sysfs nodes (et al). Note that connectors cannot disappear at runtime. For panel orientation, we are creating an immutable connector properly, meaning that as soon as drm_dev_register() is called we must ensure that the property is available (if applicable) and set to the correct value.
For illustration, consider the following scenario: - DRM modules are loaded late - are not built-in and not part of initrd (or there's no initrd) - kernel boots - plymouth/similar user-space component kicks in before the driver/module is loaded - module gets loaded, drm_dev_register() kicks in populating /dev/dri/card0 - plymouth opens the dev node and reads DRM_MODE_PANEL_ORIENTATION_UNKNOWN - module updates the orientation property
Thanks Emil
On Tue, Feb 15, 2022 at 8:04 PM Emil Velikov emil.l.velikov@gmail.com wrote:
Greetings everyone,
Padron for joining in so late o/
On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang hsinyi@chromium.org wrote:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
The warning illustrates a genuine race condition, where userspace will read the old/invalid property value/state. So this patch masks away the WARNING without addressing the actual issue. Instead can we fix the respective drivers, so that no properties are created after drm_dev_register()?
1. How about the proposal in previous version: v7 https://patchwork.kernel.org/project/linux-mediatek/patch/20220208073714.154... we separate property creation (drm_connector_init_panel_orientation_property) and value setting (drm_connector_set_panel_orientation). This is also similar to some of other optional properties are created, eg. vrr_capable.
And drm drivers have to make sure that if they want to use this property, they have to create it before drm_dev_register(). For example, in the 2nd patch, mtk_drm sets the property before calling drm_dev_register().
2. I'm not sure how to handle the case that if user space tries to read the property before the proper value is set. Currently drm creates this property and the panels[1] will set the correct value parsed from DT. If userspace calls before the panel sets the correct value, it will get unknown (similar to the illustration you mentioned below). Do you think that the drm should be responsible for parsing the value if the panel provides it? In this way it's guaranteed that the value is set when the property is created.
[1] https://elixir.bootlin.com/linux/latest/A/ident/drm_connector_set_panel_orie...
Longer version: As we look into drm_dev_register() it's in charge of creating the dev/sysfs nodes (et al). Note that connectors cannot disappear at runtime. For panel orientation, we are creating an immutable connector properly, meaning that as soon as drm_dev_register() is called we must ensure that the property is available (if applicable) and set to the correct value.
For illustration, consider the following scenario:
- DRM modules are loaded late - are not built-in and not part of
initrd (or there's no initrd)
- kernel boots
- plymouth/similar user-space component kicks in before the
driver/module is loaded
- module gets loaded, drm_dev_register() kicks in populating /dev/dri/card0
- plymouth opens the dev node and reads DRM_MODE_PANEL_ORIENTATION_UNKNOWN
- module updates the orientation property
Thanks Emil
On Tuesday, February 15th, 2022 at 13:04, Emil Velikov emil.l.velikov@gmail.com wrote:
Greetings everyone,
Padron for joining in so late o/
On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang hsinyi@chromium.org wrote:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
The warning illustrates a genuine race condition, where userspace will read the old/invalid property value/state. So this patch masks away the WARNING without addressing the actual issue. Instead can we fix the respective drivers, so that no properties are created after drm_dev_register()?
Longer version: As we look into drm_dev_register() it's in charge of creating the dev/sysfs nodes (et al). Note that connectors cannot disappear at runtime. For panel orientation, we are creating an immutable connector properly, meaning that as soon as drm_dev_register() is called we must ensure that the property is available (if applicable) and set to the correct value.
Unfortunately we can't quite do this. To apply the panel orientation quirks we need to grab the EDID of the eDP connector, and this happened too late in my testing.
What we can do is create the prop early during module load, and update it when we read the EDID (at the place where we create it right now). User-space will receive a hotplug event after the EDID is read, so will be able to pick up the new value if any.
On Tue, 15 Feb 2022 at 13:55, Simon Ser contact@emersion.fr wrote:
On Tuesday, February 15th, 2022 at 13:04, Emil Velikov emil.l.velikov@gmail.com wrote:
Greetings everyone,
Padron for joining in so late o/
On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang hsinyi@chromium.org wrote:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
The warning illustrates a genuine race condition, where userspace will read the old/invalid property value/state. So this patch masks away the WARNING without addressing the actual issue. Instead can we fix the respective drivers, so that no properties are created after drm_dev_register()?
Longer version: As we look into drm_dev_register() it's in charge of creating the dev/sysfs nodes (et al). Note that connectors cannot disappear at runtime. For panel orientation, we are creating an immutable connector properly, meaning that as soon as drm_dev_register() is called we must ensure that the property is available (if applicable) and set to the correct value.
Unfortunately we can't quite do this. To apply the panel orientation quirks we need to grab the EDID of the eDP connector, and this happened too late in my testing.
What we can do is create the prop early during module load, and update it when we read the EDID (at the place where we create it right now). User-space will receive a hotplug event after the EDID is read, so will be able to pick up the new value if any.
Didn't quite get that, are you saying that a GETPROPERTY for the EDID, the ioctl blocks or that we get an empty EDID?
The EDID hotplug even thing is neat - sounds like it also signals on panel orientation, correct? On such an event, which properties userspace should be re-fetching - everything or guess randomly?
Looking through the documentation, I cannot see a clear answer :-\
Thanks Emil
On Tuesday, February 15th, 2022 at 15:38, Emil Velikov emil.l.velikov@gmail.com wrote:
On Tue, 15 Feb 2022 at 13:55, Simon Ser contact@emersion.fr wrote:
On Tuesday, February 15th, 2022 at 13:04, Emil Velikov emil.l.velikov@gmail.com wrote:
Greetings everyone,
Padron for joining in so late o/
On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang hsinyi@chromium.org wrote:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
The warning illustrates a genuine race condition, where userspace will read the old/invalid property value/state. So this patch masks away the WARNING without addressing the actual issue. Instead can we fix the respective drivers, so that no properties are created after drm_dev_register()?
Longer version: As we look into drm_dev_register() it's in charge of creating the dev/sysfs nodes (et al). Note that connectors cannot disappear at runtime. For panel orientation, we are creating an immutable connector properly, meaning that as soon as drm_dev_register() is called we must ensure that the property is available (if applicable) and set to the correct value.
Unfortunately we can't quite do this. To apply the panel orientation quirks we need to grab the EDID of the eDP connector, and this happened too late in my testing.
What we can do is create the prop early during module load, and update it when we read the EDID (at the place where we create it right now). User-space will receive a hotplug event after the EDID is read, so will be able to pick up the new value if any.
Didn't quite get that, are you saying that a GETPROPERTY for the EDID, the ioctl blocks or that we get an empty EDID?
I'm not referring to GETPROPERTY, I'm referring to the driver getting the EDID from the sink (here, the eDP panel). In my experimentations with amdgpu I noticed that the driver module load finished before the EDID was available to the driver. Maybe other drivers behave differently and probe connectors when loaded, not sure.
The EDID hotplug even thing is neat - sounds like it also signals on panel orientation, correct? On such an event, which properties userspace should be re-fetching - everything or guess randomly?
Looking through the documentation, I cannot see a clear answer :-\
User-space should re-fetch *all* properties. In practice some user-space may only be fetching some properties, but that should get fixed in user-space.
Also the kernel can indicate that only a single connector changed via the "CONNECTOR" uevent prop, or even a single connector property via "PROPERTY". See [1] for a user-space implementation. But all of this is purely an optional optimization. Re-fetching all properties is a bit slower (especially if some drmModeGetConnector calls force-probe connectors) but works perfectly fine.
It would be nice to document, if you have the time feel free to send a patch and CC danvet, pq and me.
[1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/252b2348bd62170d97c4e8...
On Tue, 15 Feb 2022 at 16:37, Simon Ser contact@emersion.fr wrote:
On Tuesday, February 15th, 2022 at 15:38, Emil Velikov emil.l.velikov@gmail.com wrote:
On Tue, 15 Feb 2022 at 13:55, Simon Ser contact@emersion.fr wrote:
On Tuesday, February 15th, 2022 at 13:04, Emil Velikov emil.l.velikov@gmail.com wrote:
Greetings everyone,
Padron for joining in so late o/
On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang hsinyi@chromium.org wrote:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
The warning illustrates a genuine race condition, where userspace will read the old/invalid property value/state. So this patch masks away the WARNING without addressing the actual issue. Instead can we fix the respective drivers, so that no properties are created after drm_dev_register()?
Longer version: As we look into drm_dev_register() it's in charge of creating the dev/sysfs nodes (et al). Note that connectors cannot disappear at runtime. For panel orientation, we are creating an immutable connector properly, meaning that as soon as drm_dev_register() is called we must ensure that the property is available (if applicable) and set to the correct value.
Unfortunately we can't quite do this. To apply the panel orientation quirks we need to grab the EDID of the eDP connector, and this happened too late in my testing.
What we can do is create the prop early during module load, and update it when we read the EDID (at the place where we create it right now). User-space will receive a hotplug event after the EDID is read, so will be able to pick up the new value if any.
Didn't quite get that, are you saying that a GETPROPERTY for the EDID, the ioctl blocks or that we get an empty EDID?
I'm not referring to GETPROPERTY, I'm referring to the driver getting the EDID from the sink (here, the eDP panel). In my experimentations with amdgpu I noticed that the driver module load finished before the EDID was available to the driver. Maybe other drivers behave differently and probe connectors when loaded, not sure.
I see thanks.
The EDID hotplug even thing is neat - sounds like it also signals on panel orientation, correct? On such an event, which properties userspace should be re-fetching - everything or guess randomly?
Looking through the documentation, I cannot see a clear answer :-\
User-space should re-fetch *all* properties. In practice some user-space may only be fetching some properties, but that should get fixed in user-space.
Also the kernel can indicate that only a single connector changed via the "CONNECTOR" uevent prop, or even a single connector property via "PROPERTY". See [1] for a user-space implementation. But all of this is purely an optional optimization. Re-fetching all properties is a bit slower (especially if some drmModeGetConnector calls force-probe connectors) but works perfectly fine.
Looking at KDE/kwin (the one I'm running) - doesn't seem like it handles any of the three "HOTPLUG", "PROPERTY" or "CONNECTOR" uevents :-\ Skimming through GNOME/mutter - it handles "HOTPLUG", but not the optional ones.
Guess we're in the clear wrt potential races, even though the documentation on the topic is lacklustre.
It would be nice to document, if you have the time feel free to send a patch and CC danvet, pq and me.
Doubt I will have the time in the upcoming weeks, but I'll add it to my ever-growing TODO list :-P
Thanks Emil
Hi,
Sorry for jumping in in the middle of the thread I did not notice this thread before.
On 2/16/22 13:00, Emil Velikov wrote:
On Tue, 15 Feb 2022 at 16:37, Simon Ser contact@emersion.fr wrote:
On Tuesday, February 15th, 2022 at 15:38, Emil Velikov emil.l.velikov@gmail.com wrote:
On Tue, 15 Feb 2022 at 13:55, Simon Ser contact@emersion.fr wrote:
On Tuesday, February 15th, 2022 at 13:04, Emil Velikov emil.l.velikov@gmail.com wrote:
Greetings everyone,
Padron for joining in so late o/
On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang hsinyi@chromium.org wrote:
drm_dev_register() sets connector->registration_state to DRM_CONNECTOR_REGISTERED and dev->registered to true. If drm_connector_set_panel_orientation() is first called after drm_dev_register(), it will fail several checks and results in following warning.
Add a function to create panel orientation property and set default value to UNKNOWN, so drivers can call this function to init the property earlier , and let the panel set the real value later.
The warning illustrates a genuine race condition, where userspace will read the old/invalid property value/state. So this patch masks away the WARNING without addressing the actual issue. Instead can we fix the respective drivers, so that no properties are created after drm_dev_register()?
Longer version: As we look into drm_dev_register() it's in charge of creating the dev/sysfs nodes (et al). Note that connectors cannot disappear at runtime. For panel orientation, we are creating an immutable connector properly, meaning that as soon as drm_dev_register() is called we must ensure that the property is available (if applicable) and set to the correct value.
Unfortunately we can't quite do this. To apply the panel orientation quirks we need to grab the EDID of the eDP connector, and this happened too late in my testing.
What we can do is create the prop early during module load, and update it when we read the EDID (at the place where we create it right now). User-space will receive a hotplug event after the EDID is read, so will be able to pick up the new value if any.
Didn't quite get that, are you saying that a GETPROPERTY for the EDID, the ioctl blocks or that we get an empty EDID?
I'm not referring to GETPROPERTY, I'm referring to the driver getting the EDID from the sink (here, the eDP panel). In my experimentations with amdgpu I noticed that the driver module load finished before the EDID was available to the driver. Maybe other drivers behave differently and probe connectors when loaded, not sure.
I see thanks.
The EDID hotplug even thing is neat - sounds like it also signals on panel orientation, correct? On such an event, which properties userspace should be re-fetching - everything or guess randomly?
Looking through the documentation, I cannot see a clear answer :-\
User-space should re-fetch *all* properties. In practice some user-space may only be fetching some properties, but that should get fixed in user-space.
Also the kernel can indicate that only a single connector changed via the "CONNECTOR" uevent prop, or even a single connector property via "PROPERTY". See [1] for a user-space implementation. But all of this is purely an optional optimization. Re-fetching all properties is a bit slower (especially if some drmModeGetConnector calls force-probe connectors) but works perfectly fine
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
1. amdgpu exports a connector for the LCD panel to userspace without setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down 3. amdgpu adjusts the panel-orient prop to upside-down, sends out uevents 4. Lets assume plymouth handles this well (i) and now adjust its rendering and renders the next frame of the bootsplash 180° rotated to compensate for the panel being upside down. Then from now on the user will see the splash normally
So this means that the user will briefly see the bootsplash rendered upside down which IMHO is not acceptable behavior. Also see my footnote about how I seriously doubt plymouth will see the panel-orient change at all.
I'm also a bit unsure about:
a) How you can register the panel connector with userspace before reading the edid, don't you need the edid to give the physical size + modeline to userspace, which you cannot just leave out ?
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
Regards,
Hans
i) I don't think plymouth will handle this well though, since it tries to skip unchanged connectors and I believe it only checks the crtc routing + preferred modeline to determine "unchanged".
On Friday, February 18th, 2022 at 11:38, Hans de Goede hdegoede@redhat.com wrote:
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
- amdgpu exports a connector for the LCD panel to userspace without
setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down
At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything.
- amdgpu adjusts the panel-orient prop to upside-down, sends out
uevents
That's when amdgpu marks the connector as connected. So everything should be fine I believe, no bad frame.
- Lets assume plymouth handles this well (i) and now adjust its
rendering and renders the next frame of the bootsplash 180° rotated to compensate for the panel being upside down. Then from now on the user will see the splash normally
So this means that the user will briefly see the bootsplash rendered upside down which IMHO is not acceptable behavior. Also see my footnote about how I seriously doubt plymouth will see the panel-orient change at all.
I'm also a bit unsure about:
a) How you can register the panel connector with userspace before reading the edid, don't you need the edid to give the physical size + modeline to userspace, which you cannot just leave out ?
Yup. The KMS EDID property is created before the EDID is read, and is set to zero (NULL blob). The width/height in mm and other info are also zero. You can try inspecting the state printed by drm_info on any disconnected connector to see for yourself.
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
This is an internal DRM thing. The orientation quirks logic uses the mode size advertised by the EDID. I agree that at least in the Steam Deck case it may not make a lot of sense to use any info from the EDID, but that's needed for the current status quo.
Also note, DisplayID has a bit to indicate the panel orientation IIRC. Would be nice to support parsing this at some point.
Hi,
On 2/18/22 12:39, Simon Ser wrote:
On Friday, February 18th, 2022 at 11:38, Hans de Goede hdegoede@redhat.com wrote:
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
- amdgpu exports a connector for the LCD panel to userspace without
setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down
At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything.
If before the initial probe of the connector there is a /dev/dri/card0 which plymouth can access, then plymouth may at this point decide to disable any seemingly unused crtcs, which will make the screen go black...
I'm not sure if plymouth will actually do this, but AFAICT this would not be invalid behavior for a userspace kms consumer to do and I believe it is likely that mutter will disable unused crtcs.
IMHO it is just a bad idea to register /dev/dri/card0 with userspace before the initial connector probe is done. Nothing good can come of that.
If all the exposed connectors initially are going to show up as disconnected anyways what is the value in registering /dev/dri/card0 with userspace early ?
- amdgpu adjusts the panel-orient prop to upside-down, sends out
uevents
That's when amdgpu marks the connector as connected. So everything should be fine I believe, no bad frame.
See above.
- Lets assume plymouth handles this well (i) and now adjust its
rendering and renders the next frame of the bootsplash 180° rotated to compensate for the panel being upside down. Then from now on the user will see the splash normally
So this means that the user will briefly see the bootsplash rendered upside down which IMHO is not acceptable behavior. Also see my footnote about how I seriously doubt plymouth will see the panel-orient change at all.
I'm also a bit unsure about:
a) How you can register the panel connector with userspace before reading the edid, don't you need the edid to give the physical size + modeline to userspace, which you cannot just leave out ?
Yup. The KMS EDID property is created before the EDID is read, and is set to zero (NULL blob). The width/height in mm and other info are also zero. You can try inspecting the state printed by drm_info on any disconnected connector to see for yourself.
Right, I missed the detail hat the connector is initially marked as disconnected. That solves the issue of invalid panel-orient / mode / dpi info, bit it opens up other problems.
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
This is an internal DRM thing. The orientation quirks logic uses the mode size advertised by the EDID.
The DMI based quirking does, yes. But e.g. the quirk code directly reading this from the Intel VBT does not rely on the mode.
But if you are planning on using a DMI based quirk for the steamdeck then yes that needs the mode.
Thee mode check is there for 2 reasons:
1. To avoid also applying the quirk to external displays, but I think that that is also solved in most drivers by only checking for a quirk at all on the eDP connector
2. Some laptop models ship with different panels in different badges some of these are portrait (so need a panel-orient) setting and others are landscape.
I agree that at least in the Steam Deck case it may not make a lot of sense to use any info from the EDID, but that's needed for the current status quo.
We could extend the DMI quirk mechanism to allow quirks which don't do the mode check, for use on devices where we can guarantee neither 1 nor 2 happens, then amdgpu could call the quirk code early simply passing 0x0 as resolution.
Also note, DisplayID has a bit to indicate the panel orientation IIRC. Would be nice to support parsing this at some point.
Ack.
Regards,
Hans
On Friday, February 18th, 2022 at 12:54, Hans de Goede hdegoede@redhat.com wrote:
On 2/18/22 12:39, Simon Ser wrote:
On Friday, February 18th, 2022 at 11:38, Hans de Goede hdegoede@redhat.com wrote:
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
- amdgpu exports a connector for the LCD panel to userspace without
setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down
At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything.
If before the initial probe of the connector there is a /dev/dri/card0 which plymouth can access, then plymouth may at this point decide to disable any seemingly unused crtcs, which will make the screen go black...
I'm not sure if plymouth will actually do this, but AFAICT this would not be invalid behavior for a userspace kms consumer to do and I believe it is likely that mutter will disable unused crtcs.
IMHO it is just a bad idea to register /dev/dri/card0 with userspace before the initial connector probe is done. Nothing good can come of that.
If all the exposed connectors initially are going to show up as disconnected anyways what is the value in registering /dev/dri/card0 with userspace early ?
OK. I'm still unsure how I feel about this, but I think I agree with you. That said, the amdgpu architecture is quite involved with multiple abstraction levels, so I don't think I'm equipped to write a patch to fix this...
cc Daniel Vetter: can you confirm probing all connectors is a good thing to do on driver module load?
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
This is an internal DRM thing. The orientation quirks logic uses the mode size advertised by the EDID.
The DMI based quirking does, yes. But e.g. the quirk code directly reading this from the Intel VBT does not rely on the mode.
But if you are planning on using a DMI based quirk for the steamdeck then yes that needs the mode.
Thee mode check is there for 2 reasons:
- To avoid also applying the quirk to external displays, but
I think that that is also solved in most drivers by only checking for a quirk at all on the eDP connector
- Some laptop models ship with different panels in different badges
some of these are portrait (so need a panel-orient) setting and others are landscape.
That makes sense. So yeah the EDID mode based matching logic needs to stay to accomodate for these cases.
I agree that at least in the Steam Deck case it may not make a lot of sense to use any info from the EDID, but that's needed for the current status quo.
We could extend the DMI quirk mechanism to allow quirks which don't do the mode check, for use on devices where we can guarantee neither 1 nor 2 happens, then amdgpu could call the quirk code early simply passing 0x0 as resolution.
Yeah. But per the above amdgpu should maybe probe connectors on module load. If/when amdgpu is fixed to do this, then we don't need to disable the mode matching logic in panel-orientation quirks anymore.
On Fri, Feb 18, 2022 at 7:13 AM Simon Ser contact@emersion.fr wrote:
On Friday, February 18th, 2022 at 12:54, Hans de Goede hdegoede@redhat.com wrote:
On 2/18/22 12:39, Simon Ser wrote:
On Friday, February 18th, 2022 at 11:38, Hans de Goede hdegoede@redhat.com wrote:
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
- amdgpu exports a connector for the LCD panel to userspace without
setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down
At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything.
If before the initial probe of the connector there is a /dev/dri/card0 which plymouth can access, then plymouth may at this point decide to disable any seemingly unused crtcs, which will make the screen go black...
I'm not sure if plymouth will actually do this, but AFAICT this would not be invalid behavior for a userspace kms consumer to do and I believe it is likely that mutter will disable unused crtcs.
IMHO it is just a bad idea to register /dev/dri/card0 with userspace before the initial connector probe is done. Nothing good can come of that.
If all the exposed connectors initially are going to show up as disconnected anyways what is the value in registering /dev/dri/card0 with userspace early ?
OK. I'm still unsure how I feel about this, but I think I agree with you. That said, the amdgpu architecture is quite involved with multiple abstraction levels, so I don't think I'm equipped to write a patch to fix this...
cc Daniel Vetter: can you confirm probing all connectors is a good thing to do on driver module load?
I don't think it's a big deal to change, but at least my understanding, albeit this was back in the early KMS days, was that probing was driven by things outside of the driver. I.e., there is no need to probe displays if nothing is going to use them. If you want to use the displays, you'd call probe first before trying to use them so you know what is available.
Alex
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
This is an internal DRM thing. The orientation quirks logic uses the mode size advertised by the EDID.
The DMI based quirking does, yes. But e.g. the quirk code directly reading this from the Intel VBT does not rely on the mode.
But if you are planning on using a DMI based quirk for the steamdeck then yes that needs the mode.
Thee mode check is there for 2 reasons:
- To avoid also applying the quirk to external displays, but
I think that that is also solved in most drivers by only checking for a quirk at all on the eDP connector
- Some laptop models ship with different panels in different badges
some of these are portrait (so need a panel-orient) setting and others are landscape.
That makes sense. So yeah the EDID mode based matching logic needs to stay to accomodate for these cases.
I agree that at least in the Steam Deck case it may not make a lot of sense to use any info from the EDID, but that's needed for the current status quo.
We could extend the DMI quirk mechanism to allow quirks which don't do the mode check, for use on devices where we can guarantee neither 1 nor 2 happens, then amdgpu could call the quirk code early simply passing 0x0 as resolution.
Yeah. But per the above amdgpu should maybe probe connectors on module load. If/when amdgpu is fixed to do this, then we don't need to disable the mode matching logic in panel-orientation quirks anymore.
On 2022-02-18 07:12, Simon Ser wrote:
On Friday, February 18th, 2022 at 12:54, Hans de Goede hdegoede@redhat.com wrote:
On 2/18/22 12:39, Simon Ser wrote:
On Friday, February 18th, 2022 at 11:38, Hans de Goede hdegoede@redhat.com wrote:
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
- amdgpu exports a connector for the LCD panel to userspace without
setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down
At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything.
If before the initial probe of the connector there is a /dev/dri/card0 which plymouth can access, then plymouth may at this point decide to disable any seemingly unused crtcs, which will make the screen go black...
I'm not sure if plymouth will actually do this, but AFAICT this would not be invalid behavior for a userspace kms consumer to do and I believe it is likely that mutter will disable unused crtcs.
IMHO it is just a bad idea to register /dev/dri/card0 with userspace before the initial connector probe is done. Nothing good can come of that.
If all the exposed connectors initially are going to show up as disconnected anyways what is the value in registering /dev/dri/card0 with userspace early ?
OK. I'm still unsure how I feel about this, but I think I agree with you. That said, the amdgpu architecture is quite involved with multiple abstraction levels, so I don't think I'm equipped to write a patch to fix this...
amdgpu_dm's connector registration already triggers a detection. See the calls to dc_link_detect and amdgpu_dm_update_connector_after_detect in amdgpu_dm_initialize_drm_device.
dc_link_detect is supposed to read the edid via dm_helpers_read_local_edid and amdgpu_dm_update_connector_after_detect will update the EDID on the connector via a drm_connector_update_edid_property call.
This all happens at driver load.
I don't know why you're seeing the embedded connector as disconnected unless the DP-MIPI bridge for some reason doesn't indicate that the panel is connected at driver load.
Harry
cc Daniel Vetter: can you confirm probing all connectors is a good thing to do on driver module load?
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
This is an internal DRM thing. The orientation quirks logic uses the mode size advertised by the EDID.
The DMI based quirking does, yes. But e.g. the quirk code directly reading this from the Intel VBT does not rely on the mode.
But if you are planning on using a DMI based quirk for the steamdeck then yes that needs the mode.
Thee mode check is there for 2 reasons:
- To avoid also applying the quirk to external displays, but
I think that that is also solved in most drivers by only checking for a quirk at all on the eDP connector
- Some laptop models ship with different panels in different badges
some of these are portrait (so need a panel-orient) setting and others are landscape.
That makes sense. So yeah the EDID mode based matching logic needs to stay to accomodate for these cases.
I agree that at least in the Steam Deck case it may not make a lot of sense to use any info from the EDID, but that's needed for the current status quo.
We could extend the DMI quirk mechanism to allow quirks which don't do the mode check, for use on devices where we can guarantee neither 1 nor 2 happens, then amdgpu could call the quirk code early simply passing 0x0 as resolution.
Yeah. But per the above amdgpu should maybe probe connectors on module load. If/when amdgpu is fixed to do this, then we don't need to disable the mode matching logic in panel-orientation quirks anymore.
On Fri, Feb 18, 2022 at 11:57 PM Harry Wentland harry.wentland@amd.com wrote:
On 2022-02-18 07:12, Simon Ser wrote:
On Friday, February 18th, 2022 at 12:54, Hans de Goede hdegoede@redhat.com wrote:
On 2/18/22 12:39, Simon Ser wrote:
On Friday, February 18th, 2022 at 11:38, Hans de Goede hdegoede@redhat.com wrote:
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
- amdgpu exports a connector for the LCD panel to userspace without
setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down
At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything.
If before the initial probe of the connector there is a /dev/dri/card0 which plymouth can access, then plymouth may at this point decide to disable any seemingly unused crtcs, which will make the screen go black...
I'm not sure if plymouth will actually do this, but AFAICT this would not be invalid behavior for a userspace kms consumer to do and I believe it is likely that mutter will disable unused crtcs.
IMHO it is just a bad idea to register /dev/dri/card0 with userspace before the initial connector probe is done. Nothing good can come of that.
If all the exposed connectors initially are going to show up as disconnected anyways what is the value in registering /dev/dri/card0 with userspace early ?
OK. I'm still unsure how I feel about this, but I think I agree with you. That said, the amdgpu architecture is quite involved with multiple abstraction levels, so I don't think I'm equipped to write a patch to fix this...
amdgpu_dm's connector registration already triggers a detection. See the calls to dc_link_detect and amdgpu_dm_update_connector_after_detect in amdgpu_dm_initialize_drm_device.
dc_link_detect is supposed to read the edid via dm_helpers_read_local_edid and amdgpu_dm_update_connector_after_detect will update the EDID on the connector via a drm_connector_update_edid_property call.
This all happens at driver load.
I don't know why you're seeing the embedded connector as disconnected unless the DP-MIPI bridge for some reason doesn't indicate that the panel is connected at driver load.
Harry
cc Daniel Vetter: can you confirm probing all connectors is a good thing to do on driver module load?
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
This is an internal DRM thing. The orientation quirks logic uses the mode size advertised by the EDID.
The DMI based quirking does, yes. But e.g. the quirk code directly reading this from the Intel VBT does not rely on the mode.
But if you are planning on using a DMI based quirk for the steamdeck then yes that needs the mode.
Thee mode check is there for 2 reasons:
- To avoid also applying the quirk to external displays, but
I think that that is also solved in most drivers by only checking for a quirk at all on the eDP connector
- Some laptop models ship with different panels in different badges
some of these are portrait (so need a panel-orient) setting and others are landscape.
That makes sense. So yeah the EDID mode based matching logic needs to stay to accomodate for these cases.
I agree that at least in the Steam Deck case it may not make a lot of sense to use any info from the EDID, but that's needed for the current status quo.
We could extend the DMI quirk mechanism to allow quirks which don't do the mode check, for use on devices where we can guarantee neither 1 nor 2 happens, then amdgpu could call the quirk code early simply passing 0x0 as resolution.
Yeah. But per the above amdgpu should maybe probe connectors on module load. If/when amdgpu is fixed to do this, then we don't need to disable the mode matching logic in panel-orientation quirks anymore.
Hi all,
Thanks for all of the discussion. I'm not sure about how amd drm works, but for some SoC, the panel orientation is set in panel[1]. The goal of this patch is to separate the property creation, so some drm can optionally create it earlier before drm_dev_register().
I've sent the v9 to address some issues in v8, but the basic idea is still the same. It has no effect to drm_connector_set_panel_orientation_with_quirk() used in amdgpu and i915, they work the same as before. Do you think this is reasonable?
[1] https://elixir.bootlin.com/linux/v5.17-rc7/source/drivers/gpu/drm/panel/pane...
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Emil Velikov -
Gabriel Krisman Bertazi -
Hans de Goede -
Harry Wentland -
Hsin-Yi Wang -
Simon Ser