[PATCH] drm/amd/mst: clean DP main link status only when unplug mst 1st link

[Lin, Wayne]

[AMD Public Use]



> -----Original Message-----
> From: Siqueira, Rodrigo <Rodrigo.Siqueira at amd.com>
> Sent: Wednesday, August 5, 2020 10:55 PM
> To: Lin, Wayne <Wayne.Lin at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; Lakha, Bhawanpreet 
> <Bhawanpreet.Lakha at amd.com>; Wu, Hersen <hersenxs.wu at amd.com>; 
> Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Zuo, Jerry 
> <Jerry.Zuo at amd.com>
> Subject: Re: [PATCH] drm/amd/mst: clean DP main link status only when 
> unplug mst 1st link
> 
> On 08/04, Wayne Lin wrote:
> > [Why]
> > Under DP daisy chain scenario as below:
> >
> > 	Src - Monitor_1 - Monitor_2
> >
> > If unplug 2nd Monitor_2 and plug in again, observe that Monitor_1 
> > doesn't light up.
> >
> > When unplug 2nd monitor, we clear the 
> > dc_link->cur_link_settings.lane_count in dm_dp_destroy_mst_connector().
> > However this link status is a shared data structure by all connected 
> > mst monitors. Although the 2nd monitor is gone, this link status 
> > should still be retained for other connected mst monitors. 
> > Otherwise, when we plug the 2nd monitor in again, we find out that 
> > main link is not trained and do link training again. Payload ID 
> > Table for Monitor_1 is ruined and we don't reallocate it.
> >
> > [How]
> > In dm_dp_destroy_mst_connector(), only clean the cur_link_settings 
> > when we no longer do mst mode.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin at amd.com>
> > ---
> >  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 5
> ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > 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 2c10352fa514..526f29598403 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
> > @@ -415,7 +415,10 @@ static void dm_dp_destroy_mst_connector(struct
> drm_dp_mst_topology_mgr *mgr,
> >  					   aconnector->dc_sink);
> >  		dc_sink_release(aconnector->dc_sink);
> >  		aconnector->dc_sink = NULL;
> > -		aconnector->dc_link->cur_link_settings.lane_count = 0;
> > +		mutex_lock(&mgr->lock);
> > +		if (!mgr->mst_state)
> > +			aconnector->dc_link->cur_link_settings.lane_count = 0;
> > +		mutex_unlock(&mgr->lock);
> Hi Wayne,
> 
> The change looks good to me.
> 
> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
> 
> Just for curiosity, do you know why we use a mutex instead of 
> spin_lock_irq for this case? FWIU, the spin_lock_irq behavior looks 
> better for this sort of manipulation.

Hi Siqueira,

Thanks for your time.
AFAIK, changing mst_state (e.g. enabling MST mode) involves some reading/writing steps on DPCD which might cost some time. 
> 
> Thanks
> 
> >  	}
> >
> >  	drm_connector_unregister(connector);
> > --
> > 2.17.1
> >
> 
> --
> Rodrigo Siqueira
> https://siqueira.tech
--
Wayne Lin