[PATCH v3 0/4] drm/msm: Enable widebus for DSI

[PATCH v3 0/4] drm/msm: Enable widebus for DSI

[Jessica Zhang]

DSI 6G v2.5.x+ and DPU support a data-bus widen mode that allows DSI
to send 48 bits of compressed data per pclk instead of 24.

For all chipsets that support this mode, enable it whenever DSC is
enabled as recommended by the hardware programming guide.

Only enable this for command mode as we are currently unable to validate
widebus for video mode.

Depends on: "drm/msm/dpu: Drop encoder vsync_event" [1]

Note: The dsi.xml.h changes were generated using the headergen2 script in
envytools [2], but the changes to the copyright and rules-ng-ng source file
paths were dropped.

[1] https://patchwork.freedesktop.org/series/121742/
[2] https://github.com/freedreno/envytools/

--
Changes in v3:
- Split commit into DPU, dsi.xml.h, and DSI changes (Dmitry)
- Add DSC enabled check to DSI *_is_widebus_enabled() helper (Dmitry)
- Dropped mention of DPU in cover letter title
- Moved setting of dpu_enc->wide_bus_en to dpu_encoder_virt_atomic_enable()
- Link to v2: https://lore.kernel.org/r/20230713-add-widebus-support-v2-1-ad0added17b6@quicinc.com

Changes in v2:
- Rebased on top of "drm/msm/dpu: Re-introduce dpu core revision"
- Squashed all commits to avoid breaking feature if the series is only partially applied
- Moved DATABUS_WIDEN bit setting to dsi_ctr_enable() (Marijn)
- Have DPU check if wide bus is requested by output driver (Dmitry)
- Introduced bytes_per_pclk variable for dsi_timing_setup() hdisplay adjustment (Marijn)
- Link to v1: https://lore.kernel.org/r/20230525-add-widebus-support-v1-0-c7069f2efca1@quicinc.com

---
Jessica Zhang (4):
      drm/msm/dpu: Move DPU encoder wide_bus_en setting
      drm/msm/dpu: Enable widebus for DSI INTF
      drm/msm/dsi: Add DATABUS_WIDEN MDP_CTRL2 bit
      drm/msm/dsi: Enable widebus for DSI

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 16 +++++++++---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  3 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  1 +
 drivers/gpu/drm/msm/dsi/dsi.c                      |  5 ++++
 drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
 drivers/gpu/drm/msm/dsi/dsi.xml.h                  |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 30 +++++++++++++++++++---
 drivers/gpu/drm/msm/msm_drv.h                      |  6 ++++-
 9 files changed, 57 insertions(+), 10 deletions(-)
---
base-commit: e5046e719774f833d32e3e6064416bb792564c95
change-id: 20230525-add-widebus-support-f785546ee751

Best regards,
-- 
Jessica Zhang <quic_jesszhan@quicinc.com>

[PATCH v3 0/4] drm/msm: Enable widebus for DSI

[Jessica Zhang]

DSI 6G v2.5.x+ and DPU support a data-bus widen mode that allows DSI
to send 48 bits of compressed data per pclk instead of 24.

For all chipsets that support this mode, enable it whenever DSC is
enabled as recommended by the hardware programming guide.

Only enable this for command mode as we are currently unable to validate
widebus for video mode.

Depends on: "drm/msm/dpu: Drop encoder vsync_event" [1]

Note: The dsi.xml.h changes were generated using the headergen2 script in
envytools [2], but the changes to the copyright and rules-ng-ng source file
paths were dropped.

[1] https://patchwork.freedesktop.org/series/121742/
[2] https://github.com/freedreno/envytools/

--
Changes in v3:
- Split commit into DPU, dsi.xml.h, and DSI changes (Dmitry)
- Add DSC enabled check to DSI *_is_widebus_enabled() helper (Dmitry)
- Dropped mention of DPU in cover letter title
- Moved setting of dpu_enc->wide_bus_en to dpu_encoder_virt_atomic_enable()
- Link to v2: https://lore.kernel.org/r/20230713-add-widebus-support-v2-1-ad0added17b6@quicinc.com

Changes in v2:
- Rebased on top of "drm/msm/dpu: Re-introduce dpu core revision"
- Squashed all commits to avoid breaking feature if the series is only partially applied
- Moved DATABUS_WIDEN bit setting to dsi_ctr_enable() (Marijn)
- Have DPU check if wide bus is requested by output driver (Dmitry)
- Introduced bytes_per_pclk variable for dsi_timing_setup() hdisplay adjustment (Marijn)
- Link to v1: https://lore.kernel.org/r/20230525-add-widebus-support-v1-0-c7069f2efca1@quicinc.com

---
Jessica Zhang (4):
      drm/msm/dpu: Move DPU encoder wide_bus_en setting
      drm/msm/dpu: Enable widebus for DSI INTF
      drm/msm/dsi: Add DATABUS_WIDEN MDP_CTRL2 bit
      drm/msm/dsi: Enable widebus for DSI

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 16 +++++++++---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  3 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  1 +
 drivers/gpu/drm/msm/dsi/dsi.c                      |  5 ++++
 drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
 drivers/gpu/drm/msm/dsi/dsi.xml.h                  |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 30 +++++++++++++++++++---
 drivers/gpu/drm/msm/msm_drv.h                      |  6 ++++-
 9 files changed, 57 insertions(+), 10 deletions(-)
---
base-commit: e5046e719774f833d32e3e6064416bb792564c95
change-id: 20230525-add-widebus-support-f785546ee751

Best regards,
-- 
Jessica Zhang <quic_jesszhan@quicinc.com>

[PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Jessica Zhang]

Move the setting of dpu_enc.wide_bus_en to
dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
dpu_enc.dsc.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d34e684a4178..3dcd37c48aac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
 	struct dpu_encoder_virt *dpu_enc = NULL;
 	int ret = 0;
 	struct drm_display_mode *cur_mode = NULL;
+	struct msm_drm_private *priv = drm_enc->dev->dev_private;
+	struct msm_display_info *disp_info;
 
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	disp_info = &dpu_enc->disp_info;
 
 	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
 
+	if (disp_info->intf_type == INTF_DP)
+		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
+				priv->dp[disp_info->h_tile_instance[0]]);
+
 	mutex_lock(&dpu_enc->enc_lock);
 	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
 
@@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
 	timer_setup(&dpu_enc->frame_done_timer,
 			dpu_encoder_frame_done_timeout, 0);
 
-	if (disp_info->intf_type == INTF_DP)
-		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
-				priv->dp[disp_info->h_tile_instance[0]]);
-
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
 	dpu_enc->idle_timeout = IDLE_TIMEOUT;

-- 
2.41.0

[PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Jessica Zhang]

Move the setting of dpu_enc.wide_bus_en to
dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
dpu_enc.dsc.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d34e684a4178..3dcd37c48aac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
 	struct dpu_encoder_virt *dpu_enc = NULL;
 	int ret = 0;
 	struct drm_display_mode *cur_mode = NULL;
+	struct msm_drm_private *priv = drm_enc->dev->dev_private;
+	struct msm_display_info *disp_info;
 
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	disp_info = &dpu_enc->disp_info;
 
 	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
 
+	if (disp_info->intf_type == INTF_DP)
+		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
+				priv->dp[disp_info->h_tile_instance[0]]);
+
 	mutex_lock(&dpu_enc->enc_lock);
 	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
 
@@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
 	timer_setup(&dpu_enc->frame_done_timer,
 			dpu_encoder_frame_done_timeout, 0);
 
-	if (disp_info->intf_type == INTF_DP)
-		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
-				priv->dp[disp_info->h_tile_instance[0]]);
-
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
 	dpu_enc->idle_timeout = IDLE_TIMEOUT;

-- 
2.41.0

[PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Jessica Zhang]

DPU supports a data-bus widen mode for DSI INTF.

Enable this mode for all supported chipsets if widebus is enabled for DSI.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
 drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3dcd37c48aac..de08aad39e15 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
 	struct drm_display_mode *cur_mode = NULL;
 	struct msm_drm_private *priv = drm_enc->dev->dev_private;
 	struct msm_display_info *disp_info;
+	int index;
 
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 	disp_info = &dpu_enc->disp_info;
 
+	disp_info = &dpu_enc->disp_info;
+	index = disp_info->h_tile_instance[0];
+
 	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
 
-	if (disp_info->intf_type == INTF_DP)
-		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
-				priv->dp[disp_info->h_tile_instance[0]]);
+	if (disp_info->intf_type == INTF_DSI)
+		dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
+	else if (disp_info->intf_type == INTF_DP)
+		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
 
 	mutex_lock(&dpu_enc->enc_lock);
 	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index df88358e7037..dace6168be2d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
 				phys_enc->hw_intf,
 				phys_enc->hw_pp->idx);
 
-	if (intf_cfg.dsc != 0)
+	if (intf_cfg.dsc != 0) {
 		cmd_mode_cfg.data_compress = true;
+		cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
+	}
 
 	if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
 		phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 8ec6505d9e78..dc6f3febb574 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
 	if (cmd_mode_cfg->data_compress)
 		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
 
+	if (cmd_mode_cfg->wide_bus_en)
+		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
+
 	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
 }
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 77f80531782b..c539025c418b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
 
 struct dpu_hw_intf_cmd_mode_cfg {
 	u8 data_compress;	/* enable data compress between dpu and dsi */
+	u8 wide_bus_en;		/* enable databus widen mode */
 };
 
 /**
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 9d9d5e009163..e4f706b16aad 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
 bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
+bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
 struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
@@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 {
 	return false;
 }
-
+static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
+{
+	return false;
+}
 static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
 {
 	return NULL;

-- 
2.41.0

[PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Jessica Zhang]

DPU supports a data-bus widen mode for DSI INTF.

Enable this mode for all supported chipsets if widebus is enabled for DSI.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
 drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3dcd37c48aac..de08aad39e15 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
 	struct drm_display_mode *cur_mode = NULL;
 	struct msm_drm_private *priv = drm_enc->dev->dev_private;
 	struct msm_display_info *disp_info;
+	int index;
 
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 	disp_info = &dpu_enc->disp_info;
 
+	disp_info = &dpu_enc->disp_info;
+	index = disp_info->h_tile_instance[0];
+
 	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
 
-	if (disp_info->intf_type == INTF_DP)
-		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
-				priv->dp[disp_info->h_tile_instance[0]]);
+	if (disp_info->intf_type == INTF_DSI)
+		dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
+	else if (disp_info->intf_type == INTF_DP)
+		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
 
 	mutex_lock(&dpu_enc->enc_lock);
 	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index df88358e7037..dace6168be2d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
 				phys_enc->hw_intf,
 				phys_enc->hw_pp->idx);
 
-	if (intf_cfg.dsc != 0)
+	if (intf_cfg.dsc != 0) {
 		cmd_mode_cfg.data_compress = true;
+		cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
+	}
 
 	if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
 		phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 8ec6505d9e78..dc6f3febb574 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
 	if (cmd_mode_cfg->data_compress)
 		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
 
+	if (cmd_mode_cfg->wide_bus_en)
+		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
+
 	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
 }
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 77f80531782b..c539025c418b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
 
 struct dpu_hw_intf_cmd_mode_cfg {
 	u8 data_compress;	/* enable data compress between dpu and dsi */
+	u8 wide_bus_en;		/* enable databus widen mode */
 };
 
 /**
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 9d9d5e009163..e4f706b16aad 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
 bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
+bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
 struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
@@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 {
 	return false;
 }
-
+static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
+{
+	return false;
+}
 static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
 {
 	return NULL;

-- 
2.41.0

[PATCH v3 3/4] drm/msm/dsi: Add DATABUS_WIDEN MDP_CTRL2 bit

[Jessica Zhang]

Add a DATABUS_WIDEN bit to the MDP_CTRL2 register to allow DSI to enable
databus widen mode.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index a4a154601114..2a7d980e12c3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
 	return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
 }
 #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE			0x00010000
+#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN			0x00100000
 
 #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL			0x000001b8
 #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK		0x0000003f

-- 
2.41.0

[PATCH v3 3/4] drm/msm/dsi: Add DATABUS_WIDEN MDP_CTRL2 bit

[Jessica Zhang]

Add a DATABUS_WIDEN bit to the MDP_CTRL2 register to allow DSI to enable
databus widen mode.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index a4a154601114..2a7d980e12c3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
 	return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
 }
 #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE			0x00010000
+#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN			0x00100000
 
 #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL			0x000001b8
 #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK		0x0000003f

-- 
2.41.0

[PATCH v3 4/4] drm/msm/dsi: Enable widebus for DSI

[Jessica Zhang]

DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
48 bits of compressed data instead of 24.

Enable this mode whenever DSC is enabled for supported chipsets.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi.c      |  5 +++++
 drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c | 30 ++++++++++++++++++++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index baab79ab6e74..4fa738dea680 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -17,6 +17,11 @@ struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
 	return msm_dsi_host_get_dsc_config(msm_dsi->host);
 }
 
+bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
+{
+	return msm_dsi_host_is_widebus_enabled(msm_dsi->host);
+}
+
 static int dsi_get_phy(struct msm_dsi *msm_dsi)
 {
 	struct platform_device *pdev = msm_dsi->pdev;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index bd3763a5d723..a557d2c1aaff 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -134,6 +134,7 @@ int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
 void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
 void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
 struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host);
+bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host);
 
 /* dsi phy */
 struct msm_dsi_phy;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 645927214871..231b02e5ab6e 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -710,6 +710,14 @@ static void dsi_ctrl_disable(struct msm_dsi_host *msm_host)
 	dsi_write(msm_host, REG_DSI_CTRL, 0);
 }
 
+bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	return msm_host->dsc && (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
+			msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0);
+}
+
 static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
 			struct msm_dsi_phy_shared_timings *phy_shared_timings, struct msm_dsi_phy *phy)
 {
@@ -753,10 +761,16 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
 		data |= DSI_CMD_CFG1_INSERT_DCS_COMMAND;
 		dsi_write(msm_host, REG_DSI_CMD_CFG1, data);
 
-		if (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
-		    msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3) {
+		if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
 			data = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
-			data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
+
+			if (cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3)
+				data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
+
+			/* TODO: Allow for video-mode support once tested/fixed */
+			if (msm_dsi_host_is_widebus_enabled(&msm_host->base))
+				data |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
+
 			dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, data);
 		}
 	}
@@ -894,6 +908,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 	u32 hdisplay = mode->hdisplay;
 	u32 wc;
 	int ret;
+	bool widebus_enabled = msm_dsi_host_is_widebus_enabled(&msm_host->base);
 
 	DBG("");
 
@@ -914,6 +929,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 	if (msm_host->dsc) {
 		struct drm_dsc_config *dsc = msm_host->dsc;
+		u32 bytes_per_pclk;
 
 		/* update dsc params with timing params */
 		if (!dsc || !mode->hdisplay || !mode->vdisplay) {
@@ -937,7 +953,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		 * pulse width same
 		 */
 		h_total -= hdisplay;
-		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
+		if (widebus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
+			bytes_per_pclk = 6;
+		else
+			bytes_per_pclk = 3;
+
+		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
+
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
 	}

-- 
2.41.0

[PATCH v3 4/4] drm/msm/dsi: Enable widebus for DSI

[Jessica Zhang]

DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
48 bits of compressed data instead of 24.

Enable this mode whenever DSC is enabled for supported chipsets.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi.c      |  5 +++++
 drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c | 30 ++++++++++++++++++++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index baab79ab6e74..4fa738dea680 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -17,6 +17,11 @@ struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
 	return msm_dsi_host_get_dsc_config(msm_dsi->host);
 }
 
+bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
+{
+	return msm_dsi_host_is_widebus_enabled(msm_dsi->host);
+}
+
 static int dsi_get_phy(struct msm_dsi *msm_dsi)
 {
 	struct platform_device *pdev = msm_dsi->pdev;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index bd3763a5d723..a557d2c1aaff 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -134,6 +134,7 @@ int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
 void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
 void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
 struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host);
+bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host);
 
 /* dsi phy */
 struct msm_dsi_phy;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 645927214871..231b02e5ab6e 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -710,6 +710,14 @@ static void dsi_ctrl_disable(struct msm_dsi_host *msm_host)
 	dsi_write(msm_host, REG_DSI_CTRL, 0);
 }
 
+bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	return msm_host->dsc && (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
+			msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0);
+}
+
 static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
 			struct msm_dsi_phy_shared_timings *phy_shared_timings, struct msm_dsi_phy *phy)
 {
@@ -753,10 +761,16 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
 		data |= DSI_CMD_CFG1_INSERT_DCS_COMMAND;
 		dsi_write(msm_host, REG_DSI_CMD_CFG1, data);
 
-		if (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
-		    msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3) {
+		if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
 			data = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
-			data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
+
+			if (cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3)
+				data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
+
+			/* TODO: Allow for video-mode support once tested/fixed */
+			if (msm_dsi_host_is_widebus_enabled(&msm_host->base))
+				data |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
+
 			dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, data);
 		}
 	}
@@ -894,6 +908,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 	u32 hdisplay = mode->hdisplay;
 	u32 wc;
 	int ret;
+	bool widebus_enabled = msm_dsi_host_is_widebus_enabled(&msm_host->base);
 
 	DBG("");
 
@@ -914,6 +929,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 	if (msm_host->dsc) {
 		struct drm_dsc_config *dsc = msm_host->dsc;
+		u32 bytes_per_pclk;
 
 		/* update dsc params with timing params */
 		if (!dsc || !mode->hdisplay || !mode->vdisplay) {
@@ -937,7 +953,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		 * pulse width same
 		 */
 		h_total -= hdisplay;
-		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
+		if (widebus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
+			bytes_per_pclk = 6;
+		else
+			bytes_per_pclk = 3;
+
+		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
+
 		h_total += hdisplay;
 		ha_end = ha_start + hdisplay;
 	}

-- 
2.41.0

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> DPU supports a data-bus widen mode for DSI INTF.
>
> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>  drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>  5 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3dcd37c48aac..de08aad39e15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>         struct drm_display_mode *cur_mode = NULL;
>         struct msm_drm_private *priv = drm_enc->dev->dev_private;
>         struct msm_display_info *disp_info;
> +       int index;
>
>         dpu_enc = to_dpu_encoder_virt(drm_enc);
>         disp_info = &dpu_enc->disp_info;
>
> +       disp_info = &dpu_enc->disp_info;
> +       index = disp_info->h_tile_instance[0];
> +
>         dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>
> -       if (disp_info->intf_type == INTF_DP)
> -               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -                               priv->dp[disp_info->h_tile_instance[0]]);
> +       if (disp_info->intf_type == INTF_DSI)
> +               dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
> +       else if (disp_info->intf_type == INTF_DP)
> +               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);

If you change the order, you won't have to touch DP lines.

>
>         mutex_lock(&dpu_enc->enc_lock);
>         cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index df88358e7037..dace6168be2d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>                                 phys_enc->hw_intf,
>                                 phys_enc->hw_pp->idx);
>
> -       if (intf_cfg.dsc != 0)
> +       if (intf_cfg.dsc != 0) {
>                 cmd_mode_cfg.data_compress = true;
> +               cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> +       }

This embeds the knowledge that a wide bus can only be enabled when DSC
is in use. Please move the wide_bus_en assignment out of conditional
code.

>
>         if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>                 phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 8ec6505d9e78..dc6f3febb574 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,

This function is only enabled for DPU >= 7.0, while IIRC wide bus can
be enabled even for some of the earlier chipsets.

>         if (cmd_mode_cfg->data_compress)
>                 intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>
> +       if (cmd_mode_cfg->wide_bus_en)
> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> +
>         DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>  }
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 77f80531782b..c539025c418b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>
>  struct dpu_hw_intf_cmd_mode_cfg {
>         u8 data_compress;       /* enable data compress between dpu and dsi */
> +       u8 wide_bus_en;         /* enable databus widen mode */
>  };
>
>  /**
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9d9d5e009163..e4f706b16aad 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>  bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>  struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>  #else
>  static inline void __init msm_dsi_register(void)
> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>  {
>         return false;
>  }
> -
> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
> +{
> +       return false;
> +}

Empty line, please.

>  static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>  {
>         return NULL;
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> DPU supports a data-bus widen mode for DSI INTF.
>
> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>  drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>  5 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3dcd37c48aac..de08aad39e15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>         struct drm_display_mode *cur_mode = NULL;
>         struct msm_drm_private *priv = drm_enc->dev->dev_private;
>         struct msm_display_info *disp_info;
> +       int index;
>
>         dpu_enc = to_dpu_encoder_virt(drm_enc);
>         disp_info = &dpu_enc->disp_info;
>
> +       disp_info = &dpu_enc->disp_info;
> +       index = disp_info->h_tile_instance[0];
> +
>         dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>
> -       if (disp_info->intf_type == INTF_DP)
> -               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -                               priv->dp[disp_info->h_tile_instance[0]]);
> +       if (disp_info->intf_type == INTF_DSI)
> +               dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
> +       else if (disp_info->intf_type == INTF_DP)
> +               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);

If you change the order, you won't have to touch DP lines.

>
>         mutex_lock(&dpu_enc->enc_lock);
>         cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index df88358e7037..dace6168be2d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>                                 phys_enc->hw_intf,
>                                 phys_enc->hw_pp->idx);
>
> -       if (intf_cfg.dsc != 0)
> +       if (intf_cfg.dsc != 0) {
>                 cmd_mode_cfg.data_compress = true;
> +               cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> +       }

This embeds the knowledge that a wide bus can only be enabled when DSC
is in use. Please move the wide_bus_en assignment out of conditional
code.

>
>         if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>                 phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 8ec6505d9e78..dc6f3febb574 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,

This function is only enabled for DPU >= 7.0, while IIRC wide bus can
be enabled even for some of the earlier chipsets.

>         if (cmd_mode_cfg->data_compress)
>                 intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>
> +       if (cmd_mode_cfg->wide_bus_en)
> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> +
>         DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>  }
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 77f80531782b..c539025c418b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>
>  struct dpu_hw_intf_cmd_mode_cfg {
>         u8 data_compress;       /* enable data compress between dpu and dsi */
> +       u8 wide_bus_en;         /* enable databus widen mode */
>  };
>
>  /**
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9d9d5e009163..e4f706b16aad 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>  bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>  struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>  #else
>  static inline void __init msm_dsi_register(void)
> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>  {
>         return false;
>  }
> -
> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
> +{
> +       return false;
> +}

Empty line, please.

>  static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>  {
>         return NULL;
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Move the setting of dpu_enc.wide_bus_en to
> dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
> dpu_enc.dsc.

because ... ?

>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Other than the commit message:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index d34e684a4178..3dcd37c48aac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>         struct dpu_encoder_virt *dpu_enc = NULL;
>         int ret = 0;
>         struct drm_display_mode *cur_mode = NULL;
> +       struct msm_drm_private *priv = drm_enc->dev->dev_private;
> +       struct msm_display_info *disp_info;
>
>         dpu_enc = to_dpu_encoder_virt(drm_enc);
> +       disp_info = &dpu_enc->disp_info;
>
>         dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>
> +       if (disp_info->intf_type == INTF_DP)
> +               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> +                               priv->dp[disp_info->h_tile_instance[0]]);
> +
>         mutex_lock(&dpu_enc->enc_lock);
>         cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>
> @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>         timer_setup(&dpu_enc->frame_done_timer,
>                         dpu_encoder_frame_done_timeout, 0);
>
> -       if (disp_info->intf_type == INTF_DP)
> -               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -                               priv->dp[disp_info->h_tile_instance[0]]);
> -
>         INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>                         dpu_encoder_off_work);
>         dpu_enc->idle_timeout = IDLE_TIMEOUT;
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Move the setting of dpu_enc.wide_bus_en to
> dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
> dpu_enc.dsc.

because ... ?

>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Other than the commit message:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index d34e684a4178..3dcd37c48aac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>         struct dpu_encoder_virt *dpu_enc = NULL;
>         int ret = 0;
>         struct drm_display_mode *cur_mode = NULL;
> +       struct msm_drm_private *priv = drm_enc->dev->dev_private;
> +       struct msm_display_info *disp_info;
>
>         dpu_enc = to_dpu_encoder_virt(drm_enc);
> +       disp_info = &dpu_enc->disp_info;
>
>         dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>
> +       if (disp_info->intf_type == INTF_DP)
> +               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> +                               priv->dp[disp_info->h_tile_instance[0]]);
> +
>         mutex_lock(&dpu_enc->enc_lock);
>         cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>
> @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>         timer_setup(&dpu_enc->frame_done_timer,
>                         dpu_encoder_frame_done_timeout, 0);
>
> -       if (disp_info->intf_type == INTF_DP)
> -               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -                               priv->dp[disp_info->h_tile_instance[0]]);
> -
>         INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>                         dpu_encoder_off_work);
>         dpu_enc->idle_timeout = IDLE_TIMEOUT;
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 4/4] drm/msm/dsi: Enable widebus for DSI

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
> 48 bits of compressed data instead of 24.
>
> Enable this mode whenever DSC is enabled for supported chipsets.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.c      |  5 +++++
>  drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 30 ++++++++++++++++++++++++++----
>  3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index baab79ab6e74..4fa738dea680 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -17,6 +17,11 @@ struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>         return msm_dsi_host_get_dsc_config(msm_dsi->host);
>  }
>
> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
> +{
> +       return msm_dsi_host_is_widebus_enabled(msm_dsi->host);
> +}

If this function is not provided at the time of the previous patch,
compilation will break. I'd suggest to provide a stub first and then
change it in this patch.

> +
>  static int dsi_get_phy(struct msm_dsi *msm_dsi)
>  {
>         struct platform_device *pdev = msm_dsi->pdev;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index bd3763a5d723..a557d2c1aaff 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -134,6 +134,7 @@ int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
>  void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
>  void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>  struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host);
> +bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host);
>
>  /* dsi phy */
>  struct msm_dsi_phy;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 645927214871..231b02e5ab6e 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -710,6 +710,14 @@ static void dsi_ctrl_disable(struct msm_dsi_host *msm_host)
>         dsi_write(msm_host, REG_DSI_CTRL, 0);
>  }
>
> +bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host)
> +{
> +       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +
> +       return msm_host->dsc && (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&

Please add a line break after the first &&. Compare two following statements:

> +                       msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0);
> +}
> +
>  static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
>                         struct msm_dsi_phy_shared_timings *phy_shared_timings, struct msm_dsi_phy *phy)
>  {
> @@ -753,10 +761,16 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
>                 data |= DSI_CMD_CFG1_INSERT_DCS_COMMAND;
>                 dsi_write(msm_host, REG_DSI_CMD_CFG1, data);
>
> -               if (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
> -                   msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3) {
> +               if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
>                         data = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
> -                       data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
> +
> +                       if (cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3)
> +                               data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
> +
> +                       /* TODO: Allow for video-mode support once tested/fixed */
> +                       if (msm_dsi_host_is_widebus_enabled(&msm_host->base))
> +                               data |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
> +
>                         dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, data);
>                 }
>         }
> @@ -894,6 +908,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>         u32 hdisplay = mode->hdisplay;
>         u32 wc;
>         int ret;
> +       bool widebus_enabled = msm_dsi_host_is_widebus_enabled(&msm_host->base);
>
>         DBG("");
>
> @@ -914,6 +929,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>
>         if (msm_host->dsc) {
>                 struct drm_dsc_config *dsc = msm_host->dsc;
> +               u32 bytes_per_pclk;
>
>                 /* update dsc params with timing params */
>                 if (!dsc || !mode->hdisplay || !mode->vdisplay) {
> @@ -937,7 +953,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>                  * pulse width same
>                  */
>                 h_total -= hdisplay;
> -               hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> +               if (widebus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
> +                       bytes_per_pclk = 6;
> +               else
> +                       bytes_per_pclk = 3;
> +
> +               hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
> +
>                 h_total += hdisplay;
>                 ha_end = ha_start + hdisplay;
>         }
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 4/4] drm/msm/dsi: Enable widebus for DSI

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
> 48 bits of compressed data instead of 24.
>
> Enable this mode whenever DSC is enabled for supported chipsets.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.c      |  5 +++++
>  drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 30 ++++++++++++++++++++++++++----
>  3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index baab79ab6e74..4fa738dea680 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -17,6 +17,11 @@ struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>         return msm_dsi_host_get_dsc_config(msm_dsi->host);
>  }
>
> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
> +{
> +       return msm_dsi_host_is_widebus_enabled(msm_dsi->host);
> +}

If this function is not provided at the time of the previous patch,
compilation will break. I'd suggest to provide a stub first and then
change it in this patch.

> +
>  static int dsi_get_phy(struct msm_dsi *msm_dsi)
>  {
>         struct platform_device *pdev = msm_dsi->pdev;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index bd3763a5d723..a557d2c1aaff 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -134,6 +134,7 @@ int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
>  void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
>  void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>  struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host);
> +bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host);
>
>  /* dsi phy */
>  struct msm_dsi_phy;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 645927214871..231b02e5ab6e 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -710,6 +710,14 @@ static void dsi_ctrl_disable(struct msm_dsi_host *msm_host)
>         dsi_write(msm_host, REG_DSI_CTRL, 0);
>  }
>
> +bool msm_dsi_host_is_widebus_enabled(struct mipi_dsi_host *host)
> +{
> +       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +
> +       return msm_host->dsc && (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&

Please add a line break after the first &&. Compare two following statements:

> +                       msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0);
> +}
> +
>  static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
>                         struct msm_dsi_phy_shared_timings *phy_shared_timings, struct msm_dsi_phy *phy)
>  {
> @@ -753,10 +761,16 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
>                 data |= DSI_CMD_CFG1_INSERT_DCS_COMMAND;
>                 dsi_write(msm_host, REG_DSI_CMD_CFG1, data);
>
> -               if (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
> -                   msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3) {
> +               if (cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) {
>                         data = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
> -                       data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
> +
> +                       if (cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3)
> +                               data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
> +
> +                       /* TODO: Allow for video-mode support once tested/fixed */
> +                       if (msm_dsi_host_is_widebus_enabled(&msm_host->base))
> +                               data |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
> +
>                         dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, data);
>                 }
>         }
> @@ -894,6 +908,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>         u32 hdisplay = mode->hdisplay;
>         u32 wc;
>         int ret;
> +       bool widebus_enabled = msm_dsi_host_is_widebus_enabled(&msm_host->base);
>
>         DBG("");
>
> @@ -914,6 +929,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>
>         if (msm_host->dsc) {
>                 struct drm_dsc_config *dsc = msm_host->dsc;
> +               u32 bytes_per_pclk;
>
>                 /* update dsc params with timing params */
>                 if (!dsc || !mode->hdisplay || !mode->vdisplay) {
> @@ -937,7 +953,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>                  * pulse width same
>                  */
>                 h_total -= hdisplay;
> -               hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> +               if (widebus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
> +                       bytes_per_pclk = 6;
> +               else
> +                       bytes_per_pclk = 3;
> +
> +               hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
> +
>                 h_total += hdisplay;
>                 ha_end = ha_start + hdisplay;
>         }
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 3/4] drm/msm/dsi: Add DATABUS_WIDEN MDP_CTRL2 bit

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Add a DATABUS_WIDEN bit to the MDP_CTRL2 register to allow DSI to enable
> databus widen mode.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

(The patch will probably be replaced by Rob syncing up msm headers).

>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index a4a154601114..2a7d980e12c3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>         return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>  }
>  #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE                      0x00010000
> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN                   0x00100000
>
>  #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL                      0x000001b8
>  #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK          0x0000003f
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 3/4] drm/msm/dsi: Add DATABUS_WIDEN MDP_CTRL2 bit

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Add a DATABUS_WIDEN bit to the MDP_CTRL2 register to allow DSI to enable
> databus widen mode.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

(The patch will probably be replaced by Rob syncing up msm headers).

>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index a4a154601114..2a7d980e12c3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>         return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>  }
>  #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE                      0x00010000
> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN                   0x00100000
>
>  #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL                      0x000001b8
>  #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK          0x0000003f
>
> --
> 2.41.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Marijn Suijten]

I find this title very undescriptive, it doesn't really explain from/to
where this move is happening nor why.

On 2023-08-02 11:08:48, Jessica Zhang wrote:
> Move the setting of dpu_enc.wide_bus_en to
> dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
> dpu_enc.dsc.

mirroring "the setting of dpu_enc.dsc" very much sounds like you are
mirroring _its value_, but that is not the case.  You are moving the
initialization (or just setting, because it could also be overwriting?)
to _the same place_ where .dsc is assigned.

I am pretty sure that this has a runtime impact which we discussed
before (hotplug...?) but the commit message omits that.  This is
mandatory.

- Marijn

> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index d34e684a4178..3dcd37c48aac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>  	struct dpu_encoder_virt *dpu_enc = NULL;
>  	int ret = 0;
>  	struct drm_display_mode *cur_mode = NULL;
> +	struct msm_drm_private *priv = drm_enc->dev->dev_private;
> +	struct msm_display_info *disp_info;
>  
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	disp_info = &dpu_enc->disp_info;
>  
>  	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>  
> +	if (disp_info->intf_type == INTF_DP)
> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> +				priv->dp[disp_info->h_tile_instance[0]]);
> +
>  	mutex_lock(&dpu_enc->enc_lock);
>  	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>  
> @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>  	timer_setup(&dpu_enc->frame_done_timer,
>  			dpu_encoder_frame_done_timeout, 0);
>  
> -	if (disp_info->intf_type == INTF_DP)
> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -				priv->dp[disp_info->h_tile_instance[0]]);
> -
>  	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>  			dpu_encoder_off_work);
>  	dpu_enc->idle_timeout = IDLE_TIMEOUT;
> 
> -- 
> 2.41.0
> 

Re: [PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Marijn Suijten]

I find this title very undescriptive, it doesn't really explain from/to
where this move is happening nor why.

On 2023-08-02 11:08:48, Jessica Zhang wrote:
> Move the setting of dpu_enc.wide_bus_en to
> dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
> dpu_enc.dsc.

mirroring "the setting of dpu_enc.dsc" very much sounds like you are
mirroring _its value_, but that is not the case.  You are moving the
initialization (or just setting, because it could also be overwriting?)
to _the same place_ where .dsc is assigned.

I am pretty sure that this has a runtime impact which we discussed
before (hotplug...?) but the commit message omits that.  This is
mandatory.

- Marijn

> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index d34e684a4178..3dcd37c48aac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>  	struct dpu_encoder_virt *dpu_enc = NULL;
>  	int ret = 0;
>  	struct drm_display_mode *cur_mode = NULL;
> +	struct msm_drm_private *priv = drm_enc->dev->dev_private;
> +	struct msm_display_info *disp_info;
>  
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	disp_info = &dpu_enc->disp_info;
>  
>  	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>  
> +	if (disp_info->intf_type == INTF_DP)
> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> +				priv->dp[disp_info->h_tile_instance[0]]);
> +
>  	mutex_lock(&dpu_enc->enc_lock);
>  	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>  
> @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>  	timer_setup(&dpu_enc->frame_done_timer,
>  			dpu_encoder_frame_done_timeout, 0);
>  
> -	if (disp_info->intf_type == INTF_DP)
> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -				priv->dp[disp_info->h_tile_instance[0]]);
> -
>  	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>  			dpu_encoder_off_work);
>  	dpu_enc->idle_timeout = IDLE_TIMEOUT;
> 
> -- 
> 2.41.0
> 

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Marijn Suijten]

On 2023-08-02 11:08:49, Jessica Zhang wrote:
> DPU supports a data-bus widen mode for DSI INTF.
> 
> Enable this mode for all supported chipsets if widebus is enabled for DSI.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>  drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>  5 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3dcd37c48aac..de08aad39e15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>  	struct drm_display_mode *cur_mode = NULL;
>  	struct msm_drm_private *priv = drm_enc->dev->dev_private;
>  	struct msm_display_info *disp_info;
> +	int index;
>  
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
>  	disp_info = &dpu_enc->disp_info;
>  
> +	disp_info = &dpu_enc->disp_info;
> +	index = disp_info->h_tile_instance[0];
> +
>  	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>  
> -	if (disp_info->intf_type == INTF_DP)
> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -				priv->dp[disp_info->h_tile_instance[0]]);
> +	if (disp_info->intf_type == INTF_DSI)
> +		dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
> +	else if (disp_info->intf_type == INTF_DP)
> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);

This inconsistency really is killing.  wide_bus vs widebus, and one
function has an is_ while the other does not.

>  
>  	mutex_lock(&dpu_enc->enc_lock);
>  	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index df88358e7037..dace6168be2d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>  				phys_enc->hw_intf,
>  				phys_enc->hw_pp->idx);
>  
> -	if (intf_cfg.dsc != 0)
> +	if (intf_cfg.dsc != 0) {
>  		cmd_mode_cfg.data_compress = true;
> +		cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> +	}
>  
>  	if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>  		phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 8ec6505d9e78..dc6f3febb574 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>  	if (cmd_mode_cfg->data_compress)
>  		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>  
> +	if (cmd_mode_cfg->wide_bus_en)
> +		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> +
>  	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>  }
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 77f80531782b..c539025c418b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>  
>  struct dpu_hw_intf_cmd_mode_cfg {
>  	u8 data_compress;	/* enable data compress between dpu and dsi */
> +	u8 wide_bus_en;		/* enable databus widen mode */

Any clue why these weren't just bool types?  These suffix-comments also
aren't adhering to the kerneldoc format, or is there a different
variant?

>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9d9d5e009163..e4f706b16aad 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>  bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>  struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>  #else
>  static inline void __init msm_dsi_register(void)
> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>  {
>  	return false;
>  }
> -
> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
> +{
> +	return false;
> +}

Only this default inline implementation is defined, but the function is
declared in this commit.  Since there's no real functional
implementation yet your commit should clarify that it comes later (in a
followup commit in the same series?  I can't know because I am reviewing
this series linearly from start to finish...) or reorder the patches so
that this lack of clarity is circumvented entirely.

- Marijn

>  static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>  {
>  	return NULL;
> 
> -- 
> 2.41.0
> 

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Marijn Suijten]

On 2023-08-02 11:08:49, Jessica Zhang wrote:
> DPU supports a data-bus widen mode for DSI INTF.
> 
> Enable this mode for all supported chipsets if widebus is enabled for DSI.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>  drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>  5 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3dcd37c48aac..de08aad39e15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>  	struct drm_display_mode *cur_mode = NULL;
>  	struct msm_drm_private *priv = drm_enc->dev->dev_private;
>  	struct msm_display_info *disp_info;
> +	int index;
>  
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
>  	disp_info = &dpu_enc->disp_info;
>  
> +	disp_info = &dpu_enc->disp_info;
> +	index = disp_info->h_tile_instance[0];
> +
>  	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>  
> -	if (disp_info->intf_type == INTF_DP)
> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> -				priv->dp[disp_info->h_tile_instance[0]]);
> +	if (disp_info->intf_type == INTF_DSI)
> +		dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
> +	else if (disp_info->intf_type == INTF_DP)
> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);

This inconsistency really is killing.  wide_bus vs widebus, and one
function has an is_ while the other does not.

>  
>  	mutex_lock(&dpu_enc->enc_lock);
>  	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index df88358e7037..dace6168be2d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>  				phys_enc->hw_intf,
>  				phys_enc->hw_pp->idx);
>  
> -	if (intf_cfg.dsc != 0)
> +	if (intf_cfg.dsc != 0) {
>  		cmd_mode_cfg.data_compress = true;
> +		cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> +	}
>  
>  	if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>  		phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 8ec6505d9e78..dc6f3febb574 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>  	if (cmd_mode_cfg->data_compress)
>  		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>  
> +	if (cmd_mode_cfg->wide_bus_en)
> +		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> +
>  	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>  }
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 77f80531782b..c539025c418b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>  
>  struct dpu_hw_intf_cmd_mode_cfg {
>  	u8 data_compress;	/* enable data compress between dpu and dsi */
> +	u8 wide_bus_en;		/* enable databus widen mode */

Any clue why these weren't just bool types?  These suffix-comments also
aren't adhering to the kerneldoc format, or is there a different
variant?

>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9d9d5e009163..e4f706b16aad 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>  bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>  bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>  struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>  #else
>  static inline void __init msm_dsi_register(void)
> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>  {
>  	return false;
>  }
> -
> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
> +{
> +	return false;
> +}

Only this default inline implementation is defined, but the function is
declared in this commit.  Since there's no real functional
implementation yet your commit should clarify that it comes later (in a
followup commit in the same series?  I can't know because I am reviewing
this series linearly from start to finish...) or reorder the patches so
that this lack of clarity is circumvented entirely.

- Marijn

>  static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>  {
>  	return NULL;
> 
> -- 
> 2.41.0
> 

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Jessica Zhang]

On 8/2/2023 11:20 AM, Dmitry Baryshkov wrote:
> On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>> DPU supports a data-bus widen mode for DSI INTF.
>>
>> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>>   drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>>   5 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 3dcd37c48aac..de08aad39e15 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>>          struct drm_display_mode *cur_mode = NULL;
>>          struct msm_drm_private *priv = drm_enc->dev->dev_private;
>>          struct msm_display_info *disp_info;
>> +       int index;
>>
>>          dpu_enc = to_dpu_encoder_virt(drm_enc);
>>          disp_info = &dpu_enc->disp_info;
>>
>> +       disp_info = &dpu_enc->disp_info;
>> +       index = disp_info->h_tile_instance[0];
>> +
>>          dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>>
>> -       if (disp_info->intf_type == INTF_DP)
>> -               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> -                               priv->dp[disp_info->h_tile_instance[0]]);
>> +       if (disp_info->intf_type == INTF_DSI)
>> +               dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
>> +       else if (disp_info->intf_type == INTF_DP)
>> +               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
> 
> If you change the order, you won't have to touch DP lines.

Hi Dmitry,

Acked.

> 
>>
>>          mutex_lock(&dpu_enc->enc_lock);
>>          cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index df88358e7037..dace6168be2d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>                                  phys_enc->hw_intf,
>>                                  phys_enc->hw_pp->idx);
>>
>> -       if (intf_cfg.dsc != 0)
>> +       if (intf_cfg.dsc != 0) {
>>                  cmd_mode_cfg.data_compress = true;
>> +               cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> +       }
> 
> This embeds the knowledge that a wide bus can only be enabled when DSC
> is in use. Please move the wide_bus_en assignment out of conditional
> code.

Wide bus for DSI will only be enabled if DSC is enabled, so this is 
technically not wrong, as DP will use the video mode path.

> 
>>
>>          if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>>                  phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 8ec6505d9e78..dc6f3febb574 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
> 
> This function is only enabled for DPU >= 7.0, while IIRC wide bus can
> be enabled even for some of the earlier chipsets.

The command mode path is only called for DSI, which only supports wide 
bus for DPU 7.0+.

> 
>>          if (cmd_mode_cfg->data_compress)
>>                  intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>
>> +       if (cmd_mode_cfg->wide_bus_en)
>> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>> +
>>          DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>   }
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> index 77f80531782b..c539025c418b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>>
>>   struct dpu_hw_intf_cmd_mode_cfg {
>>          u8 data_compress;       /* enable data compress between dpu and dsi */
>> +       u8 wide_bus_en;         /* enable databus widen mode */
>>   };
>>
>>   /**
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 9d9d5e009163..e4f706b16aad 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>>   bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
>> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>>   struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>>   #else
>>   static inline void __init msm_dsi_register(void)
>> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>>   {
>>          return false;
>>   }
>> -
>> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
>> +{
>> +       return false;
>> +}
> 
> Empty line, please.

Acked.

Thanks,

Jessica Zhang

> 
>>   static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>>   {
>>          return NULL;
>>
>> --
>> 2.41.0
>>
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Jessica Zhang]

On 8/2/2023 11:20 AM, Dmitry Baryshkov wrote:
> On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>> DPU supports a data-bus widen mode for DSI INTF.
>>
>> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>>   drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>>   5 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 3dcd37c48aac..de08aad39e15 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>>          struct drm_display_mode *cur_mode = NULL;
>>          struct msm_drm_private *priv = drm_enc->dev->dev_private;
>>          struct msm_display_info *disp_info;
>> +       int index;
>>
>>          dpu_enc = to_dpu_encoder_virt(drm_enc);
>>          disp_info = &dpu_enc->disp_info;
>>
>> +       disp_info = &dpu_enc->disp_info;
>> +       index = disp_info->h_tile_instance[0];
>> +
>>          dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>>
>> -       if (disp_info->intf_type == INTF_DP)
>> -               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> -                               priv->dp[disp_info->h_tile_instance[0]]);
>> +       if (disp_info->intf_type == INTF_DSI)
>> +               dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
>> +       else if (disp_info->intf_type == INTF_DP)
>> +               dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
> 
> If you change the order, you won't have to touch DP lines.

Hi Dmitry,

Acked.

> 
>>
>>          mutex_lock(&dpu_enc->enc_lock);
>>          cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index df88358e7037..dace6168be2d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>                                  phys_enc->hw_intf,
>>                                  phys_enc->hw_pp->idx);
>>
>> -       if (intf_cfg.dsc != 0)
>> +       if (intf_cfg.dsc != 0) {
>>                  cmd_mode_cfg.data_compress = true;
>> +               cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> +       }
> 
> This embeds the knowledge that a wide bus can only be enabled when DSC
> is in use. Please move the wide_bus_en assignment out of conditional
> code.

Wide bus for DSI will only be enabled if DSC is enabled, so this is 
technically not wrong, as DP will use the video mode path.

> 
>>
>>          if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>>                  phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 8ec6505d9e78..dc6f3febb574 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
> 
> This function is only enabled for DPU >= 7.0, while IIRC wide bus can
> be enabled even for some of the earlier chipsets.

The command mode path is only called for DSI, which only supports wide 
bus for DPU 7.0+.

> 
>>          if (cmd_mode_cfg->data_compress)
>>                  intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>
>> +       if (cmd_mode_cfg->wide_bus_en)
>> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>> +
>>          DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>   }
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> index 77f80531782b..c539025c418b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>>
>>   struct dpu_hw_intf_cmd_mode_cfg {
>>          u8 data_compress;       /* enable data compress between dpu and dsi */
>> +       u8 wide_bus_en;         /* enable databus widen mode */
>>   };
>>
>>   /**
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 9d9d5e009163..e4f706b16aad 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>>   bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
>> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>>   struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>>   #else
>>   static inline void __init msm_dsi_register(void)
>> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>>   {
>>          return false;
>>   }
>> -
>> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
>> +{
>> +       return false;
>> +}
> 
> Empty line, please.

Acked.

Thanks,

Jessica Zhang

> 
>>   static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>>   {
>>          return NULL;
>>
>> --
>> 2.41.0
>>
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Jessica Zhang]

On 8/2/2023 12:39 PM, Marijn Suijten wrote:
> On 2023-08-02 11:08:49, Jessica Zhang wrote:
>> DPU supports a data-bus widen mode for DSI INTF.
>>
>> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>>   drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>>   5 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 3dcd37c48aac..de08aad39e15 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>>   	struct drm_display_mode *cur_mode = NULL;
>>   	struct msm_drm_private *priv = drm_enc->dev->dev_private;
>>   	struct msm_display_info *disp_info;
>> +	int index;
>>   
>>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>>   	disp_info = &dpu_enc->disp_info;
>>   
>> +	disp_info = &dpu_enc->disp_info;
>> +	index = disp_info->h_tile_instance[0];
>> +
>>   	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>>   
>> -	if (disp_info->intf_type == INTF_DP)
>> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> -				priv->dp[disp_info->h_tile_instance[0]]);
>> +	if (disp_info->intf_type == INTF_DSI)
>> +		dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
>> +	else if (disp_info->intf_type == INTF_DP)
>> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
> 
> This inconsistency really is killing.  wide_bus vs widebus, and one
> function has an is_ while the other does not.

Hi Marijn,

Acked. Will change the DSI function name to match DP.

> 
>>   
>>   	mutex_lock(&dpu_enc->enc_lock);
>>   	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index df88358e7037..dace6168be2d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>   				phys_enc->hw_intf,
>>   				phys_enc->hw_pp->idx);
>>   
>> -	if (intf_cfg.dsc != 0)
>> +	if (intf_cfg.dsc != 0) {
>>   		cmd_mode_cfg.data_compress = true;
>> +		cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> +	}
>>   
>>   	if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>>   		phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 8ec6505d9e78..dc6f3febb574 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>>   	if (cmd_mode_cfg->data_compress)
>>   		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>   
>> +	if (cmd_mode_cfg->wide_bus_en)
>> +		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>> +
>>   	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> index 77f80531782b..c539025c418b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>>   
>>   struct dpu_hw_intf_cmd_mode_cfg {
>>   	u8 data_compress;	/* enable data compress between dpu and dsi */
>> +	u8 wide_bus_en;		/* enable databus widen mode */
> 
> Any clue why these weren't just bool types?  These suffix-comments also
> aren't adhering to the kerneldoc format, or is there a different
> variant?

It seems that the `u8` declaration and comment docs were meant to mirror 
the other dpu_hw_intf_* structs [1]

[1] 
https://elixir.bootlin.com/linux/v6.5-rc5/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h#L44

> 
>>   };
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 9d9d5e009163..e4f706b16aad 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>>   bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
>> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>>   struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>>   #else
>>   static inline void __init msm_dsi_register(void)
>> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>>   {
>>   	return false;
>>   }
>> -
>> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
>> +{
>> +	return false;
>> +}
> 
> Only this default inline implementation is defined, but the function is
> declared in this commit.  Since there's no real functional
> implementation yet your commit should clarify that it comes later (in a
> followup commit in the same series?  I can't know because I am reviewing
> this series linearly from start to finish...) or reorder the patches so
> that this lack of clarity is circumvented entirely.

This was so that there wouldn't be a compiler error in cases where 
CONFIG_MSM_DSI=n (since DPU support is added before DSI support).

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>>   static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>>   {
>>   	return NULL;
>>
>> -- 
>> 2.41.0
>>

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Jessica Zhang]

On 8/2/2023 12:39 PM, Marijn Suijten wrote:
> On 2023-08-02 11:08:49, Jessica Zhang wrote:
>> DPU supports a data-bus widen mode for DSI INTF.
>>
>> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 11 ++++++++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  4 +++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          |  3 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  1 +
>>   drivers/gpu/drm/msm/msm_drv.h                        |  6 +++++-
>>   5 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 3dcd37c48aac..de08aad39e15 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>>   	struct drm_display_mode *cur_mode = NULL;
>>   	struct msm_drm_private *priv = drm_enc->dev->dev_private;
>>   	struct msm_display_info *disp_info;
>> +	int index;
>>   
>>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>>   	disp_info = &dpu_enc->disp_info;
>>   
>> +	disp_info = &dpu_enc->disp_info;
>> +	index = disp_info->h_tile_instance[0];
>> +
>>   	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>>   
>> -	if (disp_info->intf_type == INTF_DP)
>> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> -				priv->dp[disp_info->h_tile_instance[0]]);
>> +	if (disp_info->intf_type == INTF_DSI)
>> +		dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
>> +	else if (disp_info->intf_type == INTF_DP)
>> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
> 
> This inconsistency really is killing.  wide_bus vs widebus, and one
> function has an is_ while the other does not.

Hi Marijn,

Acked. Will change the DSI function name to match DP.

> 
>>   
>>   	mutex_lock(&dpu_enc->enc_lock);
>>   	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index df88358e7037..dace6168be2d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>   				phys_enc->hw_intf,
>>   				phys_enc->hw_pp->idx);
>>   
>> -	if (intf_cfg.dsc != 0)
>> +	if (intf_cfg.dsc != 0) {
>>   		cmd_mode_cfg.data_compress = true;
>> +		cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> +	}
>>   
>>   	if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>>   		phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 8ec6505d9e78..dc6f3febb574 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>>   	if (cmd_mode_cfg->data_compress)
>>   		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>   
>> +	if (cmd_mode_cfg->wide_bus_en)
>> +		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>> +
>>   	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> index 77f80531782b..c539025c418b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>>   
>>   struct dpu_hw_intf_cmd_mode_cfg {
>>   	u8 data_compress;	/* enable data compress between dpu and dsi */
>> +	u8 wide_bus_en;		/* enable databus widen mode */
> 
> Any clue why these weren't just bool types?  These suffix-comments also
> aren't adhering to the kerneldoc format, or is there a different
> variant?

It seems that the `u8` declaration and comment docs were meant to mirror 
the other dpu_hw_intf_* structs [1]

[1] 
https://elixir.bootlin.com/linux/v6.5-rc5/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h#L44

> 
>>   };
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 9d9d5e009163..e4f706b16aad 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>>   bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>>   bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
>> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>>   struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>>   #else
>>   static inline void __init msm_dsi_register(void)
>> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>>   {
>>   	return false;
>>   }
>> -
>> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
>> +{
>> +	return false;
>> +}
> 
> Only this default inline implementation is defined, but the function is
> declared in this commit.  Since there's no real functional
> implementation yet your commit should clarify that it comes later (in a
> followup commit in the same series?  I can't know because I am reviewing
> this series linearly from start to finish...) or reorder the patches so
> that this lack of clarity is circumvented entirely.

This was so that there wouldn't be a compiler error in cases where 
CONFIG_MSM_DSI=n (since DPU support is added before DSI support).

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>>   static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>>   {
>>   	return NULL;
>>
>> -- 
>> 2.41.0
>>

Re: [PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Jessica Zhang]

On 8/2/2023 12:32 PM, Marijn Suijten wrote:
> I find this title very undescriptive, it doesn't really explain from/to
> where this move is happening nor why.
> 
> On 2023-08-02 11:08:48, Jessica Zhang wrote:
>> Move the setting of dpu_enc.wide_bus_en to
>> dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
>> dpu_enc.dsc.
> 
> mirroring "the setting of dpu_enc.dsc" very much sounds like you are
> mirroring _its value_, but that is not the case.  You are moving the
> initialization (or just setting, because it could also be overwriting?)
> to _the same place_ where .dsc is assigned.

Hi Marijn,

Hmm.. got it. Will reword it to "mirror how dpu_enc.dsc is being set" if 
that makes it clearer.

> 
> I am pretty sure that this has a runtime impact which we discussed
> before (hotplug...?) but the commit message omits that.  This is
> mandatory.

I'm assuming the prior discussion you're referring to is with Kuogee on 
his DSC fix [1]. Unlike DSC, both DSI and DP know if wide bus is enabled 
upon initialization.

The main reasons the setting of the wide_bus_en flag was moved here were

1) to mirror how dpu_enc.dsc was being set (as stated in the commit 
message) as wide bus is related to DSC,

and 2) account for the possibility of DSC for DSI being set during 
runtime in the future.

Thanks,

Jessica Zhang

[1] https://patchwork.freedesktop.org/patch/543867/

> 
> - Marijn
> 
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index d34e684a4178..3dcd37c48aac 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>>   	struct dpu_encoder_virt *dpu_enc = NULL;
>>   	int ret = 0;
>>   	struct drm_display_mode *cur_mode = NULL;
>> +	struct msm_drm_private *priv = drm_enc->dev->dev_private;
>> +	struct msm_display_info *disp_info;
>>   
>>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +	disp_info = &dpu_enc->disp_info;
>>   
>>   	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>>   
>> +	if (disp_info->intf_type == INTF_DP)
>> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> +				priv->dp[disp_info->h_tile_instance[0]]);
>> +
>>   	mutex_lock(&dpu_enc->enc_lock);
>>   	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>>   
>> @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>   	timer_setup(&dpu_enc->frame_done_timer,
>>   			dpu_encoder_frame_done_timeout, 0);
>>   
>> -	if (disp_info->intf_type == INTF_DP)
>> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> -				priv->dp[disp_info->h_tile_instance[0]]);
>> -
>>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>   			dpu_encoder_off_work);
>>   	dpu_enc->idle_timeout = IDLE_TIMEOUT;
>>
>> -- 
>> 2.41.0
>>

Re: [PATCH v3 1/4] drm/msm/dpu: Move DPU encoder wide_bus_en setting

[Jessica Zhang]

On 8/2/2023 12:32 PM, Marijn Suijten wrote:
> I find this title very undescriptive, it doesn't really explain from/to
> where this move is happening nor why.
> 
> On 2023-08-02 11:08:48, Jessica Zhang wrote:
>> Move the setting of dpu_enc.wide_bus_en to
>> dpu_encoder_virt_atomic_enable() so that it mirrors the setting of
>> dpu_enc.dsc.
> 
> mirroring "the setting of dpu_enc.dsc" very much sounds like you are
> mirroring _its value_, but that is not the case.  You are moving the
> initialization (or just setting, because it could also be overwriting?)
> to _the same place_ where .dsc is assigned.

Hi Marijn,

Hmm.. got it. Will reword it to "mirror how dpu_enc.dsc is being set" if 
that makes it clearer.

> 
> I am pretty sure that this has a runtime impact which we discussed
> before (hotplug...?) but the commit message omits that.  This is
> mandatory.

I'm assuming the prior discussion you're referring to is with Kuogee on 
his DSC fix [1]. Unlike DSC, both DSI and DP know if wide bus is enabled 
upon initialization.

The main reasons the setting of the wide_bus_en flag was moved here were

1) to mirror how dpu_enc.dsc was being set (as stated in the commit 
message) as wide bus is related to DSC,

and 2) account for the possibility of DSC for DSI being set during 
runtime in the future.

Thanks,

Jessica Zhang

[1] https://patchwork.freedesktop.org/patch/543867/

> 
> - Marijn
> 
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index d34e684a4178..3dcd37c48aac 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>>   	struct dpu_encoder_virt *dpu_enc = NULL;
>>   	int ret = 0;
>>   	struct drm_display_mode *cur_mode = NULL;
>> +	struct msm_drm_private *priv = drm_enc->dev->dev_private;
>> +	struct msm_display_info *disp_info;
>>   
>>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +	disp_info = &dpu_enc->disp_info;
>>   
>>   	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>>   
>> +	if (disp_info->intf_type == INTF_DP)
>> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> +				priv->dp[disp_info->h_tile_instance[0]]);
>> +
>>   	mutex_lock(&dpu_enc->enc_lock);
>>   	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>>   
>> @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>   	timer_setup(&dpu_enc->frame_done_timer,
>>   			dpu_encoder_frame_done_timeout, 0);
>>   
>> -	if (disp_info->intf_type == INTF_DP)
>> -		dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> -				priv->dp[disp_info->h_tile_instance[0]]);
>> -
>>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>   			dpu_encoder_off_work);
>>   	dpu_enc->idle_timeout = IDLE_TIMEOUT;
>>
>> -- 
>> 2.41.0
>>

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Dmitry Baryshkov]

On 08/08/2023 00:40, Jessica Zhang wrote:
> 
> 
> On 8/2/2023 11:20 AM, Dmitry Baryshkov wrote:
>> On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> 
>> wrote:

>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index df88358e7037..dace6168be2d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>                                  phys_enc->hw_intf,
>>>                                  phys_enc->hw_pp->idx);
>>>
>>> -       if (intf_cfg.dsc != 0)
>>> +       if (intf_cfg.dsc != 0) {
>>>                  cmd_mode_cfg.data_compress = true;
>>> +               cmd_mode_cfg.wide_bus_en = 
>>> dpu_encoder_is_widebus_enabled(phys_enc->parent);
>>> +       }
>>
>> This embeds the knowledge that a wide bus can only be enabled when DSC
>> is in use. Please move the wide_bus_en assignment out of conditional
>> code.
> 
> Wide bus for DSI will only be enabled if DSC is enabled, so this is 
> technically not wrong, as DP will use the video mode path.
> 
>>
>>>
>>>          if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>>>                  
>>> phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, 
>>> &cmd_mode_cfg);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> index 8ec6505d9e78..dc6f3febb574 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -521,6 +521,9 @@ static void 
>>> dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>>
>> This function is only enabled for DPU >= 7.0, while IIRC wide bus can
>> be enabled even for some of the earlier chipsets.
> 
> The command mode path is only called for DSI, which only supports wide 
> bus for DPU 7.0+.

After second consideration, let's ignore this part, as wide bus will 
only be enabled for DSI / CMD after 7.0. If we ever have SoC that has 
CMD + wide_bus earlier than 5.0, we can reconsider this code pice.

Can you please add a comment that the register itself is present earlier 
(5.0), but it doesn't have to be programmed since the flags will not be 
set anyway.

> 
>>
>>>          if (cmd_mode_cfg->data_compress)
>>>                  intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>>
>>> +       if (cmd_mode_cfg->wide_bus_en)
>>> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>>> +
>>>          DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>>   }
>>>



-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] drm/msm/dpu: Enable widebus for DSI INTF

[Dmitry Baryshkov]

On 08/08/2023 00:40, Jessica Zhang wrote:
> 
> 
> On 8/2/2023 11:20 AM, Dmitry Baryshkov wrote:
>> On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> 
>> wrote:

>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index df88358e7037..dace6168be2d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>                                  phys_enc->hw_intf,
>>>                                  phys_enc->hw_pp->idx);
>>>
>>> -       if (intf_cfg.dsc != 0)
>>> +       if (intf_cfg.dsc != 0) {
>>>                  cmd_mode_cfg.data_compress = true;
>>> +               cmd_mode_cfg.wide_bus_en = 
>>> dpu_encoder_is_widebus_enabled(phys_enc->parent);
>>> +       }
>>
>> This embeds the knowledge that a wide bus can only be enabled when DSC
>> is in use. Please move the wide_bus_en assignment out of conditional
>> code.
> 
> Wide bus for DSI will only be enabled if DSC is enabled, so this is 
> technically not wrong, as DP will use the video mode path.
> 
>>
>>>
>>>          if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>>>                  
>>> phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, 
>>> &cmd_mode_cfg);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> index 8ec6505d9e78..dc6f3febb574 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -521,6 +521,9 @@ static void 
>>> dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>>
>> This function is only enabled for DPU >= 7.0, while IIRC wide bus can
>> be enabled even for some of the earlier chipsets.
> 
> The command mode path is only called for DSI, which only supports wide 
> bus for DPU 7.0+.

After second consideration, let's ignore this part, as wide bus will 
only be enabled for DSI / CMD after 7.0. If we ever have SoC that has 
CMD + wide_bus earlier than 5.0, we can reconsider this code pice.

Can you please add a comment that the register itself is present earlier 
(5.0), but it doesn't have to be programmed since the flags will not be 
set anyway.

> 
>>
>>>          if (cmd_mode_cfg->data_compress)
>>>                  intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>>
>>> +       if (cmd_mode_cfg->wide_bus_en)
>>> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>>> +
>>>          DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>>   }
>>>



-- 
With best wishes
Dmitry