Re: [PATCH v3 3/7] drm/msm/dpu: support setting up two independent DSI connectors

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 13/07/2021 00:15, abhinavk@xxxxxxxxxxxxxx wrote:
> On 2021-07-10 15:20, 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.
>
> This looks quite neat now,so i am okay with this version of it:
>
> Reviewed-by: Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx>
>
> Just a suggestion, since we are only supporting two dsis so far, do we need
> an extra variable to get the other DSI? Can't we just do priv->dsi[DSI_1]?
> as usually DSI_0 is the master

I still hope to fix "swap links in bonded DSI mode" (in other words DSI 
0 being clock master, but driving right half of the screen) one day. 
Thus I don't think we should enforce DSI_0/DSI_1 here.

> > +        int other = (i + 1) % 2;
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 102 +++++++++++++-----------
> >  1 file changed, 57 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..3cd2011e18d4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -471,30 +471,68 @@ 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;
> > -
> > +    /*
> > +     * 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++) {
> > +        int other = (i + 1) % 2;
> > +
> >          if (!priv->dsi[i])
> >              continue;
> >
> > +        if (msm_dsi_is_bonded_dsi(priv->dsi[i]) &&
> > +            !msm_dsi_is_master_dsi(priv->dsi[i]))
> > +            continue;
> > +
> > +        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;
> > +
> >          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;
> > +        info.capabilities = msm_dsi_is_cmd_mode(priv->dsi[i]) ?
> > +            MSM_DISPLAY_CAP_CMD_MODE :
> > +            MSM_DISPLAY_CAP_VID_MODE;
> > +
> > +        if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && priv->dsi[other]) {
> > +            rc = msm_dsi_modeset_init(priv->dsi[other], dev, encoder);
> > +            if (rc) {
> > +                DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
> > +                    other, rc);
> > +                break;
> > +            }
> > +
> > +            info.h_tile_instance[info.num_of_h_tiles++] = other;
> > +        }
> > +
> > +        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 +543,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 +555,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 +564,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 +774,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 +876,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