[RFC 0/6] drm/msm/dpu: add support for idependent DSI config

[RFC 0/6] drm/msm/dpu: add support for idependent DSI config

[Dmitry Baryshkov]

This patchseries adds support for independent DSI config to DPU1 display
subdriver. This results in ability to drop one of msm_kms_funcs
callbacks.

This code was tested on RB5 (dpu, dsi). Neither DP nor MDP5 changes were
tested (thus the RFC tag).

[RFC 1/6] drm/msm/dsi: add two helper functions

[Dmitry Baryshkov]

Add two helper functions to be used by display drivers for setting up
encoders.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.c         |  6 ++++++
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 14 ++++++--------
 drivers/gpu/drm/msm/msm_drv.h         | 12 ++++++++++--
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 75afc12a7b25..874c1527d300 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -13,6 +13,12 @@ struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi)
 	return msm_dsi->encoder;
 }
 
+bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi)
+{
+	unsigned long host_flags = msm_dsi_host_get_mode_flags(msm_dsi->host);
+	return !(host_flags & MIPI_DSI_MODE_VIDEO);
+}
+
 static int dsi_get_phy(struct msm_dsi *msm_dsi)
 {
 	struct platform_device *pdev = msm_dsi->pdev;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index cd016576e8c5..7d4f6fae1ab0 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -217,12 +217,6 @@ static int dsi_mgr_bridge_get_id(struct drm_bridge *bridge)
 	return dsi_bridge->id;
 }
 
-static bool dsi_mgr_is_cmd_mode(struct msm_dsi *msm_dsi)
-{
-	unsigned long host_flags = msm_dsi_host_get_mode_flags(msm_dsi->host);
-	return !(host_flags & MIPI_DSI_MODE_VIDEO);
-}
-
 void msm_dsi_manager_setup_encoder(int id)
 {
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
@@ -232,7 +226,7 @@ void msm_dsi_manager_setup_encoder(int id)
 
 	if (encoder && kms->funcs->set_encoder_mode)
 		kms->funcs->set_encoder_mode(kms, encoder,
-					     dsi_mgr_is_cmd_mode(msm_dsi));
+					     msm_dsi_is_cmd_mode(msm_dsi));
 }
 
 static int msm_dsi_manager_panel_init(struct drm_connector *conn, u8 id)
@@ -277,7 +271,7 @@ static int msm_dsi_manager_panel_init(struct drm_connector *conn, u8 id)
 	if (other_dsi && other_dsi->panel && kms->funcs->set_split_display) {
 		kms->funcs->set_split_display(kms, master_dsi->encoder,
 					      slave_dsi->encoder,
-					      dsi_mgr_is_cmd_mode(msm_dsi));
+					      msm_dsi_is_cmd_mode(msm_dsi));
 	}
 
 out:
@@ -840,3 +834,7 @@ void msm_dsi_manager_unregister(struct msm_dsi *msm_dsi)
 		msm_dsim->dsi[msm_dsi->id] = NULL;
 }
 
+bool msm_dsi_is_dual_dsi(struct msm_dsi *msm_dsi)
+{
+	return IS_DUAL_DSI();
+}
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 3352125ce428..826cc5e25bcb 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -343,7 +343,8 @@ void __exit msm_dsi_unregister(void);
 int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 			 struct drm_encoder *encoder);
 void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi);
-
+bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
+bool msm_dsi_is_dual_dsi(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
 {
@@ -360,7 +361,14 @@ static inline int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
 static inline void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
 {
 }
-
+static inline bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi)
+{
+	return false;
+}
+static bool msm_dsi_is_dual_dsi(struct msm_dsi *msm_dsi)
+{
+	return false;
+}
 #endif
 
 #ifdef CONFIG_DRM_MSM_DP
-- 
2.30.2

[RFC 2/6] drm/msm/dpu: support setting up two independent DSI connectors

[Dmitry Baryshkov]

Move setting up encoders from set_encoder_mode to
_dpu_kms_initialize_dsi() / _dpu_kms_initialize_displayport(). This
allows us to support not only "single DSI" and "dual DSI" but also "two
independent DSI" configurations. In future this would also help adding
support for multiple DP connectors.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 89 ++++++++++++-------------
 1 file changed, 44 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 1d3a4f395e74..b63e1c948ff2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -471,30 +471,55 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct dpu_kms *dpu_kms)
 {
 	struct drm_encoder *encoder = NULL;
+	struct msm_display_info info;
 	int i, rc = 0;
 
 	if (!(priv->dsi[0] || priv->dsi[1]))
 		return rc;
 
-	/*TODO: Support two independent DSI connectors */
-	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
-	if (IS_ERR(encoder)) {
-		DPU_ERROR("encoder init failed for dsi display\n");
-		return PTR_ERR(encoder);
-	}
-
-	priv->encoders[priv->num_encoders++] = encoder;
-
 	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
 		if (!priv->dsi[i])
 			continue;
 
+		if (!encoder) {
+			encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
+			if (IS_ERR(encoder)) {
+				DPU_ERROR("encoder init failed for dsi display\n");
+				return PTR_ERR(encoder);
+			}
+
+			priv->encoders[priv->num_encoders++] = encoder;
+
+			memset(&info, 0, sizeof(info));
+			info.intf_type = encoder->encoder_type;
+			info.capabilities = msm_dsi_is_cmd_mode(priv->dsi[i]) ?
+				MSM_DISPLAY_CAP_CMD_MODE :
+				MSM_DISPLAY_CAP_VID_MODE;
+		}
+
 		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
 		if (rc) {
 			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
 				i, rc);
 			break;
 		}
+
+		info.h_tile_instance[info.num_of_h_tiles++] = i;
+
+		if (!msm_dsi_is_dual_dsi(priv->dsi[i])) {
+			rc = dpu_encoder_setup(dev, encoder, &info);
+			if (rc)
+				DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
+						encoder->base.id, rc);
+			encoder = NULL;
+		}
+	}
+
+	if (encoder) {
+		rc = dpu_encoder_setup(dev, encoder, &info);
+		if (rc)
+			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
+					encoder->base.id, rc);
 	}
 
 	return rc;
@@ -505,6 +530,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 					    struct dpu_kms *dpu_kms)
 {
 	struct drm_encoder *encoder = NULL;
+	struct msm_display_info info;
 	int rc = 0;
 
 	if (!priv->dp)
@@ -516,6 +542,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 		return PTR_ERR(encoder);
 	}
 
+	memset(&info, 0, sizeof(info));
 	rc = msm_dp_modeset_init(priv->dp, dev, encoder);
 	if (rc) {
 		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
@@ -524,6 +551,14 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 	}
 
 	priv->encoders[priv->num_encoders++] = encoder;
+
+	info.num_of_h_tiles = 1;
+	info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
+	info.intf_type = encoder->encoder_type;
+	rc = dpu_encoder_setup(dev, encoder, &info);
+	if (rc)
+		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
+			encoder->base.id, rc);
 	return rc;
 }
 
@@ -726,41 +761,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
 	msm_kms_destroy(&dpu_kms->base);
 }
 
-static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
-				 struct drm_encoder *encoder,
-				 bool cmd_mode)
-{
-	struct msm_display_info info;
-	struct msm_drm_private *priv = encoder->dev->dev_private;
-	int i, rc = 0;
-
-	memset(&info, 0, sizeof(info));
-
-	info.intf_type = encoder->encoder_type;
-	info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE :
-			MSM_DISPLAY_CAP_VID_MODE;
-
-	switch (info.intf_type) {
-	case DRM_MODE_ENCODER_DSI:
-		/* TODO: No support for DSI swap */
-		for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
-			if (priv->dsi[i]) {
-				info.h_tile_instance[info.num_of_h_tiles] = i;
-				info.num_of_h_tiles++;
-			}
-		}
-		break;
-	case DRM_MODE_ENCODER_TMDS:
-		info.num_of_h_tiles = 1;
-		break;
-	}
-
-	rc = dpu_encoder_setup(encoder->dev, encoder, &info);
-	if (rc)
-		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-			encoder->base.id, rc);
-}
-
 static irqreturn_t dpu_irq(struct msm_kms *kms)
 {
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
@@ -863,7 +863,6 @@ static const struct msm_kms_funcs kms_funcs = {
 	.get_format      = dpu_get_msm_format,
 	.round_pixclk    = dpu_kms_round_pixclk,
 	.destroy         = dpu_kms_destroy,
-	.set_encoder_mode = _dpu_kms_set_encoder_mode,
 	.snapshot        = dpu_kms_mdp_snapshot,
 #ifdef CONFIG_DEBUG_FS
 	.debugfs_init    = dpu_kms_debugfs_init,
-- 
2.30.2

Re: [Freedreno] [RFC 2/6] drm/msm/dpu: support setting up two independent DSI connectors

[abhinavk]

On 2021-06-09 14:17, Dmitry Baryshkov wrote:
> Move setting up encoders from set_encoder_mode to
> _dpu_kms_initialize_dsi() / _dpu_kms_initialize_displayport(). This
> allows us to support not only "single DSI" and "dual DSI" but also "two
> independent DSI" configurations. In future this would also help adding
> support for multiple DP connectors.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
I will have to see Bjorn's changes to check why it was dependent on this 
cleanup.
Is the plan to call _dpu_kms_initialize_displayport() twice?
But still I am not able to put together where is the dependency on that 
series
with this one. Can you please elaborate on that a little bit?

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 89 ++++++++++++-------------
>  1 file changed, 44 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 1d3a4f395e74..b63e1c948ff2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -471,30 +471,55 @@ static int _dpu_kms_initialize_dsi(struct 
> drm_device *dev,
>  				    struct dpu_kms *dpu_kms)
>  {
>  	struct drm_encoder *encoder = NULL;
> +	struct msm_display_info info;
>  	int i, rc = 0;
> 
>  	if (!(priv->dsi[0] || priv->dsi[1]))
>  		return rc;
> 
> -	/*TODO: Support two independent DSI connectors */
> -	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
> -	if (IS_ERR(encoder)) {
> -		DPU_ERROR("encoder init failed for dsi display\n");
> -		return PTR_ERR(encoder);
> -	}
> -
> -	priv->encoders[priv->num_encoders++] = encoder;
> -
>  	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>  		if (!priv->dsi[i])
>  			continue;
> 
> +		if (!encoder) {
> +			encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
> +			if (IS_ERR(encoder)) {
> +				DPU_ERROR("encoder init failed for dsi display\n");
> +				return PTR_ERR(encoder);
> +			}
> +
> +			priv->encoders[priv->num_encoders++] = encoder;
> +
> +			memset(&info, 0, sizeof(info));
> +			info.intf_type = encoder->encoder_type;
> +			info.capabilities = msm_dsi_is_cmd_mode(priv->dsi[i]) ?
> +				MSM_DISPLAY_CAP_CMD_MODE :
> +				MSM_DISPLAY_CAP_VID_MODE;
> +		}
> +
>  		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>  		if (rc) {
>  			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>  				i, rc);
>  			break;
>  		}
> +
> +		info.h_tile_instance[info.num_of_h_tiles++] = i;
> +
> +		if (!msm_dsi_is_dual_dsi(priv->dsi[i])) {

I would like to clarify the terminology of dual_dsi in the current DSI 
driver before the rest of the reviews.
Today IS_DUAL_DSI() means that two DSIs are driving the same display and 
the two DSIs are operating in master-slave mode
and are being driven by the same PLL.
Usually, dual independent DSI means two DSIs driving two separate panels 
using two separate PLLs ( DSI0 with PLL0 and DSI1 with PLL1)
I assume thats happening due to the foll logic and both DSI PHYs are 
operating in STANDALONE mode:

     if (!IS_DUAL_DSI()) {
         ret = msm_dsi_host_register(msm_dsi->host, true);
         if (ret)
             return ret;

         msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
         ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);

> +			rc = dpu_encoder_setup(dev, encoder, &info);
> +			if (rc)
> +				DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> +						encoder->base.id, rc);
> +			encoder = NULL;
> +		}
> +	}
> +
> +	if (encoder) {

We will hit this case only for split-DSI right? ( that is two DSIs 
driving the same panel ).
Even single DSI will be created in the above loop now. So this looks a 
bit confusing at the moment.

I think we need to be more clear on dual-DSI Vs split-DSI to avoid 
confusion in the code about which one means what and the one
which we are currently using. So what about having IS_DUAL_DSI() and 
IS_SPLIT_DSI() to distinguish the terminologies and chaging
DSI driver accordingly.

> +		rc = dpu_encoder_setup(dev, encoder, &info);
> +		if (rc)
> +			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> +					encoder->base.id, rc);
>  	}
> 
>  	return rc;
> @@ -505,6 +530,7 @@ static int _dpu_kms_initialize_displayport(struct
> drm_device *dev,
>  					    struct dpu_kms *dpu_kms)
>  {
>  	struct drm_encoder *encoder = NULL;
> +	struct msm_display_info info;
>  	int rc = 0;
> 
>  	if (!priv->dp)
> @@ -516,6 +542,7 @@ static int _dpu_kms_initialize_displayport(struct
> drm_device *dev,
>  		return PTR_ERR(encoder);
>  	}
> 
> +	memset(&info, 0, sizeof(info));
>  	rc = msm_dp_modeset_init(priv->dp, dev, encoder);
>  	if (rc) {
>  		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
> @@ -524,6 +551,14 @@ static int _dpu_kms_initialize_displayport(struct
> drm_device *dev,
>  	}
> 
>  	priv->encoders[priv->num_encoders++] = encoder;
> +
> +	info.num_of_h_tiles = 1;
> +	info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
> +	info.intf_type = encoder->encoder_type;
> +	rc = dpu_encoder_setup(dev, encoder, &info);
> +	if (rc)
> +		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> +			encoder->base.id, rc);
>  	return rc;
>  }
> 
> @@ -726,41 +761,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>  	msm_kms_destroy(&dpu_kms->base);
>  }
> 
> -static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
> -				 struct drm_encoder *encoder,
> -				 bool cmd_mode)
> -{
> -	struct msm_display_info info;
> -	struct msm_drm_private *priv = encoder->dev->dev_private;
> -	int i, rc = 0;
> -
> -	memset(&info, 0, sizeof(info));
> -
> -	info.intf_type = encoder->encoder_type;
> -	info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE :
> -			MSM_DISPLAY_CAP_VID_MODE;
> -
> -	switch (info.intf_type) {
> -	case DRM_MODE_ENCODER_DSI:
> -		/* TODO: No support for DSI swap */
> -		for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
> -			if (priv->dsi[i]) {
> -				info.h_tile_instance[info.num_of_h_tiles] = i;
> -				info.num_of_h_tiles++;
> -			}
> -		}
> -		break;
> -	case DRM_MODE_ENCODER_TMDS:
> -		info.num_of_h_tiles = 1;
> -		break;
> -	}
> -
> -	rc = dpu_encoder_setup(encoder->dev, encoder, &info);
> -	if (rc)
> -		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -			encoder->base.id, rc);
> -}
> -
It seems we can get rid of set_encoder_mode for DP because the way we 
are using it today seems not right.
Ideally, the purpose was that once we read the EDID, the information we 
read like the tile group etc
can be used when we are setting up the encoder. But today, we are just 
hard-coding the number of tiles.
But I just think whether looking ahead, we should still have some 
callback which can be called after
EDID has been read instead of doing it in 
_dpu_kms_initialize_displayport. Perhaps that can be a separate patch.

>  static irqreturn_t dpu_irq(struct msm_kms *kms)
>  {
>  	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> @@ -863,7 +863,6 @@ static const struct msm_kms_funcs kms_funcs = {
>  	.get_format      = dpu_get_msm_format,
>  	.round_pixclk    = dpu_kms_round_pixclk,
>  	.destroy         = dpu_kms_destroy,
> -	.set_encoder_mode = _dpu_kms_set_encoder_mode,
I would like to get Rob's comment on why we had set_encoder_mode in the 
first place. Its there even in mdp5.

in current msm dsi, the dsi bind will happen only after the panel has 
attached
and the msm_drv's bind will happen only after that since its the 
component master
in that case what was the need for set_encoder_mode  because we will 
know the panel's video/cmd mode in the dsi_bind call
am i missing something about why mdp5 had this?

 From the dpu perspective, since dsi_bind() happens only once panel has 
attached.
>  	.snapshot        = dpu_kms_mdp_snapshot,
>  #ifdef CONFIG_DEBUG_FS
>  	.debugfs_init    = dpu_kms_debugfs_init,

Re: [Freedreno] [RFC 2/6] drm/msm/dpu: support setting up two independent DSI connectors

[Dmitry Baryshkov]

On 02/07/2021 00:12, abhinavk@codeaurora.org wrote:
> On 2021-06-09 14:17, Dmitry Baryshkov wrote:
>> Move setting up encoders from set_encoder_mode to
>> _dpu_kms_initialize_dsi() / _dpu_kms_initialize_displayport(). This
>> allows us to support not only "single DSI" and "dual DSI" but also "two
>> independent DSI" configurations. In future this would also help adding
>> support for multiple DP connectors.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> I will have to see Bjorn's changes to check why it was dependent on this 
> cleanup.
> Is the plan to call _dpu_kms_initialize_displayport() twice?

Yes. He needs to initialize several displayport interfaces. With the 
current code he has to map ids in the set_encoder_mode, using encoder 
ids (to fill up the info.h_tile_instance, which is hardcoded to 0 for DP 
in the current code).

> But still I am not able to put together where is the dependency on that 
> series
> with this one. Can you please elaborate on that a little bit?

It is possible to support independent outputs with the current code. I 
did that for DSI, Bjorn did for DP. However it results in quite an ugly 
code to map received encoder in set_encoder_mode back to the DSI (DP) 
instances to fill the h_tiles. If we drop the whole set_encoder_mode 
story and call dpu_encoder_setup right from the 
_dpu_kms_initialize_dsi() (or _dpu_kms_initialize_displayport()), 
supporting multiple outputs becomes an easy task.

> 
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 89 ++++++++++++-------------
>>  1 file changed, 44 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 1d3a4f395e74..b63e1c948ff2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -471,30 +471,55 @@ static int _dpu_kms_initialize_dsi(struct 
>> drm_device *dev,
>>                      struct dpu_kms *dpu_kms)
>>  {
>>      struct drm_encoder *encoder = NULL;
>> +    struct msm_display_info info;
>>      int i, rc = 0;
>>
>>      if (!(priv->dsi[0] || priv->dsi[1]))
>>          return rc;
>>
>> -    /*TODO: Support two independent DSI connectors */
>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>> -    if (IS_ERR(encoder)) {
>> -        DPU_ERROR("encoder init failed for dsi display\n");
>> -        return PTR_ERR(encoder);
>> -    }
>> -
>> -    priv->encoders[priv->num_encoders++] = encoder;
>> -
>>      for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>>          if (!priv->dsi[i])
>>              continue;
>>
>> +        if (!encoder) {
>> +            encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>> +            if (IS_ERR(encoder)) {
>> +                DPU_ERROR("encoder init failed for dsi display\n");
>> +                return PTR_ERR(encoder);
>> +            }
>> +
>> +            priv->encoders[priv->num_encoders++] = encoder;
>> +
>> +            memset(&info, 0, sizeof(info));
>> +            info.intf_type = encoder->encoder_type;
>> +            info.capabilities = msm_dsi_is_cmd_mode(priv->dsi[i]) ?
>> +                MSM_DISPLAY_CAP_CMD_MODE :
>> +                MSM_DISPLAY_CAP_VID_MODE;
>> +        }
>> +
>>          rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>          if (rc) {
>>              DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>>                  i, rc);
>>              break;
>>          }
>> +
>> +        info.h_tile_instance[info.num_of_h_tiles++] = i;
>> +
>> +        if (!msm_dsi_is_dual_dsi(priv->dsi[i])) {
> 
> I would like to clarify the terminology of dual_dsi in the current DSI 
> driver before the rest of the reviews.
> Today IS_DUAL_DSI() means that two DSIs are driving the same display and 
> the two DSIs are operating in master-slave mode
> and are being driven by the same PLL.

Yes

> Usually, dual independent DSI means two DSIs driving two separate panels 
> using two separate PLLs ( DSI0 with PLL0 and DSI1 with PLL1)

Let's stop calling it 'dual'. I'd suggest to continue using what was 
there in the source file: 'two independent DSI'.

> I assume thats happening due to the foll logic and both DSI PHYs are 
> operating in STANDALONE mode:
> 
>      if (!IS_DUAL_DSI()) {
>          ret = msm_dsi_host_register(msm_dsi->host, true);
>          if (ret)
>              return ret;
> 
>          msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
>          ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);

Yes. If we have two independent DSI outputs, we'd like them to work in 
STANDALONE mode.


>> +            rc = dpu_encoder_setup(dev, encoder, &info);
>> +            if (rc)
>> +                DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> +                        encoder->base.id, rc);
>> +            encoder = NULL;
>> +        }
>> +    }
>> +
>> +    if (encoder) {
> 
> We will hit this case only for split-DSI right? ( that is two DSIs 
> driving the same panel ).

Yes, only in this case.

> Even single DSI will be created in the above loop now. So this looks a 
> bit confusing at the moment.

What is so confusing? I can probably add a comment there. If the encoder 
drivers single DSI output, we setup it after creating the DSI. If the 
encoder drives dual DSI outpu, we have to setup it after creating both 
DSI outputs.

I have tried calling dpu_encoder_setup from a separate if/loop 
condition, but it resulted in even uglier code.

> I think we need to be more clear on dual-DSI Vs split-DSI to avoid 
> confusion in the code about which one means what and the one
> which we are currently using. So what about having IS_DUAL_DSI() and 
> IS_SPLIT_DSI() to distinguish the terminologies and chaging
> DSI driver accordingly.

The word 'SPLIT' is already overloaded in my opinion. I'd prefer to keep 
on using 'dual DSI' for the master/slave case and not to use 'dual' for 
just two standalone DSI interfaces.

> 
>> +        rc = dpu_encoder_setup(dev, encoder, &info);
>> +        if (rc)
>> +            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> +                    encoder->base.id, rc);
>>      }
>>
>>      return rc;
>> @@ -505,6 +530,7 @@ static int _dpu_kms_initialize_displayport(struct
>> drm_device *dev,
>>                          struct dpu_kms *dpu_kms)
>>  {
>>      struct drm_encoder *encoder = NULL;
>> +    struct msm_display_info info;
>>      int rc = 0;
>>
>>      if (!priv->dp)
>> @@ -516,6 +542,7 @@ static int _dpu_kms_initialize_displayport(struct
>> drm_device *dev,
>>          return PTR_ERR(encoder);
>>      }
>>
>> +    memset(&info, 0, sizeof(info));
>>      rc = msm_dp_modeset_init(priv->dp, dev, encoder);
>>      if (rc) {
>>          DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>> @@ -524,6 +551,14 @@ static int _dpu_kms_initialize_displayport(struct
>> drm_device *dev,
>>      }
>>
>>      priv->encoders[priv->num_encoders++] = encoder;
>> +
>> +    info.num_of_h_tiles = 1;
>> +    info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>> +    info.intf_type = encoder->encoder_type;
>> +    rc = dpu_encoder_setup(dev, encoder, &info);
>> +    if (rc)
>> +        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> +            encoder->base.id, rc);
>>      return rc;
>>  }
>>
>> @@ -726,41 +761,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>>      msm_kms_destroy(&dpu_kms->base);
>>  }
>>
>> -static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
>> -                 struct drm_encoder *encoder,
>> -                 bool cmd_mode)
>> -{
>> -    struct msm_display_info info;
>> -    struct msm_drm_private *priv = encoder->dev->dev_private;
>> -    int i, rc = 0;
>> -
>> -    memset(&info, 0, sizeof(info));
>> -
>> -    info.intf_type = encoder->encoder_type;
>> -    info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE :
>> -            MSM_DISPLAY_CAP_VID_MODE;
>> -
>> -    switch (info.intf_type) {
>> -    case DRM_MODE_ENCODER_DSI:
>> -        /* TODO: No support for DSI swap */
>> -        for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>> -            if (priv->dsi[i]) {
>> -                info.h_tile_instance[info.num_of_h_tiles] = i;
>> -                info.num_of_h_tiles++;
>> -            }
>> -        }
>> -        break;
>> -    case DRM_MODE_ENCODER_TMDS:
>> -        info.num_of_h_tiles = 1;
>> -        break;
>> -    }
>> -
>> -    rc = dpu_encoder_setup(encoder->dev, encoder, &info);
>> -    if (rc)
>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> -            encoder->base.id, rc);
>> -}
>> -
> It seems we can get rid of set_encoder_mode for DP because the way we 
> are using it today seems not right.
> Ideally, the purpose was that once we read the EDID, the information we 
> read like the tile group etc
> can be used when we are setting up the encoder. But today, we are just 
> hard-coding the number of tiles.
> But I just think whether looking ahead, we should still have some 
> callback which can be called after
> EDID has been read instead of doing it in 
> _dpu_kms_initialize_displayport. Perhaps that can be a separate patch.

For the MST support? It is definitely a separate patch. For now we want 
to be able to drive a much simpler config: SST on several connected DPs.

> 
>>  static irqreturn_t dpu_irq(struct msm_kms *kms)
>>  {
>>      struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>> @@ -863,7 +863,6 @@ static const struct msm_kms_funcs kms_funcs = {
>>      .get_format      = dpu_get_msm_format,
>>      .round_pixclk    = dpu_kms_round_pixclk,
>>      .destroy         = dpu_kms_destroy,
>> -    .set_encoder_mode = _dpu_kms_set_encoder_mode,
> I would like to get Rob's comment on why we had set_encoder_mode in the 
> first place. Its there even in mdp5.
> 
> in current msm dsi, the dsi bind will happen only after the panel has 
> attached
> and the msm_drv's bind will happen only after that since its the 
> component master
> in that case what was the need for set_encoder_mode  because we will 
> know the panel's video/cmd mode in the dsi_bind call
> am i missing something about why mdp5 had this?
> 
>  From the dpu perspective, since dsi_bind() happens only once panel has 
> attached.
>>      .snapshot        = dpu_kms_mdp_snapshot,
>>  #ifdef CONFIG_DEBUG_FS
>>      .debugfs_init    = dpu_kms_debugfs_init,


-- 
With best wishes
Dmitry

Re: [Freedreno] [RFC 2/6] drm/msm/dpu: support setting up two independent DSI connectors

[abhinavk]

On 2021-07-02 02:20, Dmitry Baryshkov wrote:
> On 02/07/2021 00:12, abhinavk@codeaurora.org wrote:
>> On 2021-06-09 14:17, Dmitry Baryshkov wrote:
>>> Move setting up encoders from set_encoder_mode to
>>> _dpu_kms_initialize_dsi() / _dpu_kms_initialize_displayport(). This
>>> allows us to support not only "single DSI" and "dual DSI" but also 
>>> "two
>>> independent DSI" configurations. In future this would also help 
>>> adding
>>> support for multiple DP connectors.
>>> 
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> I will have to see Bjorn's changes to check why it was dependent on 
>> this cleanup.
>> Is the plan to call _dpu_kms_initialize_displayport() twice?
> 
> Yes. He needs to initialize several displayport interfaces. With the
> current code he has to map ids in the set_encoder_mode, using encoder
> ids (to fill up the info.h_tile_instance, which is hardcoded to 0 for
> DP in the current code).
> 
>> But still I am not able to put together where is the dependency on 
>> that series
>> with this one. Can you please elaborate on that a little bit?
> 
> It is possible to support independent outputs with the current code. I
> did that for DSI, Bjorn did for DP. However it results in quite an
> ugly code to map received encoder in set_encoder_mode back to the DSI
> (DP) instances to fill the h_tiles. If we drop the whole
> set_encoder_mode story and call dpu_encoder_setup right from the
> _dpu_kms_initialize_dsi() (or _dpu_kms_initialize_displayport()),
> supporting multiple outputs becomes an easy task.
> 
Okay got it, I think it will become more clear once he posts.
>> 
>>> ---
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 89 
>>> ++++++++++++-------------
>>>  1 file changed, 44 insertions(+), 45 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 1d3a4f395e74..b63e1c948ff2 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -471,30 +471,55 @@ static int _dpu_kms_initialize_dsi(struct 
>>> drm_device *dev,
>>>                      struct dpu_kms *dpu_kms)
>>>  {
>>>      struct drm_encoder *encoder = NULL;
>>> +    struct msm_display_info info;
>>>      int i, rc = 0;
>>> 
>>>      if (!(priv->dsi[0] || priv->dsi[1]))
>>>          return rc;
>>> 
>>> -    /*TODO: Support two independent DSI connectors */
>>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>> -    if (IS_ERR(encoder)) {
>>> -        DPU_ERROR("encoder init failed for dsi display\n");
>>> -        return PTR_ERR(encoder);
>>> -    }
>>> -
>>> -    priv->encoders[priv->num_encoders++] = encoder;
>>> -
>>>      for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>>>          if (!priv->dsi[i])
>>>              continue;
>>> 
>>> +        if (!encoder) {
>>> +            encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>> +            if (IS_ERR(encoder)) {
>>> +                DPU_ERROR("encoder init failed for dsi display\n");
>>> +                return PTR_ERR(encoder);
>>> +            }
>>> +
>>> +            priv->encoders[priv->num_encoders++] = encoder;
>>> +
>>> +            memset(&info, 0, sizeof(info));
>>> +            info.intf_type = encoder->encoder_type;
>>> +            info.capabilities = msm_dsi_is_cmd_mode(priv->dsi[i]) ?
>>> +                MSM_DISPLAY_CAP_CMD_MODE :
>>> +                MSM_DISPLAY_CAP_VID_MODE;
>>> +        }
>>> +
>>>          rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>          if (rc) {
>>>              DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>>>                  i, rc);
>>>              break;
>>>          }
>>> +
>>> +        info.h_tile_instance[info.num_of_h_tiles++] = i;
>>> +
>>> +        if (!msm_dsi_is_dual_dsi(priv->dsi[i])) {
>> 
>> I would like to clarify the terminology of dual_dsi in the current DSI 
>> driver before the rest of the reviews.
>> Today IS_DUAL_DSI() means that two DSIs are driving the same display 
>> and the two DSIs are operating in master-slave mode
>> and are being driven by the same PLL.
> 
> Yes
> 
>> Usually, dual independent DSI means two DSIs driving two separate 
>> panels using two separate PLLs ( DSI0 with PLL0 and DSI1 with PLL1)
> 
> Let's stop calling it 'dual'. I'd suggest to continue using what was
> there in the source file: 'two independent DSI'.
> 
>> I assume thats happening due to the foll logic and both DSI PHYs are 
>> operating in STANDALONE mode:
>> 
>>      if (!IS_DUAL_DSI()) {
>>          ret = msm_dsi_host_register(msm_dsi->host, true);
>>          if (ret)
>>              return ret;
>> 
>>          msm_dsi_phy_set_usecase(msm_dsi->phy, 
>> MSM_DSI_PHY_STANDALONE);
>>          ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);
> 
> Yes. If we have two independent DSI outputs, we'd like them to work in
> STANDALONE mode.
> 
> 
>>> +            rc = dpu_encoder_setup(dev, encoder, &info);
>>> +            if (rc)
>>> +                DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> +                        encoder->base.id, rc);
>>> +            encoder = NULL;
>>> +        }
>>> +    }
>>> +
>>> +    if (encoder) {
>> 
>> We will hit this case only for split-DSI right? ( that is two DSIs 
>> driving the same panel ).
> 
> Yes, only in this case.
> 
>> Even single DSI will be created in the above loop now. So this looks a 
>> bit confusing at the moment.
> 
> What is so confusing? I can probably add a comment there. If the
> encoder drivers single DSI output, we setup it after creating the DSI.
> If the encoder drives dual DSI outpu, we have to setup it after
> creating both DSI outputs.
Its not just this loop and this small piece of code. Lets look at 
dsi_manager.c.

static int dsi_mgr_setup_components(int id)
{
     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
     struct msm_dsi *other_dsi = dsi_mgr_get_other_dsi(id);
     struct msm_dsi *clk_master_dsi = dsi_mgr_get_dsi(DSI_CLOCK_MASTER);
     struct msm_dsi *clk_slave_dsi = dsi_mgr_get_dsi(DSI_CLOCK_SLAVE);
     int ret;

     if (!IS_DUAL_DSI()) {
         ret = msm_dsi_host_register(msm_dsi->host, true);
         if (ret)
             return ret;

         msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
         ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);
     } else if (!other_dsi) {

Here you are actually hitting this condition even though you are 
operating in dual dsi mode.
But because of the current terminology, it seems like this use-case is 
not dual DSI but it actually is.
So the confusion is more in the DSI driver than in this loop where you 
are calling dpu_encoder_setup()
For this loop a comment should be good enough.

> 
> I have tried calling dpu_encoder_setup from a separate if/loop
> condition, but it resulted in even uglier code.
> 
>> I think we need to be more clear on dual-DSI Vs split-DSI to avoid 
>> confusion in the code about which one means what and the one
>> which we are currently using. So what about having IS_DUAL_DSI() and 
>> IS_SPLIT_DSI() to distinguish the terminologies and chaging
>> DSI driver accordingly.
> 
> The word 'SPLIT' is already overloaded in my opinion. I'd prefer to
> keep on using 'dual DSI' for the master/slave case and not to use
> 'dual' for just two standalone DSI interfaces.
Ok, if SPLIT is overloaded how about using IS_DUAL_DSI() and adding one 
more condition like IS_DUAL_DSI_SYNC()
to emphasize that both the DSIs are not independent and are operating in 
sync.
Like the above condition i mentioned in dsi_manager there are many more 
places where identifying two independent DSIs
and two DSIs operating together for a single panel will be useful in my 
opinion.
So rather than leaving comments everywhere in the dsi code, I was 
suggesting a different terminology.
In this series, the only change which will happen will be that this API 
will change a bit but I dont think its a big change.

+bool msm_dsi_is_dual_dsi(struct msm_dsi *msm_dsi)
+{
+	return IS_DUAL_DSI();
+}

> 
>> 
>>> +        rc = dpu_encoder_setup(dev, encoder, &info);
>>> +        if (rc)
>>> +            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> +                    encoder->base.id, rc);
>>>      }
>>> 
>>>      return rc;
>>> @@ -505,6 +530,7 @@ static int _dpu_kms_initialize_displayport(struct
>>> drm_device *dev,
>>>                          struct dpu_kms *dpu_kms)
>>>  {
>>>      struct drm_encoder *encoder = NULL;
>>> +    struct msm_display_info info;
>>>      int rc = 0;
>>> 
>>>      if (!priv->dp)
>>> @@ -516,6 +542,7 @@ static int _dpu_kms_initialize_displayport(struct
>>> drm_device *dev,
>>>          return PTR_ERR(encoder);
>>>      }
>>> 
>>> +    memset(&info, 0, sizeof(info));
>>>      rc = msm_dp_modeset_init(priv->dp, dev, encoder);
>>>      if (rc) {
>>>          DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>> @@ -524,6 +551,14 @@ static int 
>>> _dpu_kms_initialize_displayport(struct
>>> drm_device *dev,
>>>      }
>>> 
>>>      priv->encoders[priv->num_encoders++] = encoder;
>>> +
>>> +    info.num_of_h_tiles = 1;
>>> +    info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>> +    info.intf_type = encoder->encoder_type;
>>> +    rc = dpu_encoder_setup(dev, encoder, &info);
>>> +    if (rc)
>>> +        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> +            encoder->base.id, rc);
>>>      return rc;
>>>  }
>>> 
>>> @@ -726,41 +761,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>>>      msm_kms_destroy(&dpu_kms->base);
>>>  }
>>> 
>>> -static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
>>> -                 struct drm_encoder *encoder,
>>> -                 bool cmd_mode)
>>> -{
>>> -    struct msm_display_info info;
>>> -    struct msm_drm_private *priv = encoder->dev->dev_private;
>>> -    int i, rc = 0;
>>> -
>>> -    memset(&info, 0, sizeof(info));
>>> -
>>> -    info.intf_type = encoder->encoder_type;
>>> -    info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE :
>>> -            MSM_DISPLAY_CAP_VID_MODE;
>>> -
>>> -    switch (info.intf_type) {
>>> -    case DRM_MODE_ENCODER_DSI:
>>> -        /* TODO: No support for DSI swap */
>>> -        for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>>> -            if (priv->dsi[i]) {
>>> -                info.h_tile_instance[info.num_of_h_tiles] = i;
>>> -                info.num_of_h_tiles++;
>>> -            }
>>> -        }
>>> -        break;
>>> -    case DRM_MODE_ENCODER_TMDS:
>>> -        info.num_of_h_tiles = 1;
>>> -        break;
>>> -    }
>>> -
>>> -    rc = dpu_encoder_setup(encoder->dev, encoder, &info);
>>> -    if (rc)
>>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> -            encoder->base.id, rc);
>>> -}
>>> -
>> It seems we can get rid of set_encoder_mode for DP because the way we 
>> are using it today seems not right.
>> Ideally, the purpose was that once we read the EDID, the information 
>> we read like the tile group etc
>> can be used when we are setting up the encoder. But today, we are just 
>> hard-coding the number of tiles.
>> But I just think whether looking ahead, we should still have some 
>> callback which can be called after
>> EDID has been read instead of doing it in 
>> _dpu_kms_initialize_displayport. Perhaps that can be a separate patch.
> 
> For the MST support? It is definitely a separate patch. For now we
> want to be able to drive a much simpler config: SST on several
> connected DPs.
> 
Agreed on this.
>> 
>>>  static irqreturn_t dpu_irq(struct msm_kms *kms)
>>>  {
>>>      struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>> @@ -863,7 +863,6 @@ static const struct msm_kms_funcs kms_funcs = {
>>>      .get_format      = dpu_get_msm_format,
>>>      .round_pixclk    = dpu_kms_round_pixclk,
>>>      .destroy         = dpu_kms_destroy,
>>> -    .set_encoder_mode = _dpu_kms_set_encoder_mode,
>> I would like to get Rob's comment on why we had set_encoder_mode in 
>> the first place. Its there even in mdp5.
>> 
>> in current msm dsi, the dsi bind will happen only after the panel has 
>> attached
>> and the msm_drv's bind will happen only after that since its the 
>> component master
>> in that case what was the need for set_encoder_mode  because we will 
>> know the panel's video/cmd mode in the dsi_bind call
>> am i missing something about why mdp5 had this?
As discussed with Rob on IRC, we both dont fully recall why this 
set_encoder_mode callback was added for mdp5.
DPU just carried this forward from mdp5.
So to be safe, maybe we can just validate this once on mdp5 to make sure 
it doesnt break.
Otherwise this part of removing the set_encoder_mode seems fine with me.
>> 
>>  From the dpu perspective, since dsi_bind() happens only once panel 
>> has attached.
>>>      .snapshot        = dpu_kms_mdp_snapshot,
>>>  #ifdef CONFIG_DEBUG_FS
>>>      .debugfs_init    = dpu_kms_debugfs_init,

Re: [Freedreno] [RFC 2/6] drm/msm/dpu: support setting up two independent DSI connectors

[Dmitry Baryshkov]

On 02/07/2021 18:52, abhinavk@codeaurora.org wrote:
> On 2021-07-02 02:20, Dmitry Baryshkov wrote:
>> On 02/07/2021 00:12, abhinavk@codeaurora.org wrote:
>>> On 2021-06-09 14:17, Dmitry Baryshkov wrote:
>>>> Move setting up encoders from set_encoder_mode to
>>>> _dpu_kms_initialize_dsi() / _dpu_kms_initialize_displayport(). This
>>>> allows us to support not only "single DSI" and "dual DSI" but also "two
>>>> independent DSI" configurations. In future this would also help adding
>>>> support for multiple DP connectors.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> I will have to see Bjorn's changes to check why it was dependent on 
>>> this cleanup.
>>> Is the plan to call _dpu_kms_initialize_displayport() twice?
>>
>> Yes. He needs to initialize several displayport interfaces. With the
>> current code he has to map ids in the set_encoder_mode, using encoder
>> ids (to fill up the info.h_tile_instance, which is hardcoded to 0 for
>> DP in the current code).
>>
>>> But still I am not able to put together where is the dependency on 
>>> that series
>>> with this one. Can you please elaborate on that a little bit?
>>
>> It is possible to support independent outputs with the current code. I
>> did that for DSI, Bjorn did for DP. However it results in quite an
>> ugly code to map received encoder in set_encoder_mode back to the DSI
>> (DP) instances to fill the h_tiles. If we drop the whole
>> set_encoder_mode story and call dpu_encoder_setup right from the
>> _dpu_kms_initialize_dsi() (or _dpu_kms_initialize_displayport()),
>> supporting multiple outputs becomes an easy task.
>>
> Okay got it, I think it will become more clear once he posts.
>>>
>>>> ---
>>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 89 ++++++++++++-------------
>>>>  1 file changed, 44 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index 1d3a4f395e74..b63e1c948ff2 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -471,30 +471,55 @@ static int _dpu_kms_initialize_dsi(struct 
>>>> drm_device *dev,
>>>>                      struct dpu_kms *dpu_kms)
>>>>  {
>>>>      struct drm_encoder *encoder = NULL;
>>>> +    struct msm_display_info info;
>>>>      int i, rc = 0;
>>>>
>>>>      if (!(priv->dsi[0] || priv->dsi[1]))
>>>>          return rc;
>>>>
>>>> -    /*TODO: Support two independent DSI connectors */
>>>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>>> -    if (IS_ERR(encoder)) {
>>>> -        DPU_ERROR("encoder init failed for dsi display\n");
>>>> -        return PTR_ERR(encoder);
>>>> -    }
>>>> -
>>>> -    priv->encoders[priv->num_encoders++] = encoder;
>>>> -
>>>>      for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>>>>          if (!priv->dsi[i])
>>>>              continue;
>>>>
>>>> +        if (!encoder) {
>>>> +            encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>>> +            if (IS_ERR(encoder)) {
>>>> +                DPU_ERROR("encoder init failed for dsi display\n");
>>>> +                return PTR_ERR(encoder);
>>>> +            }
>>>> +
>>>> +            priv->encoders[priv->num_encoders++] = encoder;
>>>> +
>>>> +            memset(&info, 0, sizeof(info));
>>>> +            info.intf_type = encoder->encoder_type;
>>>> +            info.capabilities = msm_dsi_is_cmd_mode(priv->dsi[i]) ?
>>>> +                MSM_DISPLAY_CAP_CMD_MODE :
>>>> +                MSM_DISPLAY_CAP_VID_MODE;
>>>> +        }
>>>> +
>>>>          rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>>          if (rc) {
>>>>              DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>>>>                  i, rc);
>>>>              break;
>>>>          }
>>>> +
>>>> +        info.h_tile_instance[info.num_of_h_tiles++] = i;
>>>> +
>>>> +        if (!msm_dsi_is_dual_dsi(priv->dsi[i])) {
>>>
>>> I would like to clarify the terminology of dual_dsi in the current 
>>> DSI driver before the rest of the reviews.
>>> Today IS_DUAL_DSI() means that two DSIs are driving the same display 
>>> and the two DSIs are operating in master-slave mode
>>> and are being driven by the same PLL.
>>
>> Yes
>>
>>> Usually, dual independent DSI means two DSIs driving two separate 
>>> panels using two separate PLLs ( DSI0 with PLL0 and DSI1 with PLL1)
>>
>> Let's stop calling it 'dual'. I'd suggest to continue using what was
>> there in the source file: 'two independent DSI'.
>>
>>> I assume thats happening due to the foll logic and both DSI PHYs are 
>>> operating in STANDALONE mode:
>>>
>>>      if (!IS_DUAL_DSI()) {
>>>          ret = msm_dsi_host_register(msm_dsi->host, true);
>>>          if (ret)
>>>              return ret;
>>>
>>>          msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
>>>          ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);
>>
>> Yes. If we have two independent DSI outputs, we'd like them to work in
>> STANDALONE mode.
>>
>>
>>>> +            rc = dpu_encoder_setup(dev, encoder, &info);
>>>> +            if (rc)
>>>> +                DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> +                        encoder->base.id, rc);
>>>> +            encoder = NULL;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (encoder) {
>>>
>>> We will hit this case only for split-DSI right? ( that is two DSIs 
>>> driving the same panel ).
>>
>> Yes, only in this case.
>>
>>> Even single DSI will be created in the above loop now. So this looks 
>>> a bit confusing at the moment.
>>
>> What is so confusing? I can probably add a comment there. If the
>> encoder drivers single DSI output, we setup it after creating the DSI.
>> If the encoder drives dual DSI outpu, we have to setup it after
>> creating both DSI outputs.
> Its not just this loop and this small piece of code. Lets look at 
> dsi_manager.c.
> 
> static int dsi_mgr_setup_components(int id)
> {
>      struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>      struct msm_dsi *other_dsi = dsi_mgr_get_other_dsi(id);
>      struct msm_dsi *clk_master_dsi = dsi_mgr_get_dsi(DSI_CLOCK_MASTER);
>      struct msm_dsi *clk_slave_dsi = dsi_mgr_get_dsi(DSI_CLOCK_SLAVE);
>      int ret;
> 
>      if (!IS_DUAL_DSI()) {
>          ret = msm_dsi_host_register(msm_dsi->host, true);
>          if (ret)
>              return ret;
> 
>          msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
>          ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);
>      } else if (!other_dsi) {
> 
> Here you are actually hitting this condition even though you are 
> operating in dual dsi mode.

Please correct me if I'm wrong. I'm not operating in dual DSI mode. With 
two independent DSIs I'm operating both DSIs in standalone modes. There 
should be no master/slave relationship, etc.

I have two dsi interfaces, neither of them having "qcom,dual-dsi" property.

> But because of the current terminology, it seems like this use-case is 
> not dual DSI but it actually is.

Why?

> So the confusion is more in the DSI driver than in this loop where you 
> are calling dpu_encoder_setup()
> For this loop a comment should be good enough.
> 
>>
>> I have tried calling dpu_encoder_setup from a separate if/loop
>> condition, but it resulted in even uglier code.
>>
>>> I think we need to be more clear on dual-DSI Vs split-DSI to avoid 
>>> confusion in the code about which one means what and the one
>>> which we are currently using. So what about having IS_DUAL_DSI() and 
>>> IS_SPLIT_DSI() to distinguish the terminologies and chaging
>>> DSI driver accordingly.
>>
>> The word 'SPLIT' is already overloaded in my opinion. I'd prefer to
>> keep on using 'dual DSI' for the master/slave case and not to use
>> 'dual' for just two standalone DSI interfaces.
> Ok, if SPLIT is overloaded how about using IS_DUAL_DSI() and adding one 
> more condition like IS_DUAL_DSI_SYNC()
> to emphasize that both the DSIs are not independent and are operating in 
> sync.

Coul you please describe, why would you like to have additional 
conditions? I did not have to add any conditional code. dsi_manager 
perfectly handles everything.

> Like the above condition i mentioned in dsi_manager there are many more 
> places where identifying two independent DSIs
> and two DSIs operating together for a single panel will be useful in my 
> opinion.

What for?

> So rather than leaving comments everywhere in the dsi code, I was 
> suggesting a different terminology.
> In this series, the only change which will happen will be that this API 
> will change a bit but I dont think its a big change.
> 
> +bool msm_dsi_is_dual_dsi(struct msm_dsi *msm_dsi)
> +{
> +    return IS_DUAL_DSI();
> +}
> 
>>
>>>
>>>> +        rc = dpu_encoder_setup(dev, encoder, &info);
>>>> +        if (rc)
>>>> +            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> +                    encoder->base.id, rc);
>>>>      }
>>>>
>>>>      return rc;
>>>> @@ -505,6 +530,7 @@ static int _dpu_kms_initialize_displayport(struct
>>>> drm_device *dev,
>>>>                          struct dpu_kms *dpu_kms)
>>>>  {
>>>>      struct drm_encoder *encoder = NULL;
>>>> +    struct msm_display_info info;
>>>>      int rc = 0;
>>>>
>>>>      if (!priv->dp)
>>>> @@ -516,6 +542,7 @@ static int _dpu_kms_initialize_displayport(struct
>>>> drm_device *dev,
>>>>          return PTR_ERR(encoder);
>>>>      }
>>>>
>>>> +    memset(&info, 0, sizeof(info));
>>>>      rc = msm_dp_modeset_init(priv->dp, dev, encoder);
>>>>      if (rc) {
>>>>          DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>>> @@ -524,6 +551,14 @@ static int _dpu_kms_initialize_displayport(struct
>>>> drm_device *dev,
>>>>      }
>>>>
>>>>      priv->encoders[priv->num_encoders++] = encoder;
>>>> +
>>>> +    info.num_of_h_tiles = 1;
>>>> +    info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>>> +    info.intf_type = encoder->encoder_type;
>>>> +    rc = dpu_encoder_setup(dev, encoder, &info);
>>>> +    if (rc)
>>>> +        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> +            encoder->base.id, rc);
>>>>      return rc;
>>>>  }
>>>>
>>>> @@ -726,41 +761,6 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>>>>      msm_kms_destroy(&dpu_kms->base);
>>>>  }
>>>>
>>>> -static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
>>>> -                 struct drm_encoder *encoder,
>>>> -                 bool cmd_mode)
>>>> -{
>>>> -    struct msm_display_info info;
>>>> -    struct msm_drm_private *priv = encoder->dev->dev_private;
>>>> -    int i, rc = 0;
>>>> -
>>>> -    memset(&info, 0, sizeof(info));
>>>> -
>>>> -    info.intf_type = encoder->encoder_type;
>>>> -    info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE :
>>>> -            MSM_DISPLAY_CAP_VID_MODE;
>>>> -
>>>> -    switch (info.intf_type) {
>>>> -    case DRM_MODE_ENCODER_DSI:
>>>> -        /* TODO: No support for DSI swap */
>>>> -        for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>>>> -            if (priv->dsi[i]) {
>>>> -                info.h_tile_instance[info.num_of_h_tiles] = i;
>>>> -                info.num_of_h_tiles++;
>>>> -            }
>>>> -        }
>>>> -        break;
>>>> -    case DRM_MODE_ENCODER_TMDS:
>>>> -        info.num_of_h_tiles = 1;
>>>> -        break;
>>>> -    }
>>>> -
>>>> -    rc = dpu_encoder_setup(encoder->dev, encoder, &info);
>>>> -    if (rc)
>>>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> -            encoder->base.id, rc);
>>>> -}
>>>> -
>>> It seems we can get rid of set_encoder_mode for DP because the way we 
>>> are using it today seems not right.
>>> Ideally, the purpose was that once we read the EDID, the information 
>>> we read like the tile group etc
>>> can be used when we are setting up the encoder. But today, we are 
>>> just hard-coding the number of tiles.
>>> But I just think whether looking ahead, we should still have some 
>>> callback which can be called after
>>> EDID has been read instead of doing it in 
>>> _dpu_kms_initialize_displayport. Perhaps that can be a separate patch.
>>
>> For the MST support? It is definitely a separate patch. For now we
>> want to be able to drive a much simpler config: SST on several
>> connected DPs.
>>
> Agreed on this.
>>>
>>>>  static irqreturn_t dpu_irq(struct msm_kms *kms)
>>>>  {
>>>>      struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>>> @@ -863,7 +863,6 @@ static const struct msm_kms_funcs kms_funcs = {
>>>>      .get_format      = dpu_get_msm_format,
>>>>      .round_pixclk    = dpu_kms_round_pixclk,
>>>>      .destroy         = dpu_kms_destroy,
>>>> -    .set_encoder_mode = _dpu_kms_set_encoder_mode,
>>> I would like to get Rob's comment on why we had set_encoder_mode in 
>>> the first place. Its there even in mdp5.
>>>
>>> in current msm dsi, the dsi bind will happen only after the panel has 
>>> attached
>>> and the msm_drv's bind will happen only after that since its the 
>>> component master
>>> in that case what was the need for set_encoder_mode  because we will 
>>> know the panel's video/cmd mode in the dsi_bind call
>>> am i missing something about why mdp5 had this?
> As discussed with Rob on IRC, we both dont fully recall why this 
> set_encoder_mode callback was added for mdp5.
> DPU just carried this forward from mdp5.
> So to be safe, maybe we can just validate this once on mdp5 to make sure 
> it doesnt break.
> Otherwise this part of removing the set_encoder_mode seems fine with me.

Yes, fine with me.


>>>
>>>  From the dpu perspective, since dsi_bind() happens only once panel 
>>> has attached.
>>>>      .snapshot        = dpu_kms_mdp_snapshot,
>>>>  #ifdef CONFIG_DEBUG_FS
>>>>      .debugfs_init    = dpu_kms_debugfs_init,


-- 
With best wishes
Dmitry

Re: [Freedreno] [RFC 2/6] drm/msm/dpu: support setting up two independent DSI connectors

[abhinavk]

On 2021-07-02 10:10, Dmitry Baryshkov wrote:
> On 02/07/2021 18:52, abhinavk@codeaurora.org wrote:
>> On 2021-07-02 02:20, Dmitry Baryshkov wrote:
>>> On 02/07/2021 00:12, abhinavk@codeaurora.org wrote:
>>>> On 2021-06-09 14:17, Dmitry Baryshkov wrote:
>>>>> Move setting up encoders from set_encoder_mode to
>>>>> _dpu_kms_initialize_dsi() / _dpu_kms_initialize_displayport(). This
>>>>> allows us to support not only "single DSI" and "dual DSI" but also 
>>>>> "two
>>>>> independent DSI" configurations. In future this would also help 
>>>>> adding
>>>>> support for multiple DP connectors.
>>>>> 
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> I will have to see Bjorn's changes to check why it was dependent on 
>>>> this cleanup.
>>>> Is the plan to call _dpu_kms_initialize_displayport() twice?
>>> 
>>> Yes. He needs to initialize several displayport interfaces. With the
>>> current code he has to map ids in the set_encoder_mode, using encoder
>>> ids (to fill up the info.h_tile_instance, which is hardcoded to 0 for
>>> DP in the current code).
>>> 
>>>> But still I am not able to put together where is the dependency on 
>>>> that series
>>>> with this one. Can you please elaborate on that a little bit?
>>> 
>>> It is possible to support independent outputs with the current code. 
>>> I
>>> did that for DSI, Bjorn did for DP. However it results in quite an
>>> ugly code to map received encoder in set_encoder_mode back to the DSI
>>> (DP) instances to fill the h_tiles. If we drop the whole
>>> set_encoder_mode story and call dpu_encoder_setup right from the
>>> _dpu_kms_initialize_dsi() (or _dpu_kms_initialize_displayport()),
>>> supporting multiple outputs becomes an easy task.
>>> 
>> Okay got it, I think it will become more clear once he posts.
>>>> 
>>>>> ---
>>>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 89 
>>>>> ++++++++++++-------------
>>>>>  1 file changed, 44 insertions(+), 45 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> index 1d3a4f395e74..b63e1c948ff2 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> @@ -471,30 +471,55 @@ static int _dpu_kms_initialize_dsi(struct 
>>>>> drm_device *dev,
>>>>>                      struct dpu_kms *dpu_kms)
>>>>>  {
>>>>>      struct drm_encoder *encoder = NULL;
>>>>> +    struct msm_display_info info;
>>>>>      int i, rc = 0;
>>>>> 
>>>>>      if (!(priv->dsi[0] || priv->dsi[1]))
>>>>>          return rc;
>>>>> 
>>>>> -    /*TODO: Support two independent DSI connectors */
>>>>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>>>> -    if (IS_ERR(encoder)) {
>>>>> -        DPU_ERROR("encoder init failed for dsi display\n");
>>>>> -        return PTR_ERR(encoder);
>>>>> -    }
>>>>> -
>>>>> -    priv->encoders[priv->num_encoders++] = encoder;
>>>>> -
>>>>>      for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>>>>>          if (!priv->dsi[i])
>>>>>              continue;
>>>>> 
>>>>> +        if (!encoder) {
>>>>> +            encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>>>> +            if (IS_ERR(encoder)) {
>>>>> +                DPU_ERROR("encoder init failed for dsi 
>>>>> display\n");
>>>>> +                return PTR_ERR(encoder);
>>>>> +            }
>>>>> +
>>>>> +            priv->encoders[priv->num_encoders++] = encoder;
>>>>> +
>>>>> +            memset(&info, 0, sizeof(info));
>>>>> +            info.intf_type = encoder->encoder_type;
>>>>> +            info.capabilities = msm_dsi_is_cmd_mode(priv->dsi[i]) 
>>>>> ?
>>>>> +                MSM_DISPLAY_CAP_CMD_MODE :
>>>>> +                MSM_DISPLAY_CAP_VID_MODE;
>>>>> +        }
>>>>> +
>>>>>          rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>>>          if (rc) {
>>>>>              DPU_ERROR("modeset_init failed for dsi[%d], rc = 
>>>>> %d\n",
>>>>>                  i, rc);
>>>>>              break;
>>>>>          }
>>>>> +
>>>>> +        info.h_tile_instance[info.num_of_h_tiles++] = i;
>>>>> +
>>>>> +        if (!msm_dsi_is_dual_dsi(priv->dsi[i])) {
>>>> 
>>>> I would like to clarify the terminology of dual_dsi in the current 
>>>> DSI driver before the rest of the reviews.
>>>> Today IS_DUAL_DSI() means that two DSIs are driving the same display 
>>>> and the two DSIs are operating in master-slave mode
>>>> and are being driven by the same PLL.
>>> 
>>> Yes
>>> 
>>>> Usually, dual independent DSI means two DSIs driving two separate 
>>>> panels using two separate PLLs ( DSI0 with PLL0 and DSI1 with PLL1)
>>> 
>>> Let's stop calling it 'dual'. I'd suggest to continue using what was
>>> there in the source file: 'two independent DSI'.
>>> 
>>>> I assume thats happening due to the foll logic and both DSI PHYs are 
>>>> operating in STANDALONE mode:
>>>> 
>>>>      if (!IS_DUAL_DSI()) {
>>>>          ret = msm_dsi_host_register(msm_dsi->host, true);
>>>>          if (ret)
>>>>              return ret;
>>>> 
>>>>          msm_dsi_phy_set_usecase(msm_dsi->phy, 
>>>> MSM_DSI_PHY_STANDALONE);
>>>>          ret = msm_dsi_host_set_src_pll(msm_dsi->host, 
>>>> msm_dsi->phy);
>>> 
>>> Yes. If we have two independent DSI outputs, we'd like them to work 
>>> in
>>> STANDALONE mode.
>>> 
>>> 
>>>>> +            rc = dpu_encoder_setup(dev, encoder, &info);
>>>>> +            if (rc)
>>>>> +                DPU_ERROR("failed to setup DPU encoder %d: 
>>>>> rc:%d\n",
>>>>> +                        encoder->base.id, rc);
>>>>> +            encoder = NULL;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    if (encoder) {
>>>> 
>>>> We will hit this case only for split-DSI right? ( that is two DSIs 
>>>> driving the same panel ).
>>> 
>>> Yes, only in this case.
>>> 
>>>> Even single DSI will be created in the above loop now. So this looks 
>>>> a bit confusing at the moment.
>>> 
>>> What is so confusing? I can probably add a comment there. If the
>>> encoder drivers single DSI output, we setup it after creating the 
>>> DSI.
>>> If the encoder drives dual DSI outpu, we have to setup it after
>>> creating both DSI outputs.
>> Its not just this loop and this small piece of code. Lets look at 
>> dsi_manager.c.
>> 
>> static int dsi_mgr_setup_components(int id)
>> {
>>      struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>      struct msm_dsi *other_dsi = dsi_mgr_get_other_dsi(id);
>>      struct msm_dsi *clk_master_dsi = 
>> dsi_mgr_get_dsi(DSI_CLOCK_MASTER);
>>      struct msm_dsi *clk_slave_dsi = dsi_mgr_get_dsi(DSI_CLOCK_SLAVE);
>>      int ret;
>> 
>>      if (!IS_DUAL_DSI()) {
>>          ret = msm_dsi_host_register(msm_dsi->host, true);
>>          if (ret)
>>              return ret;
>> 
>>          msm_dsi_phy_set_usecase(msm_dsi->phy, 
>> MSM_DSI_PHY_STANDALONE);
>>          ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);
>>      } else if (!other_dsi) {
>> 
>> Here you are actually hitting this condition even though you are 
>> operating in dual dsi mode.
> 
> Please correct me if I'm wrong. I'm not operating in dual DSI mode.
> With two independent DSIs I'm operating both DSIs in standalone modes.
> There should be no master/slave relationship, etc.
> 
This is again just a difference in terminology.
Yes, you are not operating in "dual_dsi" mode because today "dual_dsi" 
means using two DSIs operating
in master-slave mode.
But just in literal terms and also from the MSM perspective you are 
still using dual DSI because
both the DSIs are in use. So literally speaking, this is still dual DSI 
from MSM perspective as both
the DSIs are in use just not in master-slave mode but in standalone 
mode.
Thats why I am suggesting that it should be more clearly handled in code 
that you are still using both
DSIs but just not in master-slave mode.

Another thing I can think of is that, can there be a use-case where 
there are two DSIs but for
sending the commands, we still need to sync both of them.
For example for VR goggles, there are two separate panels but we might 
need to send commands to both
at the same time. In that case also this will be useful.

     /* We assume 2 dsi nodes have the same information of dual-dsi and
      * sync-mode, and only one node specifies master in case of dual 
mode.
      */
     if (!msm_dsim->is_dual_dsi)
         msm_dsim->is_dual_dsi = of_property_read_bool(
                         np, "qcom,dual-dsi-mode");

     if (msm_dsim->is_dual_dsi) {
         if (of_property_read_bool(np, "qcom,master-dsi"))
             msm_dsim->master_dsi_link_id = id;
         if (!msm_dsim->is_sync_needed)
             msm_dsim->is_sync_needed = of_property_read_bool(
                     np, "qcom,sync-dual-dsi");
     }


> I have two dsi interfaces, neither of them having "qcom,dual-dsi" 
> property.
> 
>> But because of the current terminology, it seems like this use-case is 
>> not dual DSI but it actually is.
> 
> Why?
> 
>> So the confusion is more in the DSI driver than in this loop where you 
>> are calling dpu_encoder_setup()
>> For this loop a comment should be good enough.
>> 
>>> 
>>> I have tried calling dpu_encoder_setup from a separate if/loop
>>> condition, but it resulted in even uglier code.
>>> 
>>>> I think we need to be more clear on dual-DSI Vs split-DSI to avoid 
>>>> confusion in the code about which one means what and the one
>>>> which we are currently using. So what about having IS_DUAL_DSI() and 
>>>> IS_SPLIT_DSI() to distinguish the terminologies and chaging
>>>> DSI driver accordingly.
>>> 
>>> The word 'SPLIT' is already overloaded in my opinion. I'd prefer to
>>> keep on using 'dual DSI' for the master/slave case and not to use
>>> 'dual' for just two standalone DSI interfaces.
>> Ok, if SPLIT is overloaded how about using IS_DUAL_DSI() and adding 
>> one more condition like IS_DUAL_DSI_SYNC()
>> to emphasize that both the DSIs are not independent and are operating 
>> in sync.
> 
> Coul you please describe, why would you like to have additional
> conditions? I did not have to add any conditional code. dsi_manager
> perfectly handles everything.



> 
>> Like the above condition i mentioned in dsi_manager there are many 
>> more places where identifying two independent DSIs
>> and two DSIs operating together for a single panel will be useful in 
>> my opinion.
> 
> What for?
> 
>> So rather than leaving comments everywhere in the dsi code, I was 
>> suggesting a different terminology.
>> In this series, the only change which will happen will be that this 
>> API will change a bit but I dont think its a big change.
>> 
>> +bool msm_dsi_is_dual_dsi(struct msm_dsi *msm_dsi)
>> +{
>> +    return IS_DUAL_DSI();
>> +}
>> 
>>> 
>>>> 
>>>>> +        rc = dpu_encoder_setup(dev, encoder, &info);
>>>>> +        if (rc)
>>>>> +            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>>> +                    encoder->base.id, rc);
>>>>>      }
>>>>> 
>>>>>      return rc;
>>>>> @@ -505,6 +530,7 @@ static int 
>>>>> _dpu_kms_initialize_displayport(struct
>>>>> drm_device *dev,
>>>>>                          struct dpu_kms *dpu_kms)
>>>>>  {
>>>>>      struct drm_encoder *encoder = NULL;
>>>>> +    struct msm_display_info info;
>>>>>      int rc = 0;
>>>>> 
>>>>>      if (!priv->dp)
>>>>> @@ -516,6 +542,7 @@ static int 
>>>>> _dpu_kms_initialize_displayport(struct
>>>>> drm_device *dev,
>>>>>          return PTR_ERR(encoder);
>>>>>      }
>>>>> 
>>>>> +    memset(&info, 0, sizeof(info));
>>>>>      rc = msm_dp_modeset_init(priv->dp, dev, encoder);
>>>>>      if (rc) {
>>>>>          DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>>>> @@ -524,6 +551,14 @@ static int 
>>>>> _dpu_kms_initialize_displayport(struct
>>>>> drm_device *dev,
>>>>>      }
>>>>> 
>>>>>      priv->encoders[priv->num_encoders++] = encoder;
>>>>> +
>>>>> +    info.num_of_h_tiles = 1;
>>>>> +    info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>>>> +    info.intf_type = encoder->encoder_type;
>>>>> +    rc = dpu_encoder_setup(dev, encoder, &info);
>>>>> +    if (rc)
>>>>> +        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>>> +            encoder->base.id, rc);
>>>>>      return rc;
>>>>>  }
>>>>> 
>>>>> @@ -726,41 +761,6 @@ static void dpu_kms_destroy(struct msm_kms 
>>>>> *kms)
>>>>>      msm_kms_destroy(&dpu_kms->base);
>>>>>  }
>>>>> 
>>>>> -static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
>>>>> -                 struct drm_encoder *encoder,
>>>>> -                 bool cmd_mode)
>>>>> -{
>>>>> -    struct msm_display_info info;
>>>>> -    struct msm_drm_private *priv = encoder->dev->dev_private;
>>>>> -    int i, rc = 0;
>>>>> -
>>>>> -    memset(&info, 0, sizeof(info));
>>>>> -
>>>>> -    info.intf_type = encoder->encoder_type;
>>>>> -    info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE :
>>>>> -            MSM_DISPLAY_CAP_VID_MODE;
>>>>> -
>>>>> -    switch (info.intf_type) {
>>>>> -    case DRM_MODE_ENCODER_DSI:
>>>>> -        /* TODO: No support for DSI swap */
>>>>> -        for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>>>>> -            if (priv->dsi[i]) {
>>>>> -                info.h_tile_instance[info.num_of_h_tiles] = i;
>>>>> -                info.num_of_h_tiles++;
>>>>> -            }
>>>>> -        }
>>>>> -        break;
>>>>> -    case DRM_MODE_ENCODER_TMDS:
>>>>> -        info.num_of_h_tiles = 1;
>>>>> -        break;
>>>>> -    }
>>>>> -
>>>>> -    rc = dpu_encoder_setup(encoder->dev, encoder, &info);
>>>>> -    if (rc)
>>>>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>>> -            encoder->base.id, rc);
>>>>> -}
>>>>> -
>>>> It seems we can get rid of set_encoder_mode for DP because the way 
>>>> we are using it today seems not right.
>>>> Ideally, the purpose was that once we read the EDID, the information 
>>>> we read like the tile group etc
>>>> can be used when we are setting up the encoder. But today, we are 
>>>> just hard-coding the number of tiles.
>>>> But I just think whether looking ahead, we should still have some 
>>>> callback which can be called after
>>>> EDID has been read instead of doing it in 
>>>> _dpu_kms_initialize_displayport. Perhaps that can be a separate 
>>>> patch.
>>> 
>>> For the MST support? It is definitely a separate patch. For now we
>>> want to be able to drive a much simpler config: SST on several
>>> connected DPs.
>>> 
>> Agreed on this.
>>>> 
>>>>>  static irqreturn_t dpu_irq(struct msm_kms *kms)
>>>>>  {
>>>>>      struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>>>> @@ -863,7 +863,6 @@ static const struct msm_kms_funcs kms_funcs = {
>>>>>      .get_format      = dpu_get_msm_format,
>>>>>      .round_pixclk    = dpu_kms_round_pixclk,
>>>>>      .destroy         = dpu_kms_destroy,
>>>>> -    .set_encoder_mode = _dpu_kms_set_encoder_mode,
>>>> I would like to get Rob's comment on why we had set_encoder_mode in 
>>>> the first place. Its there even in mdp5.
>>>> 
>>>> in current msm dsi, the dsi bind will happen only after the panel 
>>>> has attached
>>>> and the msm_drv's bind will happen only after that since its the 
>>>> component master
>>>> in that case what was the need for set_encoder_mode  because we will 
>>>> know the panel's video/cmd mode in the dsi_bind call
>>>> am i missing something about why mdp5 had this?
>> As discussed with Rob on IRC, we both dont fully recall why this 
>> set_encoder_mode callback was added for mdp5.
>> DPU just carried this forward from mdp5.
>> So to be safe, maybe we can just validate this once on mdp5 to make 
>> sure it doesnt break.
>> Otherwise this part of removing the set_encoder_mode seems fine with 
>> me.
> 
> Yes, fine with me.
> 
> 
>>>> 
>>>>  From the dpu perspective, since dsi_bind() happens only once panel 
>>>> has attached.
>>>>>      .snapshot        = dpu_kms_mdp_snapshot,
>>>>>  #ifdef CONFIG_DEBUG_FS
>>>>>      .debugfs_init    = dpu_kms_debugfs_init,

[RFC 3/6] drm/msm/mdp5: move mdp5_encoder_set_intf_mode after msm_dsi_modeset_init

[Dmitry Baryshkov]

Move a call to mdp5_encoder_set_intf_mode() after
msm_dsi_modeset_init(), removing set_encoder_mode callback.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 15aed45022bc..b3b42672b2d4 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -209,13 +209,6 @@ static int mdp5_set_split_display(struct msm_kms *kms,
 							  slave_encoder);
 }
 
-static void mdp5_set_encoder_mode(struct msm_kms *kms,
-				  struct drm_encoder *encoder,
-				  bool cmd_mode)
-{
-	mdp5_encoder_set_intf_mode(encoder, cmd_mode);
-}
-
 static void mdp5_kms_destroy(struct msm_kms *kms)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
@@ -287,7 +280,6 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.get_format      = mdp_get_format,
 		.round_pixclk    = mdp5_round_pixclk,
 		.set_split_display = mdp5_set_split_display,
-		.set_encoder_mode = mdp5_set_encoder_mode,
 		.destroy         = mdp5_kms_destroy,
 #ifdef CONFIG_DEBUG_FS
 		.debugfs_init    = mdp5_kms_debugfs_init,
@@ -448,6 +440,9 @@ static int modeset_init_intf(struct mdp5_kms *mdp5_kms,
 		}
 
 		ret = msm_dsi_modeset_init(priv->dsi[dsi_id], dev, encoder);
+		if (!ret)
+			mdp5_encoder_set_intf_mode(encoder, msm_dsi_is_cmd_mode(priv->dsi[dsi_id]));
+
 		break;
 	}
 	default:
-- 
2.30.2

Re: [Freedreno] [RFC 3/6] drm/msm/mdp5: move mdp5_encoder_set_intf_mode after msm_dsi_modeset_init

[Alexey Minnekhanov]

On 6/10/21 12:17 AM, Dmitry Baryshkov wrote:
 > Move a call to mdp5_encoder_set_intf_mode() after
 > msm_dsi_modeset_init(), removing set_encoder_mode callback.
 >
 > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
 > ---
 >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 +++--------
 >   1 file changed, 3 insertions(+), 8 deletions(-)
 >

My Samsung Galaxy S5 with mdp5 and cmd mode panel seems to work same as 
before with these patches applied.

Tested-by: Alexey Minnekhanov <alexeymin@postmarketos.org>

[RFC 4/6] drm/msm/dp: stop calling set_encoder_mode callback

[Dmitry Baryshkov]

None of the display drivers now implement set_encoder_mode callback.
Stop calling it from the modeset init code.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 051c1be1de7e..70b319a8fe83 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -102,8 +102,6 @@ struct dp_display_private {
 	struct dp_display_mode dp_mode;
 	struct msm_dp dp_display;
 
-	bool encoder_mode_set;
-
 	/* wait for audio signaling */
 	struct completion audio_comp;
 
@@ -283,20 +281,6 @@ static void dp_display_send_hpd_event(struct msm_dp *dp_display)
 }
 
 
-static void dp_display_set_encoder_mode(struct dp_display_private *dp)
-{
-	struct msm_drm_private *priv = dp->dp_display.drm_dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	if (!dp->encoder_mode_set && dp->dp_display.encoder &&
-				kms->funcs->set_encoder_mode) {
-		kms->funcs->set_encoder_mode(kms,
-				dp->dp_display.encoder, false);
-
-		dp->encoder_mode_set = true;
-	}
-}
-
 static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 					    bool hpd)
 {
@@ -369,8 +353,6 @@ static void dp_display_host_init(struct dp_display_private *dp, int reset)
 	if (dp->usbpd->orientation == ORIENTATION_CC2)
 		flip = true;
 
-	dp_display_set_encoder_mode(dp);
-
 	dp_power_init(dp->power, flip);
 	dp_ctrl_host_init(dp->ctrl, flip, reset);
 	dp_aux_init(dp->aux);
-- 
2.30.2

[RFC 5/6] drm/msm/dsi: stop calling set_encoder_mode callback

[Dmitry Baryshkov]

None of the display drivers now implement set_encoder_mode callback.
Stop calling it from the modeset init code.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.c         |  2 --
 drivers/gpu/drm/msm/dsi/dsi.h         |  1 -
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 12 ------------
 3 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 874c1527d300..881f14bc022d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -250,8 +250,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 		goto fail;
 	}
 
-	msm_dsi_manager_setup_encoder(msm_dsi->id);
-
 	priv->bridges[priv->num_bridges++]       = msm_dsi->bridge;
 	priv->connectors[priv->num_connectors++] = msm_dsi->connector;
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 9b8e9b07eced..c0bdbe63050a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -80,7 +80,6 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id);
 struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id);
 int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
 bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
-void msm_dsi_manager_setup_encoder(int id);
 int msm_dsi_manager_register(struct msm_dsi *msm_dsi);
 void msm_dsi_manager_unregister(struct msm_dsi *msm_dsi);
 bool msm_dsi_manager_validate_current_config(u8 id);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 7d4f6fae1ab0..1996b40d2ae9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -217,18 +217,6 @@ static int dsi_mgr_bridge_get_id(struct drm_bridge *bridge)
 	return dsi_bridge->id;
 }
 
-void msm_dsi_manager_setup_encoder(int id)
-{
-	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
-	struct msm_drm_private *priv = msm_dsi->dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-	struct drm_encoder *encoder = msm_dsi_get_encoder(msm_dsi);
-
-	if (encoder && kms->funcs->set_encoder_mode)
-		kms->funcs->set_encoder_mode(kms, encoder,
-					     msm_dsi_is_cmd_mode(msm_dsi));
-}
-
 static int msm_dsi_manager_panel_init(struct drm_connector *conn, u8 id)
 {
 	struct msm_drm_private *priv = conn->dev->dev_private;
-- 
2.30.2

[RFC 6/6] drm/msm/kms: drop set_encoder_mode callback

[Dmitry Baryshkov]

set_encoder_mode callback is completely unused now. Drop it from
msm_kms_func().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_kms.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 086a2d59b8c8..9484e8b62630 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -117,9 +117,6 @@ struct msm_kms_funcs {
 			struct drm_encoder *encoder,
 			struct drm_encoder *slave_encoder,
 			bool is_cmd_mode);
-	void (*set_encoder_mode)(struct msm_kms *kms,
-				 struct drm_encoder *encoder,
-				 bool cmd_mode);
 	/* cleanup: */
 	void (*destroy)(struct msm_kms *kms);
 
-- 
2.30.2