[PATCH v4 0/2] enable widebus feature base on chip hardware revision

[PATCH v4 0/2] enable widebus feature base on chip hardware revision

[Kuogee Hsieh]

split into 2 patches
1) widebus timing engine programming
2) enable widebus feature base on chip hardware revision

Kuogee Hsieh (2):
  drm/msm/dp:  revise timing engine programming to support widebus
    feature
  drm/msm/dp: enable widebus feature for display port

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 14 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  2 +
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 14 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 99 ++++++++++++++++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  6 ++
 drivers/gpu/drm/msm/dp/dp_catalog.c                | 36 +++++++-
 drivers/gpu/drm/msm/dp/dp_catalog.h                |  3 +-
 drivers/gpu/drm/msm/dp/dp_ctrl.c                   | 13 ++-
 drivers/gpu/drm/msm/dp/dp_ctrl.h                   |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c                | 30 +++++++
 drivers/gpu/drm/msm/dp/dp_display.h                |  2 +
 drivers/gpu/drm/msm/dp/dp_panel.c                  |  4 +-
 drivers/gpu/drm/msm/dp/dp_panel.h                  |  2 +-
 drivers/gpu/drm/msm/msm_drv.h                      |  6 ++
 14 files changed, 205 insertions(+), 27 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 1/2] drm/msm/dp: revise timing engine programming to support widebus feature

[Kuogee Hsieh]

Widebus feature will transmit two pixel data per pixel clock to interface.
Timing engine provides driving force for this purpose. This patch base
on HPG (Hardware Programming Guide) to revise timing engine register
setting to accommodate both widebus and non widebus application. Also
horizontal width parameters need to be reduced by half since two pixel
data are clocked out per pixel clock when widebus feature enabled.

Widebus can be enabled individually at DP. However at DSI, widebus have
to be enabled along with DSC enabled to achieve pixel clock rate be
scaled down with same ratio as compression ratio when 10 bits per source
component. Therefore this patch have both widebus and compression covered
together so tat less efforts will be required when DSC enabled later.

Changes in v2:
-- remove compression related code from timing
-- remove op_info from  struct msm_drm_private
-- remove unnecessary wide_bus_en variables
-- pass wide_bus_en into timing configuration by struct msm_dp

Changes in v3:
-- split patch into 3 patches

Changes in v4:
-- rework timing engine to not interfere with dsi/hdmi
-- cover both widebus and compression

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 10 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  2 +
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 14 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 99 ++++++++++++++++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  6 ++
 5 files changed, 115 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0d315b4..0c22839 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -208,6 +208,8 @@ struct dpu_encoder_virt {
 
 	u32 idle_timeout;
 
+	bool wide_bus_en;
+
 	struct msm_dp *dp;
 };
 
@@ -217,6 +219,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
 	15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
 };
 
+
+bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	return dpu_enc->wide_bus_en;
+}
+
 static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
 {
 	struct dpu_hw_dither_cfg dither_cfg = { 0 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 99a5d73..893d74d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -168,4 +168,6 @@ int dpu_encoder_get_linecount(struct drm_encoder *drm_enc);
  */
 int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc);
 
+bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc);
+
 #endif /* __DPU_ENCODER_H__ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 185379b..2af2bb7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -110,6 +110,20 @@ static void drm_mode_to_intf_timing_params(
 		timing->v_back_porch += timing->v_front_porch;
 		timing->v_front_porch = 0;
 	}
+
+	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
+
+	/*
+	 * for DP, divide the horizonal parameters by 2 when
+	 * widebus is enabled
+	 */
+	if (phys_enc->hw_intf->cap->type == INTF_DP && timing->wide_bus_en) {
+		timing->width = timing->width >> 1;
+		timing->xres = timing->xres >> 1;
+		timing->h_back_porch = timing->h_back_porch >> 1;
+		timing->h_front_porch = timing->h_front_porch >> 1;
+		timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
+	}
 }
 
 static u32 get_horizontal_total(const struct intf_timing_params *timing)
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 116e2b5..3b9273e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -33,6 +33,7 @@
 #define INTF_TP_COLOR1                  0x05C
 #define INTF_CONFIG2                    0x060
 #define INTF_DISPLAY_DATA_HCTL          0x064
+#define INTF_ACTIVE_DATA_HCTL           0x068
 #define INTF_FRAME_LINE_COUNT_EN        0x0A8
 #define INTF_FRAME_COUNT                0x0AC
 #define   INTF_LINE_COUNT               0x0B0
@@ -60,6 +61,14 @@
 
 #define   INTF_MUX                      0x25C
 
+#define BIT_INTF_CFG_ACTIVE_H_EN	BIT(29)
+#define BIT_INTF_CFG_ACTIVE_V_EN	BIT(30)
+
+#define BIT_INTF_CFG2_DATABUS_WIDEN	BIT(0)
+#define BIT_INTF_CFG2_DATA_HCTL_EN	BIT(4)
+#define BIT_INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
+
+
 static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
 		const struct dpu_mdss_cfg *m,
 		void __iomem *addr,
@@ -90,15 +99,23 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	u32 hsync_period, vsync_period;
 	u32 display_v_start, display_v_end;
 	u32 hsync_start_x, hsync_end_x;
+	u32 hsync_data_start_x, hsync_data_end_x;
 	u32 active_h_start, active_h_end;
 	u32 active_v_start, active_v_end;
 	u32 active_hctl, display_hctl, hsync_ctl;
 	u32 polarity_ctl, den_polarity, hsync_polarity, vsync_polarity;
 	u32 panel_format;
-	u32 intf_cfg, intf_cfg2 = 0, display_data_hctl = 0;
+	u32 intf_cfg, intf_cfg2 = 0;
+	u32 display_data_hctl = 0, active_data_hctl = 0;
+	u32 data_width;
+	bool dp_intf = false;
 
 	/* read interface_cfg */
 	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
+
+	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
+		dp_intf = true;
+
 	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
 	p->h_front_porch;
 	vsync_period = p->vsync_pulse_width + p->v_back_porch + p->height +
@@ -112,7 +129,10 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
 	hsync_end_x = hsync_period - p->h_front_porch - 1;
 
-	if (p->width != p->xres) {
+	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
+	display_hctl = (hsync_end_x << 16) | hsync_start_x;
+
+	if (p->width != p->xres) { /* border fill added */
 		active_h_start = hsync_start_x;
 		active_h_end = active_h_start + p->xres - 1;
 	} else {
@@ -130,27 +150,78 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 
 	if (active_h_end) {
 		active_hctl = (active_h_end << 16) | active_h_start;
-		intf_cfg |= BIT(29);	/* ACTIVE_H_ENABLE */
+		intf_cfg |= BIT_INTF_CFG_ACTIVE_H_EN;
 	} else {
 		active_hctl = 0;
 	}
 
 	if (active_v_end)
-		intf_cfg |= BIT(30); /* ACTIVE_V_ENABLE */
+		intf_cfg |= BIT_INTF_CFG_ACTIVE_V_EN;
 
-	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
-	display_hctl = (hsync_end_x << 16) | hsync_start_x;
+	/*
+	 * DATA_HCTL_EN controls data timing which can be different from
+	 * video timing. It is recommended to enable it for all cases, except
+	 * if compression is enabled in 1 pixel per clock mode
+	 */
+	if (!p->compression_en || p->wide_bus_en)
+		intf_cfg2 |= BIT_INTF_CFG2_DATA_HCTL_EN;
+
+	if (p->wide_bus_en)
+		intf_cfg2 |=  BIT_INTF_CFG2_DATABUS_WIDEN;
+
+	/*
+	 * If widebus is disabled:
+	 * For uncompressed stream, the data is valid for the entire active
+	 * window period.
+	 * For compressed stream, data is valid for a shorter time period
+	 * inside the active window depending on the compression ratio.
+	 *
+	 * If widebus is enabled:
+	 * For uncompressed stream, data is valid for only half the active
+	 * window, since the data rate is doubled in this mode.
+	 * p->width holds the adjusted width for DP but unadjusted width for DSI
+	 * For compressed stream, data validity window needs to be adjusted for
+	 * compression ratio and then further halved.
+	 */
+	data_width = p->width;
+
+	if (p->compression_en) {
+		data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
+
+		if (p->wide_bus_en)
+			data_width >>= 1;
+	} else if (!dp_intf && p->wide_bus_en) {
+		data_width = p->width >> 1;
+	}
+
+	hsync_data_start_x = hsync_start_x;
+	hsync_data_end_x =  hsync_start_x + data_width - 1;
+
+	display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
+
+	if (dp_intf) {
+		/* DP timing adjustment */
+		display_v_start += p->hsync_pulse_width + p->h_back_porch;
+		display_v_end   -= p->h_front_porch;
 
-	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP) {
 		active_h_start = hsync_start_x;
 		active_h_end = active_h_start + p->xres - 1;
 		active_v_start = display_v_start;
 		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
 
-		display_v_start += p->hsync_pulse_width + p->h_back_porch;
-
 		active_hctl = (active_h_end << 16) | active_h_start;
 		display_hctl = active_hctl;
+
+		intf_cfg |= BIT_INTF_CFG_ACTIVE_H_EN;
+		intf_cfg |= BIT_INTF_CFG_ACTIVE_V_EN;
+
+		if (p->compression_en) {
+			active_data_hctl = (hsync_start_x +
+					p->extra_dto_cycles) << 16;
+			active_data_hctl += hsync_start_x;
+
+			display_data_hctl = active_data_hctl;
+		}
 	}
 
 	den_polarity = 0;
@@ -180,13 +251,6 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 				(COLOR_8BIT << 4) |
 				(0x21 << 8));
 
-	if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
-		intf_cfg2 |= BIT(4);
-		display_data_hctl = display_hctl;
-		DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
-		DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
-	}
-
 	DPU_REG_WRITE(c, INTF_HSYNC_CTL, hsync_ctl);
 	DPU_REG_WRITE(c, INTF_VSYNC_PERIOD_F0, vsync_period * hsync_period);
 	DPU_REG_WRITE(c, INTF_VSYNC_PULSE_WIDTH_F0,
@@ -204,6 +268,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
 	DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
 	DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
+	DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
+	DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
+	DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
 }
 
 static void dpu_hw_intf_enable_timing_engine(
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 3568be8..299c9c1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -30,6 +30,12 @@ struct intf_timing_params {
 	u32 border_clr;
 	u32 underflow_clr;
 	u32 hsync_skew;
+
+	bool wide_bus_en;
+	bool compression_en;
+	u32 extra_dto_cycles;   /* for DP only */
+	bool dsc_4hs_merge;     /* DSC 4HS merge */
+	u32 dce_bytes_per_line;
 };
 
 struct intf_prog_fetch {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Kuogee Hsieh]

Widebus feature will transmit two pixel data per pixel clock to interface.
This feature now is required to be enabled to easy migrant to higher
resolution applications in future. However since some legacy chipsets
does not support this feature, this feature is enabled base on chip's
hardware revision.

changes in v2:
-- remove compression related code from timing
-- remove op_info from  struct msm_drm_private
-- remove unnecessary wide_bus_en variables
-- pass wide_bus_en into timing configuration by struct msm_dp

Changes in v3:
-- split patch into 3 patches
-- enable widebus feature base on chip hardware revision

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
 drivers/gpu/drm/msm/dp/dp_catalog.c         | 36 +++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
 drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
 drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c         | 30 ++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
 drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
 drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
 drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
 10 files changed, 90 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0c22839..b2d23c2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 		timer_setup(&dpu_enc->vsync_event_timer,
 				dpu_encoder_vsync_event_handler,
 				0);
-	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
 		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
+		dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
+	}
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 64f0b26..99d087e 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog,
 }
 
 /**
+ * dp_catalog_hw_revision() - retrieve DP hw revision
+ *
+ * @dp_catalog: DP catalog structure
+ *
+ * return: u32
+ *
+ * This function return the DP controller hw revision
+ *
+ */
+u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
+{
+	u32 revision;
+	struct dp_catalog_private *catalog = container_of(dp_catalog,
+				struct dp_catalog_private, dp_catalog);
+
+	revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
+
+	return revision;
+}
+
+/**
  * dp_catalog_ctrl_reset() - reset DP controller
  *
  * @dp_catalog: DP catalog structure
@@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog)
 }
 
 /* panel related catalog functions */
-int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
+int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en)
 {
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 				struct dp_catalog_private, dp_catalog);
+	u32 reg;
 
 	dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
 				dp_catalog->total);
@@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
 	dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
 				dp_catalog->width_blanking);
 	dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, dp_catalog->dp_active);
-	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
+
+	reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
+
+	if (wide_bus_en)
+		reg |= BIT(4);	/* DATABUS_WIDEN */
+	else
+		reg &= ~BIT(4);
+
+	DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
+
+	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..a3a0129 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32 cc, u32 tb);
 void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 rate,
 				u32 stream_rate_khz, bool fixed_nvid);
 int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog, u32 pattern);
+u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
 void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
 bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
 void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
@@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog,
 u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
 
 /* DP Panel APIs */
-int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
+int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en);
 void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
 void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
 				struct drm_display_mode *drm_mode);
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 245e1b9..1c4cf9d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
 	dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
 }
 
-static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
+static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl, bool wide_bus_en)
 {
 	u32 cc, tb;
 
@@ -167,7 +167,7 @@ static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
 		ctrl->panel->dp_mode.bpp);
 	cc = dp_link_get_colorimetry_config(ctrl->link);
 	dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
-	dp_panel_timing_cfg(ctrl->panel);
+	dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
 }
 
 /*
@@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 	int ret = 0;
 	bool mainlink_ready = false;
 	struct dp_ctrl_private *ctrl;
+	u32 pixel_rate_orig;
 
 	if (!dp_ctrl)
 		return -EINVAL;
@@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 
 	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 
+	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
+	if (dp_ctrl->wide_bus_en)
+		ctrl->dp_ctrl.pixel_rate >>= 1;
+
 	DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
 		ctrl->link->link_params.rate,
 		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
@@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 	 */
 	reinit_completion(&ctrl->video_comp);
 
-	dp_ctrl_configure_source_params(ctrl);
+	dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
 
 	dp_catalog_ctrl_config_msa(ctrl->catalog,
 		ctrl->link->link_params.rate,
-		ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
+		pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
 
 	dp_ctrl_setup_tr_unit(ctrl);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..4dff44d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -17,6 +17,7 @@ struct dp_ctrl {
 	bool orientation;
 	atomic_t aborted;
 	u32 pixel_rate;
+	bool wide_bus_en;
 };
 
 int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index e89556ad..d45a3aa 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -117,6 +117,8 @@ struct dp_display_private {
 	struct dp_event event_list[DP_EVENT_Q_MAX];
 	spinlock_t event_lock;
 
+	bool wide_bus_en;
+
 	struct dp_audio *audio;
 };
 
@@ -845,6 +847,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
 		return 0;
 	}
 
+	dp->ctrl->wide_bus_en = dp->wide_bus_en;
+
 	rc = dp_ctrl_on_stream(dp->ctrl);
 	if (!rc)
 		dp_display->power_on = true;
@@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
 		dp->connector, dp_mode);
 	if (dp_mode->drm_mode.clock)
 		dp->max_pclk_khz = dp_mode->drm_mode.clock;
+
 	return ret;
 }
 
@@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
 	}
 }
 
+bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
+{
+	struct dp_display_private *dp;
+	u32 revision, major, minor;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+	/* for the time being widebus only support on DP */
+	if (dp_display->connector_type  == DRM_MODE_CONNECTOR_DisplayPort) {
+		revision = dp_catalog_hw_revision(dp->catalog);
+		major = ((revision >> 28) & 0x0ff);
+		minor = ((revision >> 16) & 0x0fff);
+
+	DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
+
+		if (major >= 1 && minor >= 2)
+			return true;
+	}
+
+	return false;
+}
+
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 {
 	struct dp_display_private *dp;
@@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 	dp_priv->panel->connector = dp_display->connector;
 
 	priv->connectors[priv->num_connectors++] = dp_display->connector;
+
+	dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 8e80e3b..d9cb9ee 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -23,6 +23,8 @@ struct msm_dp {
 
 	hdmi_codec_plugged_cb plugged_cb;
 
+	bool wide_bus_en;
+
 	u32 max_pclk_khz;
 
 	u32 max_dp_lanes;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 71db10c..71deb1e 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
 	dp_catalog_dump_regs(catalog);
 }
 
-int dp_panel_timing_cfg(struct dp_panel *dp_panel)
+int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
 {
 	u32 data, total_ver, total_hor;
 	struct dp_catalog *catalog;
@@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
 
 	catalog->dp_active = data;
 
-	dp_catalog_panel_timing_cfg(catalog);
+	dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
 	panel->panel_on = true;
 
 	return 0;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 9023e5b..5ec341a 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -57,7 +57,7 @@ struct dp_panel {
 
 int dp_panel_init_panel_info(struct dp_panel *dp_panel);
 int dp_panel_deinit(struct dp_panel *dp_panel);
-int dp_panel_timing_cfg(struct dp_panel *dp_panel);
+int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
 void dp_panel_dump_regs(struct dp_panel *dp_panel);
 int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
 		struct drm_connector *connector);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 07f6c41..667f3a8 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display);
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
+bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
 
 #else
 static inline int __init msm_dp_register(void)
@@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
 {
 }
 
+bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
+{
+	return false;
+}
+
 #endif
 
 void __init msm_mdp_register(void);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 1/2] drm/msm/dp: revise timing engine programming to support widebus feature

[Bjorn Andersson]

On Mon 14 Feb 16:39 CST 2022, Kuogee Hsieh wrote:

> Widebus feature will transmit two pixel data per pixel clock to interface.
> Timing engine provides driving force for this purpose. This patch base
> on HPG (Hardware Programming Guide) to revise timing engine register
> setting to accommodate both widebus and non widebus application. Also
> horizontal width parameters need to be reduced by half since two pixel
> data are clocked out per pixel clock when widebus feature enabled.
> 
> Widebus can be enabled individually at DP. However at DSI, widebus have
> to be enabled along with DSC enabled to achieve pixel clock rate be
> scaled down with same ratio as compression ratio when 10 bits per source
> component. Therefore this patch have both widebus and compression covered
> together so tat less efforts will be required when DSC enabled later.
> 
> Changes in v2:
> -- remove compression related code from timing
> -- remove op_info from  struct msm_drm_private
> -- remove unnecessary wide_bus_en variables
> -- pass wide_bus_en into timing configuration by struct msm_dp
> 
> Changes in v3:
> -- split patch into 3 patches
> 
> Changes in v4:
> -- rework timing engine to not interfere with dsi/hdmi
> -- cover both widebus and compression
> 

Even though the change relates to DP, I think it would be appropriate to
change the $subject prefix to "drm/msm/dpu".

When booting sc8180x the bootloader leaves widebus enabled in the eDP
controller, and the two patches takes care of this problem for me. I
also checked the DP still works.

Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Thanks,
Bjorn

> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 10 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  2 +
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 14 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 99 ++++++++++++++++++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  6 ++
>  5 files changed, 115 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 0d315b4..0c22839 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -208,6 +208,8 @@ struct dpu_encoder_virt {
>  
>  	u32 idle_timeout;
>  
> +	bool wide_bus_en;
> +
>  	struct msm_dp *dp;
>  };
>  
> @@ -217,6 +219,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
>  	15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
>  };
>  
> +
> +bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc)
> +{
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> +	return dpu_enc->wide_bus_en;
> +}
> +
>  static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
>  {
>  	struct dpu_hw_dither_cfg dither_cfg = { 0 };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 99a5d73..893d74d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -168,4 +168,6 @@ int dpu_encoder_get_linecount(struct drm_encoder *drm_enc);
>   */
>  int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc);
>  
> +bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc);
> +
>  #endif /* __DPU_ENCODER_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 185379b..2af2bb7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -110,6 +110,20 @@ static void drm_mode_to_intf_timing_params(
>  		timing->v_back_porch += timing->v_front_porch;
>  		timing->v_front_porch = 0;
>  	}
> +
> +	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> +
> +	/*
> +	 * for DP, divide the horizonal parameters by 2 when
> +	 * widebus is enabled
> +	 */
> +	if (phys_enc->hw_intf->cap->type == INTF_DP && timing->wide_bus_en) {
> +		timing->width = timing->width >> 1;
> +		timing->xres = timing->xres >> 1;
> +		timing->h_back_porch = timing->h_back_porch >> 1;
> +		timing->h_front_porch = timing->h_front_porch >> 1;
> +		timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
> +	}
>  }
>  
>  static u32 get_horizontal_total(const struct intf_timing_params *timing)
> 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 116e2b5..3b9273e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -33,6 +33,7 @@
>  #define INTF_TP_COLOR1                  0x05C
>  #define INTF_CONFIG2                    0x060
>  #define INTF_DISPLAY_DATA_HCTL          0x064
> +#define INTF_ACTIVE_DATA_HCTL           0x068
>  #define INTF_FRAME_LINE_COUNT_EN        0x0A8
>  #define INTF_FRAME_COUNT                0x0AC
>  #define   INTF_LINE_COUNT               0x0B0
> @@ -60,6 +61,14 @@
>  
>  #define   INTF_MUX                      0x25C
>  
> +#define BIT_INTF_CFG_ACTIVE_H_EN	BIT(29)
> +#define BIT_INTF_CFG_ACTIVE_V_EN	BIT(30)
> +
> +#define BIT_INTF_CFG2_DATABUS_WIDEN	BIT(0)
> +#define BIT_INTF_CFG2_DATA_HCTL_EN	BIT(4)
> +#define BIT_INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
> +
> +
>  static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
>  		const struct dpu_mdss_cfg *m,
>  		void __iomem *addr,
> @@ -90,15 +99,23 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  	u32 hsync_period, vsync_period;
>  	u32 display_v_start, display_v_end;
>  	u32 hsync_start_x, hsync_end_x;
> +	u32 hsync_data_start_x, hsync_data_end_x;
>  	u32 active_h_start, active_h_end;
>  	u32 active_v_start, active_v_end;
>  	u32 active_hctl, display_hctl, hsync_ctl;
>  	u32 polarity_ctl, den_polarity, hsync_polarity, vsync_polarity;
>  	u32 panel_format;
> -	u32 intf_cfg, intf_cfg2 = 0, display_data_hctl = 0;
> +	u32 intf_cfg, intf_cfg2 = 0;
> +	u32 display_data_hctl = 0, active_data_hctl = 0;
> +	u32 data_width;
> +	bool dp_intf = false;
>  
>  	/* read interface_cfg */
>  	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
> +
> +	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
> +		dp_intf = true;
> +
>  	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
>  	p->h_front_porch;
>  	vsync_period = p->vsync_pulse_width + p->v_back_porch + p->height +
> @@ -112,7 +129,10 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  	hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
>  	hsync_end_x = hsync_period - p->h_front_porch - 1;
>  
> -	if (p->width != p->xres) {
> +	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
> +	display_hctl = (hsync_end_x << 16) | hsync_start_x;
> +
> +	if (p->width != p->xres) { /* border fill added */
>  		active_h_start = hsync_start_x;
>  		active_h_end = active_h_start + p->xres - 1;
>  	} else {
> @@ -130,27 +150,78 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  
>  	if (active_h_end) {
>  		active_hctl = (active_h_end << 16) | active_h_start;
> -		intf_cfg |= BIT(29);	/* ACTIVE_H_ENABLE */
> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_H_EN;
>  	} else {
>  		active_hctl = 0;
>  	}
>  
>  	if (active_v_end)
> -		intf_cfg |= BIT(30); /* ACTIVE_V_ENABLE */
> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_V_EN;
>  
> -	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
> -	display_hctl = (hsync_end_x << 16) | hsync_start_x;
> +	/*
> +	 * DATA_HCTL_EN controls data timing which can be different from
> +	 * video timing. It is recommended to enable it for all cases, except
> +	 * if compression is enabled in 1 pixel per clock mode
> +	 */
> +	if (!p->compression_en || p->wide_bus_en)
> +		intf_cfg2 |= BIT_INTF_CFG2_DATA_HCTL_EN;
> +
> +	if (p->wide_bus_en)
> +		intf_cfg2 |=  BIT_INTF_CFG2_DATABUS_WIDEN;
> +
> +	/*
> +	 * If widebus is disabled:
> +	 * For uncompressed stream, the data is valid for the entire active
> +	 * window period.
> +	 * For compressed stream, data is valid for a shorter time period
> +	 * inside the active window depending on the compression ratio.
> +	 *
> +	 * If widebus is enabled:
> +	 * For uncompressed stream, data is valid for only half the active
> +	 * window, since the data rate is doubled in this mode.
> +	 * p->width holds the adjusted width for DP but unadjusted width for DSI
> +	 * For compressed stream, data validity window needs to be adjusted for
> +	 * compression ratio and then further halved.
> +	 */
> +	data_width = p->width;
> +
> +	if (p->compression_en) {
> +		data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
> +
> +		if (p->wide_bus_en)
> +			data_width >>= 1;
> +	} else if (!dp_intf && p->wide_bus_en) {
> +		data_width = p->width >> 1;
> +	}
> +
> +	hsync_data_start_x = hsync_start_x;
> +	hsync_data_end_x =  hsync_start_x + data_width - 1;
> +
> +	display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
> +
> +	if (dp_intf) {
> +		/* DP timing adjustment */
> +		display_v_start += p->hsync_pulse_width + p->h_back_porch;
> +		display_v_end   -= p->h_front_porch;
>  
> -	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP) {
>  		active_h_start = hsync_start_x;
>  		active_h_end = active_h_start + p->xres - 1;
>  		active_v_start = display_v_start;
>  		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>  
> -		display_v_start += p->hsync_pulse_width + p->h_back_porch;
> -
>  		active_hctl = (active_h_end << 16) | active_h_start;
>  		display_hctl = active_hctl;
> +
> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_H_EN;
> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_V_EN;
> +
> +		if (p->compression_en) {
> +			active_data_hctl = (hsync_start_x +
> +					p->extra_dto_cycles) << 16;
> +			active_data_hctl += hsync_start_x;
> +
> +			display_data_hctl = active_data_hctl;
> +		}
>  	}
>  
>  	den_polarity = 0;
> @@ -180,13 +251,6 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  				(COLOR_8BIT << 4) |
>  				(0x21 << 8));
>  
> -	if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
> -		intf_cfg2 |= BIT(4);
> -		display_data_hctl = display_hctl;
> -		DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
> -		DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
> -	}
> -
>  	DPU_REG_WRITE(c, INTF_HSYNC_CTL, hsync_ctl);
>  	DPU_REG_WRITE(c, INTF_VSYNC_PERIOD_F0, vsync_period * hsync_period);
>  	DPU_REG_WRITE(c, INTF_VSYNC_PULSE_WIDTH_F0,
> @@ -204,6 +268,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  	DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
>  	DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
>  	DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
> +	DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
> +	DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
> +	DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
>  }
>  
>  static void dpu_hw_intf_enable_timing_engine(
> 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 3568be8..299c9c1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -30,6 +30,12 @@ struct intf_timing_params {
>  	u32 border_clr;
>  	u32 underflow_clr;
>  	u32 hsync_skew;
> +
> +	bool wide_bus_en;
> +	bool compression_en;
> +	u32 extra_dto_cycles;   /* for DP only */
> +	bool dsc_4hs_merge;     /* DSC 4HS merge */
> +	u32 dce_bytes_per_line;
>  };
>  
>  struct intf_prog_fetch {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Bjorn Andersson]

On Mon 14 Feb 16:39 CST 2022, Kuogee Hsieh wrote:

> Widebus feature will transmit two pixel data per pixel clock to interface.
> This feature now is required to be enabled to easy migrant to higher
> resolution applications in future. However since some legacy chipsets
> does not support this feature, this feature is enabled base on chip's
> hardware revision.
> 
> changes in v2:
> -- remove compression related code from timing
> -- remove op_info from  struct msm_drm_private
> -- remove unnecessary wide_bus_en variables
> -- pass wide_bus_en into timing configuration by struct msm_dp
> 
> Changes in v3:
> -- split patch into 3 patches
> -- enable widebus feature base on chip hardware revision
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>

Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
>  drivers/gpu/drm/msm/dp/dp_catalog.c         | 36 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
>  drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
>  drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
>  drivers/gpu/drm/msm/dp/dp_display.c         | 30 ++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
>  drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
>  drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
>  drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
>  10 files changed, 90 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 0c22839..b2d23c2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>  		timer_setup(&dpu_enc->vsync_event_timer,
>  				dpu_encoder_vsync_event_handler,
>  				0);
> -	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
> +	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>  		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
> +	}
>  
>  	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>  			dpu_encoder_off_work);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 64f0b26..99d087e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog,
>  }
>  
>  /**
> + * dp_catalog_hw_revision() - retrieve DP hw revision
> + *
> + * @dp_catalog: DP catalog structure
> + *
> + * return: u32

Q: What's 2+2?
A: Integer

This should say:

Return: the controller hardware revision

> + *
> + * This function return the DP controller hw revision

That's what "Return:" in the kernel-doc is supposed to clarify...

https://docs.kernel.org/doc-guide/kernel-doc.html is good to read.

> + *
> + */
> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
> +{
> +	u32 revision;
> +	struct dp_catalog_private *catalog = container_of(dp_catalog,
> +				struct dp_catalog_private, dp_catalog);
> +
> +	revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);

There's no need for a local variable here, just:

	return dp_read_ahb();

> +
> +	return revision;
> +}
> +
> +/**
>   * dp_catalog_ctrl_reset() - reset DP controller
>   *
>   * @dp_catalog: DP catalog structure
> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog)
>  }
>  
>  /* panel related catalog functions */
> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en)
>  {
>  	struct dp_catalog_private *catalog = container_of(dp_catalog,
>  				struct dp_catalog_private, dp_catalog);
> +	u32 reg;
>  
>  	dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
>  				dp_catalog->total);
> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>  	dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>  				dp_catalog->width_blanking);
>  	dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, dp_catalog->dp_active);
> -	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
> +
> +	reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
> +
> +	if (wide_bus_en)
> +		reg |= BIT(4);	/* DATABUS_WIDEN */

#define DATABUS_WIDEN BIT(4)

Would save you the need for writing that comment.

> +	else
> +		reg &= ~BIT(4);
> +
> +	DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
> +
> +	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 7dea101..a3a0129 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32 cc, u32 tb);
>  void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 rate,
>  				u32 stream_rate_khz, bool fixed_nvid);
>  int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog, u32 pattern);
> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
>  void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
>  bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>  void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog,
>  u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
>  
>  /* DP Panel APIs */
> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en);
>  void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
>  void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
>  				struct drm_display_mode *drm_mode);
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 245e1b9..1c4cf9d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
>  	dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>  }
>  
> -static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl, bool wide_bus_en)
>  {
>  	u32 cc, tb;
>  
> @@ -167,7 +167,7 @@ static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>  		ctrl->panel->dp_mode.bpp);
>  	cc = dp_link_get_colorimetry_config(ctrl->link);
>  	dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
> -	dp_panel_timing_cfg(ctrl->panel);
> +	dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
>  }
>  
>  /*
> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>  	int ret = 0;
>  	bool mainlink_ready = false;
>  	struct dp_ctrl_private *ctrl;
> +	u32 pixel_rate_orig;
>  
>  	if (!dp_ctrl)
>  		return -EINVAL;
> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>  
>  	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>  
> +	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
> +	if (dp_ctrl->wide_bus_en)
> +		ctrl->dp_ctrl.pixel_rate >>= 1;
> +
>  	DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>  		ctrl->link->link_params.rate,
>  		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>  	 */
>  	reinit_completion(&ctrl->video_comp);
>  
> -	dp_ctrl_configure_source_params(ctrl);
> +	dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
>  
>  	dp_catalog_ctrl_config_msa(ctrl->catalog,
>  		ctrl->link->link_params.rate,
> -		ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
> +		pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
>  
>  	dp_ctrl_setup_tr_unit(ctrl);
>  
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 2433edb..4dff44d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -17,6 +17,7 @@ struct dp_ctrl {
>  	bool orientation;
>  	atomic_t aborted;
>  	u32 pixel_rate;
> +	bool wide_bus_en;
>  };
>  
>  int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e89556ad..d45a3aa 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -117,6 +117,8 @@ struct dp_display_private {
>  	struct dp_event event_list[DP_EVENT_Q_MAX];
>  	spinlock_t event_lock;
>  
> +	bool wide_bus_en;
> +
>  	struct dp_audio *audio;
>  };
>  
> @@ -845,6 +847,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>  		return 0;
>  	}
>  
> +	dp->ctrl->wide_bus_en = dp->wide_bus_en;
> +
>  	rc = dp_ctrl_on_stream(dp->ctrl);
>  	if (!rc)
>  		dp_display->power_on = true;
> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>  		dp->connector, dp_mode);
>  	if (dp_mode->drm_mode.clock)
>  		dp->max_pclk_khz = dp_mode->drm_mode.clock;
> +
>  	return ret;
>  }
>  
> @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>  	}
>  }
>  
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
> +{
> +	struct dp_display_private *dp;
> +	u32 revision, major, minor;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +	/* for the time being widebus only support on DP */
> +	if (dp_display->connector_type  == DRM_MODE_CONNECTOR_DisplayPort) {

Is there a reason for not enabling widebus for eDP? (I'm perfectly fine
with doing us doing that separately at a later point, but I'm curious)

> +		revision = dp_catalog_hw_revision(dp->catalog);
> +		major = ((revision >> 28) & 0x0ff);
> +		minor = ((revision >> 16) & 0x0fff);
> +
> +	DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);

The indentation of this line is wrong.

> +
> +		if (major >= 1 && minor >= 2)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>  {
>  	struct dp_display_private *dp;
> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>  	dp_priv->panel->connector = dp_display->connector;
>  
>  	priv->connectors[priv->num_connectors++] = dp_display->connector;
> +
> +	dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);

Not sure if I'm missing some patches, but my branch branch doesn't have
a dp_priv in this function.

Regards,
Bjorn

> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 8e80e3b..d9cb9ee 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -23,6 +23,8 @@ struct msm_dp {
>  
>  	hdmi_codec_plugged_cb plugged_cb;
>  
> +	bool wide_bus_en;
> +
>  	u32 max_pclk_khz;
>  
>  	u32 max_dp_lanes;
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 71db10c..71deb1e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
>  	dp_catalog_dump_regs(catalog);
>  }
>  
> -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
>  {
>  	u32 data, total_ver, total_hor;
>  	struct dp_catalog *catalog;
> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>  
>  	catalog->dp_active = data;
>  
> -	dp_catalog_panel_timing_cfg(catalog);
> +	dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
>  	panel->panel_on = true;
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 9023e5b..5ec341a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -57,7 +57,7 @@ struct dp_panel {
>  
>  int dp_panel_init_panel_info(struct dp_panel *dp_panel);
>  int dp_panel_deinit(struct dp_panel *dp_panel);
> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
>  void dp_panel_dump_regs(struct dp_panel *dp_panel);
>  int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>  		struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 07f6c41..667f3a8 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display);
>  void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
>  
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
>  
>  #else
>  static inline int __init msm_dp_register(void)
> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
>  {
>  }
>  
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
> +{
> +	return false;
> +}
> +
>  #endif
>  
>  void __init msm_mdp_register(void);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Dmitry Baryshkov]

On 15/02/2022 01:39, Kuogee Hsieh wrote:
> Widebus feature will transmit two pixel data per pixel clock to interface.
> This feature now is required to be enabled to easy migrant to higher
> resolution applications in future. However since some legacy chipsets
> does not support this feature, this feature is enabled base on chip's
> hardware revision.
> 
> changes in v2:
> -- remove compression related code from timing
> -- remove op_info from  struct msm_drm_private
> -- remove unnecessary wide_bus_en variables
> -- pass wide_bus_en into timing configuration by struct msm_dp
> 
> Changes in v3:
> -- split patch into 3 patches
> -- enable widebus feature base on chip hardware revision
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
>   drivers/gpu/drm/msm/dp/dp_catalog.c         | 36 +++++++++++++++++++++++++++--
>   drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
>   drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
>   drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
>   drivers/gpu/drm/msm/dp/dp_display.c         | 30 ++++++++++++++++++++++++
>   drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
>   drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
>   drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
>   drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
>   10 files changed, 90 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 0c22839..b2d23c2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   		timer_setup(&dpu_enc->vsync_event_timer,
>   				dpu_encoder_vsync_event_handler,
>   				0);
> -	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
> +	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>   		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
> +	}

Please revert the order of the patches and move this chunk into the DPU 
patch.

>   
>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>   			dpu_encoder_off_work);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 64f0b26..99d087e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog,
>   }
>   
>   /**
> + * dp_catalog_hw_revision() - retrieve DP hw revision
> + *
> + * @dp_catalog: DP catalog structure
> + *
> + * return: u32
> + *
> + * This function return the DP controller hw revision
> + *
> + */
> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
> +{
> +	u32 revision;
> +	struct dp_catalog_private *catalog = container_of(dp_catalog,
> +				struct dp_catalog_private, dp_catalog);
> +
> +	revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
> +
> +	return revision;
> +}
> +
> +/**
>    * dp_catalog_ctrl_reset() - reset DP controller
>    *
>    * @dp_catalog: DP catalog structure
> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog)
>   }
>   
>   /* panel related catalog functions */
> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en)
>   {
>   	struct dp_catalog_private *catalog = container_of(dp_catalog,
>   				struct dp_catalog_private, dp_catalog);
> +	u32 reg;
>   
>   	dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
>   				dp_catalog->total);
> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>   	dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>   				dp_catalog->width_blanking);
>   	dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, dp_catalog->dp_active);
> -	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
> +
> +	reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
> +
> +	if (wide_bus_en)
> +		reg |= BIT(4);	/* DATABUS_WIDEN */
> +	else
> +		reg &= ~BIT(4);
> +
> +	DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
> +
> +	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 7dea101..a3a0129 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32 cc, u32 tb);
>   void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 rate,
>   				u32 stream_rate_khz, bool fixed_nvid);
>   int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog, u32 pattern);
> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
>   void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
>   bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>   void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog,
>   u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
>   
>   /* DP Panel APIs */
> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en);
>   void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
>   void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
>   				struct drm_display_mode *drm_mode);
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 245e1b9..1c4cf9d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
>   	dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>   }
>   
> -static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl, bool wide_bus_en)
>   {
>   	u32 cc, tb;
>   
> @@ -167,7 +167,7 @@ static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>   		ctrl->panel->dp_mode.bpp);
>   	cc = dp_link_get_colorimetry_config(ctrl->link);
>   	dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
> -	dp_panel_timing_cfg(ctrl->panel);
> +	dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
>   }
>   
>   /*
> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>   	int ret = 0;
>   	bool mainlink_ready = false;
>   	struct dp_ctrl_private *ctrl;
> +	u32 pixel_rate_orig;
>   
>   	if (!dp_ctrl)
>   		return -EINVAL;
> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>   
>   	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>   
> +	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
> +	if (dp_ctrl->wide_bus_en)
> +		ctrl->dp_ctrl.pixel_rate >>= 1;
> +
>   	DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>   		ctrl->link->link_params.rate,
>   		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>   	 */
>   	reinit_completion(&ctrl->video_comp);
>   
> -	dp_ctrl_configure_source_params(ctrl);
> +	dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
>   
>   	dp_catalog_ctrl_config_msa(ctrl->catalog,
>   		ctrl->link->link_params.rate,
> -		ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
> +		pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
>   
>   	dp_ctrl_setup_tr_unit(ctrl);
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 2433edb..4dff44d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -17,6 +17,7 @@ struct dp_ctrl {
>   	bool orientation;
>   	atomic_t aborted;
>   	u32 pixel_rate;
> +	bool wide_bus_en;
>   };
>   
>   int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e89556ad..d45a3aa 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -117,6 +117,8 @@ struct dp_display_private {
>   	struct dp_event event_list[DP_EVENT_Q_MAX];
>   	spinlock_t event_lock;
>   
> +	bool wide_bus_en;
> +
>   	struct dp_audio *audio;
>   };
>   
> @@ -845,6 +847,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>   		return 0;
>   	}
>   
> +	dp->ctrl->wide_bus_en = dp->wide_bus_en;
> +
>   	rc = dp_ctrl_on_stream(dp->ctrl);
>   	if (!rc)
>   		dp_display->power_on = true;
> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>   		dp->connector, dp_mode);
>   	if (dp_mode->drm_mode.clock)
>   		dp->max_pclk_khz = dp_mode->drm_mode.clock;
> +
>   	return ret;
>   }
>   
> @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>   	}
>   }
>   
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
> +{
> +	struct dp_display_private *dp;
> +	u32 revision, major, minor;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +	/* for the time being widebus only support on DP */
> +	if (dp_display->connector_type  == DRM_MODE_CONNECTOR_DisplayPort) {
> +		revision = dp_catalog_hw_revision(dp->catalog);
> +		major = ((revision >> 28) & 0x0ff);
> +		minor = ((revision >> 16) & 0x0fff);
> +
> +	DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
> +
> +		if (major >= 1 && minor >= 2)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>   {
>   	struct dp_display_private *dp;
> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>   	dp_priv->panel->connector = dp_display->connector;
>   
>   	priv->connectors[priv->num_connectors++] = dp_display->connector;
> +
> +	dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 8e80e3b..d9cb9ee 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -23,6 +23,8 @@ struct msm_dp {
>   
>   	hdmi_codec_plugged_cb plugged_cb;
>   
> +	bool wide_bus_en;
> +
>   	u32 max_pclk_khz;
>   
>   	u32 max_dp_lanes;
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 71db10c..71deb1e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
>   	dp_catalog_dump_regs(catalog);
>   }
>   
> -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
>   {
>   	u32 data, total_ver, total_hor;
>   	struct dp_catalog *catalog;
> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>   
>   	catalog->dp_active = data;
>   
> -	dp_catalog_panel_timing_cfg(catalog);
> +	dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
>   	panel->panel_on = true;
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 9023e5b..5ec341a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -57,7 +57,7 @@ struct dp_panel {
>   
>   int dp_panel_init_panel_info(struct dp_panel *dp_panel);
>   int dp_panel_deinit(struct dp_panel *dp_panel);
> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
>   void dp_panel_dump_regs(struct dp_panel *dp_panel);
>   int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>   		struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 07f6c41..667f3a8 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display);
>   void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
>   
>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
>   
>   #else
>   static inline int __init msm_dp_register(void)
> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
>   {
>   }
>   
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
> +{
> +	return false;
> +}
> +
>   #endif
>   
>   void __init msm_mdp_register(void);


-- 
With best wishes
Dmitry

Re: [PATCH v4 1/2] drm/msm/dp: revise timing engine programming to support widebus feature

[Dmitry Baryshkov]

On 15/02/2022 07:12, Bjorn Andersson wrote:
> On Mon 14 Feb 16:39 CST 2022, Kuogee Hsieh wrote:
> 
>> Widebus feature will transmit two pixel data per pixel clock to interface.
>> Timing engine provides driving force for this purpose. This patch base
>> on HPG (Hardware Programming Guide) to revise timing engine register
>> setting to accommodate both widebus and non widebus application. Also
>> horizontal width parameters need to be reduced by half since two pixel
>> data are clocked out per pixel clock when widebus feature enabled.
>>
>> Widebus can be enabled individually at DP. However at DSI, widebus have
>> to be enabled along with DSC enabled to achieve pixel clock rate be
>> scaled down with same ratio as compression ratio when 10 bits per source
>> component. Therefore this patch have both widebus and compression covered
>> together so tat less efforts will be required when DSC enabled later.

Please split compression away. It's has hanging loose ends, which are 
not tied anywhere. It can be sent as a part of this patch series, but 
will be merged later, together with the DP DSC support.

In general, sending a patch that does several items at once is not the 
best idea. Usually it's better to send two separate patches.

>>
>> Changes in v2:
>> -- remove compression related code from timing
>> -- remove op_info from  struct msm_drm_private
>> -- remove unnecessary wide_bus_en variables
>> -- pass wide_bus_en into timing configuration by struct msm_dp
>>
>> Changes in v3:
>> -- split patch into 3 patches
>>
>> Changes in v4:
>> -- rework timing engine to not interfere with dsi/hdmi

Thanks a lot, this is much cleaner now!

>> -- cover both widebus and compression
>>
> 
> Even though the change relates to DP, I think it would be appropriate to
> change the $subject prefix to "drm/msm/dpu".
> 
> When booting sc8180x the bootloader leaves widebus enabled in the eDP
> controller, and the two patches takes care of this problem for me. I
> also checked the DP still works.
> 
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Thanks,
> Bjorn
> 
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 10 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  2 +
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 14 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 99 ++++++++++++++++++----
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  6 ++
>>   5 files changed, 115 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 0d315b4..0c22839 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -208,6 +208,8 @@ struct dpu_encoder_virt {
>>   
>>   	u32 idle_timeout;
>>   
>> +	bool wide_bus_en;
>> +
>>   	struct msm_dp *dp;
>>   };
>>   
>> @@ -217,6 +219,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
>>   	15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
>>   };
>>   
>> +
>> +bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc)
>> +{
>> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +
>> +	return dpu_enc->wide_bus_en;
>> +}
>> +
>>   static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
>>   {
>>   	struct dpu_hw_dither_cfg dither_cfg = { 0 };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 99a5d73..893d74d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -168,4 +168,6 @@ int dpu_encoder_get_linecount(struct drm_encoder *drm_enc);
>>    */
>>   int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc);
>>   
>> +bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc);
>> +
>>   #endif /* __DPU_ENCODER_H__ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index 185379b..2af2bb7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -110,6 +110,20 @@ static void drm_mode_to_intf_timing_params(
>>   		timing->v_back_porch += timing->v_front_porch;
>>   		timing->v_front_porch = 0;
>>   	}
>> +
>> +	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> +
>> +	/*
>> +	 * for DP, divide the horizonal parameters by 2 when
>> +	 * widebus is enabled
>> +	 */
>> +	if (phys_enc->hw_intf->cap->type == INTF_DP && timing->wide_bus_en) {
>> +		timing->width = timing->width >> 1;
>> +		timing->xres = timing->xres >> 1;
>> +		timing->h_back_porch = timing->h_back_porch >> 1;
>> +		timing->h_front_porch = timing->h_front_porch >> 1;
>> +		timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
>> +	}
>>   }
>>   
>>   static u32 get_horizontal_total(const struct intf_timing_params *timing)
>> 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 116e2b5..3b9273e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -33,6 +33,7 @@
>>   #define INTF_TP_COLOR1                  0x05C
>>   #define INTF_CONFIG2                    0x060
>>   #define INTF_DISPLAY_DATA_HCTL          0x064
>> +#define INTF_ACTIVE_DATA_HCTL           0x068
>>   #define INTF_FRAME_LINE_COUNT_EN        0x0A8
>>   #define INTF_FRAME_COUNT                0x0AC
>>   #define   INTF_LINE_COUNT               0x0B0
>> @@ -60,6 +61,14 @@
>>   
>>   #define   INTF_MUX                      0x25C
>>   
>> +#define BIT_INTF_CFG_ACTIVE_H_EN	BIT(29)
>> +#define BIT_INTF_CFG_ACTIVE_V_EN	BIT(30)
>> +
>> +#define BIT_INTF_CFG2_DATABUS_WIDEN	BIT(0)
>> +#define BIT_INTF_CFG2_DATA_HCTL_EN	BIT(4)
>> +#define BIT_INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)

No need to prefix names with BIT_. Other DPU source files do not do this.

>> +
>> +
>>   static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
>>   		const struct dpu_mdss_cfg *m,
>>   		void __iomem *addr,
>> @@ -90,15 +99,23 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   	u32 hsync_period, vsync_period;
>>   	u32 display_v_start, display_v_end;
>>   	u32 hsync_start_x, hsync_end_x;
>> +	u32 hsync_data_start_x, hsync_data_end_x;
>>   	u32 active_h_start, active_h_end;
>>   	u32 active_v_start, active_v_end;
>>   	u32 active_hctl, display_hctl, hsync_ctl;
>>   	u32 polarity_ctl, den_polarity, hsync_polarity, vsync_polarity;
>>   	u32 panel_format;
>> -	u32 intf_cfg, intf_cfg2 = 0, display_data_hctl = 0;
>> +	u32 intf_cfg, intf_cfg2 = 0;
>> +	u32 display_data_hctl = 0, active_data_hctl = 0;
>> +	u32 data_width;
>> +	bool dp_intf = false;
>>   
>>   	/* read interface_cfg */
>>   	intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
>> +
>> +	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
>> +		dp_intf = true;
>> +
>>   	hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
>>   	p->h_front_porch;
>>   	vsync_period = p->vsync_pulse_width + p->v_back_porch + p->height +
>> @@ -112,7 +129,10 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   	hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
>>   	hsync_end_x = hsync_period - p->h_front_porch - 1;
>>   
>> -	if (p->width != p->xres) {
>> +	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>> +	display_hctl = (hsync_end_x << 16) | hsync_start_x;
>> +
>> +	if (p->width != p->xres) { /* border fill added */
>>   		active_h_start = hsync_start_x;
>>   		active_h_end = active_h_start + p->xres - 1;
>>   	} else {
>> @@ -130,27 +150,78 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   
>>   	if (active_h_end) {
>>   		active_hctl = (active_h_end << 16) | active_h_start;
>> -		intf_cfg |= BIT(29);	/* ACTIVE_H_ENABLE */
>> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_H_EN;

Such changes can go to a separate patch. You don't have to squash 
everything in a single patch. Quite the opposite. If each of the patches 
is atomic, it's easier to review and accept them.

>>   	} else {
>>   		active_hctl = 0;
>>   	}
>>   
>>   	if (active_v_end)
>> -		intf_cfg |= BIT(30); /* ACTIVE_V_ENABLE */
>> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_V_EN;
>>   
>> -	hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>> -	display_hctl = (hsync_end_x << 16) | hsync_start_x;

Why did you move these assignments?

>> +	/*
>> +	 * DATA_HCTL_EN controls data timing which can be different from
>> +	 * video timing. It is recommended to enable it for all cases, except
>> +	 * if compression is enabled in 1 pixel per clock mode
>> +	 */
>> +	if (!p->compression_en || p->wide_bus_en)
>> +		intf_cfg2 |= BIT_INTF_CFG2_DATA_HCTL_EN;

So, we are enabling it uncoditionally even for older platforms, which do 
not support this bit/register. I'm not a fan of writing to registers 
which are not supported by the hardware.

>> +
>> +	if (p->wide_bus_en)
>> +		intf_cfg2 |=  BIT_INTF_CFG2_DATABUS_WIDEN;
>> +
>> +	/*
>> +	 * If widebus is disabled:
>> +	 * For uncompressed stream, the data is valid for the entire active
>> +	 * window period.
>> +	 * For compressed stream, data is valid for a shorter time period
>> +	 * inside the active window depending on the compression ratio.
>> +	 *
>> +	 * If widebus is enabled:
>> +	 * For uncompressed stream, data is valid for only half the active
>> +	 * window, since the data rate is doubled in this mode.
>> +	 * p->width holds the adjusted width for DP but unadjusted width for DSI
>> +	 * For compressed stream, data validity window needs to be adjusted for
>> +	 * compression ratio and then further halved.
>> +	 */
>> +	data_width = p->width;
>> +
>> +	if (p->compression_en) {
>> +		data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);

dce_bytes_per_line is never set.

>> +
>> +		if (p->wide_bus_en)
>> +			data_width >>= 1;
>> +	} else if (!dp_intf && p->wide_bus_en) {
>> +		data_width = p->width >> 1;
>> +	}
>> +
>> +	hsync_data_start_x = hsync_start_x;
>> +	hsync_data_end_x =  hsync_start_x + data_width - 1;
>> +
>> +	display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
>> +
>> +	if (dp_intf) {
>> +		/* DP timing adjustment */
>> +		display_v_start += p->hsync_pulse_width + p->h_back_porch;
>> +		display_v_end   -= p->h_front_porch;

So, this changes the display_v_end. Is there a mistake currently (and so 
this change should be backported to stable kernels) or is it just unoptimal?

>>   
>> -	if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP) {
>>   		active_h_start = hsync_start_x;
>>   		active_h_end = active_h_start + p->xres - 1;
>>   		active_v_start = display_v_start;
>>   		active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>>   
>> -		display_v_start += p->hsync_pulse_width + p->h_back_porch;

Another unnnecessary move.

>> -
>>   		active_hctl = (active_h_end << 16) | active_h_start;
>>   		display_hctl = active_hctl;
>> +
>> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_H_EN;
>> +		intf_cfg |= BIT_INTF_CFG_ACTIVE_V_EN;
>> +
>> +		if (p->compression_en) {
>> +			active_data_hctl = (hsync_start_x +
>> +					p->extra_dto_cycles) << 16;

extra_dto_cycles is always unset (0). Is this expected?

>> +			active_data_hctl += hsync_start_x;
>> +
>> +			display_data_hctl = active_data_hctl;
>> +		}
>>   	}
>>   
>>   	den_polarity = 0;
>> @@ -180,13 +251,6 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   				(COLOR_8BIT << 4) |
>>   				(0x21 << 8));
>>   
>> -	if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {

If you insist on always programming these registers, the 
DPU_DATA_HCTL_EN becomes useless and should be removed from the 
dpu_hw_catalog. As usual, in a separate patch.

>> -		intf_cfg2 |= BIT(4);
>> -		display_data_hctl = display_hctl;
>> -		DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
>> -		DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
>> -	}
>> -
>>   	DPU_REG_WRITE(c, INTF_HSYNC_CTL, hsync_ctl);
>>   	DPU_REG_WRITE(c, INTF_VSYNC_PERIOD_F0, vsync_period * hsync_period);
>>   	DPU_REG_WRITE(c, INTF_VSYNC_PULSE_WIDTH_F0,
>> @@ -204,6 +268,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   	DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
>>   	DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
>>   	DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
>> +	DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
>> +	DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
>> +	DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
>>   }
>>   
>>   static void dpu_hw_intf_enable_timing_engine(
>> 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 3568be8..299c9c1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -30,6 +30,12 @@ struct intf_timing_params {
>>   	u32 border_clr;
>>   	u32 underflow_clr;
>>   	u32 hsync_skew;
>> +
>> +	bool wide_bus_en;
>> +	bool compression_en;
>> +	u32 extra_dto_cycles;   /* for DP only */

Never set, so it's equal to 0

>> +	bool dsc_4hs_merge;     /* DSC 4HS merge */

Unused

>> +	u32 dce_bytes_per_line;

Never set, so it's equal to 0

>>   };
>>   
>>   struct intf_prog_fetch {
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>


-- 
With best wishes
Dmitry

Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Kuogee Hsieh]

On 2/14/2022 8:22 PM, Bjorn Andersson wrote:
> On Mon 14 Feb 16:39 CST 2022, Kuogee Hsieh wrote:
>
>> Widebus feature will transmit two pixel data per pixel clock to interface.
>> This feature now is required to be enabled to easy migrant to higher
>> resolution applications in future. However since some legacy chipsets
>> does not support this feature, this feature is enabled base on chip's
>> hardware revision.
>>
>> changes in v2:
>> -- remove compression related code from timing
>> -- remove op_info from  struct msm_drm_private
>> -- remove unnecessary wide_bus_en variables
>> -- pass wide_bus_en into timing configuration by struct msm_dp
>>
>> Changes in v3:
>> -- split patch into 3 patches
>> -- enable widebus feature base on chip hardware revision
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
>>   drivers/gpu/drm/msm/dp/dp_catalog.c         | 36 +++++++++++++++++++++++++++--
>>   drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
>>   drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
>>   drivers/gpu/drm/msm/dp/dp_display.c         | 30 ++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
>>   drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
>>   drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
>>   drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
>>   10 files changed, 90 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 0c22839..b2d23c2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>>   		timer_setup(&dpu_enc->vsync_event_timer,
>>   				dpu_encoder_vsync_event_handler,
>>   				0);
>> -	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>> +	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>>   		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>> +		dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
>> +	}
>>   
>>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>   			dpu_encoder_off_work);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 64f0b26..99d087e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog,
>>   }
>>   
>>   /**
>> + * dp_catalog_hw_revision() - retrieve DP hw revision
>> + *
>> + * @dp_catalog: DP catalog structure
>> + *
>> + * return: u32
> Q: What's 2+2?
> A: Integer
>
> This should say:
>
> Return: the controller hardware revision
>
>> + *
>> + * This function return the DP controller hw revision
> That's what "Return:" in the kernel-doc is supposed to clarify...
>
> https://docs.kernel.org/doc-guide/kernel-doc.html is good to read.
>
>> + *
>> + */
>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
>> +{
>> +	u32 revision;
>> +	struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +				struct dp_catalog_private, dp_catalog);
>> +
>> +	revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
> There's no need for a local variable here, just:
>
> 	return dp_read_ahb();
>
>> +
>> +	return revision;
>> +}
>> +
>> +/**
>>    * dp_catalog_ctrl_reset() - reset DP controller
>>    *
>>    * @dp_catalog: DP catalog structure
>> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog)
>>   }
>>   
>>   /* panel related catalog functions */
>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en)
>>   {
>>   	struct dp_catalog_private *catalog = container_of(dp_catalog,
>>   				struct dp_catalog_private, dp_catalog);
>> +	u32 reg;
>>   
>>   	dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
>>   				dp_catalog->total);
>> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>>   	dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>>   				dp_catalog->width_blanking);
>>   	dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, dp_catalog->dp_active);
>> -	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
>> +
>> +	reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
>> +
>> +	if (wide_bus_en)
>> +		reg |= BIT(4);	/* DATABUS_WIDEN */
> #define DATABUS_WIDEN BIT(4)
>
> Would save you the need for writing that comment.
>
>> +	else
>> +		reg &= ~BIT(4);
>> +
>> +	DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
>> +
>> +	dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 7dea101..a3a0129 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32 cc, u32 tb);
>>   void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 rate,
>>   				u32 stream_rate_khz, bool fixed_nvid);
>>   int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog *dp_catalog, u32 pattern);
>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
>>   void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
>>   bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>>   void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
>> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog,
>>   u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
>>   
>>   /* DP Panel APIs */
>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool wide_bus_en);
>>   void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
>>   void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
>>   				struct drm_display_mode *drm_mode);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 245e1b9..1c4cf9d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
>>   	dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>>   }
>>   
>> -static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl, bool wide_bus_en)
>>   {
>>   	u32 cc, tb;
>>   
>> @@ -167,7 +167,7 @@ static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>>   		ctrl->panel->dp_mode.bpp);
>>   	cc = dp_link_get_colorimetry_config(ctrl->link);
>>   	dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
>> -	dp_panel_timing_cfg(ctrl->panel);
>> +	dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
>>   }
>>   
>>   /*
>> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>   	int ret = 0;
>>   	bool mainlink_ready = false;
>>   	struct dp_ctrl_private *ctrl;
>> +	u32 pixel_rate_orig;
>>   
>>   	if (!dp_ctrl)
>>   		return -EINVAL;
>> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>   
>>   	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>   
>> +	pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
>> +	if (dp_ctrl->wide_bus_en)
>> +		ctrl->dp_ctrl.pixel_rate >>= 1;
>> +
>>   	DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>>   		ctrl->link->link_params.rate,
>>   		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
>> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>   	 */
>>   	reinit_completion(&ctrl->video_comp);
>>   
>> -	dp_ctrl_configure_source_params(ctrl);
>> +	dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
>>   
>>   	dp_catalog_ctrl_config_msa(ctrl->catalog,
>>   		ctrl->link->link_params.rate,
>> -		ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
>> +		pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
>>   
>>   	dp_ctrl_setup_tr_unit(ctrl);
>>   
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index 2433edb..4dff44d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -17,6 +17,7 @@ struct dp_ctrl {
>>   	bool orientation;
>>   	atomic_t aborted;
>>   	u32 pixel_rate;
>> +	bool wide_bus_en;
>>   };
>>   
>>   int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index e89556ad..d45a3aa 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -117,6 +117,8 @@ struct dp_display_private {
>>   	struct dp_event event_list[DP_EVENT_Q_MAX];
>>   	spinlock_t event_lock;
>>   
>> +	bool wide_bus_en;
>> +
>>   	struct dp_audio *audio;
>>   };
>>   
>> @@ -845,6 +847,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>>   		return 0;
>>   	}
>>   
>> +	dp->ctrl->wide_bus_en = dp->wide_bus_en;
>> +
>>   	rc = dp_ctrl_on_stream(dp->ctrl);
>>   	if (!rc)
>>   		dp_display->power_on = true;
>> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>>   		dp->connector, dp_mode);
>>   	if (dp_mode->drm_mode.clock)
>>   		dp->max_pclk_khz = dp_mode->drm_mode.clock;
>> +
>>   	return ret;
>>   }
>>   
>> @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>>   	}
>>   }
>>   
>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>> +{
>> +	struct dp_display_private *dp;
>> +	u32 revision, major, minor;
>> +
>> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +
>> +	/* for the time being widebus only support on DP */
>> +	if (dp_display->connector_type  == DRM_MODE_CONNECTOR_DisplayPort) {
> Is there a reason for not enabling widebus for eDP? (I'm perfectly fine
> with doing us doing that separately at a later point, but I'm curious)

yes, widebus can be enabled for eDP too.

Just ask hardware engineer thsi question, they said widebus is 
recommended to be enabled at both eDP and DP.

I will enable it later.


>
>> +		revision = dp_catalog_hw_revision(dp->catalog);
>> +		major = ((revision >> 28) & 0x0ff);
>> +		minor = ((revision >> 16) & 0x0fff);
>> +
>> +	DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
> The indentation of this line is wrong.
>
>> +
>> +		if (major >= 1 && minor >= 2)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>>   {
>>   	struct dp_display_private *dp;
>> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>   	dp_priv->panel->connector = dp_display->connector;
>>   
>>   	priv->connectors[priv->num_connectors++] = dp_display->connector;
>> +
>> +	dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);
> Not sure if I'm missing some patches, but my branch branch doesn't have
> a dp_priv in this function.

My fault, I uploaded patches for review at my working directory instead 
of msm-next branch.


>
> Regards,
> Bjorn
>
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 8e80e3b..d9cb9ee 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -23,6 +23,8 @@ struct msm_dp {
>>   
>>   	hdmi_codec_plugged_cb plugged_cb;
>>   
>> +	bool wide_bus_en;
>> +
>>   	u32 max_pclk_khz;
>>   
>>   	u32 max_dp_lanes;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 71db10c..71deb1e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>   	dp_catalog_dump_regs(catalog);
>>   }
>>   
>> -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
>>   {
>>   	u32 data, total_ver, total_hor;
>>   	struct dp_catalog *catalog;
>> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>   
>>   	catalog->dp_active = data;
>>   
>> -	dp_catalog_panel_timing_cfg(catalog);
>> +	dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
>>   	panel->panel_on = true;
>>   
>>   	return 0;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 9023e5b..5ec341a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -57,7 +57,7 @@ struct dp_panel {
>>   
>>   int dp_panel_init_panel_info(struct dp_panel *dp_panel);
>>   int dp_panel_deinit(struct dp_panel *dp_panel);
>> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
>>   void dp_panel_dump_regs(struct dp_panel *dp_panel);
>>   int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>>   		struct drm_connector *connector);
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 07f6c41..667f3a8 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display);
>>   void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
>>   
>>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
>>   
>>   #else
>>   static inline int __init msm_dp_register(void)
>> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
>>   {
>>   }
>>   
>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>> +{
>> +	return false;
>> +}
>> +
>>   #endif
>>   
>>   void __init msm_mdp_register(void);
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Kuogee Hsieh]

On 2/15/2022 5:34 AM, Dmitry Baryshkov wrote:
> On 15/02/2022 01:39, Kuogee Hsieh wrote:
>> Widebus feature will transmit two pixel data per pixel clock to 
>> interface.
>> This feature now is required to be enabled to easy migrant to higher
>> resolution applications in future. However since some legacy chipsets
>> does not support this feature, this feature is enabled base on chip's
>> hardware revision.
>>
>> changes in v2:
>> -- remove compression related code from timing
>> -- remove op_info from  struct msm_drm_private
>> -- remove unnecessary wide_bus_en variables
>> -- pass wide_bus_en into timing configuration by struct msm_dp
>>
>> Changes in v3:
>> -- split patch into 3 patches
>> -- enable widebus feature base on chip hardware revision
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
>>   drivers/gpu/drm/msm/dp/dp_catalog.c         | 36 
>> +++++++++++++++++++++++++++--
>>   drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
>>   drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
>>   drivers/gpu/drm/msm/dp/dp_display.c         | 30 
>> ++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
>>   drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
>>   drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
>>   drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
>>   10 files changed, 90 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 0c22839..b2d23c2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev, 
>> struct drm_encoder *enc,
>>           timer_setup(&dpu_enc->vsync_event_timer,
>>                   dpu_encoder_vsync_event_handler,
>>                   0);
>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>> +    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>> +        dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
>> +    }
>
> Please revert the order of the patches and move this chunk into the 
> DPU patch.

There is chicken  and egg issue here.

I think we should create an communication channel to pass dp/dsi driver 
info to drm, such as wide_bus_en and dsc compression, etc.

By function calls like this will create more unnecessary tangles between 
driver and drm layer.

>
>> INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>               dpu_encoder_off_work);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 64f0b26..99d087e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct 
>> dp_catalog *dp_catalog,
>>   }
>>     /**
>> + * dp_catalog_hw_revision() - retrieve DP hw revision
>> + *
>> + * @dp_catalog: DP catalog structure
>> + *
>> + * return: u32
>> + *
>> + * This function return the DP controller hw revision
>> + *
>> + */
>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
>> +{
>> +    u32 revision;
>> +    struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +                struct dp_catalog_private, dp_catalog);
>> +
>> +    revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
>> +
>> +    return revision;
>> +}
>> +
>> +/**
>>    * dp_catalog_ctrl_reset() - reset DP controller
>>    *
>>    * @dp_catalog: DP catalog structure
>> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct 
>> dp_catalog *dp_catalog)
>>   }
>>     /* panel related catalog functions */
>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool 
>> wide_bus_en)
>>   {
>>       struct dp_catalog_private *catalog = container_of(dp_catalog,
>>                   struct dp_catalog_private, dp_catalog);
>> +    u32 reg;
>>         dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
>>                   dp_catalog->total);
>> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct 
>> dp_catalog *dp_catalog)
>>       dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>>                   dp_catalog->width_blanking);
>>       dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER, 
>> dp_catalog->dp_active);
>> -    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
>> +
>> +    reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
>> +
>> +    if (wide_bus_en)
>> +        reg |= BIT(4);    /* DATABUS_WIDEN */
>> +    else
>> +        reg &= ~BIT(4);
>> +
>> +    DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
>> +
>> +    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
>>       return 0;
>>   }
>>   diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h 
>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 7dea101..a3a0129 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog 
>> *dp_catalog, u32 cc, u32 tb);
>>   void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 
>> rate,
>>                   u32 stream_rate_khz, bool fixed_nvid);
>>   int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog 
>> *dp_catalog, u32 pattern);
>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
>>   void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
>>   bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>>   void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool 
>> enable);
>> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct 
>> dp_catalog *dp_catalog,
>>   u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
>>     /* DP Panel APIs */
>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool 
>> wide_bus_en);
>>   void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
>>   void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
>>                   struct drm_display_mode *drm_mode);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 245e1b9..1c4cf9d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct 
>> dp_ctrl_private *ctrl)
>>       dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>>   }
>>   -static void dp_ctrl_configure_source_params(struct dp_ctrl_private 
>> *ctrl)
>> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private 
>> *ctrl, bool wide_bus_en)
>>   {
>>       u32 cc, tb;
>>   @@ -167,7 +167,7 @@ static void 
>> dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>>           ctrl->panel->dp_mode.bpp);
>>       cc = dp_link_get_colorimetry_config(ctrl->link);
>>       dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
>> -    dp_panel_timing_cfg(ctrl->panel);
>> +    dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
>>   }
>>     /*
>> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>       int ret = 0;
>>       bool mainlink_ready = false;
>>       struct dp_ctrl_private *ctrl;
>> +    u32 pixel_rate_orig;
>>         if (!dp_ctrl)
>>           return -EINVAL;
>> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>         ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>   +    pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
>> +    if (dp_ctrl->wide_bus_en)
>> +        ctrl->dp_ctrl.pixel_rate >>= 1;
>> +
>>       DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>>           ctrl->link->link_params.rate,
>>           ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
>> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>        */
>>       reinit_completion(&ctrl->video_comp);
>>   -    dp_ctrl_configure_source_params(ctrl);
>> +    dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
>>         dp_catalog_ctrl_config_msa(ctrl->catalog,
>>           ctrl->link->link_params.rate,
>> -        ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
>> +        pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
>>         dp_ctrl_setup_tr_unit(ctrl);
>>   diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index 2433edb..4dff44d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -17,6 +17,7 @@ struct dp_ctrl {
>>       bool orientation;
>>       atomic_t aborted;
>>       u32 pixel_rate;
>> +    bool wide_bus_en;
>>   };
>>     int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index e89556ad..d45a3aa 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -117,6 +117,8 @@ struct dp_display_private {
>>       struct dp_event event_list[DP_EVENT_Q_MAX];
>>       spinlock_t event_lock;
>>   +    bool wide_bus_en;
>> +
>>       struct dp_audio *audio;
>>   };
>>   @@ -845,6 +847,8 @@ static int dp_display_enable(struct 
>> dp_display_private *dp, u32 data)
>>           return 0;
>>       }
>>   +    dp->ctrl->wide_bus_en = dp->wide_bus_en;
>> +
>>       rc = dp_ctrl_on_stream(dp->ctrl);
>>       if (!rc)
>>           dp_display->power_on = true;
>> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>>           dp->connector, dp_mode);
>>       if (dp_mode->drm_mode.clock)
>>           dp->max_pclk_khz = dp_mode->drm_mode.clock;
>> +
>>       return ret;
>>   }
>>   @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp 
>> *dp_display)
>>       }
>>   }
>>   +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>> +{
>> +    struct dp_display_private *dp;
>> +    u32 revision, major, minor;
>> +
>> +    dp = container_of(dp_display, struct dp_display_private, 
>> dp_display);
>> +
>> +    /* for the time being widebus only support on DP */
>> +    if (dp_display->connector_type  == 
>> DRM_MODE_CONNECTOR_DisplayPort) {
>> +        revision = dp_catalog_hw_revision(dp->catalog);
>> +        major = ((revision >> 28) & 0x0ff);
>> +        minor = ((revision >> 16) & 0x0fff);
>> +
>> +    DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
>> +
>> +        if (major >= 1 && minor >= 2)
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct 
>> drm_minor *minor)
>>   {
>>       struct dp_display_private *dp;
>> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp 
>> *dp_display, struct drm_device *dev,
>>       dp_priv->panel->connector = dp_display->connector;
>>         priv->connectors[priv->num_connectors++] = 
>> dp_display->connector;
>> +
>> +    dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);
>> +
>>       return 0;
>>   }
>>   diff --git a/drivers/gpu/drm/msm/dp/dp_display.h 
>> b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 8e80e3b..d9cb9ee 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -23,6 +23,8 @@ struct msm_dp {
>>         hdmi_codec_plugged_cb plugged_cb;
>>   +    bool wide_bus_en;
>> +
>>       u32 max_pclk_khz;
>>         u32 max_dp_lanes;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 71db10c..71deb1e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>       dp_catalog_dump_regs(catalog);
>>   }
>>   -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
>>   {
>>       u32 data, total_ver, total_hor;
>>       struct dp_catalog *catalog;
>> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>         catalog->dp_active = data;
>>   -    dp_catalog_panel_timing_cfg(catalog);
>> +    dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
>>       panel->panel_on = true;
>>         return 0;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h 
>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 9023e5b..5ec341a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -57,7 +57,7 @@ struct dp_panel {
>>     int dp_panel_init_panel_info(struct dp_panel *dp_panel);
>>   int dp_panel_deinit(struct dp_panel *dp_panel);
>> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
>>   void dp_panel_dump_regs(struct dp_panel *dp_panel);
>>   int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>>           struct drm_connector *connector);
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h 
>> b/drivers/gpu/drm/msm/msm_drv.h
>> index 07f6c41..667f3a8 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp 
>> *dp_display);
>>   void msm_dp_snapshot(struct msm_disp_state *disp_state, struct 
>> msm_dp *dp_display);
>>     void msm_dp_debugfs_init(struct msm_dp *dp_display, struct 
>> drm_minor *minor);
>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
>>     #else
>>   static inline int __init msm_dp_register(void)
>> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct 
>> msm_dp *dp_display,
>>   {
>>   }
>>   +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>> +{
>> +    return false;
>> +}
>> +
>>   #endif
>>     void __init msm_mdp_register(void);
>
>

Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Dmitry Baryshkov]

On Tue, 15 Feb 2022 at 20:49, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 2/15/2022 5:34 AM, Dmitry Baryshkov wrote:
> > On 15/02/2022 01:39, Kuogee Hsieh wrote:
> >> Widebus feature will transmit two pixel data per pixel clock to
> >> interface.
> >> This feature now is required to be enabled to easy migrant to higher
> >> resolution applications in future. However since some legacy chipsets
> >> does not support this feature, this feature is enabled base on chip's
> >> hardware revision.
> >>
> >> changes in v2:
> >> -- remove compression related code from timing
> >> -- remove op_info from  struct msm_drm_private
> >> -- remove unnecessary wide_bus_en variables
> >> -- pass wide_bus_en into timing configuration by struct msm_dp
> >>
> >> Changes in v3:
> >> -- split patch into 3 patches
> >> -- enable widebus feature base on chip hardware revision
> >>
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
> >>   drivers/gpu/drm/msm/dp/dp_catalog.c         | 36
> >> +++++++++++++++++++++++++++--
> >>   drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
> >>   drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
> >>   drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
> >>   drivers/gpu/drm/msm/dp/dp_display.c         | 30
> >> ++++++++++++++++++++++++
> >>   drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
> >>   drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
> >>   drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
> >>   drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
> >>   10 files changed, 90 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index 0c22839..b2d23c2 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev,
> >> struct drm_encoder *enc,
> >>           timer_setup(&dpu_enc->vsync_event_timer,
> >>                   dpu_encoder_vsync_event_handler,
> >>                   0);
> >> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
> >> +    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
> >>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
> >> +        dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
> >> +    }
> >
> > Please revert the order of the patches and move this chunk into the
> > DPU patch.
>
> There is chicken  and egg issue here.
>
> I think we should create an communication channel to pass dp/dsi driver
> info to drm, such as wide_bus_en and dsc compression, etc.
>
> By function calls like this will create more unnecessary tangles between
> driver and drm layer.

I'm fine with the function call. I'm suggesting another thing. If you
change the order of these two patches (#1 - dp, #2 - dpu), the
msm_dp_wide_bus_enable() will get added in patch#1 and will be
available to be called in the patch#2.

>
> >
> >> INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
> >>               dpu_encoder_off_work);
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index 64f0b26..99d087e 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct
> >> dp_catalog *dp_catalog,
> >>   }
> >>     /**
> >> + * dp_catalog_hw_revision() - retrieve DP hw revision
> >> + *
> >> + * @dp_catalog: DP catalog structure
> >> + *
> >> + * return: u32
> >> + *
> >> + * This function return the DP controller hw revision
> >> + *
> >> + */
> >> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
> >> +{
> >> +    u32 revision;
> >> +    struct dp_catalog_private *catalog = container_of(dp_catalog,
> >> +                struct dp_catalog_private, dp_catalog);
> >> +
> >> +    revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
> >> +
> >> +    return revision;
> >> +}
> >> +
> >> +/**
> >>    * dp_catalog_ctrl_reset() - reset DP controller
> >>    *
> >>    * @dp_catalog: DP catalog structure
> >> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct
> >> dp_catalog *dp_catalog)
> >>   }
> >>     /* panel related catalog functions */
> >> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
> >> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
> >> wide_bus_en)
> >>   {
> >>       struct dp_catalog_private *catalog = container_of(dp_catalog,
> >>                   struct dp_catalog_private, dp_catalog);
> >> +    u32 reg;
> >>         dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
> >>                   dp_catalog->total);
> >> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct
> >> dp_catalog *dp_catalog)
> >>       dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
> >>                   dp_catalog->width_blanking);
> >>       dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER,
> >> dp_catalog->dp_active);
> >> -    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
> >> +
> >> +    reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
> >> +
> >> +    if (wide_bus_en)
> >> +        reg |= BIT(4);    /* DATABUS_WIDEN */
> >> +    else
> >> +        reg &= ~BIT(4);
> >> +
> >> +    DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
> >> +
> >> +    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
> >>       return 0;
> >>   }
> >>   diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> index 7dea101..a3a0129 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog
> >> *dp_catalog, u32 cc, u32 tb);
> >>   void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32
> >> rate,
> >>                   u32 stream_rate_khz, bool fixed_nvid);
> >>   int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog
> >> *dp_catalog, u32 pattern);
> >> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
> >>   void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
> >>   bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
> >>   void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool
> >> enable);
> >> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct
> >> dp_catalog *dp_catalog,
> >>   u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
> >>     /* DP Panel APIs */
> >> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
> >> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
> >> wide_bus_en);
> >>   void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
> >>   void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
> >>                   struct drm_display_mode *drm_mode);
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> index 245e1b9..1c4cf9d 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct
> >> dp_ctrl_private *ctrl)
> >>       dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
> >>   }
> >>   -static void dp_ctrl_configure_source_params(struct dp_ctrl_private
> >> *ctrl)
> >> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private
> >> *ctrl, bool wide_bus_en)
> >>   {
> >>       u32 cc, tb;
> >>   @@ -167,7 +167,7 @@ static void
> >> dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
> >>           ctrl->panel->dp_mode.bpp);
> >>       cc = dp_link_get_colorimetry_config(ctrl->link);
> >>       dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
> >> -    dp_panel_timing_cfg(ctrl->panel);
> >> +    dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
> >>   }
> >>     /*
> >> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> >>       int ret = 0;
> >>       bool mainlink_ready = false;
> >>       struct dp_ctrl_private *ctrl;
> >> +    u32 pixel_rate_orig;
> >>         if (!dp_ctrl)
> >>           return -EINVAL;
> >> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> >>         ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
> >>   +    pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
> >> +    if (dp_ctrl->wide_bus_en)
> >> +        ctrl->dp_ctrl.pixel_rate >>= 1;
> >> +
> >>       DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
> >>           ctrl->link->link_params.rate,
> >>           ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> >> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> >>        */
> >>       reinit_completion(&ctrl->video_comp);
> >>   -    dp_ctrl_configure_source_params(ctrl);
> >> +    dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
> >>         dp_catalog_ctrl_config_msa(ctrl->catalog,
> >>           ctrl->link->link_params.rate,
> >> -        ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
> >> +        pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
> >>         dp_ctrl_setup_tr_unit(ctrl);
> >>   diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> >> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> >> index 2433edb..4dff44d 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> >> @@ -17,6 +17,7 @@ struct dp_ctrl {
> >>       bool orientation;
> >>       atomic_t aborted;
> >>       u32 pixel_rate;
> >> +    bool wide_bus_en;
> >>   };
> >>     int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index e89556ad..d45a3aa 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -117,6 +117,8 @@ struct dp_display_private {
> >>       struct dp_event event_list[DP_EVENT_Q_MAX];
> >>       spinlock_t event_lock;
> >>   +    bool wide_bus_en;
> >> +
> >>       struct dp_audio *audio;
> >>   };
> >>   @@ -845,6 +847,8 @@ static int dp_display_enable(struct
> >> dp_display_private *dp, u32 data)
> >>           return 0;
> >>       }
> >>   +    dp->ctrl->wide_bus_en = dp->wide_bus_en;
> >> +
> >>       rc = dp_ctrl_on_stream(dp->ctrl);
> >>       if (!rc)
> >>           dp_display->power_on = true;
> >> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
> >>           dp->connector, dp_mode);
> >>       if (dp_mode->drm_mode.clock)
> >>           dp->max_pclk_khz = dp_mode->drm_mode.clock;
> >> +
> >>       return ret;
> >>   }
> >>   @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp
> >> *dp_display)
> >>       }
> >>   }
> >>   +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
> >> +{
> >> +    struct dp_display_private *dp;
> >> +    u32 revision, major, minor;
> >> +
> >> +    dp = container_of(dp_display, struct dp_display_private,
> >> dp_display);
> >> +
> >> +    /* for the time being widebus only support on DP */
> >> +    if (dp_display->connector_type  ==
> >> DRM_MODE_CONNECTOR_DisplayPort) {
> >> +        revision = dp_catalog_hw_revision(dp->catalog);
> >> +        major = ((revision >> 28) & 0x0ff);
> >> +        minor = ((revision >> 16) & 0x0fff);
> >> +
> >> +    DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
> >> +
> >> +        if (major >= 1 && minor >= 2)
> >> +            return true;
> >> +    }
> >> +
> >> +    return false;
> >> +}
> >> +
> >>   void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
> >> drm_minor *minor)
> >>   {
> >>       struct dp_display_private *dp;
> >> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp
> >> *dp_display, struct drm_device *dev,
> >>       dp_priv->panel->connector = dp_display->connector;
> >>         priv->connectors[priv->num_connectors++] =
> >> dp_display->connector;
> >> +
> >> +    dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);
> >> +
> >>       return 0;
> >>   }
> >>   diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> >> b/drivers/gpu/drm/msm/dp/dp_display.h
> >> index 8e80e3b..d9cb9ee 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> >> @@ -23,6 +23,8 @@ struct msm_dp {
> >>         hdmi_codec_plugged_cb plugged_cb;
> >>   +    bool wide_bus_en;
> >> +
> >>       u32 max_pclk_khz;
> >>         u32 max_dp_lanes;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> >> b/drivers/gpu/drm/msm/dp/dp_panel.c
> >> index 71db10c..71deb1e 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> >> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
> >>       dp_catalog_dump_regs(catalog);
> >>   }
> >>   -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
> >> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
> >>   {
> >>       u32 data, total_ver, total_hor;
> >>       struct dp_catalog *catalog;
> >> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
> >>         catalog->dp_active = data;
> >>   -    dp_catalog_panel_timing_cfg(catalog);
> >> +    dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
> >>       panel->panel_on = true;
> >>         return 0;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
> >> b/drivers/gpu/drm/msm/dp/dp_panel.h
> >> index 9023e5b..5ec341a 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> >> @@ -57,7 +57,7 @@ struct dp_panel {
> >>     int dp_panel_init_panel_info(struct dp_panel *dp_panel);
> >>   int dp_panel_deinit(struct dp_panel *dp_panel);
> >> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
> >> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
> >>   void dp_panel_dump_regs(struct dp_panel *dp_panel);
> >>   int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> >>           struct drm_connector *connector);
> >> diff --git a/drivers/gpu/drm/msm/msm_drv.h
> >> b/drivers/gpu/drm/msm/msm_drv.h
> >> index 07f6c41..667f3a8 100644
> >> --- a/drivers/gpu/drm/msm/msm_drv.h
> >> +++ b/drivers/gpu/drm/msm/msm_drv.h
> >> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp
> >> *dp_display);
> >>   void msm_dp_snapshot(struct msm_disp_state *disp_state, struct
> >> msm_dp *dp_display);
> >>     void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
> >> drm_minor *minor);
> >> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
> >>     #else
> >>   static inline int __init msm_dp_register(void)
> >> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct
> >> msm_dp *dp_display,
> >>   {
> >>   }
> >>   +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
> >> +{
> >> +    return false;
> >> +}
> >> +
> >>   #endif
> >>     void __init msm_mdp_register(void);
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Kuogee Hsieh]

On 2/15/2022 10:30 AM, Dmitry Baryshkov wrote:
> On Tue, 15 Feb 2022 at 20:49, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 2/15/2022 5:34 AM, Dmitry Baryshkov wrote:
>>> On 15/02/2022 01:39, Kuogee Hsieh wrote:
>>>> Widebus feature will transmit two pixel data per pixel clock to
>>>> interface.
>>>> This feature now is required to be enabled to easy migrant to higher
>>>> resolution applications in future. However since some legacy chipsets
>>>> does not support this feature, this feature is enabled base on chip's
>>>> hardware revision.
>>>>
>>>> changes in v2:
>>>> -- remove compression related code from timing
>>>> -- remove op_info from  struct msm_drm_private
>>>> -- remove unnecessary wide_bus_en variables
>>>> -- pass wide_bus_en into timing configuration by struct msm_dp
>>>>
>>>> Changes in v3:
>>>> -- split patch into 3 patches
>>>> -- enable widebus feature base on chip hardware revision
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
>>>>    drivers/gpu/drm/msm/dp/dp_catalog.c         | 36
>>>> +++++++++++++++++++++++++++--
>>>>    drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
>>>>    drivers/gpu/drm/msm/dp/dp_display.c         | 30
>>>> ++++++++++++++++++++++++
>>>>    drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
>>>>    drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
>>>>    drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
>>>>    drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
>>>>    10 files changed, 90 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 0c22839..b2d23c2 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev,
>>>> struct drm_encoder *enc,
>>>>            timer_setup(&dpu_enc->vsync_event_timer,
>>>>                    dpu_encoder_vsync_event_handler,
>>>>                    0);
>>>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>>> +    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>>>>            dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>> +        dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
>>>> +    }
>>> Please revert the order of the patches and move this chunk into the
>>> DPU patch.
>> There is chicken  and egg issue here.
>>
>> I think we should create an communication channel to pass dp/dsi driver
>> info to drm, such as wide_bus_en and dsc compression, etc.
>>
>> By function calls like this will create more unnecessary tangles between
>> driver and drm layer.
> I'm fine with the function call. I'm suggesting another thing. If you
> change the order of these two patches (#1 - dp, #2 - dpu), the
> msm_dp_wide_bus_enable() will get added in patch#1 and will be
> available to be called in the patch#2.

By doing that, we have wile widebus enabled at (#1 -dp) patch but has no 
timing engine changes at (#2 -dpu) to support it. This means (#1 -dp) 
will break dp until (#2 -dpu) merged.

Is this ok?


>>>> INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>>>                dpu_encoder_off_work);
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> index 64f0b26..99d087e 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct
>>>> dp_catalog *dp_catalog,
>>>>    }
>>>>      /**
>>>> + * dp_catalog_hw_revision() - retrieve DP hw revision
>>>> + *
>>>> + * @dp_catalog: DP catalog structure
>>>> + *
>>>> + * return: u32
>>>> + *
>>>> + * This function return the DP controller hw revision
>>>> + *
>>>> + */
>>>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
>>>> +{
>>>> +    u32 revision;
>>>> +    struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>> +                struct dp_catalog_private, dp_catalog);
>>>> +
>>>> +    revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
>>>> +
>>>> +    return revision;
>>>> +}
>>>> +
>>>> +/**
>>>>     * dp_catalog_ctrl_reset() - reset DP controller
>>>>     *
>>>>     * @dp_catalog: DP catalog structure
>>>> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct
>>>> dp_catalog *dp_catalog)
>>>>    }
>>>>      /* panel related catalog functions */
>>>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>>>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
>>>> wide_bus_en)
>>>>    {
>>>>        struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>>                    struct dp_catalog_private, dp_catalog);
>>>> +    u32 reg;
>>>>          dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
>>>>                    dp_catalog->total);
>>>> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct
>>>> dp_catalog *dp_catalog)
>>>>        dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>>>>                    dp_catalog->width_blanking);
>>>>        dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER,
>>>> dp_catalog->dp_active);
>>>> -    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
>>>> +
>>>> +    reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
>>>> +
>>>> +    if (wide_bus_en)
>>>> +        reg |= BIT(4);    /* DATABUS_WIDEN */
>>>> +    else
>>>> +        reg &= ~BIT(4);
>>>> +
>>>> +    DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
>>>> +
>>>> +    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
>>>>        return 0;
>>>>    }
>>>>    diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> index 7dea101..a3a0129 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog
>>>> *dp_catalog, u32 cc, u32 tb);
>>>>    void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32
>>>> rate,
>>>>                    u32 stream_rate_khz, bool fixed_nvid);
>>>>    int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog
>>>> *dp_catalog, u32 pattern);
>>>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
>>>>    void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
>>>>    bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>>>>    void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool
>>>> enable);
>>>> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct
>>>> dp_catalog *dp_catalog,
>>>>    u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
>>>>      /* DP Panel APIs */
>>>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
>>>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
>>>> wide_bus_en);
>>>>    void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
>>>>    void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
>>>>                    struct drm_display_mode *drm_mode);
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> index 245e1b9..1c4cf9d 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct
>>>> dp_ctrl_private *ctrl)
>>>>        dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>>>>    }
>>>>    -static void dp_ctrl_configure_source_params(struct dp_ctrl_private
>>>> *ctrl)
>>>> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private
>>>> *ctrl, bool wide_bus_en)
>>>>    {
>>>>        u32 cc, tb;
>>>>    @@ -167,7 +167,7 @@ static void
>>>> dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>>>>            ctrl->panel->dp_mode.bpp);
>>>>        cc = dp_link_get_colorimetry_config(ctrl->link);
>>>>        dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
>>>> -    dp_panel_timing_cfg(ctrl->panel);
>>>> +    dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
>>>>    }
>>>>      /*
>>>> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>>        int ret = 0;
>>>>        bool mainlink_ready = false;
>>>>        struct dp_ctrl_private *ctrl;
>>>> +    u32 pixel_rate_orig;
>>>>          if (!dp_ctrl)
>>>>            return -EINVAL;
>>>> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>>          ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>>>    +    pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
>>>> +    if (dp_ctrl->wide_bus_en)
>>>> +        ctrl->dp_ctrl.pixel_rate >>= 1;
>>>> +
>>>>        DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>>>>            ctrl->link->link_params.rate,
>>>>            ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
>>>> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>>         */
>>>>        reinit_completion(&ctrl->video_comp);
>>>>    -    dp_ctrl_configure_source_params(ctrl);
>>>> +    dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
>>>>          dp_catalog_ctrl_config_msa(ctrl->catalog,
>>>>            ctrl->link->link_params.rate,
>>>> -        ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
>>>> +        pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
>>>>          dp_ctrl_setup_tr_unit(ctrl);
>>>>    diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> index 2433edb..4dff44d 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> @@ -17,6 +17,7 @@ struct dp_ctrl {
>>>>        bool orientation;
>>>>        atomic_t aborted;
>>>>        u32 pixel_rate;
>>>> +    bool wide_bus_en;
>>>>    };
>>>>      int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index e89556ad..d45a3aa 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -117,6 +117,8 @@ struct dp_display_private {
>>>>        struct dp_event event_list[DP_EVENT_Q_MAX];
>>>>        spinlock_t event_lock;
>>>>    +    bool wide_bus_en;
>>>> +
>>>>        struct dp_audio *audio;
>>>>    };
>>>>    @@ -845,6 +847,8 @@ static int dp_display_enable(struct
>>>> dp_display_private *dp, u32 data)
>>>>            return 0;
>>>>        }
>>>>    +    dp->ctrl->wide_bus_en = dp->wide_bus_en;
>>>> +
>>>>        rc = dp_ctrl_on_stream(dp->ctrl);
>>>>        if (!rc)
>>>>            dp_display->power_on = true;
>>>> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>>>>            dp->connector, dp_mode);
>>>>        if (dp_mode->drm_mode.clock)
>>>>            dp->max_pclk_khz = dp_mode->drm_mode.clock;
>>>> +
>>>>        return ret;
>>>>    }
>>>>    @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>> *dp_display)
>>>>        }
>>>>    }
>>>>    +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>>>> +{
>>>> +    struct dp_display_private *dp;
>>>> +    u32 revision, major, minor;
>>>> +
>>>> +    dp = container_of(dp_display, struct dp_display_private,
>>>> dp_display);
>>>> +
>>>> +    /* for the time being widebus only support on DP */
>>>> +    if (dp_display->connector_type  ==
>>>> DRM_MODE_CONNECTOR_DisplayPort) {
>>>> +        revision = dp_catalog_hw_revision(dp->catalog);
>>>> +        major = ((revision >> 28) & 0x0ff);
>>>> +        minor = ((revision >> 16) & 0x0fff);
>>>> +
>>>> +    DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
>>>> +
>>>> +        if (major >= 1 && minor >= 2)
>>>> +            return true;
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>>    void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
>>>> drm_minor *minor)
>>>>    {
>>>>        struct dp_display_private *dp;
>>>> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp
>>>> *dp_display, struct drm_device *dev,
>>>>        dp_priv->panel->connector = dp_display->connector;
>>>>          priv->connectors[priv->num_connectors++] =
>>>> dp_display->connector;
>>>> +
>>>> +    dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);
>>>> +
>>>>        return 0;
>>>>    }
>>>>    diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
>>>> b/drivers/gpu/drm/msm/dp/dp_display.h
>>>> index 8e80e3b..d9cb9ee 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>>>> @@ -23,6 +23,8 @@ struct msm_dp {
>>>>          hdmi_codec_plugged_cb plugged_cb;
>>>>    +    bool wide_bus_en;
>>>> +
>>>>        u32 max_pclk_khz;
>>>>          u32 max_dp_lanes;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> index 71db10c..71deb1e 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>        dp_catalog_dump_regs(catalog);
>>>>    }
>>>>    -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
>>>>    {
>>>>        u32 data, total_ver, total_hor;
>>>>        struct dp_catalog *catalog;
>>>> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>>>          catalog->dp_active = data;
>>>>    -    dp_catalog_panel_timing_cfg(catalog);
>>>> +    dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
>>>>        panel->panel_on = true;
>>>>          return 0;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> index 9023e5b..5ec341a 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> @@ -57,7 +57,7 @@ struct dp_panel {
>>>>      int dp_panel_init_panel_info(struct dp_panel *dp_panel);
>>>>    int dp_panel_deinit(struct dp_panel *dp_panel);
>>>> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
>>>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
>>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel);
>>>>    int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>>>>            struct drm_connector *connector);
>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
>>>> b/drivers/gpu/drm/msm/msm_drv.h
>>>> index 07f6c41..667f3a8 100644
>>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>>> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>> *dp_display);
>>>>    void msm_dp_snapshot(struct msm_disp_state *disp_state, struct
>>>> msm_dp *dp_display);
>>>>      void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
>>>> drm_minor *minor);
>>>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
>>>>      #else
>>>>    static inline int __init msm_dp_register(void)
>>>> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct
>>>> msm_dp *dp_display,
>>>>    {
>>>>    }
>>>>    +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>>>> +{
>>>> +    return false;
>>>> +}
>>>> +
>>>>    #endif
>>>>      void __init msm_mdp_register(void);
>>>
>
>

Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display port

[Dmitry Baryshkov]

On 15/02/2022 21:39, Kuogee Hsieh wrote:
> 
> On 2/15/2022 10:30 AM, Dmitry Baryshkov wrote:
>> On Tue, 15 Feb 2022 at 20:49, Kuogee Hsieh <quic_khsieh@quicinc.com> 
>> wrote:
>>>
>>> On 2/15/2022 5:34 AM, Dmitry Baryshkov wrote:
>>>> On 15/02/2022 01:39, Kuogee Hsieh wrote:
>>>>> Widebus feature will transmit two pixel data per pixel clock to
>>>>> interface.
>>>>> This feature now is required to be enabled to easy migrant to higher
>>>>> resolution applications in future. However since some legacy chipsets
>>>>> does not support this feature, this feature is enabled base on chip's
>>>>> hardware revision.
>>>>>
>>>>> changes in v2:
>>>>> -- remove compression related code from timing
>>>>> -- remove op_info from  struct msm_drm_private
>>>>> -- remove unnecessary wide_bus_en variables
>>>>> -- pass wide_bus_en into timing configuration by struct msm_dp
>>>>>
>>>>> Changes in v3:
>>>>> -- split patch into 3 patches
>>>>> -- enable widebus feature base on chip hardware revision
>>>>>
>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
>>>>>    drivers/gpu/drm/msm/dp/dp_catalog.c         | 36
>>>>> +++++++++++++++++++++++++++--
>>>>>    drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
>>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
>>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
>>>>>    drivers/gpu/drm/msm/dp/dp_display.c         | 30
>>>>> ++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
>>>>>    drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
>>>>>    drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
>>>>>    drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
>>>>>    10 files changed, 90 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> index 0c22839..b2d23c2 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev,
>>>>> struct drm_encoder *enc,
>>>>>            timer_setup(&dpu_enc->vsync_event_timer,
>>>>>                    dpu_encoder_vsync_event_handler,
>>>>>                    0);
>>>>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>>>> +    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>>>>>            dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>>> +        dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
>>>>> +    }
>>>> Please revert the order of the patches and move this chunk into the
>>>> DPU patch.
>>> There is chicken  and egg issue here.
>>>
>>> I think we should create an communication channel to pass dp/dsi driver
>>> info to drm, such as wide_bus_en and dsc compression, etc.
>>>
>>> By function calls like this will create more unnecessary tangles between
>>> driver and drm layer.
>> I'm fine with the function call. I'm suggesting another thing. If you
>> change the order of these two patches (#1 - dp, #2 - dpu), the
>> msm_dp_wide_bus_enable() will get added in patch#1 and will be
>> available to be called in the patch#2.
> 
> By doing that, we have wile widebus enabled at (#1 -dp) patch but has no 
> timing engine changes at (#2 -dpu) to support it. This means (#1 -dp) 
> will break dp until (#2 -dpu) merged.
> 
> Is this ok?

Ah, I missed this. Please excuse me. It's fine to leave the order as is 
then.

> 
> 
>>>>> INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>>>>                dpu_encoder_off_work);
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> index 64f0b26..99d087e 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct
>>>>> dp_catalog *dp_catalog,
>>>>>    }
>>>>>      /**
>>>>> + * dp_catalog_hw_revision() - retrieve DP hw revision
>>>>> + *
>>>>> + * @dp_catalog: DP catalog structure
>>>>> + *
>>>>> + * return: u32
>>>>> + *
>>>>> + * This function return the DP controller hw revision
>>>>> + *
>>>>> + */
>>>>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
>>>>> +{
>>>>> +    u32 revision;
>>>>> +    struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>>> +                struct dp_catalog_private, dp_catalog);
>>>>> +
>>>>> +    revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
>>>>> +
>>>>> +    return revision;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>>     * dp_catalog_ctrl_reset() - reset DP controller
>>>>>     *
>>>>>     * @dp_catalog: DP catalog structure
>>>>> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct
>>>>> dp_catalog *dp_catalog)
>>>>>    }
>>>>>      /* panel related catalog functions */
>>>>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>>>>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
>>>>> wide_bus_en)
>>>>>    {
>>>>>        struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>>>                    struct dp_catalog_private, dp_catalog);
>>>>> +    u32 reg;
>>>>>          dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
>>>>>                    dp_catalog->total);
>>>>> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct
>>>>> dp_catalog *dp_catalog)
>>>>>        dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>>>>>                    dp_catalog->width_blanking);
>>>>>        dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER,
>>>>> dp_catalog->dp_active);
>>>>> -    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
>>>>> +
>>>>> +    reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
>>>>> +
>>>>> +    if (wide_bus_en)
>>>>> +        reg |= BIT(4);    /* DATABUS_WIDEN */
>>>>> +    else
>>>>> +        reg &= ~BIT(4);
>>>>> +
>>>>> +    DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
>>>>> +
>>>>> +    dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
>>>>>        return 0;
>>>>>    }
>>>>>    diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>>> index 7dea101..a3a0129 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>>> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog
>>>>> *dp_catalog, u32 cc, u32 tb);
>>>>>    void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32
>>>>> rate,
>>>>>                    u32 stream_rate_khz, bool fixed_nvid);
>>>>>    int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog
>>>>> *dp_catalog, u32 pattern);
>>>>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
>>>>>    void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
>>>>>    bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>>>>>    void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool
>>>>> enable);
>>>>> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct
>>>>> dp_catalog *dp_catalog,
>>>>>    u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog 
>>>>> *dp_catalog);
>>>>>      /* DP Panel APIs */
>>>>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
>>>>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
>>>>> wide_bus_en);
>>>>>    void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
>>>>>    void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
>>>>>                    struct drm_display_mode *drm_mode);
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> index 245e1b9..1c4cf9d 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct
>>>>> dp_ctrl_private *ctrl)
>>>>>        dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>>>>>    }
>>>>>    -static void dp_ctrl_configure_source_params(struct dp_ctrl_private
>>>>> *ctrl)
>>>>> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private
>>>>> *ctrl, bool wide_bus_en)
>>>>>    {
>>>>>        u32 cc, tb;
>>>>>    @@ -167,7 +167,7 @@ static void
>>>>> dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>>>>>            ctrl->panel->dp_mode.bpp);
>>>>>        cc = dp_link_get_colorimetry_config(ctrl->link);
>>>>>        dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
>>>>> -    dp_panel_timing_cfg(ctrl->panel);
>>>>> +    dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
>>>>>    }
>>>>>      /*
>>>>> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>>>        int ret = 0;
>>>>>        bool mainlink_ready = false;
>>>>>        struct dp_ctrl_private *ctrl;
>>>>> +    u32 pixel_rate_orig;
>>>>>          if (!dp_ctrl)
>>>>>            return -EINVAL;
>>>>> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>>>          ctrl->dp_ctrl.pixel_rate = 
>>>>> ctrl->panel->dp_mode.drm_mode.clock;
>>>>>    +    pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
>>>>> +    if (dp_ctrl->wide_bus_en)
>>>>> +        ctrl->dp_ctrl.pixel_rate >>= 1;
>>>>> +
>>>>>        DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>>>>>            ctrl->link->link_params.rate,
>>>>>            ctrl->link->link_params.num_lanes, 
>>>>> ctrl->dp_ctrl.pixel_rate);
>>>>> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>>>         */
>>>>>        reinit_completion(&ctrl->video_comp);
>>>>>    -    dp_ctrl_configure_source_params(ctrl);
>>>>> +    dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
>>>>>          dp_catalog_ctrl_config_msa(ctrl->catalog,
>>>>>            ctrl->link->link_params.rate,
>>>>> -        ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
>>>>> +        pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
>>>>>          dp_ctrl_setup_tr_unit(ctrl);
>>>>>    diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>>> index 2433edb..4dff44d 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>>> @@ -17,6 +17,7 @@ struct dp_ctrl {
>>>>>        bool orientation;
>>>>>        atomic_t aborted;
>>>>>        u32 pixel_rate;
>>>>> +    bool wide_bus_en;
>>>>>    };
>>>>>      int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index e89556ad..d45a3aa 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -117,6 +117,8 @@ struct dp_display_private {
>>>>>        struct dp_event event_list[DP_EVENT_Q_MAX];
>>>>>        spinlock_t event_lock;
>>>>>    +    bool wide_bus_en;
>>>>> +
>>>>>        struct dp_audio *audio;
>>>>>    };
>>>>>    @@ -845,6 +847,8 @@ static int dp_display_enable(struct
>>>>> dp_display_private *dp, u32 data)
>>>>>            return 0;
>>>>>        }
>>>>>    +    dp->ctrl->wide_bus_en = dp->wide_bus_en;
>>>>> +
>>>>>        rc = dp_ctrl_on_stream(dp->ctrl);
>>>>>        if (!rc)
>>>>>            dp_display->power_on = true;
>>>>> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>>>>>            dp->connector, dp_mode);
>>>>>        if (dp_mode->drm_mode.clock)
>>>>>            dp->max_pclk_khz = dp_mode->drm_mode.clock;
>>>>> +
>>>>>        return ret;
>>>>>    }
>>>>>    @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>>> *dp_display)
>>>>>        }
>>>>>    }
>>>>>    +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>>>>> +{
>>>>> +    struct dp_display_private *dp;
>>>>> +    u32 revision, major, minor;
>>>>> +
>>>>> +    dp = container_of(dp_display, struct dp_display_private,
>>>>> dp_display);
>>>>> +
>>>>> +    /* for the time being widebus only support on DP */
>>>>> +    if (dp_display->connector_type  ==
>>>>> DRM_MODE_CONNECTOR_DisplayPort) {
>>>>> +        revision = dp_catalog_hw_revision(dp->catalog);
>>>>> +        major = ((revision >> 28) & 0x0ff);
>>>>> +        minor = ((revision >> 16) & 0x0fff);
>>>>> +
>>>>> +    DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
>>>>> +
>>>>> +        if (major >= 1 && minor >= 2)
>>>>> +            return true;
>>>>> +    }
>>>>> +
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>>    void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
>>>>> drm_minor *minor)
>>>>>    {
>>>>>        struct dp_display_private *dp;
>>>>> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp
>>>>> *dp_display, struct drm_device *dev,
>>>>>        dp_priv->panel->connector = dp_display->connector;
>>>>>          priv->connectors[priv->num_connectors++] =
>>>>> dp_display->connector;
>>>>> +
>>>>> +    dp_priv->wide_bus_en = msm_dp_wide_bus_enable(dp_display);
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>    diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.h
>>>>> index 8e80e3b..d9cb9ee 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>>>>> @@ -23,6 +23,8 @@ struct msm_dp {
>>>>>          hdmi_codec_plugged_cb plugged_cb;
>>>>>    +    bool wide_bus_en;
>>>>> +
>>>>>        u32 max_pclk_khz;
>>>>>          u32 max_dp_lanes;
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>> index 71db10c..71deb1e 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>>> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>>        dp_catalog_dump_regs(catalog);
>>>>>    }
>>>>>    -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>>>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
>>>>>    {
>>>>>        u32 data, total_ver, total_hor;
>>>>>        struct dp_catalog *catalog;
>>>>> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>>>>          catalog->dp_active = data;
>>>>>    -    dp_catalog_panel_timing_cfg(catalog);
>>>>> +    dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
>>>>>        panel->panel_on = true;
>>>>>          return 0;
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>>> index 9023e5b..5ec341a 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>>> @@ -57,7 +57,7 @@ struct dp_panel {
>>>>>      int dp_panel_init_panel_info(struct dp_panel *dp_panel);
>>>>>    int dp_panel_deinit(struct dp_panel *dp_panel);
>>>>> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
>>>>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
>>>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel);
>>>>>    int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>>>>>            struct drm_connector *connector);
>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
>>>>> b/drivers/gpu/drm/msm/msm_drv.h
>>>>> index 07f6c41..667f3a8 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>>>> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>>> *dp_display);
>>>>>    void msm_dp_snapshot(struct msm_disp_state *disp_state, struct
>>>>> msm_dp *dp_display);
>>>>>      void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
>>>>> drm_minor *minor);
>>>>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
>>>>>      #else
>>>>>    static inline int __init msm_dp_register(void)
>>>>> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct
>>>>> msm_dp *dp_display,
>>>>>    {
>>>>>    }
>>>>>    +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>>>>> +{
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>>    #endif
>>>>>      void __init msm_mdp_register(void);
>>>>
>>
>>


-- 
With best wishes
Dmitry