diff mbox series

[v2,3/7] drm/msm/dpu: support setting up two independent DSI connectors

Message ID 20210709235024.1077888-4-dmitry.baryshkov@linaro.org
State New
Headers show
Series drm/msm/dpu: add support for independent DSI config | expand

Commit Message

Dmitry Baryshkov July 9, 2021, 11:50 p.m. UTC 
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 "bonded 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 | 130 ++++++++++++++----------
 1 file changed, 79 insertions(+), 51 deletions(-)

-- 
2.30.2

Comments

Abhinav Kumar July 10, 2021, 12:30 a.m. UTC | #1 
On 2021-07-09 16:50, 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 "bonded 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 | 130 ++++++++++++++----------

>  1 file changed, 79 insertions(+), 51 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..e14eb8f94cd7 100644

> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c

> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c

> @@ -466,17 +466,16 @@ static void dpu_kms_wait_flush(struct msm_kms

> *kms, unsigned crtc_mask)

>  		dpu_kms_wait_for_commit_done(kms, crtc);

>  }

> 

> -static int _dpu_kms_initialize_dsi(struct drm_device *dev,

> -				    struct msm_drm_private *priv,

> -				    struct dpu_kms *dpu_kms)

> +static int _dpu_kms_initialize_dsi_encoder(struct drm_device *dev,

> +					   struct msm_drm_private *priv,

> +					   struct dpu_kms *dpu_kms,

> +					   int dsi_id, int dsi_id1)

>  {

> +	struct msm_dsi *dsi = priv->dsi[dsi_id];

>  	struct drm_encoder *encoder = NULL;

> -	int i, rc = 0;

> -

> -	if (!(priv->dsi[0] || priv->dsi[1]))

> -		return rc;

> +	struct msm_display_info info;

> +	int rc = 0;

> 

> -	/*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");

> @@ -485,19 +484,74 @@ static int _dpu_kms_initialize_dsi(struct 

> drm_device *dev,

> 

>  	priv->encoders[priv->num_encoders++] = encoder;

> 

> -	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

> -		if (!priv->dsi[i])

> -			continue;

> +	rc = msm_dsi_modeset_init(dsi, dev, encoder);

> +	if (rc) {

> +		DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",

> +			  dsi_id, rc);

> +		return rc;

> +	}

> +

> +	memset(&info, 0, sizeof(info));

> +	info.intf_type = encoder->encoder_type;

> +	info.capabilities = msm_dsi_is_cmd_mode(dsi) ?

> +		MSM_DISPLAY_CAP_CMD_MODE :

> +		MSM_DISPLAY_CAP_VID_MODE;

> +	info.h_tile_instance[info.num_of_h_tiles++] = dsi_id;

> 

> -		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);

> +	/* For the bonded DSI setup we have second DSI host */

> +	if (dsi_id1 >= 0) {

> +		struct msm_dsi *dsi1 = priv->dsi[dsi_id1];

> +

> +		rc = msm_dsi_modeset_init(dsi1, dev, encoder);

>  		if (rc) {

>  			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",

> -				i, rc);

> -			break;

> +				  dsi_id1, rc);

> +			return rc;

>  		}

> +

> +		info.h_tile_instance[info.num_of_h_tiles++] = dsi_id1;

>  	}

> 

> -	return rc;

> +	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;

> +	}

> +

> +	return 0;

> +}

> +

> +static int _dpu_kms_initialize_dsi(struct drm_device *dev,

> +				    struct msm_drm_private *priv,

> +				    struct dpu_kms *dpu_kms)

> +{

> +	int i, rc = 0;

> +

> +	if (!(priv->dsi[0] || priv->dsi[1]))

> +		return rc;

> +

> +	/*

> +	 * We support following confiurations:

> +	 * - Single DSI host (dsi0 or dsi1)

> +	 * - Two independent DSI hosts

> +	 * - Bonded DSI0 and DSI1 hosts

> +	 *

> +	 *   TODO: Support swapping DSI0 and DSI1 in the bonded setup.

> +	 */

> +	if (priv->dsi[0] && priv->dsi[1] && 

> msm_dsi_is_bonded_dsi(priv->dsi[0]))

> +		return _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, 0, 1);

> +

> +	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

> +		if (!priv->dsi[i])

> +			continue;

> +

> +		rc = _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, i, -1);

> +		if (rc)

> +			return rc;

> +	}

> +

> +	return 0;

>  }


Can we simplify this a bit like below?

static int _dpu_kms_initialize_dsi(struct drm_device *dev,
				    struct msm_drm_private *priv,
				    struct dpu_kms *dpu_kms)
{
	int i, rc = 0;

	if (!(priv->dsi[0] || priv->dsi[1]))
		return rc;

	/*
          * We support following confiurations:
	 * - Single DSI host (dsi0 or dsi1)
	 * - Two independent DSI hosts
	 * - Bonded DSI0 and DSI1 hosts
	 *
	 *   TODO: Support swapping DSI0 and DSI1 in the bonded setup.
          for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
		if (!priv->dsi[i])
			continue;

		rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms); // this API does 
everything except encoder setup
		if (rc)
			return rc;
                 if (!is_bonded_dsi)
                      _dpu_kms_initialize_dsi_encoder(...);
                 else if (dsi_0) // only one encoder setup for dsi_0
                     _dpu_kms_initialize_dsi_encoder(...);

	}
}

Let me know if you think this is a little less complicated.

> 

>  static int _dpu_kms_initialize_displayport(struct drm_device *dev,

> @@ -505,6 +559,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 +571,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 +580,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 +790,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 +892,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,
Dmitry Baryshkov July 10, 2021, 7:38 p.m. UTC | #2 
On 10/07/2021 03:30, abhinavk@codeaurora.org wrote:
> On 2021-07-09 16:50, 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 "bonded 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 | 130 ++++++++++++++----------

>>  1 file changed, 79 insertions(+), 51 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..e14eb8f94cd7 100644

>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c

>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c

>> @@ -466,17 +466,16 @@ static void dpu_kms_wait_flush(struct msm_kms

>> *kms, unsigned crtc_mask)

>>          dpu_kms_wait_for_commit_done(kms, crtc);

>>  }

>>

>> -static int _dpu_kms_initialize_dsi(struct drm_device *dev,

>> -                    struct msm_drm_private *priv,

>> -                    struct dpu_kms *dpu_kms)

>> +static int _dpu_kms_initialize_dsi_encoder(struct drm_device *dev,

>> +                       struct msm_drm_private *priv,

>> +                       struct dpu_kms *dpu_kms,

>> +                       int dsi_id, int dsi_id1)

>>  {

>> +    struct msm_dsi *dsi = priv->dsi[dsi_id];

>>      struct drm_encoder *encoder = NULL;

>> -    int i, rc = 0;

>> -

>> -    if (!(priv->dsi[0] || priv->dsi[1]))

>> -        return rc;

>> +    struct msm_display_info info;

>> +    int rc = 0;

>>

>> -    /*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");

>> @@ -485,19 +484,74 @@ static int _dpu_kms_initialize_dsi(struct 

>> drm_device *dev,

>>

>>      priv->encoders[priv->num_encoders++] = encoder;

>>

>> -    for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

>> -        if (!priv->dsi[i])

>> -            continue;

>> +    rc = msm_dsi_modeset_init(dsi, dev, encoder);

>> +    if (rc) {

>> +        DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",

>> +              dsi_id, rc);

>> +        return rc;

>> +    }

>> +

>> +    memset(&info, 0, sizeof(info));

>> +    info.intf_type = encoder->encoder_type;

>> +    info.capabilities = msm_dsi_is_cmd_mode(dsi) ?

>> +        MSM_DISPLAY_CAP_CMD_MODE :

>> +        MSM_DISPLAY_CAP_VID_MODE;

>> +    info.h_tile_instance[info.num_of_h_tiles++] = dsi_id;

>>

>> -        rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);

>> +    /* For the bonded DSI setup we have second DSI host */

>> +    if (dsi_id1 >= 0) {

>> +        struct msm_dsi *dsi1 = priv->dsi[dsi_id1];

>> +

>> +        rc = msm_dsi_modeset_init(dsi1, dev, encoder);

>>          if (rc) {

>>              DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",

>> -                i, rc);

>> -            break;

>> +                  dsi_id1, rc);

>> +            return rc;

>>          }

>> +

>> +        info.h_tile_instance[info.num_of_h_tiles++] = dsi_id1;

>>      }

>>

>> -    return rc;

>> +    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;

>> +    }

>> +

>> +    return 0;

>> +}

>> +

>> +static int _dpu_kms_initialize_dsi(struct drm_device *dev,

>> +                    struct msm_drm_private *priv,

>> +                    struct dpu_kms *dpu_kms)

>> +{

>> +    int i, rc = 0;

>> +

>> +    if (!(priv->dsi[0] || priv->dsi[1]))

>> +        return rc;

>> +

>> +    /*

>> +     * We support following confiurations:

>> +     * - Single DSI host (dsi0 or dsi1)

>> +     * - Two independent DSI hosts

>> +     * - Bonded DSI0 and DSI1 hosts

>> +     *

>> +     *   TODO: Support swapping DSI0 and DSI1 in the bonded setup.

>> +     */

>> +    if (priv->dsi[0] && priv->dsi[1] && 

>> msm_dsi_is_bonded_dsi(priv->dsi[0]))

>> +        return _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, 0, 

>> 1);

>> +

>> +    for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

>> +        if (!priv->dsi[i])

>> +            continue;

>> +

>> +        rc = _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, i, -1);

>> +        if (rc)

>> +            return rc;

>> +    }

>> +

>> +    return 0;

>>  }

> 

> Can we simplify this a bit like below?

> 

> static int _dpu_kms_initialize_dsi(struct drm_device *dev,

>                      struct msm_drm_private *priv,

>                      struct dpu_kms *dpu_kms)

> {

>      int i, rc = 0;

> 

>      if (!(priv->dsi[0] || priv->dsi[1]))

>          return rc;

> 

>      /*

>           * We support following confiurations:

>       * - Single DSI host (dsi0 or dsi1)

>       * - Two independent DSI hosts

>       * - Bonded DSI0 and DSI1 hosts

>       *

>       *   TODO: Support swapping DSI0 and DSI1 in the bonded setup.

>           for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

>          if (!priv->dsi[i])

>              continue;

> 

>          rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms); // this API 

> does everything except encoder setup

>          if (rc)

>              return rc;

>                  if (!is_bonded_dsi)

>                       _dpu_kms_initialize_dsi_encoder(...);

>                  else if (dsi_0) // only one encoder setup for dsi_0

>                      _dpu_kms_initialize_dsi_encoder(...);

> 

>      }

> }

> 

> Let me know if you think this is a little less complicated.


I don't think this will work out of the box, currently we pass encoder 
to DSI initialization (modeset_init). Adding extra cases in the middle 
of the loop also doesn't seem like a clear solution.

> 

>>

>>  static int _dpu_kms_initialize_displayport(struct drm_device *dev,

>> @@ -505,6 +559,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 +571,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 +580,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 +790,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 +892,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,



-- 
With best wishes
Dmitry
Abhinav Kumar July 10, 2021, 7:57 p.m. UTC | #3 
On 2021-07-10 12:38, Dmitry Baryshkov wrote:
> On 10/07/2021 03:30, abhinavk@codeaurora.org wrote:

>> On 2021-07-09 16:50, 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 "bonded 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 | 130 

>>> ++++++++++++++----------

>>>  1 file changed, 79 insertions(+), 51 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..e14eb8f94cd7 100644

>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c

>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c

>>> @@ -466,17 +466,16 @@ static void dpu_kms_wait_flush(struct msm_kms

>>> *kms, unsigned crtc_mask)

>>>          dpu_kms_wait_for_commit_done(kms, crtc);

>>>  }

>>> 

>>> -static int _dpu_kms_initialize_dsi(struct drm_device *dev,

>>> -                    struct msm_drm_private *priv,

>>> -                    struct dpu_kms *dpu_kms)

>>> +static int _dpu_kms_initialize_dsi_encoder(struct drm_device *dev,

>>> +                       struct msm_drm_private *priv,

>>> +                       struct dpu_kms *dpu_kms,

>>> +                       int dsi_id, int dsi_id1)

>>>  {

>>> +    struct msm_dsi *dsi = priv->dsi[dsi_id];

>>>      struct drm_encoder *encoder = NULL;

>>> -    int i, rc = 0;

>>> -

>>> -    if (!(priv->dsi[0] || priv->dsi[1]))

>>> -        return rc;

>>> +    struct msm_display_info info;

>>> +    int rc = 0;

>>> 

>>> -    /*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");

>>> @@ -485,19 +484,74 @@ static int _dpu_kms_initialize_dsi(struct 

>>> drm_device *dev,

>>> 

>>>      priv->encoders[priv->num_encoders++] = encoder;

>>> 

>>> -    for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

>>> -        if (!priv->dsi[i])

>>> -            continue;

>>> +    rc = msm_dsi_modeset_init(dsi, dev, encoder);

>>> +    if (rc) {

>>> +        DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",

>>> +              dsi_id, rc);

>>> +        return rc;

>>> +    }

>>> +

>>> +    memset(&info, 0, sizeof(info));

>>> +    info.intf_type = encoder->encoder_type;

>>> +    info.capabilities = msm_dsi_is_cmd_mode(dsi) ?

>>> +        MSM_DISPLAY_CAP_CMD_MODE :

>>> +        MSM_DISPLAY_CAP_VID_MODE;

>>> +    info.h_tile_instance[info.num_of_h_tiles++] = dsi_id;

>>> 

>>> -        rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);

>>> +    /* For the bonded DSI setup we have second DSI host */

>>> +    if (dsi_id1 >= 0) {

>>> +        struct msm_dsi *dsi1 = priv->dsi[dsi_id1];

>>> +

>>> +        rc = msm_dsi_modeset_init(dsi1, dev, encoder);

>>>          if (rc) {

>>>              DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",

>>> -                i, rc);

>>> -            break;

>>> +                  dsi_id1, rc);

>>> +            return rc;

>>>          }

>>> +

>>> +        info.h_tile_instance[info.num_of_h_tiles++] = dsi_id1;

>>>      }

>>> 

>>> -    return rc;

>>> +    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;

>>> +    }

>>> +

>>> +    return 0;

>>> +}

>>> +

>>> +static int _dpu_kms_initialize_dsi(struct drm_device *dev,

>>> +                    struct msm_drm_private *priv,

>>> +                    struct dpu_kms *dpu_kms)

>>> +{

>>> +    int i, rc = 0;

>>> +

>>> +    if (!(priv->dsi[0] || priv->dsi[1]))

>>> +        return rc;

>>> +

>>> +    /*

>>> +     * We support following confiurations:

>>> +     * - Single DSI host (dsi0 or dsi1)

>>> +     * - Two independent DSI hosts

>>> +     * - Bonded DSI0 and DSI1 hosts

>>> +     *

>>> +     *   TODO: Support swapping DSI0 and DSI1 in the bonded setup.

>>> +     */

>>> +    if (priv->dsi[0] && priv->dsi[1] && 

>>> msm_dsi_is_bonded_dsi(priv->dsi[0]))

>>> +        return _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, 

>>> 0, 1);

>>> +

>>> +    for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

>>> +        if (!priv->dsi[i])

>>> +            continue;

>>> +

>>> +        rc = _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, i, 

>>> -1);

>>> +        if (rc)

>>> +            return rc;

>>> +    }

>>> +

>>> +    return 0;

>>>  }

>> 

>> Can we simplify this a bit like below?

>> 

>> static int _dpu_kms_initialize_dsi(struct drm_device *dev,

>>                      struct msm_drm_private *priv,

>>                      struct dpu_kms *dpu_kms)

>> {

>>      int i, rc = 0;

>> 

>>      if (!(priv->dsi[0] || priv->dsi[1]))

>>          return rc;

>> 

>>      /*

>>           * We support following confiurations:

>>       * - Single DSI host (dsi0 or dsi1)

>>       * - Two independent DSI hosts

>>       * - Bonded DSI0 and DSI1 hosts

>>       *

>>       *   TODO: Support swapping DSI0 and DSI1 in the bonded setup.

>>           for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {

>>          if (!priv->dsi[i])

>>              continue;

>> 

>>          rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms); // this API 

>> does everything except encoder setup

>>          if (rc)

>>              return rc;

>>                  if (!is_bonded_dsi)

>>                       _dpu_kms_initialize_dsi_encoder(...);

>>                  else if (dsi_0) // only one encoder setup for dsi_0

>>                      _dpu_kms_initialize_dsi_encoder(...);

>> 

>>      }

>> }

>> 

>> Let me know if you think this is a little less complicated.

> 

> I don't think this will work out of the box, currently we pass encoder

> to DSI initialization (modeset_init). Adding extra cases in the middle

> of the loop also doesn't seem like a clear solution.


In that case the previous attempt was actually a little better with the 
change I suggested
of using the msm_dsi_is_bonded_dsi() API instead of hard-coding the 
encoder to NULL.

This one has some additional changes like passing 1 and/or -1 of the 
dsi1. Just felt a bit of an
overkill. The previous one was better than this one.

> 

>> 

>>> 

>>>  static int _dpu_kms_initialize_displayport(struct drm_device *dev,

>>> @@ -505,6 +559,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 +571,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 +580,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 +790,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 +892,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,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 1d3a4f395e74..e14eb8f94cd7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -466,17 +466,16 @@  static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 		dpu_kms_wait_for_commit_done(kms, crtc);
 }
 
-static int _dpu_kms_initialize_dsi(struct drm_device *dev,
-				    struct msm_drm_private *priv,
-				    struct dpu_kms *dpu_kms)
+static int _dpu_kms_initialize_dsi_encoder(struct drm_device *dev,
+					   struct msm_drm_private *priv,
+					   struct dpu_kms *dpu_kms,
+					   int dsi_id, int dsi_id1)
 {
+	struct msm_dsi *dsi = priv->dsi[dsi_id];
 	struct drm_encoder *encoder = NULL;
-	int i, rc = 0;
-
-	if (!(priv->dsi[0] || priv->dsi[1]))
-		return rc;
+	struct msm_display_info info;
+	int rc = 0;
 
-	/*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");
@@ -485,19 +484,74 @@  static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 
 	priv->encoders[priv->num_encoders++] = encoder;
 
-	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
-		if (!priv->dsi[i])
-			continue;
+	rc = msm_dsi_modeset_init(dsi, dev, encoder);
+	if (rc) {
+		DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
+			  dsi_id, rc);
+		return rc;
+	}
+
+	memset(&info, 0, sizeof(info));
+	info.intf_type = encoder->encoder_type;
+	info.capabilities = msm_dsi_is_cmd_mode(dsi) ?
+		MSM_DISPLAY_CAP_CMD_MODE :
+		MSM_DISPLAY_CAP_VID_MODE;
+	info.h_tile_instance[info.num_of_h_tiles++] = dsi_id;
 
-		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
+	/* For the bonded DSI setup we have second DSI host */
+	if (dsi_id1 >= 0) {
+		struct msm_dsi *dsi1 = priv->dsi[dsi_id1];
+
+		rc = msm_dsi_modeset_init(dsi1, dev, encoder);
 		if (rc) {
 			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
-				i, rc);
-			break;
+				  dsi_id1, rc);
+			return rc;
 		}
+
+		info.h_tile_instance[info.num_of_h_tiles++] = dsi_id1;
 	}
 
-	return rc;
+	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;
+	}
+
+	return 0;
+}
+
+static int _dpu_kms_initialize_dsi(struct drm_device *dev,
+				    struct msm_drm_private *priv,
+				    struct dpu_kms *dpu_kms)
+{
+	int i, rc = 0;
+
+	if (!(priv->dsi[0] || priv->dsi[1]))
+		return rc;
+
+	/*
+	 * We support following confiurations:
+	 * - Single DSI host (dsi0 or dsi1)
+	 * - Two independent DSI hosts
+	 * - Bonded DSI0 and DSI1 hosts
+	 *
+	 *   TODO: Support swapping DSI0 and DSI1 in the bonded setup.
+	 */
+	if (priv->dsi[0] && priv->dsi[1] && msm_dsi_is_bonded_dsi(priv->dsi[0]))
+		return _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, 0, 1);
+
+	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
+		if (!priv->dsi[i])
+			continue;
+
+		rc = _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, i, -1);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
 }
 
 static int _dpu_kms_initialize_displayport(struct drm_device *dev,
@@ -505,6 +559,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 +571,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 +580,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 +790,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 +892,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,