Re: [[RFC]DPU PATCH] drm/msm/dsi: Use one connector for dual DSI mode

[Archit Taneja <architt@xxxxxxxxxxxxxx>]

On Friday 02 March 2018 05:57 AM, chandanu@xxxxxxxxxxxxxx wrote:
> On 2018-03-01 07:53, Sean Paul wrote:
> > On Wed, Feb 28, 2018 at 04:44:49PM -0800, Chandan Uddaraju wrote:
> > > Current DSI driver uses two connectors for dual DSI case even
> > > though we only have one panel. Fix this by implementing one
> > > connector/bridge for dual DSI use case.
> > >
> > > Current patch is not yet tested on dual-dsi setup.
> >
> > This seems like something that might benefit from being part of a 
> > patch series
> > that can be tested end-to-end (ie: a patch series to add full dual-dsi 
> > support
> > to the dsi driver). It's kind of hard to see where things are going 
> > with just
> > this one small piece.
>
> This is more like a initial patch to get early comments/suggestions
> regarding this approach on the DSI upstream driver.
> We are working on enabling dual-dsi using DPU with upstream dsi driver.
> This change will be based on archit's dual DSI changes on SDM845.
> Once we have verified, we will post all the relevant patches along with 
> this one.

The approach of entirely bypassing msm_dsi_modeset_init() for DSI1 
sounds like a decent idea. However, this may not work well if we need to 
dynamically switch between dual DSI mode vs operating the 2 DSIs
independently. If we don't have this use case in mind, then we should
be okay.

Another approach could be to create 2 separate bridge/connectors for
irrespective of whether we are in dual DSI mode or not, but report one
of the pairs as unavailable (connector's detect() should report 
disconnected).

> > > Signed-off-by: Chandan Uddaraju <chandanu@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/msm/dsi/dsi.c         |   3 +
> > >  drivers/gpu/drm/msm/dsi/dsi.h         |   1 +
> > >  drivers/gpu/drm/msm/dsi/dsi_manager.c | 100 
> > > +++++++---------------------------
> > >  3 files changed, 24 insertions(+), 80 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
> > > b/drivers/gpu/drm/msm/dsi/dsi.c
> > > index b744bcc..ff8164c 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> > > @@ -208,6 +208,9 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, 
> > > struct drm_device *dev,
> > >          goto fail;
> > >      }
> > >
> > > +    if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
> > > +        goto fail;
> > > +
> > >      msm_dsi->encoder = encoder;
> > >
> > >      msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
> > > b/drivers/gpu/drm/msm/dsi/dsi.h
> > > index 70d9a9a..2d9763f 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi.h
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> > > @@ -100,6 +100,7 @@ struct msm_dsi {
> > >  void msm_dsi_manager_attach_dsi_device(int id, u32 device_flags);
> > >  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);
> > >
> > >  /* msm dsi */
> > >  static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi)
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
> > > b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > index 4cb1cb6..bf92f25 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > @@ -305,67 +305,6 @@ static void dsi_mgr_connector_destroy(struct 
> > > drm_connector *connector)
> > >      kfree(dsi_connector);
> > >  }
> > >
> > > -static void dsi_dual_connector_fix_modes(struct drm_connector 
> > > *connector)
> > > -{
> > > -    struct drm_display_mode *mode, *m;
> > > -
> > > -    /* Only support left-right mode */
> > > -    list_for_each_entry_safe(mode, m, &connector->probed_modes, head) {
> > > -        mode->clock >>= 1;
> > > -        mode->hdisplay >>= 1;
> > > -        mode->hsync_start >>= 1;
> > > -        mode->hsync_end >>= 1;
> > > -        mode->htotal >>= 1;
> > > -        drm_mode_set_name(mode);
> > > -    }
> > > -}
> > > -
> > > -static int dsi_dual_connector_tile_init(
> > > -            struct drm_connector *connector, int id)
> > > -{
> > > -    struct drm_display_mode *mode;
> > > -    /* Fake topology id */
> > > -    char topo_id[8] = {'M', 'S', 'M', 'D', 'U', 'D', 'S', 'I'};
> > > -
> > > -    if (connector->tile_group) {
> > > -        DBG("Tile property has been initialized");
> > > -        return 0;
> > > -    }
> > > -
> > > -    /* Use the first mode only for now */
> > > -    mode = list_first_entry(&connector->probed_modes,
> > > -                struct drm_display_mode,
> > > -                head);
> > > -    if (!mode)
> > > -        return -EINVAL;
> > > -
> > > -    connector->tile_group = drm_mode_get_tile_group(
> > > -                    connector->dev, topo_id);
> > > -    if (!connector->tile_group)
> > > -        connector->tile_group = drm_mode_create_tile_group(
> > > -                    connector->dev, topo_id);
> > > -    if (!connector->tile_group) {
> > > -        pr_err("%s: failed to create tile group\n", __func__);
> > > -        return -ENOMEM;
> > > -    }
> > > -
> > > -    connector->has_tile = true;
> > > -    connector->tile_is_single_monitor = true;
> > > -
> > > -    /* mode has been fixed */
> > > -    connector->tile_h_size = mode->hdisplay;
> > > -    connector->tile_v_size = mode->vdisplay;
> > > -
> > > -    /* Only support left-right mode */
> > > -    connector->num_h_tile = 2;
> > > -    connector->num_v_tile = 1;
> > > -
> > > -    connector->tile_v_loc = 0;
> > > -    connector->tile_h_loc = (id == DSI_RIGHT) ? 1 : 0;
> > > -
> > > -    return 0;
> > > -}
> > > -
> > >  static int dsi_mgr_connector_get_modes(struct drm_connector *connector)
> > >  {
> > >      int id = dsi_mgr_connector_get_id(connector);
> > > @@ -376,31 +315,15 @@ static int dsi_mgr_connector_get_modes(struct 
> > > drm_connector *connector)
> > >      if (!panel)
> > >          return 0;
> > >
> > > -    /* Since we have 2 connectors, but only 1 drm_panel in dual DSI 
> > > mode,
> > > -     * panel should not attach to any connector.
> > > -     * Only temporarily attach panel to the current connector here,
> > > -     * to let panel set mode to this connector.
> > > +    /*
> > > +     * In dual DSI mode, we have one connector that can be
> > > +     * attached to the drm_panel.
> > >       */
> > >      drm_panel_attach(panel, connector);
> > >      num = drm_panel_get_modes(panel);
> > > -    drm_panel_detach(panel);
> > >      if (!num)
> > >          return 0;
> > >
> > > -    if (IS_DUAL_DSI()) {
> > > -        /* report half resolution to user */
> > > -        dsi_dual_connector_fix_modes(connector);
> > > -        ret = dsi_dual_connector_tile_init(connector, id);
> > > -        if (ret)
> > > -            return ret;
> > > -        ret = drm_mode_connector_set_tile_property(connector);
> > > -        if (ret) {
> > > -            pr_err("%s: set tile property failed, %d\n",
> > > -                    __func__, ret);
> > > -            return ret;
> > > -        }
> > > -    }
> > > -
> > >      return num;
> > >  }
> > >
> > > @@ -689,6 +612,23 @@ struct drm_connector 
> > > *msm_dsi_manager_connector_init(u8 id)
> > >      return connector;
> > >  }
> > >
> > > +bool msm_dsi_manager_validate_current_config(u8 id)
> > > +{
> > > +    bool is_dual_dsi = IS_DUAL_DSI();
> > > +
> > > +    /*
> > > +     * For dual DSI, we only have one drm panel. For this
> > > +     * use case, we register only one bridge/connector.
> > > +     * Skip bridge/connector initialisation if it is
> > > +     * DSI 1 in case of dual DSI.
> >
> > I think the other dual-dsi implementations take the master/slave cues 
> > from
> > device tree as opposed to hard-coding one or the other (I know of at 
> > least one
> > case where DSI_1 was connected to the panel's first channel, whereas 
> > DSI_0 was
> > connected to the second channel.
> >
> > Sean
>
> You are right Sean. I need to add logic to check Master DSI controller 
> instead
> of DSI ids so that the implementation become more generic. Thank you for 
> sharing
> your thoughts.

Slightly unrelated: currently, the IS_DUAL_DSI() uses custom qcom
bindings, we should use more generic bindings. I'd posted a RFC for it:

https://lists.freedesktop.org/archives/dri-devel/2018-January/162913.html

Thanks,
Archit

> Chandan
> > > +     */
> > > +    if (is_dual_dsi && (DSI_1 == id)) {
> > > +        DBG("Skip DSI_1 bridge registration for dual DSI.\n");
> > > +        return false;
> > > +    }
> > > +    return true;
> > > +}
> > > +
> > >  /* initialize bridge */
> > >  struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
> > >  {
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> > > Forum,
> > > a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html