Re: [PATCH v3 26/38] drm/msm/dp: add an API to initialize MST on sink side

[Yongxing Mou <yongxing.mou@oss.qualcomm.com>]

On 8/26/2025 5:26 PM, Dmitry Baryshkov wrote:
> On Mon, Aug 25, 2025 at 10:16:12PM +0800, Yongxing Mou wrote:
>> From: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> If the DP controller is capable of supporting multiple streams
>> then initialize the DP sink in MST mode by programming the DP_MSTM_CTRL
>> DPCD register to enable MST mode.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c | 55 ++++++++++++++++++++++++++++++-------
>>   1 file changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index cb433103d439ac6b8089bdecf0ee6be35c914db1..84df34306fb557341bea288ea8c13b0c81b11919 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/string_choices.h>
>>   #include <drm/display/drm_dp_aux_bus.h>
>>   #include <drm/display/drm_hdmi_audio_helper.h>
>> +#include <drm/display/drm_dp_mst_helper.h>
>>   #include <drm/drm_edid.h>
>>   
>>   #include "msm_drv.h"
>> @@ -297,6 +298,35 @@ static int msm_dp_display_lttpr_init(struct msm_dp_display_private *dp, u8 *dpcd
>>   	return lttpr_count;
>>   }
>>   
>> +static void msm_dp_display_mst_init(struct msm_dp_display_private *dp)
>> +{
>> +	const unsigned long clear_mstm_ctrl_timeout_us = 100000;
>> +	u8 old_mstm_ctrl;
>> +	struct msm_dp *msm_dp = &dp->msm_dp_display;
>> +	int ret;
>> +
>> +	/* clear sink mst state */
>> +	drm_dp_dpcd_readb(dp->aux, DP_MSTM_CTRL, &old_mstm_ctrl);
>> +	drm_dp_dpcd_writeb(dp->aux, DP_MSTM_CTRL, 0);
>> +
>> +	/* add extra delay if MST old state is on*/
>> +	if (old_mstm_ctrl) {
>> +		drm_dbg_dp(dp->drm_dev, "wait %luus to set DP_MSTM_CTRL set 0\n",
>> +			   clear_mstm_ctrl_timeout_us);
>> +		usleep_range(clear_mstm_ctrl_timeout_us,
>> +			     clear_mstm_ctrl_timeout_us + 1000);
>> +	}
>> +
>> +	ret = drm_dp_dpcd_writeb(dp->aux, DP_MSTM_CTRL,
>> +				 DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC);
>> +	if (ret < 0) {
> 
> I didn't catch this up in the previous patches. We have two sets of DPCD
> accessors: the older ones which can return error or the size of the data
> that was actually read / written (which might be less than the size of
> the buffer passed to the function) and newer ones, which return error or
> 0. drm_dp_dpcd_writeb() is from the first group, so if it was
> successful, it should be returning 1. It's all a pain to handle, so
> please start using newer accessors in your patches (the full conversion
> of the MSM driver is on my todo list, but it's intrusive, so was
> delaying it...).
> 
> TL;DR: inside your code please use drm_dp_dpcd_read_byte() /
> drm_dp_dpcd_write_byte() / drm_dp_dpcd_read_data() /
> drm_dp_dpcd_write_data().
> 
Thanks for point it. Will move to new APIs.
>> +		DRM_ERROR("sink mst enablement failed\n");
>> +		return;
>> +	}
>> +
>> +	msm_dp->mst_active = true;
>> +}
>> +
>>   static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>>   {
>>   	struct drm_connector *connector = dp->msm_dp_display.connector;
>> @@ -315,18 +345,20 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>>   	if (rc)
>>   		goto end;
>>   
>> -	drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
>> -	drm_edid_connector_update(connector, drm_edid);
>> +	if (!dp->mst_supported || !drm_dp_read_mst_cap(dp->aux, dp->panel->dpcd)) {
> 
> In several previous patches, the functions initializing MST can
> return an error, leaving the user with half-initialized MST. Are those
> errors a safety coding or do they have some real-use usecase? In the
> latter case, it would be nice to keep SST wokring if enabling MST fails.
> 
After plug in, MST will active when mst_active == true (DP_MST_EN 
programming succeeded). Before that point, any return will continue to 
keep the system in SST mode.
>> +		drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
>> +		drm_edid_connector_update(connector, drm_edid);
>>   
>> -	if (!drm_edid) {
>> -		DRM_ERROR("panel edid read failed\n");
>> -		/* check edid read fail is due to unplug */
>> -		if (!msm_dp_aux_is_link_connected(dp->aux))
>> -			return -ETIMEDOUT;
>> -	}
>> +		if (!drm_edid) {
>> +			DRM_ERROR("panel edid read failed\n");
>> +			/* check edid read fail is due to unplug */
>> +			if (!msm_dp_aux_is_link_connected(dp->aux))
>> +				return -ETIMEDOUT;
>> +		}
>>   
>> -	if (rc)
>> -		goto end;
>> +		if (rc)
>> +			goto end;
>> +	}
>>   
>>   	msm_dp_link_process_request(dp->link);
>>   
>> @@ -349,6 +381,9 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>>   	 */
>>   	msm_dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>>   
>> +	if (dp->mst_supported && drm_dp_read_mst_cap(dp->aux, dp->panel->dpcd))
>> +		msm_dp_display_mst_init(dp);
>> +
>>   	msm_dp_link_reset_phy_params_vx_px(dp->link);
>>   
>>   	if (!dp->msm_dp_display.internal_hpd)
>>
>> -- 
>> 2.34.1
>>
>