Re: [PATCH 2/2] drm/amd/display: Drop reusing drm connector for MST
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
> ---
> 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, _iter);
> - drm_for_each_connector_iter(connector, _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(_iter);
> - aconnector->mst_connected = true;
> - return >base;
> - }
> - }
> - drm_connector_list_iter_end(_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(>mode_info.rfbdev-
> >helper, connector);
> + drm_connector_unreference(connector);
This should be drm_connector_put(), drm_connector_unreference() is deprecated
(see the kernel
Re: [PATCH 2/2] drm/amd/display: Drop reusing drm connector for MST
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
Series is
Reviewed-by: Harry Wentland
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, _iter);
> - drm_for_each_connector_iter(connector, _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(_iter);
> - aconnector->mst_connected = true;
> - return >base;
> - }
> - }
> - drm_connector_list_iter_end(_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(>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(>dev->mode_config.mutex);
> - drm_connector_set_link_status_property(connector,
> DRM_MODE_LINK_STATUS_BAD);
> - mutex_unlock(>dev->mode_config.mutex);
> -}
[PATCH 2/2] drm/amd/display: Drop reusing drm connector for MST
[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
---
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, _iter);
- drm_for_each_connector_iter(connector, _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(_iter);
- aconnector->mst_connected = true;
- return >base;
- }
- }
- drm_connector_list_iter_end(_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(>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(>dev->mode_config.mutex);
- drm_connector_set_link_status_property(connector,
DRM_MODE_LINK_STATUS_BAD);
- mutex_unlock(>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 =
