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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
> }
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
