[Freedreno] [PATCH v2 3/7] drm/msm/dpu: support setting up two independent DSI connectors
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Sat Jul 10 19:38:34 UTC 2021
On 10/07/2021 03:30, abhinavk at 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 at 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
More information about the Freedreno
mailing list