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.
Signed-off-by: Chandan Uddaraju chandanu@codeaurora.org
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_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.
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
On Friday 02 March 2018 05:57 AM, chandanu@codeaurora.org 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@codeaurora.org
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
dri-devel@lists.freedesktop.org
-
Archit Taneja -
chandanu＠codeaurora.org