| Message ID | 20250220-dual-dsi-v2-5-6c0038d5a2ef@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dsi: Add DSC support to 2 panels in dual DSI mode | expand |
On Thu, Feb 20, 2025 at 06:07:56PM +0800, Jun Nie wrote: > There is dual DSI case that every DSI link is connected to an independent > panel. In this dual panel case, the frame width for DSC on each link should > be halved to support the usage case. Isn't it the case for the DSI panel utilizing two DSI links? > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > drivers/gpu/drm/msm/dsi/dsi.h | 3 ++- > drivers/gpu/drm/msm/dsi/dsi_host.c | 13 +++++++++---- > drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 ++++++++-- > 3 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h > index 35b90c462f637111159b204269ce908614a21586..5a8978bed9f4ca897b418ced60194042d9dd8d05 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi.h > +++ b/drivers/gpu/drm/msm/dsi/dsi.h > @@ -74,7 +74,8 @@ void msm_dsi_host_enable_irq(struct mipi_dsi_host *host); > void msm_dsi_host_disable_irq(struct mipi_dsi_host *host); > int msm_dsi_host_power_on(struct mipi_dsi_host *host, > struct msm_dsi_phy_shared_timings *phy_shared_timings, > - bool is_bonded_dsi, struct msm_dsi_phy *phy); > + bool is_bonded_dsi, bool is_dual_panel, > + struct msm_dsi_phy *phy); > int msm_dsi_host_power_off(struct mipi_dsi_host *host); > int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, > const struct drm_display_mode *mode); > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 976c5d82a2efa0fc51657b8534675890be7c33a6..752a97f7181c30dade0a7745492bf16649b3197b 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -902,7 +902,8 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod > } > } > > -static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > +static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi, > + bool is_dual_panel) > { > struct drm_display_mode *mode = msm_host->mode; > u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */ > @@ -947,7 +948,10 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > return; > } > > - dsc->pic_width = mode->hdisplay; > + if (is_dual_panel) > + dsc->pic_width = hdisplay; > + else > + dsc->pic_width = mode->hdisplay; > dsc->pic_height = mode->vdisplay; > DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); > > @@ -2369,7 +2373,8 @@ static void msm_dsi_sfpb_config(struct msm_dsi_host *msm_host, bool enable) > > int msm_dsi_host_power_on(struct mipi_dsi_host *host, > struct msm_dsi_phy_shared_timings *phy_shared_timings, > - bool is_bonded_dsi, struct msm_dsi_phy *phy) > + bool is_bonded_dsi, bool is_dual_panel, > + struct msm_dsi_phy *phy) > { > struct msm_dsi_host *msm_host = to_msm_dsi_host(host); > const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd; > @@ -2412,7 +2417,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host, > goto fail_disable_clk; > } > > - dsi_timing_setup(msm_host, is_bonded_dsi); > + dsi_timing_setup(msm_host, is_bonded_dsi, is_dual_panel); > dsi_sw_reset(msm_host); > dsi_ctrl_enable(msm_host, phy_shared_timings, phy); > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index be13bf682a9601484c9c14e8419563f37c2281ee..158b6cc907cb39cc3b182d3088b793d322a3527c 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -24,6 +24,7 @@ struct msm_dsi_manager { > struct msm_dsi *dsi[DSI_MAX]; > > bool is_bonded_dsi; > + bool is_dual_panel; > bool is_sync_needed; > int master_dsi_link_id; > }; > @@ -31,6 +32,7 @@ struct msm_dsi_manager { > static struct msm_dsi_manager msm_dsim_glb; > > #define IS_BONDED_DSI() (msm_dsim_glb.is_bonded_dsi) > +#define IS_DUAL_PANEL() (msm_dsim_glb.is_dual_panel) > #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) > #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) > > @@ -55,6 +57,7 @@ static int dsi_mgr_parse_of(struct device_node *np, int id) > msm_dsim->is_bonded_dsi = of_property_read_bool(np, "qcom,dual-dsi-mode"); > > if (msm_dsim->is_bonded_dsi) { > + msm_dsim->is_dual_panel = of_property_read_bool(np, "qcom,dual-panel"); > if (of_property_read_bool(np, "qcom,master-dsi")) > msm_dsim->master_dsi_link_id = id; > if (!msm_dsim->is_sync_needed) > @@ -214,6 +217,7 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > struct mipi_dsi_host *host = msm_dsi->host; > struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; > bool is_bonded_dsi = IS_BONDED_DSI(); > + bool is_dual_panel = IS_DUAL_PANEL(); > int ret; > > DBG("id=%d", id); > @@ -222,7 +226,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > if (ret) > goto phy_en_fail; > > - ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], is_bonded_dsi, msm_dsi->phy); > + ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], > + is_bonded_dsi, is_dual_panel, msm_dsi->phy); > if (ret) { > pr_err("%s: power on host %d failed, %d\n", __func__, id, ret); > goto host_on_fail; > @@ -230,7 +235,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > if (is_bonded_dsi && msm_dsi1) { > ret = msm_dsi_host_power_on(msm_dsi1->host, > - &phy_shared_timings[DSI_1], is_bonded_dsi, msm_dsi1->phy); > + &phy_shared_timings[DSI_1], is_bonded_dsi, > + is_dual_panel, msm_dsi1->phy); > if (ret) { > pr_err("%s: power on host1 failed, %d\n", > __func__, ret); > > -- > 2.34.1 >
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2025年2月20日周四 18:39写道： > > On Thu, Feb 20, 2025 at 06:07:56PM +0800, Jun Nie wrote: > > There is dual DSI case that every DSI link is connected to an independent > > panel. In this dual panel case, the frame width for DSC on each link should > > be halved to support the usage case. > > Isn't it the case for the DSI panel utilizing two DSI links? The added case here is 2 DSI panel utilizing two DSI links, 1 DSI link in each panel. I assume default case is 1 panel with 2 DSI link, such as Marijn's case. > > > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > --- > > drivers/gpu/drm/msm/dsi/dsi.h | 3 ++- > > drivers/gpu/drm/msm/dsi/dsi_host.c | 13 +++++++++---- > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 ++++++++-- > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h > > index 35b90c462f637111159b204269ce908614a21586..5a8978bed9f4ca897b418ced60194042d9dd8d05 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi.h > > +++ b/drivers/gpu/drm/msm/dsi/dsi.h > > @@ -74,7 +74,8 @@ void msm_dsi_host_enable_irq(struct mipi_dsi_host *host); > > void msm_dsi_host_disable_irq(struct mipi_dsi_host *host); > > int msm_dsi_host_power_on(struct mipi_dsi_host *host, > > struct msm_dsi_phy_shared_timings *phy_shared_timings, > > - bool is_bonded_dsi, struct msm_dsi_phy *phy); > > + bool is_bonded_dsi, bool is_dual_panel, > > + struct msm_dsi_phy *phy); > > int msm_dsi_host_power_off(struct mipi_dsi_host *host); > > int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, > > const struct drm_display_mode *mode); > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > > index 976c5d82a2efa0fc51657b8534675890be7c33a6..752a97f7181c30dade0a7745492bf16649b3197b 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > @@ -902,7 +902,8 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod > > } > > } > > > > -static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > > +static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi, > > + bool is_dual_panel) > > { > > struct drm_display_mode *mode = msm_host->mode; > > u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */ > > @@ -947,7 +948,10 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > > return; > > } > > > > - dsc->pic_width = mode->hdisplay; > > + if (is_dual_panel) > > + dsc->pic_width = hdisplay; > > + else > > + dsc->pic_width = mode->hdisplay; > > dsc->pic_height = mode->vdisplay; > > DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); > > > > @@ -2369,7 +2373,8 @@ static void msm_dsi_sfpb_config(struct msm_dsi_host *msm_host, bool enable) > > > > int msm_dsi_host_power_on(struct mipi_dsi_host *host, > > struct msm_dsi_phy_shared_timings *phy_shared_timings, > > - bool is_bonded_dsi, struct msm_dsi_phy *phy) > > + bool is_bonded_dsi, bool is_dual_panel, > > + struct msm_dsi_phy *phy) > > { > > struct msm_dsi_host *msm_host = to_msm_dsi_host(host); > > const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd; > > @@ -2412,7 +2417,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host, > > goto fail_disable_clk; > > } > > > > - dsi_timing_setup(msm_host, is_bonded_dsi); > > + dsi_timing_setup(msm_host, is_bonded_dsi, is_dual_panel); > > dsi_sw_reset(msm_host); > > dsi_ctrl_enable(msm_host, phy_shared_timings, phy); > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > index be13bf682a9601484c9c14e8419563f37c2281ee..158b6cc907cb39cc3b182d3088b793d322a3527c 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > @@ -24,6 +24,7 @@ struct msm_dsi_manager { > > struct msm_dsi *dsi[DSI_MAX]; > > > > bool is_bonded_dsi; > > + bool is_dual_panel; > > bool is_sync_needed; > > int master_dsi_link_id; > > }; > > @@ -31,6 +32,7 @@ struct msm_dsi_manager { > > static struct msm_dsi_manager msm_dsim_glb; > > > > #define IS_BONDED_DSI() (msm_dsim_glb.is_bonded_dsi) > > +#define IS_DUAL_PANEL() (msm_dsim_glb.is_dual_panel) > > #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) > > #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) > > > > @@ -55,6 +57,7 @@ static int dsi_mgr_parse_of(struct device_node *np, int id) > > msm_dsim->is_bonded_dsi = of_property_read_bool(np, "qcom,dual-dsi-mode"); > > > > if (msm_dsim->is_bonded_dsi) { > > + msm_dsim->is_dual_panel = of_property_read_bool(np, "qcom,dual-panel"); > > if (of_property_read_bool(np, "qcom,master-dsi")) > > msm_dsim->master_dsi_link_id = id; > > if (!msm_dsim->is_sync_needed) > > @@ -214,6 +217,7 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > struct mipi_dsi_host *host = msm_dsi->host; > > struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; > > bool is_bonded_dsi = IS_BONDED_DSI(); > > + bool is_dual_panel = IS_DUAL_PANEL(); > > int ret; > > > > DBG("id=%d", id); > > @@ -222,7 +226,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > if (ret) > > goto phy_en_fail; > > > > - ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], is_bonded_dsi, msm_dsi->phy); > > + ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], > > + is_bonded_dsi, is_dual_panel, msm_dsi->phy); > > if (ret) { > > pr_err("%s: power on host %d failed, %d\n", __func__, id, ret); > > goto host_on_fail; > > @@ -230,7 +235,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > > > if (is_bonded_dsi && msm_dsi1) { > > ret = msm_dsi_host_power_on(msm_dsi1->host, > > - &phy_shared_timings[DSI_1], is_bonded_dsi, msm_dsi1->phy); > > + &phy_shared_timings[DSI_1], is_bonded_dsi, > > + is_dual_panel, msm_dsi1->phy); > > if (ret) { > > pr_err("%s: power on host1 failed, %d\n", > > __func__, ret); > > > > -- > > 2.34.1 > > > > -- > With best wishes > Dmitry
On Thu, Feb 20, 2025 at 11:42:28PM +0800, Jun Nie wrote: > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2025年2月20日周四 18:39写道： > > > > On Thu, Feb 20, 2025 at 06:07:56PM +0800, Jun Nie wrote: > > > There is dual DSI case that every DSI link is connected to an independent > > > panel. In this dual panel case, the frame width for DSC on each link should > > > be halved to support the usage case. > > > > Isn't it the case for the DSI panel utilizing two DSI links? > > The added case here is 2 DSI panel utilizing two DSI links, 1 DSI link > in each panel. > I assume default case is 1 panel with 2 DSI link, such as Marijn's case. So it should be halved in your case, but not in Marijn's case? I can suspect that if you are describing two DSI panels as a single instance, you should also adjust drm_dsc_config accordingly (on the panel's side?) Maybe drm_dsc_config.pic_width and drm_dsc_config.pic_height should be set on the panel's side? But then, how will that function for the DSI panels or bridges which can change the mode? > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > > --- > > > drivers/gpu/drm/msm/dsi/dsi.h | 3 ++- > > > drivers/gpu/drm/msm/dsi/dsi_host.c | 13 +++++++++---- > > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 ++++++++-- > > > 3 files changed, 19 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h > > > index 35b90c462f637111159b204269ce908614a21586..5a8978bed9f4ca897b418ced60194042d9dd8d05 100644 > > > --- a/drivers/gpu/drm/msm/dsi/dsi.h > > > +++ b/drivers/gpu/drm/msm/dsi/dsi.h > > > @@ -74,7 +74,8 @@ void msm_dsi_host_enable_irq(struct mipi_dsi_host *host); > > > void msm_dsi_host_disable_irq(struct mipi_dsi_host *host); > > > int msm_dsi_host_power_on(struct mipi_dsi_host *host, > > > struct msm_dsi_phy_shared_timings *phy_shared_timings, > > > - bool is_bonded_dsi, struct msm_dsi_phy *phy); > > > + bool is_bonded_dsi, bool is_dual_panel, > > > + struct msm_dsi_phy *phy); > > > int msm_dsi_host_power_off(struct mipi_dsi_host *host); > > > int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, > > > const struct drm_display_mode *mode); > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > > > index 976c5d82a2efa0fc51657b8534675890be7c33a6..752a97f7181c30dade0a7745492bf16649b3197b 100644 > > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > > @@ -902,7 +902,8 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod > > > } > > > } > > > > > > -static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > > > +static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi, > > > + bool is_dual_panel) > > > { > > > struct drm_display_mode *mode = msm_host->mode; > > > u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */ > > > @@ -947,7 +948,10 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) > > > return; > > > } > > > > > > - dsc->pic_width = mode->hdisplay; > > > + if (is_dual_panel) > > > + dsc->pic_width = hdisplay; > > > + else > > > + dsc->pic_width = mode->hdisplay; > > > dsc->pic_height = mode->vdisplay; > > > DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); > > > > > > @@ -2369,7 +2373,8 @@ static void msm_dsi_sfpb_config(struct msm_dsi_host *msm_host, bool enable) > > > > > > int msm_dsi_host_power_on(struct mipi_dsi_host *host, > > > struct msm_dsi_phy_shared_timings *phy_shared_timings, > > > - bool is_bonded_dsi, struct msm_dsi_phy *phy) > > > + bool is_bonded_dsi, bool is_dual_panel, > > > + struct msm_dsi_phy *phy) > > > { > > > struct msm_dsi_host *msm_host = to_msm_dsi_host(host); > > > const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd; > > > @@ -2412,7 +2417,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host, > > > goto fail_disable_clk; > > > } > > > > > > - dsi_timing_setup(msm_host, is_bonded_dsi); > > > + dsi_timing_setup(msm_host, is_bonded_dsi, is_dual_panel); > > > dsi_sw_reset(msm_host); > > > dsi_ctrl_enable(msm_host, phy_shared_timings, phy); > > > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > > index be13bf682a9601484c9c14e8419563f37c2281ee..158b6cc907cb39cc3b182d3088b793d322a3527c 100644 > > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > > @@ -24,6 +24,7 @@ struct msm_dsi_manager { > > > struct msm_dsi *dsi[DSI_MAX]; > > > > > > bool is_bonded_dsi; > > > + bool is_dual_panel; > > > bool is_sync_needed; > > > int master_dsi_link_id; > > > }; > > > @@ -31,6 +32,7 @@ struct msm_dsi_manager { > > > static struct msm_dsi_manager msm_dsim_glb; > > > > > > #define IS_BONDED_DSI() (msm_dsim_glb.is_bonded_dsi) > > > +#define IS_DUAL_PANEL() (msm_dsim_glb.is_dual_panel) > > > #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) > > > #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) > > > > > > @@ -55,6 +57,7 @@ static int dsi_mgr_parse_of(struct device_node *np, int id) > > > msm_dsim->is_bonded_dsi = of_property_read_bool(np, "qcom,dual-dsi-mode"); > > > > > > if (msm_dsim->is_bonded_dsi) { > > > + msm_dsim->is_dual_panel = of_property_read_bool(np, "qcom,dual-panel"); > > > if (of_property_read_bool(np, "qcom,master-dsi")) > > > msm_dsim->master_dsi_link_id = id; > > > if (!msm_dsim->is_sync_needed) > > > @@ -214,6 +217,7 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > > struct mipi_dsi_host *host = msm_dsi->host; > > > struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; > > > bool is_bonded_dsi = IS_BONDED_DSI(); > > > + bool is_dual_panel = IS_DUAL_PANEL(); > > > int ret; > > > > > > DBG("id=%d", id); > > > @@ -222,7 +226,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > > if (ret) > > > goto phy_en_fail; > > > > > > - ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], is_bonded_dsi, msm_dsi->phy); > > > + ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], > > > + is_bonded_dsi, is_dual_panel, msm_dsi->phy); > > > if (ret) { > > > pr_err("%s: power on host %d failed, %d\n", __func__, id, ret); > > > goto host_on_fail; > > > @@ -230,7 +235,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > > > > > if (is_bonded_dsi && msm_dsi1) { > > > ret = msm_dsi_host_power_on(msm_dsi1->host, > > > - &phy_shared_timings[DSI_1], is_bonded_dsi, msm_dsi1->phy); > > > + &phy_shared_timings[DSI_1], is_bonded_dsi, > > > + is_dual_panel, msm_dsi1->phy); > > > if (ret) { > > > pr_err("%s: power on host1 failed, %d\n", > > > __func__, ret); > > > > > > -- > > > 2.34.1 > > > > > > > -- > > With best wishes > > Dmitry
On 2025-02-20 18:06:01, Dmitry Baryshkov wrote: > On Thu, Feb 20, 2025 at 11:42:28PM +0800, Jun Nie wrote: > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2025年2月20日周四 18:39写道： > > > > > > On Thu, Feb 20, 2025 at 06:07:56PM +0800, Jun Nie wrote: > > > > There is dual DSI case that every DSI link is connected to an independent There is a dual-DSI case where every DSI link ... > > > > panel. In this dual panel case, the frame width for DSC on each link should > > > > be halved to support the usage case. use* case. Also, it shouldn't be "halved" just... because? It should be "halved" because apparently hdisplay here is the width of the two panels together, while the width coded in pic_width should contain that of a single panel (since they're independent), which is equal to the width of a single interface. Tl;dr for below: this needs a *lot* more text to explain the setup and possibilities. How is a DSI panel driver supposed to configure this on their end? Hint: look at my previous drm/msm patches that explain how we expect to interface with the parameters set by the panel driver. > > > > > > Isn't it the case for the DSI panel utilizing two DSI links? > > > > The added case here is 2 DSI panel utilizing two DSI links, 1 DSI link > > in each panel. > > I assume default case is 1 panel with 2 DSI link, such as Marijn's case. > > So it should be halved in your case, but not in Marijn's case? I can > suspect that if you are describing two DSI panels as a single instance, > you should also adjust drm_dsc_config accordingly (on the panel's side?) > > Maybe drm_dsc_config.pic_width and drm_dsc_config.pic_height should be > set on the panel's side? But then, how will that function for the DSI > panels or bridges which can change the mode? It appears that these patches are missing a proper description of the setup or use-case. I previously NAK'd those "dual DSI" patches because of this, but reading between the lines I think I came to understand the reason without anyone else explaining it, unfortunately. Needless to say that this needs very careful documentation and wording in both code (DT and/or header structs) and commit messages. In my case I have a single high-resolution high-refresh-rate panel that can simply not be driven over a single DSI link. A dual-DSI link is used in bonded mode, most likely to keep the clocks and other things in sync, and to make it easier to be represented by one virtual encoder in DPU? All control commands only need the sent over one DSI link, not over both. In this case pic_width is equal to the entire width of the panel, hence it is double the width of a single interface. Jun seems to have a strangely different use-case for bonded-DSI / dual-DSI that isn't explained: two "independent" panels. It is clear to me that pic_width here has to contain the width of the entire panel, and is hence equal to the entire width of a single interface. (And in the future, it seems the quad setup can drive two "bonded" panels with two DSI-merge's each) But what we're missing here is what the **advantages and limitations** are of this setup: why would one run two DSI links for "independent" panels in bonded-DSI mode? Is it more power-optimal? Will userspace see this as one panel that's simply twice as wide? Do these panels have to be "identical" so that they behave and are clocked the same? How is the driver expected to prepare the mode and DSC configuration parameters to make this work? Perhaps it's possible to scrape this info from [1] and prior commits but I rather have a more digestible description in the commit message, thanks. - Marijn [1]: https://gitlab.com/jun.nie/linux/-/commit/98c0f411a85d68a76be500f8421df467d6cc53f3
On Thu, Feb 20, 2025 at 10:50:57PM +0100, Marijn Suijten wrote: > On 2025-02-20 18:06:01, Dmitry Baryshkov wrote: > > On Thu, Feb 20, 2025 at 11:42:28PM +0800, Jun Nie wrote: > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2025年2月20日周四 18:39写道： > > > > > > > > On Thu, Feb 20, 2025 at 06:07:56PM +0800, Jun Nie wrote: > > > > > There is dual DSI case that every DSI link is connected to an independent > > There is a dual-DSI case where every DSI link ... > > > > > > panel. In this dual panel case, the frame width for DSC on each link should > > > > > be halved to support the usage case. > > use* case. Also, it shouldn't be "halved" just... because? It should be > "halved" because apparently hdisplay here is the width of the two panels > together, while the width coded in pic_width should contain that of a single > panel (since they're independent), which is equal to the width of a single > interface. > > Tl;dr for below: this needs a *lot* more text to explain the setup and > possibilities. How is a DSI panel driver supposed to configure this on their > end? Hint: look at my previous drm/msm patches that explain how we expect to > interface with the parameters set by the panel driver. > > > > > > > > > Isn't it the case for the DSI panel utilizing two DSI links? > > > > > > The added case here is 2 DSI panel utilizing two DSI links, 1 DSI link > > > in each panel. > > > I assume default case is 1 panel with 2 DSI link, such as Marijn's case. > > > > So it should be halved in your case, but not in Marijn's case? I can > > suspect that if you are describing two DSI panels as a single instance, > > you should also adjust drm_dsc_config accordingly (on the panel's side?) > > > > Maybe drm_dsc_config.pic_width and drm_dsc_config.pic_height should be > > set on the panel's side? But then, how will that function for the DSI > > panels or bridges which can change the mode? > > It appears that these patches are missing a proper description of the setup > or use-case. I previously NAK'd those "dual DSI" patches because of this, but > reading between the lines I think I came to understand the reason without anyone > else explaining it, unfortunately. Needless to say that this needs very careful > documentation and wording in both code (DT and/or header structs) and commit > messages. > > In my case I have a single high-resolution high-refresh-rate panel that can > simply not be driven over a single DSI link. A dual-DSI link is used in bonded > mode, most likely to keep the clocks and other things in sync, and to make it > easier to be represented by one virtual encoder in DPU? All control commands > only need the sent over one DSI link, not over both. > > In this case pic_width is equal to the entire width of the panel, hence it is > double the width of a single interface. > > Jun seems to have a strangely different use-case for bonded-DSI / dual-DSI that > isn't explained: two "independent" panels. It is clear to me that pic_width > here has to contain the width of the entire panel, and is hence equal to the > entire width of a single interface. > (And in the future, it seems the quad setup can drive two "bonded" panels with > two DSI-merge's each) > > But what we're missing here is what the **advantages and limitations** are > of this setup: why would one run two DSI links for "independent" panels in > bonded-DSI mode? Is it more power-optimal? Will userspace see this as one > panel that's simply twice as wide? Do these panels have to be "identical" > so that they behave and are clocked the same? How is the driver expected to > prepare the mode and DSC configuration parameters to make this work? Fair enough. Maybe you will suggest how to handle it in a more efficient way. We have been running such configurations (2 independent panels over a bonded DSI link) in order to get a single synchronous CRTC vblank for both panels. This is a nice hack for the software that outputs data for both panels, but doesn't want to be concerned with separate vblanks. > Perhaps it's possible to scrape this info from [1] and prior commits but I > rather have a more digestible description in the commit message, thanks. > > - Marijn > > [1]: https://gitlab.com/jun.nie/linux/-/commit/98c0f411a85d68a76be500f8421df467d6cc53f3
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 35b90c462f637111159b204269ce908614a21586..5a8978bed9f4ca897b418ced60194042d9dd8d05 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -74,7 +74,8 @@ void msm_dsi_host_enable_irq(struct mipi_dsi_host *host); void msm_dsi_host_disable_irq(struct mipi_dsi_host *host); int msm_dsi_host_power_on(struct mipi_dsi_host *host, struct msm_dsi_phy_shared_timings *phy_shared_timings, - bool is_bonded_dsi, struct msm_dsi_phy *phy); + bool is_bonded_dsi, bool is_dual_panel, + struct msm_dsi_phy *phy); int msm_dsi_host_power_off(struct mipi_dsi_host *host); int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host, const struct drm_display_mode *mode); diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 976c5d82a2efa0fc51657b8534675890be7c33a6..752a97f7181c30dade0a7745492bf16649b3197b 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -902,7 +902,8 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod } } -static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) +static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi, + bool is_dual_panel) { struct drm_display_mode *mode = msm_host->mode; u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */ @@ -947,7 +948,10 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi) return; } - dsc->pic_width = mode->hdisplay; + if (is_dual_panel) + dsc->pic_width = hdisplay; + else + dsc->pic_width = mode->hdisplay; dsc->pic_height = mode->vdisplay; DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height); @@ -2369,7 +2373,8 @@ static void msm_dsi_sfpb_config(struct msm_dsi_host *msm_host, bool enable) int msm_dsi_host_power_on(struct mipi_dsi_host *host, struct msm_dsi_phy_shared_timings *phy_shared_timings, - bool is_bonded_dsi, struct msm_dsi_phy *phy) + bool is_bonded_dsi, bool is_dual_panel, + struct msm_dsi_phy *phy) { struct msm_dsi_host *msm_host = to_msm_dsi_host(host); const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd; @@ -2412,7 +2417,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host, goto fail_disable_clk; } - dsi_timing_setup(msm_host, is_bonded_dsi); + dsi_timing_setup(msm_host, is_bonded_dsi, is_dual_panel); dsi_sw_reset(msm_host); dsi_ctrl_enable(msm_host, phy_shared_timings, phy); diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index be13bf682a9601484c9c14e8419563f37c2281ee..158b6cc907cb39cc3b182d3088b793d322a3527c 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -24,6 +24,7 @@ struct msm_dsi_manager { struct msm_dsi *dsi[DSI_MAX]; bool is_bonded_dsi; + bool is_dual_panel; bool is_sync_needed; int master_dsi_link_id; }; @@ -31,6 +32,7 @@ struct msm_dsi_manager { static struct msm_dsi_manager msm_dsim_glb; #define IS_BONDED_DSI() (msm_dsim_glb.is_bonded_dsi) +#define IS_DUAL_PANEL() (msm_dsim_glb.is_dual_panel) #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) @@ -55,6 +57,7 @@ static int dsi_mgr_parse_of(struct device_node *np, int id) msm_dsim->is_bonded_dsi = of_property_read_bool(np, "qcom,dual-dsi-mode"); if (msm_dsim->is_bonded_dsi) { + msm_dsim->is_dual_panel = of_property_read_bool(np, "qcom,dual-panel"); if (of_property_read_bool(np, "qcom,master-dsi")) msm_dsim->master_dsi_link_id = id; if (!msm_dsim->is_sync_needed) @@ -214,6 +217,7 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) struct mipi_dsi_host *host = msm_dsi->host; struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; bool is_bonded_dsi = IS_BONDED_DSI(); + bool is_dual_panel = IS_DUAL_PANEL(); int ret; DBG("id=%d", id); @@ -222,7 +226,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) if (ret) goto phy_en_fail; - ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], is_bonded_dsi, msm_dsi->phy); + ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], + is_bonded_dsi, is_dual_panel, msm_dsi->phy); if (ret) { pr_err("%s: power on host %d failed, %d\n", __func__, id, ret); goto host_on_fail; @@ -230,7 +235,8 @@ static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) if (is_bonded_dsi && msm_dsi1) { ret = msm_dsi_host_power_on(msm_dsi1->host, - &phy_shared_timings[DSI_1], is_bonded_dsi, msm_dsi1->phy); + &phy_shared_timings[DSI_1], is_bonded_dsi, + is_dual_panel, msm_dsi1->phy); if (ret) { pr_err("%s: power on host1 failed, %d\n", __func__, ret);
There is dual DSI case that every DSI link is connected to an independent panel. In this dual panel case, the frame width for DSC on each link should be halved to support the usage case. Signed-off-by: Jun Nie <jun.nie@linaro.org> --- drivers/gpu/drm/msm/dsi/dsi.h | 3 ++- drivers/gpu/drm/msm/dsi/dsi_host.c | 13 +++++++++---- drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 ++++++++-- 3 files changed, 19 insertions(+), 7 deletions(-)