[PATCH 1/2] drm/amd/display: Cleanup MST non-atomic code workaround

[PATCH 1/2] drm/amd/display: Cleanup MST non-atomic code workaround

[Jerry (Fangzhi) Zuo]

[why]
It is not correct to touch aconnector within atomic_check.

[How]
It was added as workaround before, and no longer needed.

Signed-off-by: Jerry (Fangzhi) Zuo <Jerry.Zuo@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 15 +++-------
 .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c    | 34 ----------------------
 .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.h    |  1 -
 3 files changed, 4 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index f8ec8a146663..6c2441bfce90 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2760,18 +2760,11 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 	drm_connector = &aconnector->base;
 
 	if (!aconnector->dc_sink) {
-		/*
-		 * Create dc_sink when necessary to MST
-		 * Don't apply fake_sink to MST
-		 */
-		if (aconnector->mst_port) {
-			dm_dp_mst_dc_sink_create(drm_connector);
-			return stream;
+		if (!aconnector->mst_port) {
+			sink = create_fake_sink(aconnector);
+			if (!sink)
+				return stream;
 		}
-
-		sink = create_fake_sink(aconnector);
-		if (!sink)
-			return stream;
 	} else {
 		sink = aconnector->dc_sink;
 	}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 67683645ce2c..744d97cc0d39 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -205,40 +205,6 @@ static const struct drm_connector_funcs dm_dp_mst_connector_funcs = {
 	.atomic_get_property = amdgpu_dm_connector_atomic_get_property
 };
 
-void dm_dp_mst_dc_sink_create(struct drm_connector *connector)
-{
-	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
-	struct dc_sink *dc_sink;
-	struct dc_sink_init_data init_params = {
-			.link = aconnector->dc_link,
-			.sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
-
-	/* FIXME none of this is safe. we shouldn't touch aconnector here in
-	 * atomic_check
-	 */
-
-	/*
-	 * TODO: Need to further figure out why ddc.algo is NULL while MST port exists
-	 */
-	if (!aconnector->port || !aconnector->port->aux.ddc.algo)
-		return;
-
-	ASSERT(aconnector->edid);
-
-	dc_sink = dc_link_add_remote_sink(
-		aconnector->dc_link,
-		(uint8_t *)aconnector->edid,
-		(aconnector->edid->extensions + 1) * EDID_LENGTH,
-		&init_params);
-
-	dc_sink->priv = aconnector;
-	aconnector->dc_sink = dc_sink;
-
-	if (aconnector->dc_sink)
-		amdgpu_dm_update_freesync_caps(
-				connector, aconnector->edid);
-}
-
 static int dm_dp_mst_get_modes(struct drm_connector *connector)
 {
 	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
index 8cf51da26657..2da851b40042 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
@@ -31,6 +31,5 @@ struct amdgpu_dm_connector;
 
 void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
 				       struct amdgpu_dm_connector *aconnector);
-void dm_dp_mst_dc_sink_create(struct drm_connector *connector);
 
 #endif
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amd/display: Drop reusing drm connector for MST

[Jerry (Fangzhi) Zuo]

[why]
It is not safe to keep existing connector while entire topology
has been removed. Could lead potential impact to uapi.
Entirely unregister all the connectors on the topology,
and use a new set of connectors when the topology is plugged back
on.

[How]
Remove the drm connector entirely each time when the
corresponding MST topology is gone.
When hotunplug a connector (e.g., DP2)
1. Remove connector from userspace.
2. Drop it's reference.
When hotplug back on:
1. Detect new topology, and create new connectors.
2. Notify userspace with sysfs hotplug event.
3. Reprobe new connectors, and reassign CRTC from old (e.g., DP2)
to new (e.g., DP3) connector.

Signed-off-by: Jerry (Fangzhi) Zuo <Jerry.Zuo@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  2 --
 .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c    | 40 ++++------------------
 2 files changed, 7 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 101e122945a4..23f2d05cf07e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -208,8 +208,6 @@ struct amdgpu_dm_connector {
 	struct mutex hpd_lock;
 
 	bool fake_enable;
-
-	bool mst_connected;
 };
 
 #define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 744d97cc0d39..621afe39de13 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -320,25 +320,6 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_dm_connector *aconnector;
 	struct drm_connector *connector;
-	struct drm_connector_list_iter conn_iter;
-
-	drm_connector_list_iter_begin(dev, &conn_iter);
-	drm_for_each_connector_iter(connector, &conn_iter) {
-		aconnector = to_amdgpu_dm_connector(connector);
-		if (aconnector->mst_port == master
-				&& !aconnector->port) {
-			DRM_INFO("DM_MST: reusing connector: %p [id: %d] [master: %p]\n",
-						aconnector, connector->base.id, aconnector->mst_port);
-
-			aconnector->port = port;
-			drm_connector_set_path_property(connector, pathprop);
-
-			drm_connector_list_iter_end(&conn_iter);
-			aconnector->mst_connected = true;
-			return &aconnector->base;
-		}
-	}
-	drm_connector_list_iter_end(&conn_iter);
 
 	aconnector = kzalloc(sizeof(*aconnector), GFP_KERNEL);
 	if (!aconnector)
@@ -387,8 +368,6 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	 */
 	amdgpu_dm_connector_funcs_reset(connector);
 
-	aconnector->mst_connected = true;
-
 	DRM_INFO("DM_MST: added connector: %p [id: %d] [master: %p]\n",
 			aconnector, connector->base.id, aconnector->mst_port);
 
@@ -400,6 +379,9 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 static void dm_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					struct drm_connector *connector)
 {
+	struct amdgpu_dm_connector *master = container_of(mgr, struct amdgpu_dm_connector, mst_mgr);
+	struct drm_device *dev = master->base.dev;
+	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 
 	DRM_INFO("DM_MST: Disabling connector: %p [id: %d] [master: %p]\n",
@@ -413,7 +395,10 @@ static void dm_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 		aconnector->dc_sink = NULL;
 	}
 
-	aconnector->mst_connected = false;
+	drm_connector_unregister(connector);
+	if (adev->mode_info.rfbdev)
+		drm_fb_helper_remove_one_connector(&adev->mode_info.rfbdev->helper, connector);
+	drm_connector_unreference(connector);
 }
 
 static void dm_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
@@ -424,18 +409,10 @@ static void dm_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
 	drm_kms_helper_hotplug_event(dev);
 }
 
-static void dm_dp_mst_link_status_reset(struct drm_connector *connector)
-{
-	mutex_lock(&connector->dev->mode_config.mutex);
-	drm_connector_set_link_status_property(connector, DRM_MODE_LINK_STATUS_BAD);
-	mutex_unlock(&connector->dev->mode_config.mutex);
-}
-
 static void dm_dp_mst_register_connector(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
 	struct amdgpu_device *adev = dev->dev_private;
-	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 
 	if (adev->mode_info.rfbdev)
 		drm_fb_helper_add_one_connector(&adev->mode_info.rfbdev->helper, connector);
@@ -443,9 +420,6 @@ static void dm_dp_mst_register_connector(struct drm_connector *connector)
 		DRM_ERROR("adev->mode_info.rfbdev is NULL\n");
 
 	drm_connector_register(connector);
-
-	if (aconnector->mst_connected)
-		dm_dp_mst_link_status_reset(connector);
 }
 
 static const struct drm_dp_mst_topology_cbs dm_mst_cbs = {
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amd/display: Drop reusing drm connector for MST

[Wentland, Harry]

On 2018-10-30 6:09 p.m., Jerry (Fangzhi) Zuo wrote:
> [why]
> It is not safe to keep existing connector while entire topology
> has been removed. Could lead potential impact to uapi.
> Entirely unregister all the connectors on the topology,
> and use a new set of connectors when the topology is plugged back
> on.
> 
> [How]
> Remove the drm connector entirely each time when the
> corresponding MST topology is gone.
> When hotunplug a connector (e.g., DP2)
> 1. Remove connector from userspace.
> 2. Drop it's reference.
> When hotplug back on:
> 1. Detect new topology, and create new connectors.
> 2. Notify userspace with sysfs hotplug event.
> 3. Reprobe new connectors, and reassign CRTC from old (e.g., DP2)
> to new (e.g., DP3) connector.
> 
> Signed-off-by: Jerry (Fangzhi) Zuo <Jerry.Zuo@amd.com>

Series is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  2 --
>  .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c    | 40 ++++------------------
>  2 files changed, 7 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 101e122945a4..23f2d05cf07e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -208,8 +208,6 @@ struct amdgpu_dm_connector {
>  	struct mutex hpd_lock;
>  
>  	bool fake_enable;
> -
> -	bool mst_connected;
>  };
>  
>  #define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 744d97cc0d39..621afe39de13 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -320,25 +320,6 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_dm_connector *aconnector;
>  	struct drm_connector *connector;
> -	struct drm_connector_list_iter conn_iter;
> -
> -	drm_connector_list_iter_begin(dev, &conn_iter);
> -	drm_for_each_connector_iter(connector, &conn_iter) {
> -		aconnector = to_amdgpu_dm_connector(connector);
> -		if (aconnector->mst_port == master
> -				&& !aconnector->port) {
> -			DRM_INFO("DM_MST: reusing connector: %p [id: %d] [master: %p]\n",
> -						aconnector, connector->base.id, aconnector->mst_port);
> -
> -			aconnector->port = port;
> -			drm_connector_set_path_property(connector, pathprop);
> -
> -			drm_connector_list_iter_end(&conn_iter);
> -			aconnector->mst_connected = true;
> -			return &aconnector->base;
> -		}
> -	}
> -	drm_connector_list_iter_end(&conn_iter);
>  
>  	aconnector = kzalloc(sizeof(*aconnector), GFP_KERNEL);
>  	if (!aconnector)
> @@ -387,8 +368,6 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	 */
>  	amdgpu_dm_connector_funcs_reset(connector);
>  
> -	aconnector->mst_connected = true;
> -
>  	DRM_INFO("DM_MST: added connector: %p [id: %d] [master: %p]\n",
>  			aconnector, connector->base.id, aconnector->mst_port);
>  
> @@ -400,6 +379,9 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  static void dm_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  					struct drm_connector *connector)
>  {
> +	struct amdgpu_dm_connector *master = container_of(mgr, struct amdgpu_dm_connector, mst_mgr);
> +	struct drm_device *dev = master->base.dev;
> +	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>  
>  	DRM_INFO("DM_MST: Disabling connector: %p [id: %d] [master: %p]\n",
> @@ -413,7 +395,10 @@ static void dm_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  		aconnector->dc_sink = NULL;
>  	}
>  
> -	aconnector->mst_connected = false;
> +	drm_connector_unregister(connector);
> +	if (adev->mode_info.rfbdev)
> +		drm_fb_helper_remove_one_connector(&adev->mode_info.rfbdev->helper, connector);
> +	drm_connector_unreference(connector);
>  }
>  
>  static void dm_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> @@ -424,18 +409,10 @@ static void dm_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>  	drm_kms_helper_hotplug_event(dev);
>  }
>  
> -static void dm_dp_mst_link_status_reset(struct drm_connector *connector)
> -{
> -	mutex_lock(&connector->dev->mode_config.mutex);
> -	drm_connector_set_link_status_property(connector, DRM_MODE_LINK_STATUS_BAD);
> -	mutex_unlock(&connector->dev->mode_config.mutex);
> -}
> -
>  static void dm_dp_mst_register_connector(struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct amdgpu_device *adev = dev->dev_private;
> -	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>  
>  	if (adev->mode_info.rfbdev)
>  		drm_fb_helper_add_one_connector(&adev->mode_info.rfbdev->helper, connector);
> @@ -443,9 +420,6 @@ static void dm_dp_mst_register_connector(struct drm_connector *connector)
>  		DRM_ERROR("adev->mode_info.rfbdev is NULL\n");
>  
>  	drm_connector_register(connector);
> -
> -	if (aconnector->mst_connected)
> -		dm_dp_mst_link_status_reset(connector);
>  }
>  
>  static const struct drm_dp_mst_topology_cbs dm_mst_cbs = {
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amd/display: Cleanup MST non-atomic code workaround

[Lyude Paul]

This patch is

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

On Tue, 2018-10-30 at 18:09 -0400, Jerry (Fangzhi) Zuo wrote:
> [why]
> It is not correct to touch aconnector within atomic_check.
> 
> [How]
> It was added as workaround before, and no longer needed.
> 
> Signed-off-by: Jerry (Fangzhi) Zuo <Jerry.Zuo@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 15 +++-------
>  .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c    | 34 -----------------
> -----
>  .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.h    |  1 -
>  3 files changed, 4 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f8ec8a146663..6c2441bfce90 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2760,18 +2760,11 @@ create_stream_for_sink(struct amdgpu_dm_connector
> *aconnector,
>  	drm_connector = &aconnector->base;
>  
>  	if (!aconnector->dc_sink) {
> -		/*
> -		 * Create dc_sink when necessary to MST
> -		 * Don't apply fake_sink to MST
> -		 */
> -		if (aconnector->mst_port) {
> -			dm_dp_mst_dc_sink_create(drm_connector);
> -			return stream;
> +		if (!aconnector->mst_port) {
> +			sink = create_fake_sink(aconnector);
> +			if (!sink)
> +				return stream;
>  		}
> -
> -		sink = create_fake_sink(aconnector);
> -		if (!sink)
> -			return stream;
>  	} else {
>  		sink = aconnector->dc_sink;
>  	}
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 67683645ce2c..744d97cc0d39 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -205,40 +205,6 @@ static const struct drm_connector_funcs
> dm_dp_mst_connector_funcs = {
>  	.atomic_get_property = amdgpu_dm_connector_atomic_get_property
>  };
>  
> -void dm_dp_mst_dc_sink_create(struct drm_connector *connector)
> -{
> -	struct amdgpu_dm_connector *aconnector =
> to_amdgpu_dm_connector(connector);
> -	struct dc_sink *dc_sink;
> -	struct dc_sink_init_data init_params = {
> -			.link = aconnector->dc_link,
> -			.sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
> -
> -	/* FIXME none of this is safe. we shouldn't touch aconnector here in
> -	 * atomic_check
> -	 */
> -
> -	/*
> -	 * TODO: Need to further figure out why ddc.algo is NULL while MST
> port exists
> -	 */
> -	if (!aconnector->port || !aconnector->port->aux.ddc.algo)
> -		return;
> -
> -	ASSERT(aconnector->edid);
> -
> -	dc_sink = dc_link_add_remote_sink(
> -		aconnector->dc_link,
> -		(uint8_t *)aconnector->edid,
> -		(aconnector->edid->extensions + 1) * EDID_LENGTH,
> -		&init_params);
> -
> -	dc_sink->priv = aconnector;
> -	aconnector->dc_sink = dc_sink;
> -
> -	if (aconnector->dc_sink)
> -		amdgpu_dm_update_freesync_caps(
> -				connector, aconnector->edid);
> -}
> -
>  static int dm_dp_mst_get_modes(struct drm_connector *connector)
>  {
>  	struct amdgpu_dm_connector *aconnector =
> to_amdgpu_dm_connector(connector);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
> index 8cf51da26657..2da851b40042 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
> @@ -31,6 +31,5 @@ struct amdgpu_dm_connector;
>  
>  void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>  				       struct amdgpu_dm_connector
> *aconnector);
> -void dm_dp_mst_dc_sink_create(struct drm_connector *connector);
>  
>  #endif
-- 
Cheers,
	Lyude Paul

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amd/display: Drop reusing drm connector for MST

[Lyude Paul]

Almost there. First thing I should mention: removing the connector reusage
with this patch ended up exposing an issue that I think was getting papered
over before because of it. It ended up being rather trivial to fix though, so
I'll send the patch with more information on the issue/a fix for it in just a
moment. We should definitely make sure it gets included with this patch series
when it gets pushed.

In the mean time though, one nitpick:

On Tue, 2018-10-30 at 18:09 -0400, Jerry (Fangzhi) Zuo wrote:
> [why]
> It is not safe to keep existing connector while entire topology
> has been removed. Could lead potential impact to uapi.
> Entirely unregister all the connectors on the topology,
> and use a new set of connectors when the topology is plugged back
> on.
> 
> [How]
> Remove the drm connector entirely each time when the
> corresponding MST topology is gone.
> When hotunplug a connector (e.g., DP2)
> 1. Remove connector from userspace.
> 2. Drop it's reference.
> When hotplug back on:
> 1. Detect new topology, and create new connectors.
> 2. Notify userspace with sysfs hotplug event.
> 3. Reprobe new connectors, and reassign CRTC from old (e.g., DP2)
> to new (e.g., DP3) connector.
> 
> Signed-off-by: Jerry (Fangzhi) Zuo <Jerry.Zuo@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  2 --
>  .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c    | 40 ++++---------------
> ---
>  2 files changed, 7 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 101e122945a4..23f2d05cf07e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -208,8 +208,6 @@ struct amdgpu_dm_connector {
>  	struct mutex hpd_lock;
>  
>  	bool fake_enable;
> -
> -	bool mst_connected;
>  };
>  
>  #define to_amdgpu_dm_connector(x) container_of(x, struct
> amdgpu_dm_connector, base)
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 744d97cc0d39..621afe39de13 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -320,25 +320,6 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr
> *mgr,
>  	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_dm_connector *aconnector;
>  	struct drm_connector *connector;
> -	struct drm_connector_list_iter conn_iter;
> -
> -	drm_connector_list_iter_begin(dev, &conn_iter);
> -	drm_for_each_connector_iter(connector, &conn_iter) {
> -		aconnector = to_amdgpu_dm_connector(connector);
> -		if (aconnector->mst_port == master
> -				&& !aconnector->port) {
> -			DRM_INFO("DM_MST: reusing connector: %p [id: %d]
> [master: %p]\n",
> -						aconnector, connector-
> >base.id, aconnector->mst_port);
> -
> -			aconnector->port = port;
> -			drm_connector_set_path_property(connector, pathprop);
> -
> -			drm_connector_list_iter_end(&conn_iter);
> -			aconnector->mst_connected = true;
> -			return &aconnector->base;
> -		}
> -	}
> -	drm_connector_list_iter_end(&conn_iter);
>  
>  	aconnector = kzalloc(sizeof(*aconnector), GFP_KERNEL);
>  	if (!aconnector)
> @@ -387,8 +368,6 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr
> *mgr,
>  	 */
>  	amdgpu_dm_connector_funcs_reset(connector);
>  
> -	aconnector->mst_connected = true;
> -
>  	DRM_INFO("DM_MST: added connector: %p [id: %d] [master: %p]\n",
>  			aconnector, connector->base.id, aconnector->mst_port);
>  
> @@ -400,6 +379,9 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr
> *mgr,
>  static void dm_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr
> *mgr,
>  					struct drm_connector *connector)
>  {
> +	struct amdgpu_dm_connector *master = container_of(mgr, struct
> amdgpu_dm_connector, mst_mgr);
> +	struct drm_device *dev = master->base.dev;
> +	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_dm_connector *aconnector =
> to_amdgpu_dm_connector(connector);
>  
>  	DRM_INFO("DM_MST: Disabling connector: %p [id: %d] [master: %p]\n",
> @@ -413,7 +395,10 @@ static void dm_dp_destroy_mst_connector(struct
> drm_dp_mst_topology_mgr *mgr,
>  		aconnector->dc_sink = NULL;
>  	}
>  
> -	aconnector->mst_connected = false;
> +	drm_connector_unregister(connector);
> +	if (adev->mode_info.rfbdev)
> +		drm_fb_helper_remove_one_connector(&adev->mode_info.rfbdev-
> >helper, connector);
> +	drm_connector_unreference(connector);
This should be drm_connector_put(), drm_connector_unreference() is deprecated
(see the kernel doc comments for it).

With that change, and the patch I'm about to respond to this email with
included in the final series:

Reviewed-by: Lyude Paul <lyude@redhat.com>
>  }
>  
>  static void dm_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> @@ -424,18 +409,10 @@ static void dm_dp_mst_hotplug(struct
> drm_dp_mst_topology_mgr *mgr)
>  	drm_kms_helper_hotplug_event(dev);
>  }
>  
> -static void dm_dp_mst_link_status_reset(struct drm_connector *connector)
> -{
> -	mutex_lock(&connector->dev->mode_config.mutex);
> -	drm_connector_set_link_status_property(connector,
> DRM_MODE_LINK_STATUS_BAD);
> -	mutex_unlock(&connector->dev->mode_config.mutex);
> -}
> -
>  static void dm_dp_mst_register_connector(struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct amdgpu_device *adev = dev->dev_private;
> -	struct amdgpu_dm_connector *aconnector =
> to_amdgpu_dm_connector(connector);
>  
>  	if (adev->mode_info.rfbdev)
>  		drm_fb_helper_add_one_connector(&adev->mode_info.rfbdev-
> >helper, connector);
> @@ -443,9 +420,6 @@ static void dm_dp_mst_register_connector(struct
> drm_connector *connector)
>  		DRM_ERROR("adev->mode_info.rfbdev is NULL\n");
>  
>  	drm_connector_register(connector);
> -
> -	if (aconnector->mst_connected)
> -		dm_dp_mst_link_status_reset(connector);
>  }
>  
>  static const struct drm_dp_mst_topology_cbs dm_mst_cbs = {
-- 
Cheers,
	Lyude Paul

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/amdgpu/dm: Fix dm_dp_create_fake_mst_encoder()

[Lyude Paul]

[why]
Removing connector reusage from DM to match the rest of the tree ended
up revealing an issue that was surprisingly subtle. The original amdgpu
code for DC that was submitted appears to have left a chunk in
dm_dp_create_fake_mst_encoder() that tries to find a "master encoder",
the likes of which isn't actually used or stored anywhere. It does so at
the wrong time as well by trying to access parts of the drm_connector
from the encoder init before it's actually been initialized. This
results in a NULL pointer deref on MST hotplugs:

[  160.696613] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
[  160.697234] PGD 0 P4D 0
[  160.697814] Oops: 0010 [#1] SMP PTI
[  160.698430] CPU: 2 PID: 64 Comm: kworker/2:1 Kdump: loaded Tainted: G           O      4.19.0Lyude-Test+ #2
[  160.699020] Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.22 05/17/2018
[  160.699672] Workqueue: events_long drm_dp_mst_link_probe_work [drm_kms_helper]
[  160.700322] RIP: 0010:          (null)
[  160.700920] Code: Bad RIP value.
[  160.701541] RSP: 0018:ffffc9000029fc78 EFLAGS: 00010206
[  160.702183] RAX: 0000000000000000 RBX: ffff8804440ed468 RCX: ffff8804440e9158
[  160.702778] RDX: 0000000000000000 RSI: ffff8804556c5700 RDI: ffff8804440ed000
[  160.703408] RBP: ffff880458e21800 R08: 0000000000000002 R09: 000000005fca0a25
[  160.704002] R10: ffff88045a077a3d R11: ffff88045a077a3c R12: ffff8804440ed000
[  160.704614] R13: ffff880458e21800 R14: ffff8804440e9000 R15: ffff8804440e9000
[  160.705260] FS:  0000000000000000(0000) GS:ffff88045f280000(0000) knlGS:0000000000000000
[  160.705854] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  160.706478] CR2: ffffffffffffffd6 CR3: 000000000200a001 CR4: 00000000003606e0
[  160.707124] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  160.707724] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  160.708372] Call Trace:
[  160.708998]  ? dm_dp_add_mst_connector+0xed/0x1d0 [amdgpu]
[  160.709625]  ? drm_dp_add_port+0x2fa/0x470 [drm_kms_helper]
[  160.710284]  ? wake_up_q+0x54/0x70
[  160.710877]  ? __mutex_unlock_slowpath.isra.18+0xb3/0x110
[  160.711512]  ? drm_dp_dpcd_access+0xe7/0x110 [drm_kms_helper]
[  160.712161]  ? drm_dp_send_link_address+0x155/0x1e0 [drm_kms_helper]
[  160.712762]  ? drm_dp_check_and_send_link_address+0xa3/0xd0 [drm_kms_helper]
[  160.713408]  ? drm_dp_mst_link_probe_work+0x4b/0x80 [drm_kms_helper]
[  160.714013]  ? process_one_work+0x1a1/0x3a0
[  160.714667]  ? worker_thread+0x30/0x380
[  160.715326]  ? wq_update_unbound_numa+0x10/0x10
[  160.715939]  ? kthread+0x112/0x130
[  160.716591]  ? kthread_create_worker_on_cpu+0x70/0x70
[  160.717262]  ? ret_from_fork+0x35/0x40
[  160.717886] Modules linked in: amdgpu(O) vfat fat snd_hda_codec_generic joydev i915 chash gpu_sched ttm i2c_algo_bit drm_kms_helper snd_hda_codec_hdmi hp_wmi syscopyarea iTCO_wdt sysfillrect sparse_keymap sysimgblt fb_sys_fops snd_hda_intel usbhid wmi_bmof drm snd_hda_codec btusb snd_hda_core intel_rapl btrtl x86_pkg_temp_thermal btbcm btintel coretemp snd_pcm crc32_pclmul bluetooth psmouse snd_timer snd pcspkr i2c_i801 mei_me i2c_core soundcore mei tpm_tis wmi tpm_tis_core hp_accel ecdh_generic lis3lv02d tpm video rfkill acpi_pad input_polldev hp_wireless pcc_cpufreq crc32c_intel serio_raw tg3 xhci_pci xhci_hcd [last unloaded: amdgpu]
[  160.720141] CR2: 0000000000000000

Somehow the connector reusage DM was using for MST connectors managed to
paper over this issue entirely; hence why this was never caught until
now.

[how]
Since this code isn't used anywhere and seems useless anyway, we can
just drop it entirely. This appears to fix the issue on my HP ZBook with
an AMD WX4150.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
Hey! This is the patch that I was talking about, feel free to review
it-we should make sure this goes in with the rest of the patches you've
got so far.

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 6aa7359d61a3..5432a1862b41 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -285,12 +285,7 @@ dm_dp_create_fake_mst_encoder(struct amdgpu_dm_connector *connector)
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_encoder *amdgpu_encoder;
 	struct drm_encoder *encoder;
-	const struct drm_connector_helper_funcs *connector_funcs =
-		connector->base.helper_private;
-	struct drm_encoder *enc_master =
-		connector_funcs->best_encoder(&connector->base);
 
-	DRM_DEBUG_KMS("enc master is %p\n", enc_master);
 	amdgpu_encoder = kzalloc(sizeof(*amdgpu_encoder), GFP_KERNEL);
 	if (!amdgpu_encoder)
 		return NULL;
-- 
2.19.1

[PATCH] drm/amd/amdgpu/dm: Fix dm_dp_create_fake_mst_encoder()

[Lyude Paul]

[why]
Removing connector reusage from DM to match the rest of the tree ended
up revealing an issue that was surprisingly subtle. The original amdgpu
code for DC that was submitted appears to have left a chunk in
dm_dp_create_fake_mst_encoder() that tries to find a "master encoder",
the likes of which isn't actually used or stored anywhere. It does so at
the wrong time as well by trying to access parts of the drm_connector
from the encoder init before it's actually been initialized. This
results in a NULL pointer deref on MST hotplugs:

[  160.696613] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
[  160.697234] PGD 0 P4D 0
[  160.697814] Oops: 0010 [#1] SMP PTI
[  160.698430] CPU: 2 PID: 64 Comm: kworker/2:1 Kdump: loaded Tainted: G           O      4.19.0Lyude-Test+ #2
[  160.699020] Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.22 05/17/2018
[  160.699672] Workqueue: events_long drm_dp_mst_link_probe_work [drm_kms_helper]
[  160.700322] RIP: 0010:          (null)
[  160.700920] Code: Bad RIP value.
[  160.701541] RSP: 0018:ffffc9000029fc78 EFLAGS: 00010206
[  160.702183] RAX: 0000000000000000 RBX: ffff8804440ed468 RCX: ffff8804440e9158
[  160.702778] RDX: 0000000000000000 RSI: ffff8804556c5700 RDI: ffff8804440ed000
[  160.703408] RBP: ffff880458e21800 R08: 0000000000000002 R09: 000000005fca0a25
[  160.704002] R10: ffff88045a077a3d R11: ffff88045a077a3c R12: ffff8804440ed000
[  160.704614] R13: ffff880458e21800 R14: ffff8804440e9000 R15: ffff8804440e9000
[  160.705260] FS:  0000000000000000(0000) GS:ffff88045f280000(0000) knlGS:0000000000000000
[  160.705854] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  160.706478] CR2: ffffffffffffffd6 CR3: 000000000200a001 CR4: 00000000003606e0
[  160.707124] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  160.707724] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  160.708372] Call Trace:
[  160.708998]  ? dm_dp_add_mst_connector+0xed/0x1d0 [amdgpu]
[  160.709625]  ? drm_dp_add_port+0x2fa/0x470 [drm_kms_helper]
[  160.710284]  ? wake_up_q+0x54/0x70
[  160.710877]  ? __mutex_unlock_slowpath.isra.18+0xb3/0x110
[  160.711512]  ? drm_dp_dpcd_access+0xe7/0x110 [drm_kms_helper]
[  160.712161]  ? drm_dp_send_link_address+0x155/0x1e0 [drm_kms_helper]
[  160.712762]  ? drm_dp_check_and_send_link_address+0xa3/0xd0 [drm_kms_helper]
[  160.713408]  ? drm_dp_mst_link_probe_work+0x4b/0x80 [drm_kms_helper]
[  160.714013]  ? process_one_work+0x1a1/0x3a0
[  160.714667]  ? worker_thread+0x30/0x380
[  160.715326]  ? wq_update_unbound_numa+0x10/0x10
[  160.715939]  ? kthread+0x112/0x130
[  160.716591]  ? kthread_create_worker_on_cpu+0x70/0x70
[  160.717262]  ? ret_from_fork+0x35/0x40
[  160.717886] Modules linked in: amdgpu(O) vfat fat snd_hda_codec_generic joydev i915 chash gpu_sched ttm i2c_algo_bit drm_kms_helper snd_hda_codec_hdmi hp_wmi syscopyarea iTCO_wdt sysfillrect sparse_keymap sysimgblt fb_sys_fops snd_hda_intel usbhid wmi_bmof drm snd_hda_codec btusb snd_hda_core intel_rapl btrtl x86_pkg_temp_thermal btbcm btintel coretemp snd_pcm crc32_pclmul bluetooth psmouse snd_timer snd pcspkr i2c_i801 mei_me i2c_core soundcore mei tpm_tis wmi tpm_tis_core hp_accel ecdh_generic lis3lv02d tpm video rfkill acpi_pad input_polldev hp_wireless pcc_cpufreq crc32c_intel serio_raw tg3 xhci_pci xhci_hcd [last unloaded: amdgpu]
[  160.720141] CR2: 0000000000000000

Somehow the connector reusage DM was using for MST connectors managed to
paper over this issue entirely; hence why this was never caught until
now.

[how]
Since this code isn't used anywhere and seems useless anyway, we can
just drop it entirely. This appears to fix the issue on my HP ZBook with
an AMD WX4150.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
Hey! This is the patch that I was talking about, feel free to review
it-we should make sure this goes in with the rest of the patches you've
got so far.

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 6aa7359d61a3..5432a1862b41 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -285,12 +285,7 @@ dm_dp_create_fake_mst_encoder(struct amdgpu_dm_connector *connector)
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_encoder *amdgpu_encoder;
 	struct drm_encoder *encoder;
-	const struct drm_connector_helper_funcs *connector_funcs =
-		connector->base.helper_private;
-	struct drm_encoder *enc_master =
-		connector_funcs->best_encoder(&connector->base);
 
-	DRM_DEBUG_KMS("enc master is %p\n", enc_master);
 	amdgpu_encoder = kzalloc(sizeof(*amdgpu_encoder), GFP_KERNEL);
 	if (!amdgpu_encoder)
 		return NULL;
-- 
2.19.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/amdgpu/dm: Fix dm_dp_create_fake_mst_encoder()

[Wentland, Harry]

On 2018-11-01 9:51 p.m., Lyude Paul wrote:
> [why]
> Removing connector reusage from DM to match the rest of the tree ended
> up revealing an issue that was surprisingly subtle. The original amdgpu
> code for DC that was submitted appears to have left a chunk in
> dm_dp_create_fake_mst_encoder() that tries to find a "master encoder",
> the likes of which isn't actually used or stored anywhere. It does so at
> the wrong time as well by trying to access parts of the drm_connector
> from the encoder init before it's actually been initialized. This
> results in a NULL pointer deref on MST hotplugs:
> 
> [  160.696613] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> [  160.697234] PGD 0 P4D 0
> [  160.697814] Oops: 0010 [#1] SMP PTI
> [  160.698430] CPU: 2 PID: 64 Comm: kworker/2:1 Kdump: loaded Tainted: G           O      4.19.0Lyude-Test+ #2
> [  160.699020] Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.22 05/17/2018
> [  160.699672] Workqueue: events_long drm_dp_mst_link_probe_work [drm_kms_helper]
> [  160.700322] RIP: 0010:          (null)
> [  160.700920] Code: Bad RIP value.
> [  160.701541] RSP: 0018:ffffc9000029fc78 EFLAGS: 00010206
> [  160.702183] RAX: 0000000000000000 RBX: ffff8804440ed468 RCX: ffff8804440e9158
> [  160.702778] RDX: 0000000000000000 RSI: ffff8804556c5700 RDI: ffff8804440ed000
> [  160.703408] RBP: ffff880458e21800 R08: 0000000000000002 R09: 000000005fca0a25
> [  160.704002] R10: ffff88045a077a3d R11: ffff88045a077a3c R12: ffff8804440ed000
> [  160.704614] R13: ffff880458e21800 R14: ffff8804440e9000 R15: ffff8804440e9000
> [  160.705260] FS:  0000000000000000(0000) GS:ffff88045f280000(0000) knlGS:0000000000000000
> [  160.705854] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  160.706478] CR2: ffffffffffffffd6 CR3: 000000000200a001 CR4: 00000000003606e0
> [  160.707124] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  160.707724] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [  160.708372] Call Trace:
> [  160.708998]  ? dm_dp_add_mst_connector+0xed/0x1d0 [amdgpu]
> [  160.709625]  ? drm_dp_add_port+0x2fa/0x470 [drm_kms_helper]
> [  160.710284]  ? wake_up_q+0x54/0x70
> [  160.710877]  ? __mutex_unlock_slowpath.isra.18+0xb3/0x110
> [  160.711512]  ? drm_dp_dpcd_access+0xe7/0x110 [drm_kms_helper]
> [  160.712161]  ? drm_dp_send_link_address+0x155/0x1e0 [drm_kms_helper]
> [  160.712762]  ? drm_dp_check_and_send_link_address+0xa3/0xd0 [drm_kms_helper]
> [  160.713408]  ? drm_dp_mst_link_probe_work+0x4b/0x80 [drm_kms_helper]
> [  160.714013]  ? process_one_work+0x1a1/0x3a0
> [  160.714667]  ? worker_thread+0x30/0x380
> [  160.715326]  ? wq_update_unbound_numa+0x10/0x10
> [  160.715939]  ? kthread+0x112/0x130
> [  160.716591]  ? kthread_create_worker_on_cpu+0x70/0x70
> [  160.717262]  ? ret_from_fork+0x35/0x40
> [  160.717886] Modules linked in: amdgpu(O) vfat fat snd_hda_codec_generic joydev i915 chash gpu_sched ttm i2c_algo_bit drm_kms_helper snd_hda_codec_hdmi hp_wmi syscopyarea iTCO_wdt sysfillrect sparse_keymap sysimgblt fb_sys_fops snd_hda_intel usbhid wmi_bmof drm snd_hda_codec btusb snd_hda_core intel_rapl btrtl x86_pkg_temp_thermal btbcm btintel coretemp snd_pcm crc32_pclmul bluetooth psmouse snd_timer snd pcspkr i2c_i801 mei_me i2c_core soundcore mei tpm_tis wmi tpm_tis_core hp_accel ecdh_generic lis3lv02d tpm video rfkill acpi_pad input_polldev hp_wireless pcc_cpufreq crc32c_intel serio_raw tg3 xhci_pci xhci_hcd [last unloaded: amdgpu]
> [  160.720141] CR2: 0000000000000000
> 
> Somehow the connector reusage DM was using for MST connectors managed to
> paper over this issue entirely; hence why this was never caught until
> now.
> 
> [how]
> Since this code isn't used anywhere and seems useless anyway, we can
> just drop it entirely. This appears to fix the issue on my HP ZBook with
> an AMD WX4150.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Good catch. Probably got broken with some of the best_encoder cleanup that happened in the last few months. I really should've caught it then.

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
> Hey! This is the patch that I was talking about, feel free to review
> it-we should make sure this goes in with the rest of the patches you've
> got so far.
> 
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 6aa7359d61a3..5432a1862b41 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -285,12 +285,7 @@ dm_dp_create_fake_mst_encoder(struct amdgpu_dm_connector *connector)
>  	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_encoder *amdgpu_encoder;
>  	struct drm_encoder *encoder;
> -	const struct drm_connector_helper_funcs *connector_funcs =
> -		connector->base.helper_private;
> -	struct drm_encoder *enc_master =
> -		connector_funcs->best_encoder(&connector->base);
>  
> -	DRM_DEBUG_KMS("enc master is %p\n", enc_master);
>  	amdgpu_encoder = kzalloc(sizeof(*amdgpu_encoder), GFP_KERNEL);
>  	if (!amdgpu_encoder)
>  		return NULL;
>