| Message ID | 20220208084234.1684930-1-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v8,1/3] gpu: drm: separate panel orientation property creating and value setting | expand |
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);
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.1540390-1-hsinyi@chromium.org/ 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_orientation > 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 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
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/252b2348bd62170d97c4e81fb2050f757b56d67e/backend/session/session.c#L144
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: > > 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 At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything. > 3. 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. > 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 ? 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: >> >> 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 > > 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 ? >> 3. 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. >> 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 ? > > 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: > >> > >> 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 > > > > 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: > > 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. 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: > > >> > > >> 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 > > > > > > 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: > > > > 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. > > 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: >>>> >>>> 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 >>> >>> 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: >> >> 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. > > 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: > >>>> > >>>> 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 > >>> > >>> 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: > >> > >> 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. > > > > 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/panel-edp.c#L556
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);