Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect

2020-11-26 Thread Aurabindo Pillai


On 2020-11-26 9:35 a.m., Kazlauskas, Nicholas wrote:
> On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote:
>> [Why]
>> Recent changes to upstream mst code remove the callback which
>> cleared the internal state for mst. Move the missing functionality
>> that was previously called through the destroy call back for mst
>> connector
>> destroy
>>
>> Signed-off-by: Aurabindo Pillai 
>> Signed-off-by: Eryk Brol 
>> ---
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +--
>>   drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  2 +-
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index b7d7ec3ba00d..d8b0f07deaf2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr(
>>     void dm_helpers_dp_mst_stop_top_mgr(
>>   struct dc_context *ctx,
>> -    const struct dc_link *link)
>> +    struct dc_link *link)
>>   {
>>   struct amdgpu_dm_connector *aconnector = link->priv;
>> +    uint8_t i;
>>     if (!aconnector) {
>>   DRM_ERROR("Failed to find connector for link!");
>> @@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr(
>>   DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n",
>>   aconnector, aconnector->base.base.id);
>>   -    if (aconnector->mst_mgr.mst_state == true)
>> +    if (aconnector->mst_mgr.mst_state == true) {
>>   drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false);
>> +
>> +    for (i = 0; i < MAX_SINKS_PER_LINK; i++) {
>> +    if (link->remote_sinks[i] == NULL)
>> +    continue;
>> +
>> +    if (link->remote_sinks[i]->sink_signal ==
>> +    SIGNAL_TYPE_DISPLAY_PORT_MST) {
>> +    dc_link_remove_remote_sink(link, link->remote_sinks[i]);
> 
> In general I think this patch looks fine, and you can have the:
> 
> Reviewed-by: Nicholas Kazlauskas 
> 
> But I think that this loop is redundant - dc_link_remove_remote_sink
> should be removing all the remote sinks. Not sure if remote_sinks can
> start at an index other than 0 though.

dc_link_remove_remote_sink() will only remove one sink. It returns as
soon as it is done removing the sink we asked for.


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


Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect

2020-11-26 Thread Harry Wentland

On 2020-11-26 9:35 a.m., Kazlauskas, Nicholas wrote:

On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote:

[Why]
Recent changes to upstream mst code remove the callback which
cleared the internal state for mst. Move the missing functionality
that was previously called through the destroy call back for mst 
connector

destroy

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +--
  drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  2 +-
  2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c

index b7d7ec3ba00d..d8b0f07deaf2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr(
  void dm_helpers_dp_mst_stop_top_mgr(
  struct dc_context *ctx,
-    const struct dc_link *link)
+    struct dc_link *link)
  {
  struct amdgpu_dm_connector *aconnector = link->priv;
+    uint8_t i;
  if (!aconnector) {
  DRM_ERROR("Failed to find connector for link!");
@@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr(
  DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n",
  aconnector, aconnector->base.base.id);
-    if (aconnector->mst_mgr.mst_state == true)
+    if (aconnector->mst_mgr.mst_state == true) {
  drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false);
+
+    for (i = 0; i < MAX_SINKS_PER_LINK; i++) {
+    if (link->remote_sinks[i] == NULL)
+    continue;
+
+    if (link->remote_sinks[i]->sink_signal ==
+    SIGNAL_TYPE_DISPLAY_PORT_MST) {
+    dc_link_remove_remote_sink(link, link->remote_sinks[i]);


In general I think this patch looks fine, and you can have the:

Reviewed-by: Nicholas Kazlauskas 

But I think that this loop is redundant - dc_link_remove_remote_sink 
should be removing all the remote sinks. Not sure if remote_sinks can 
start at an index other than 0 though.




I'd prefer we clean this up if it's redundant. Otherwise redundant code 
stays around forever and makes future debug more cumbersome.


Harry


Regards,
Nicholas Kazlauskas


+
+    if (aconnector->dc_sink) {
+    dc_sink_release(aconnector->dc_sink);
+    aconnector->dc_sink = NULL;
+    aconnector->dc_link->cur_link_settings.lane_count 
= 0;

+    }
+    }
+    }
+    } >   }
  bool dm_helpers_dp_read_dpcd(
diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h 
b/drivers/gpu/drm/amd/display/dc/dm_helpers.h

index b2cd8491c707..07e349b1067b 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
@@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr(
  void dm_helpers_dp_mst_stop_top_mgr(
  struct dc_context *ctx,
-    const struct dc_link *link);
+    struct dc_link *link);
  /**
   * OS specific aux read callback.
   */



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7CHARRY.WENTLAND%40amd.com%7C7576866a05154b66cdb208d8921898da%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63741998164920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=sm7jvbxW%2BgEUevWgjACydnjuiE7SgtunMBUIpLT99r4%3Dreserved=0 


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


Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect

2020-11-26 Thread Kazlauskas, Nicholas

On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote:

[Why]
Recent changes to upstream mst code remove the callback which
cleared the internal state for mst. Move the missing functionality
that was previously called through the destroy call back for mst connector
destroy

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +--
  drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  2 +-
  2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index b7d7ec3ba00d..d8b0f07deaf2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr(
  
  void dm_helpers_dp_mst_stop_top_mgr(

struct dc_context *ctx,
-   const struct dc_link *link)
+   struct dc_link *link)
  {
struct amdgpu_dm_connector *aconnector = link->priv;
+   uint8_t i;
  
  	if (!aconnector) {

DRM_ERROR("Failed to find connector for link!");
@@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr(
DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n",
aconnector, aconnector->base.base.id);
  
-	if (aconnector->mst_mgr.mst_state == true)

+   if (aconnector->mst_mgr.mst_state == true) {
drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false);
+
+   for (i = 0; i < MAX_SINKS_PER_LINK; i++) {
+   if (link->remote_sinks[i] == NULL)
+   continue;
+
+   if (link->remote_sinks[i]->sink_signal ==
+   SIGNAL_TYPE_DISPLAY_PORT_MST) {
+   dc_link_remove_remote_sink(link, 
link->remote_sinks[i]);


In general I think this patch looks fine, and you can have the:

Reviewed-by: Nicholas Kazlauskas 

But I think that this loop is redundant - dc_link_remove_remote_sink 
should be removing all the remote sinks. Not sure if remote_sinks can 
start at an index other than 0 though.


Regards,
Nicholas Kazlauskas


+
+   if (aconnector->dc_sink) {
+   dc_sink_release(aconnector->dc_sink);
+   aconnector->dc_sink = NULL;
+   
aconnector->dc_link->cur_link_settings.lane_count = 0;
+   }
+   }
+   }
+   } >   }
  
  bool dm_helpers_dp_read_dpcd(

diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h 
b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
index b2cd8491c707..07e349b1067b 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
@@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr(
  
  void dm_helpers_dp_mst_stop_top_mgr(

struct dc_context *ctx,
-   const struct dc_link *link);
+   struct dc_link *link);
  /**
   * OS specific aux read callback.
   */



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