[PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Simon Ser]

This reverts commit 981f09295687f856d5345e19c7084aca481c1395.

It turns out this breaks Mutter.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
---
 drivers/gpu/drm/drm_mode_config.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 939d621c9ad4..688c8afe0bf1 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	count = 0;
 	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
 	drm_for_each_connector_iter(connector, &conn_iter) {
-		if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
-			continue;
-
 		/* only expose writeback connectors if userspace understands them */
 		if (!file_priv->writeback_connectors &&
 		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
-- 
2.38.0

[PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Simon Ser]

A typical DP-MST unplug removes a KMS connector. However care must
be taken to properly synchronize with user-space. The expected
sequence of events is the following:

1. The kernel notices that the DP-MST port is gone.
2. The kernel marks the connector as disconnected, then sends a
   uevent to make user-space re-scan the connector list.
3. User-space notices the connector goes from connected to disconnected,
   disables it.
4. Kernel handles the the IOCTL disabling the connector. On success,
   the very last reference to the struct drm_connector is dropped and
   drm_connector_cleanup() is called.
5. The connector is removed from the list, and a uevent is sent to tell
   user-space that the connector disappeared.

The very last step was missing. As a result, user-space thought the
connector still existed and could try to disable it again. Since the
kernel no longer knows about the connector, that would end up with
EINVAL and confused user-space.

Fix this by sending a hotplug uevent from drm_connector_cleanup().

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: stable@vger.kernel.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
---
 drivers/gpu/drm/drm_connector.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index e3142c8142b3..90dad87e9ad0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -582,6 +582,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	mutex_destroy(&connector->mutex);
 
 	memset(connector, 0, sizeof(*connector));
+
+	if (dev->registered)
+		drm_sysfs_hotplug_event(dev);
 }
 EXPORT_SYMBOL(drm_connector_cleanup);
 
-- 
2.38.0

[PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Simon Ser]

A typical DP-MST unplug removes a KMS connector. However care must
be taken to properly synchronize with user-space. The expected
sequence of events is the following:

1. The kernel notices that the DP-MST port is gone.
2. The kernel marks the connector as disconnected, then sends a
   uevent to make user-space re-scan the connector list.
3. User-space notices the connector goes from connected to disconnected,
   disables it.
4. Kernel handles the the IOCTL disabling the connector. On success,
   the very last reference to the struct drm_connector is dropped and
   drm_connector_cleanup() is called.
5. The connector is removed from the list, and a uevent is sent to tell
   user-space that the connector disappeared.

The very last step was missing. As a result, user-space thought the
connector still existed and could try to disable it again. Since the
kernel no longer knows about the connector, that would end up with
EINVAL and confused user-space.

Fix this by sending a hotplug uevent from drm_connector_cleanup().

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: stable@vger.kernel.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Jonas Ådahl <jadahl@redhat.com>
---
 drivers/gpu/drm/drm_connector.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index e3142c8142b3..90dad87e9ad0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -582,6 +582,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	mutex_destroy(&connector->mutex);
 
 	memset(connector, 0, sizeof(*connector));
+
+	if (dev->registered)
+		drm_sysfs_hotplug_event(dev);
 }
 EXPORT_SYMBOL(drm_connector_cleanup);
 
-- 
2.38.0

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Jonas Ådahl]

On Mon, Oct 17, 2022 at 03:32:01PM +0000, Simon Ser wrote:
> A typical DP-MST unplug removes a KMS connector. However care must
> be taken to properly synchronize with user-space. The expected
> sequence of events is the following:
> 
> 1. The kernel notices that the DP-MST port is gone.
> 2. The kernel marks the connector as disconnected, then sends a
>    uevent to make user-space re-scan the connector list.
> 3. User-space notices the connector goes from connected to disconnected,
>    disables it.
> 4. Kernel handles the the IOCTL disabling the connector. On success,
>    the very last reference to the struct drm_connector is dropped and
>    drm_connector_cleanup() is called.
> 5. The connector is removed from the list, and a uevent is sent to tell
>    user-space that the connector disappeared.
> 
> The very last step was missing. As a result, user-space thought the
> connector still existed and could try to disable it again. Since the
> kernel no longer knows about the connector, that would end up with
> EINVAL and confused user-space.
> 
> Fix this by sending a hotplug uevent from drm_connector_cleanup().
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>

Tested-by: Jonas Ådahl <jadahl@redhat.com>


Jonas

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Jonas Ådahl]

On Mon, Oct 17, 2022 at 03:32:01PM +0000, Simon Ser wrote:
> A typical DP-MST unplug removes a KMS connector. However care must
> be taken to properly synchronize with user-space. The expected
> sequence of events is the following:
> 
> 1. The kernel notices that the DP-MST port is gone.
> 2. The kernel marks the connector as disconnected, then sends a
>    uevent to make user-space re-scan the connector list.
> 3. User-space notices the connector goes from connected to disconnected,
>    disables it.
> 4. Kernel handles the the IOCTL disabling the connector. On success,
>    the very last reference to the struct drm_connector is dropped and
>    drm_connector_cleanup() is called.
> 5. The connector is removed from the list, and a uevent is sent to tell
>    user-space that the connector disappeared.
> 
> The very last step was missing. As a result, user-space thought the
> connector still existed and could try to disable it again. Since the
> kernel no longer knows about the connector, that would end up with
> EINVAL and confused user-space.
> 
> Fix this by sending a hotplug uevent from drm_connector_cleanup().
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>

Tested-by: Jonas Ådahl <jadahl@redhat.com>


Jonas

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Lyude Paul]

LGTM! Thank you for the help with this:

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Mon, 2022-10-17 at 15:32 +0000, Simon Ser wrote:
> A typical DP-MST unplug removes a KMS connector. However care must
> be taken to properly synchronize with user-space. The expected
> sequence of events is the following:
> 
> 1. The kernel notices that the DP-MST port is gone.
> 2. The kernel marks the connector as disconnected, then sends a
>    uevent to make user-space re-scan the connector list.
> 3. User-space notices the connector goes from connected to disconnected,
>    disables it.
> 4. Kernel handles the the IOCTL disabling the connector. On success,
>    the very last reference to the struct drm_connector is dropped and
>    drm_connector_cleanup() is called.
> 5. The connector is removed from the list, and a uevent is sent to tell
>    user-space that the connector disappeared.
> 
> The very last step was missing. As a result, user-space thought the
> connector still existed and could try to disable it again. Since the
> kernel no longer knows about the connector, that would end up with
> EINVAL and confused user-space.
> 
> Fix this by sending a hotplug uevent from drm_connector_cleanup().
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index e3142c8142b3..90dad87e9ad0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -582,6 +582,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	mutex_destroy(&connector->mutex);
>  
>  	memset(connector, 0, sizeof(*connector));
> +
> +	if (dev->registered)
> +		drm_sysfs_hotplug_event(dev);
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Lyude Paul]

LGTM! Thank you for the help with this:

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Mon, 2022-10-17 at 15:32 +0000, Simon Ser wrote:
> A typical DP-MST unplug removes a KMS connector. However care must
> be taken to properly synchronize with user-space. The expected
> sequence of events is the following:
> 
> 1. The kernel notices that the DP-MST port is gone.
> 2. The kernel marks the connector as disconnected, then sends a
>    uevent to make user-space re-scan the connector list.
> 3. User-space notices the connector goes from connected to disconnected,
>    disables it.
> 4. Kernel handles the the IOCTL disabling the connector. On success,
>    the very last reference to the struct drm_connector is dropped and
>    drm_connector_cleanup() is called.
> 5. The connector is removed from the list, and a uevent is sent to tell
>    user-space that the connector disappeared.
> 
> The very last step was missing. As a result, user-space thought the
> connector still existed and could try to disable it again. Since the
> kernel no longer knows about the connector, that would end up with
> EINVAL and confused user-space.
> 
> Fix this by sending a hotplug uevent from drm_connector_cleanup().
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index e3142c8142b3..90dad87e9ad0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -582,6 +582,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	mutex_destroy(&connector->mutex);
>  
>  	memset(connector, 0, sizeof(*connector));
> +
> +	if (dev->registered)
> +		drm_sysfs_hotplug_event(dev);
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Ville Syrjälä]

On Mon, Oct 17, 2022 at 03:31:57PM +0000, Simon Ser wrote:
> This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> 
> It turns out this breaks Mutter.

A bit more detail would be a good to help future archaeologists.

> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> ---
>  drivers/gpu/drm/drm_mode_config.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 939d621c9ad4..688c8afe0bf1 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  	count = 0;
>  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
> -		if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
> -			continue;
> -
>  		/* only expose writeback connectors if userspace understands them */
>  		if (!file_priv->writeback_connectors &&
>  		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> -- 
> 2.38.0
> 

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Ville Syrjälä]

On Mon, Oct 17, 2022 at 03:32:01PM +0000, Simon Ser wrote:
> A typical DP-MST unplug removes a KMS connector. However care must
> be taken to properly synchronize with user-space. The expected
> sequence of events is the following:
> 
> 1. The kernel notices that the DP-MST port is gone.
> 2. The kernel marks the connector as disconnected, then sends a
>    uevent to make user-space re-scan the connector list.
> 3. User-space notices the connector goes from connected to disconnected,
>    disables it.
> 4. Kernel handles the the IOCTL disabling the connector. On success,
>    the very last reference to the struct drm_connector is dropped and
>    drm_connector_cleanup() is called.
> 5. The connector is removed from the list, and a uevent is sent to tell
>    user-space that the connector disappeared.
> 
> The very last step was missing. As a result, user-space thought the
> connector still existed and could try to disable it again. Since the
> kernel no longer knows about the connector, that would end up with
> EINVAL and confused user-space.

So is the uevent sent by the mst delayed destroy work
useless now, or what is going on there?

> 
> Fix this by sending a hotplug uevent from drm_connector_cleanup().
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index e3142c8142b3..90dad87e9ad0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -582,6 +582,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	mutex_destroy(&connector->mutex);
>  
>  	memset(connector, 0, sizeof(*connector));
> +
> +	if (dev->registered)
> +		drm_sysfs_hotplug_event(dev);
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  
> -- 
> 2.38.0
> 

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Ville Syrjälä]

On Mon, Oct 17, 2022 at 03:32:01PM +0000, Simon Ser wrote:
> A typical DP-MST unplug removes a KMS connector. However care must
> be taken to properly synchronize with user-space. The expected
> sequence of events is the following:
> 
> 1. The kernel notices that the DP-MST port is gone.
> 2. The kernel marks the connector as disconnected, then sends a
>    uevent to make user-space re-scan the connector list.
> 3. User-space notices the connector goes from connected to disconnected,
>    disables it.
> 4. Kernel handles the the IOCTL disabling the connector. On success,
>    the very last reference to the struct drm_connector is dropped and
>    drm_connector_cleanup() is called.
> 5. The connector is removed from the list, and a uevent is sent to tell
>    user-space that the connector disappeared.
> 
> The very last step was missing. As a result, user-space thought the
> connector still existed and could try to disable it again. Since the
> kernel no longer knows about the connector, that would end up with
> EINVAL and confused user-space.

So is the uevent sent by the mst delayed destroy work
useless now, or what is going on there?

> 
> Fix this by sending a hotplug uevent from drm_connector_cleanup().
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index e3142c8142b3..90dad87e9ad0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -582,6 +582,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	mutex_destroy(&connector->mutex);
>  
>  	memset(connector, 0, sizeof(*connector));
> +
> +	if (dev->registered)
> +		drm_sysfs_hotplug_event(dev);
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
>  
> -- 
> 2.38.0
> 

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Simon Ser]

On Tuesday, October 18th, 2022 at 11:24, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Oct 17, 2022 at 03:32:01PM +0000, Simon Ser wrote:
> 
> > A typical DP-MST unplug removes a KMS connector. However care must
> > be taken to properly synchronize with user-space. The expected
> > sequence of events is the following:
> > 
> > 1. The kernel notices that the DP-MST port is gone.
> > 2. The kernel marks the connector as disconnected, then sends a
> > uevent to make user-space re-scan the connector list.
> > 3. User-space notices the connector goes from connected to disconnected,
> > disables it.
> > 4. Kernel handles the the IOCTL disabling the connector. On success,
> > the very last reference to the struct drm_connector is dropped and
> > drm_connector_cleanup() is called.
> > 5. The connector is removed from the list, and a uevent is sent to tell
> > user-space that the connector disappeared.
> > 
> > The very last step was missing. As a result, user-space thought the
> > connector still existed and could try to disable it again. Since the
> > kernel no longer knows about the connector, that would end up with
> > EINVAL and confused user-space.
> 
> So is the uevent sent by the mst delayed destroy work
> useless now, or what is going on there?

No, that one is still useful, step (2) above.

Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup

[Simon Ser]

On Tuesday, October 18th, 2022 at 11:24, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Oct 17, 2022 at 03:32:01PM +0000, Simon Ser wrote:
> 
> > A typical DP-MST unplug removes a KMS connector. However care must
> > be taken to properly synchronize with user-space. The expected
> > sequence of events is the following:
> > 
> > 1. The kernel notices that the DP-MST port is gone.
> > 2. The kernel marks the connector as disconnected, then sends a
> > uevent to make user-space re-scan the connector list.
> > 3. User-space notices the connector goes from connected to disconnected,
> > disables it.
> > 4. Kernel handles the the IOCTL disabling the connector. On success,
> > the very last reference to the struct drm_connector is dropped and
> > drm_connector_cleanup() is called.
> > 5. The connector is removed from the list, and a uevent is sent to tell
> > user-space that the connector disappeared.
> > 
> > The very last step was missing. As a result, user-space thought the
> > connector still existed and could try to disable it again. Since the
> > kernel no longer knows about the connector, that would end up with
> > EINVAL and confused user-space.
> 
> So is the uevent sent by the mst delayed destroy work
> useless now, or what is going on there?

No, that one is still useful, step (2) above.

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Jonas Ådahl]

On Tue, Oct 18, 2022 at 12:14:09PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 17, 2022 at 03:31:57PM +0000, Simon Ser wrote:
> > This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> > 
> > It turns out this breaks Mutter.
> 
> A bit more detail would be a good to help future archaeologists.

Perhaps a better explanation is

It turns out this causes logically active but disconnected MST display
port connectors to disappear from the drmModeGetResources() list,
meaning userspace is made to believe the connector is already disabled.

When userspace then attempts post a new mode set commit, if that commit
uses the same CRTC used to previously drive the disconnected connector,
it will fail as that CRTC is logically still tied to the disconnected
connector.

This was discovered by a bisecting docking station hot plugging
regression using mutter.


Jonas

> 
> > 
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Jonas Ådahl <jadahl@redhat.com>
> > ---
> >  drivers/gpu/drm/drm_mode_config.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 939d621c9ad4..688c8afe0bf1 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> >  	count = 0;
> >  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> >  	drm_for_each_connector_iter(connector, &conn_iter) {
> > -		if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
> > -			continue;
> > -
> >  		/* only expose writeback connectors if userspace understands them */
> >  		if (!file_priv->writeback_connectors &&
> >  		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> > -- 
> > 2.38.0
> > 
> 
> -- 
> Ville Syrjälä
> Intel
> 

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Ville Syrjälä]

On Tue, Oct 18, 2022 at 11:27:19AM +0200, Jonas Ådahl wrote:
> On Tue, Oct 18, 2022 at 12:14:09PM +0300, Ville Syrjälä wrote:
> > On Mon, Oct 17, 2022 at 03:31:57PM +0000, Simon Ser wrote:
> > > This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> > > 
> > > It turns out this breaks Mutter.
> > 
> > A bit more detail would be a good to help future archaeologists.
> 
> Perhaps a better explanation is
> 
> It turns out this causes logically active but disconnected MST display
> port connectors to disappear from the drmModeGetResources() list,

That was the whole point was it not? So I'd drop the
"it turns out" part.

> meaning userspace is made to believe the connector is already disabled.

That wording to me implies its a generic issue affecting all
userspace when so far it looks like only mutter is affected.
So apparently mutter (for some reason) assumes that the
connector has somehow magically been disabled by someone
else if it disappears from the list of resources?

> 
> When userspace then attempts post a new mode set commit, if that commit
> uses the same CRTC used to previously drive the disconnected connector,
> it will fail as that CRTC is logically still tied to the disconnected
> connector.
> 
> This was discovered by a bisecting docking station hot plugging
> regression using mutter.
> 
> 
> Jonas
> 
> > 
> > > 
> > > Signed-off-by: Simon Ser <contact@emersion.fr>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Jonas Ådahl <jadahl@redhat.com>
> > > ---
> > >  drivers/gpu/drm/drm_mode_config.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index 939d621c9ad4..688c8afe0bf1 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> > >  	count = 0;
> > >  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> > >  	drm_for_each_connector_iter(connector, &conn_iter) {
> > > -		if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
> > > -			continue;
> > > -
> > >  		/* only expose writeback connectors if userspace understands them */
> > >  		if (!file_priv->writeback_connectors &&
> > >  		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> > > -- 
> > > 2.38.0
> > > 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Jonas Ådahl]

On Tue, Oct 18, 2022 at 12:58:53PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 18, 2022 at 11:27:19AM +0200, Jonas Ådahl wrote:
> > On Tue, Oct 18, 2022 at 12:14:09PM +0300, Ville Syrjälä wrote:
> > > On Mon, Oct 17, 2022 at 03:31:57PM +0000, Simon Ser wrote:
> > > > This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> > > > 
> > > > It turns out this breaks Mutter.
> > > 
> > > A bit more detail would be a good to help future archaeologists.
> > 
> > Perhaps a better explanation is
> > 
> > It turns out this causes logically active but disconnected MST display
> > port connectors to disappear from the drmModeGetResources() list,
> 
> That was the whole point was it not? So I'd drop the
> "it turns out" part.
> 
> > meaning userspace is made to believe the connector is already disabled.
> 
> That wording to me implies its a generic issue affecting all
> userspace when so far it looks like only mutter is affected.

Maybe other userspace was? I only found out by testing drm-next, and
only tried using mutter when bisecting.

> So apparently mutter (for some reason) assumes that the
> connector has somehow magically been disabled by someone
> else if it disappears from the list of resources?

Mutter makes the assumption that connectors it can interact with are the
ones that drmModeGetResources() return - nothing magic about that.


Jonas

> 
> > 
> > When userspace then attempts post a new mode set commit, if that commit
> > uses the same CRTC used to previously drive the disconnected connector,
> > it will fail as that CRTC is logically still tied to the disconnected
> > connector.
> > 
> > This was discovered by a bisecting docking station hot plugging
> > regression using mutter.
> > 
> > 
> > Jonas
> > 
> > > 
> > > > 
> > > > Signed-off-by: Simon Ser <contact@emersion.fr>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Jonas Ådahl <jadahl@redhat.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_mode_config.c | 3 ---
> > > >  1 file changed, 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > > index 939d621c9ad4..688c8afe0bf1 100644
> > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> > > >  	count = 0;
> > > >  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> > > >  	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > -		if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
> > > > -			continue;
> > > > -
> > > >  		/* only expose writeback connectors if userspace understands them */
> > > >  		if (!file_priv->writeback_connectors &&
> > > >  		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> > > > -- 
> > > > 2.38.0
> > > > 
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > > 
> 
> -- 
> Ville Syrjälä
> Intel
> 

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Ville Syrjälä]

On Tue, Oct 18, 2022 at 12:07:43PM +0200, Jonas Ådahl wrote:
> On Tue, Oct 18, 2022 at 12:58:53PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 18, 2022 at 11:27:19AM +0200, Jonas Ådahl wrote:
> > > On Tue, Oct 18, 2022 at 12:14:09PM +0300, Ville Syrjälä wrote:
> > > > On Mon, Oct 17, 2022 at 03:31:57PM +0000, Simon Ser wrote:
> > > > > This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> > > > > 
> > > > > It turns out this breaks Mutter.
> > > > 
> > > > A bit more detail would be a good to help future archaeologists.
> > > 
> > > Perhaps a better explanation is
> > > 
> > > It turns out this causes logically active but disconnected MST display
> > > port connectors to disappear from the drmModeGetResources() list,
> > 
> > That was the whole point was it not? So I'd drop the
> > "it turns out" part.
> > 
> > > meaning userspace is made to believe the connector is already disabled.
> > 
> > That wording to me implies its a generic issue affecting all
> > userspace when so far it looks like only mutter is affected.
> 
> Maybe other userspace was? I only found out by testing drm-next, and
> only tried using mutter when bisecting.
> 
> > So apparently mutter (for some reason) assumes that the
> > connector has somehow magically been disabled by someone
> > else if it disappears from the list of resources?
> 
> Mutter makes the assumption that connectors it can interact with are the
> ones that drmModeGetResources() return

I agree that on the face of it that assumption does seem
perfectly reasonable.

> - nothing magic about that.

Well it's expecting a bit magic from the kernel if it decides
that it doesn't need to disable what it already enabled.
But I guess it's more of a case that the code just never
expected this specific situation to happen, and thus the
results are what they are.

I suppose the only concern with the change is what happens
when you replug something back in before the old stuff has
disappeared and you now have two connectors for the same
thing on the list. IIRC the ddxen at least try to reuse
the same xrandr output for the connector when the path
prop matches. I suspect it might work by accident due
to the new connector appearing (hopefully) later in the
list than the old connector. But would probably need to
test this to make sure.

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Jonas Ådahl]

Can you update the commit message so at least the first patch can land
for 6.1 so we can avoid regressions? E.g. something like

````
It caused logically active but disconnected MST display port connectors to
disappear from the drmModeGetResources() list, meaning userspace is made to
believe the connector is already disabled. This conflicts with the intended
behavior of userspace, which is to detect the connector got disconnected
and then disabling it.

When userspace later attempts post a new mode set commit, if that commit
uses the same CRTC used to previously drive the disconnected connector,
it will fail as that CRTC is logically still tied to the disconnected
connector.

This was discovered by a bisecting docking station hot plugging
regression using mutter.
```

(feel free to edit in any way you want).


Jonas

On Mon, Oct 17, 2022 at 03:31:57PM +0000, Simon Ser wrote:
> This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> 
> It turns out this breaks Mutter.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> ---
>  drivers/gpu/drm/drm_mode_config.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 939d621c9ad4..688c8afe0bf1 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  	count = 0;
>  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
> -		if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
> -			continue;
> -
>  		/* only expose writeback connectors if userspace understands them */
>  		if (!file_priv->writeback_connectors &&
>  		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> -- 
> 2.38.0
> 
> 

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Simon Ser]

I've already pushed both patches to drm-misc-next.

Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"

[Jonas Ådahl]

On Tue, Nov 15, 2022 at 08:55:12AM +0000, Simon Ser wrote:
> I've already pushed both patches to drm-misc-next.

It's not in any 6.1 rc yet, and apparently it needs to be pushed to
drm-misc-fixes to get into 6.1.