drm/msm/dsi: check video mode engine status before waiting

Currently the DSI PHY timings are hard-coded for a specific panel for the 10nm PHY.

Replace this with the auto PHY timing calculator which can calculate the PHY timings for any panel.

Signed-off-by Abhinav Kumar <[email protected]>

---
  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c      | 111 +++++++++++++++++++++++++++++
  drivers/gpu/drm/msm/dsi/phy/dsi_phy.h      |   2 +
  drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c |  28 --------
  3 files changed, 113 insertions(+), 28 deletions(-)

drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +111 -0

index 8e9d5c2..5b42885 100644
@@ -265,6 +265,117 @@ int msm_dsi_dphy_timing_calc_v2(struct msm_dsi_dphy_timing *timing,
  	return 0;
  }
 
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				       struct msm_dsi_phy_clk_request *clk_req)
+{
+	const unsigned long bit_rate = clk_req->bitclk_rate;
+	const unsigned long esc_rate = clk_req->escclk_rate;
+	s32 ui, ui_x8, lpx;
+	s32 tmax, tmin;
+	s32 pcnt0 = 50;
+	s32 pcnt1 = 50;
+	s32 pcnt2 = 10;
+	s32 pcnt3 = 30;
+	s32 pcnt4 = 10;
+	s32 pcnt5 = 2;
+	s32 coeff = 1000; /* Precision, should avoid overflow */
+	s32 hb_en, hb_en_ckln;
+	s32 temp;
+
+	if (!bit_rate || !esc_rate)
+		return -EINVAL;
+
+	timing->hs_halfbyte_en = 0;
+	hb_en = 0;
+	timing->hs_halfbyte_en_ckln = 0;
+	hb_en_ckln = 0;
+
+	ui = mult_frac(NSEC_PER_MSEC, coeff, bit_rate / 1000);
+	ui_x8 = ui << 3;
+	lpx = mult_frac(NSEC_PER_MSEC, coeff, esc_rate / 1000);
+
+	temp = S_DIV_ROUND_UP(38 * coeff, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (95 * coeff) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->clk_prepare = linear_inter(tmax, tmin, pcnt0, 0, false);
+
+
+	temp = 300 * coeff - (timing->clk_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = (tmin > 255) ? 511 : 255;
+	timing->clk_zero = linear_inter(tmax, tmin, pcnt5, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 3 * ui, ui_x8);
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp + 3 * ui) / ui_x8;
+	timing->clk_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = S_DIV_ROUND_UP(40 * coeff + 4 * ui, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (85 * coeff + 6 * ui) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->hs_prepare = linear_inter(tmax, tmin, pcnt1, 0, false);
+
+	temp = 145 * coeff + 10 * ui - (timing->hs_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_zero = linear_inter(tmax, tmin, pcnt4, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 4 * ui, ui_x8) - 1;
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp / ui_x8) - 1;
+	timing->hs_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = 50 * coeff + ((hb_en << 2) - 8) * ui;
+	timing->hs_rqst = S_DIV_ROUND_UP(temp, ui_x8);
+
+	tmin = DIV_ROUND_UP(100 * coeff, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_exit = linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 50 * coeff + ((hb_en_ckln << 2) - 8) * ui;
+	timing->hs_rqst_ckln = S_DIV_ROUND_UP(temp, ui_x8);
+
+	temp = 60 * coeff + 52 * ui - 43 * ui;
+	tmin = DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	timing->shared_timings.clk_post =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 8 * ui + (timing->clk_prepare << 3) * ui;
+	temp += (((timing->clk_zero + 3) << 3) + 11) * ui;
+	temp += hb_en_ckln ? (((timing->hs_rqst_ckln << 3) + 4) * ui) :
+				(((timing->hs_rqst_ckln << 3) + 8) * ui);
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	if (tmin > tmax) {
+		temp = linear_inter(tmax << 1, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre = temp >> 1;
+		timing->shared_timings.clk_pre_inc_by_2 = 1;
+	} else {
+		timing->shared_timings.clk_pre =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre_inc_by_2 = 0;
+	}
+
+	timing->ta_go = 3;
+	timing->ta_sure = 0;
+	timing->ta_get = 4;
+
+	DBG("%d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d",
+	    timing->shared_timings.clk_pre, timing->shared_timings.clk_post,
+	    timing->shared_timings.clk_pre_inc_by_2, timing->clk_zero,
+	    timing->clk_trail, timing->clk_prepare, timing->hs_exit,
+	    timing->hs_zero, timing->hs_prepare, timing->hs_trail,
+	    timing->hs_rqst, timing->hs_rqst_ckln, timing->hs_halfbyte_en,
+	    timing->hs_halfbyte_en_ckln, timing->hs_prep_dly,
+	    timing->hs_prep_dly_ckln);
+
+
+	return 0;
+}
+
  void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id, u32 reg,
  				u32 bit_mask)
  {

drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +2 -0

index c56268c..a24ab80 100644
@@ -101,6 +101,8 @@ int msm_dsi_dphy_timing_calc(struct msm_dsi_dphy_timing *timing,
  			     struct msm_dsi_phy_clk_request *clk_req);
  int msm_dsi_dphy_timing_calc_v2(struct msm_dsi_dphy_timing *timing,
  				struct msm_dsi_phy_clk_request *clk_req);
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				struct msm_dsi_phy_clk_request *clk_req);
  void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id, u32 reg,
  				u32 bit_mask);
  int msm_dsi_phy_init_common(struct msm_dsi_phy *phy);

drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c +0 -21

index 0af951a..b3fffc8 100644
@@ -79,34 +79,6 @@ static void dsi_phy_hw_v3_0_lane_settings(struct msm_dsi_phy *phy)
  	dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 0x04);
  }
 
-static int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
-				       struct msm_dsi_phy_clk_request *clk_req)
-{
-	/*
-	 * TODO: These params need to be computed, they're currently hardcoded
-	 * for a 1440x2560@60Hz panel with a byteclk of 100.618 Mhz, and a
-	 * default escape clock of 19.2 Mhz.
-	 */
-
-	timing->hs_halfbyte_en = 0;
-	timing->clk_zero = 0x1c;
-	timing->clk_prepare = 0x07;
-	timing->clk_trail = 0x07;
-	timing->hs_exit = 0x23;
-	timing->hs_zero = 0x21;
-	timing->hs_prepare = 0x07;
-	timing->hs_trail = 0x07;
-	timing->hs_rqst = 0x05;
-	timing->ta_sure = 0x00;
-	timing->ta_go = 0x03;
-	timing->ta_get = 0x04;

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +3 -0

index 7a03a94..24766c3 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,18 @@ static void dsi_set_tx_power_mode(int mode, struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);

Quoted 31 lines · Abhinav Kumar

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +3 -0

index 7a03a94..24766c3 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,18 @@ static void dsi_set_tx_power_mode(int mode, 
 struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);

drivers/gpu/drm/msm/dsi/dsi_host.c static void dsi_set_tx_power_mode(int mode, · line 1002

Quoted 10 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +4 -1

 
 	reinit_completion(&msm_host->video_comp);
 
-	wait_for_completion_timeout(&msm_host->video_comp,
+	ret = wait_for_completion_timeout(&msm_host->video_comp,
 			msecs_to_jiffies(70));
 
+	if (ret <= 0)
+		pr_err("wait for video done failed\n");
+

Does this need to be rate limited? How often will it print? Also, 'wait for video done timed out" would be more descriptive for the situation. I'm not sure from the context if you have a pr_fmt defined for this file but if you don't you might want to add the appropriate prefixes (or use dev_err) so the reader can get a bit more detailed information about where to look and what is going on.

Jordan

Currently the DSI PHY timings are hard-coded for a specific panel for the 10nm PHY.

Replace this with the auto PHY timing calculator which can calculate the PHY timings for any panel.

Changes in v2:
- None

Reviewed-by Archit Taneja <[email protected]>

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c      | 111 +++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h      |   2 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c |  28 --------
 3 files changed, 113 insertions(+), 28 deletions(-)

drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +111 -0

index 8e9d5c2..5b42885 100644
@@ -265,6 +265,117 @@ int msm_dsi_dphy_timing_calc_v2(struct msm_dsi_dphy_timing *timing,
 	return 0;
 }
 
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				       struct msm_dsi_phy_clk_request *clk_req)
+{
+	const unsigned long bit_rate = clk_req->bitclk_rate;
+	const unsigned long esc_rate = clk_req->escclk_rate;
+	s32 ui, ui_x8, lpx;
+	s32 tmax, tmin;
+	s32 pcnt0 = 50;
+	s32 pcnt1 = 50;
+	s32 pcnt2 = 10;
+	s32 pcnt3 = 30;
+	s32 pcnt4 = 10;
+	s32 pcnt5 = 2;
+	s32 coeff = 1000; /* Precision, should avoid overflow */
+	s32 hb_en, hb_en_ckln;
+	s32 temp;
+
+	if (!bit_rate || !esc_rate)
+		return -EINVAL;
+
+	timing->hs_halfbyte_en = 0;
+	hb_en = 0;
+	timing->hs_halfbyte_en_ckln = 0;
+	hb_en_ckln = 0;
+
+	ui = mult_frac(NSEC_PER_MSEC, coeff, bit_rate / 1000);
+	ui_x8 = ui << 3;
+	lpx = mult_frac(NSEC_PER_MSEC, coeff, esc_rate / 1000);
+
+	temp = S_DIV_ROUND_UP(38 * coeff, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (95 * coeff) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->clk_prepare = linear_inter(tmax, tmin, pcnt0, 0, false);
+
+
+	temp = 300 * coeff - (timing->clk_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = (tmin > 255) ? 511 : 255;
+	timing->clk_zero = linear_inter(tmax, tmin, pcnt5, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 3 * ui, ui_x8);
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp + 3 * ui) / ui_x8;
+	timing->clk_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = S_DIV_ROUND_UP(40 * coeff + 4 * ui, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (85 * coeff + 6 * ui) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->hs_prepare = linear_inter(tmax, tmin, pcnt1, 0, false);
+
+	temp = 145 * coeff + 10 * ui - (timing->hs_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_zero = linear_inter(tmax, tmin, pcnt4, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 4 * ui, ui_x8) - 1;
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp / ui_x8) - 1;
+	timing->hs_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = 50 * coeff + ((hb_en << 2) - 8) * ui;
+	timing->hs_rqst = S_DIV_ROUND_UP(temp, ui_x8);
+
+	tmin = DIV_ROUND_UP(100 * coeff, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_exit = linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 50 * coeff + ((hb_en_ckln << 2) - 8) * ui;
+	timing->hs_rqst_ckln = S_DIV_ROUND_UP(temp, ui_x8);
+
+	temp = 60 * coeff + 52 * ui - 43 * ui;
+	tmin = DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	timing->shared_timings.clk_post =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 8 * ui + (timing->clk_prepare << 3) * ui;
+	temp += (((timing->clk_zero + 3) << 3) + 11) * ui;
+	temp += hb_en_ckln ? (((timing->hs_rqst_ckln << 3) + 4) * ui) :
+				(((timing->hs_rqst_ckln << 3) + 8) * ui);
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	if (tmin > tmax) {
+		temp = linear_inter(tmax << 1, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre = temp >> 1;
+		timing->shared_timings.clk_pre_inc_by_2 = 1;
+	} else {
+		timing->shared_timings.clk_pre =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre_inc_by_2 = 0;
+	}
+
+	timing->ta_go = 3;
+	timing->ta_sure = 0;
+	timing->ta_get = 4;
+
+	DBG("%d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d",
+	    timing->shared_timings.clk_pre, timing->shared_timings.clk_post,
+	    timing->shared_timings.clk_pre_inc_by_2, timing->clk_zero,
+	    timing->clk_trail, timing->clk_prepare, timing->hs_exit,
+	    timing->hs_zero, timing->hs_prepare, timing->hs_trail,
+	    timing->hs_rqst, timing->hs_rqst_ckln, timing->hs_halfbyte_en,
+	    timing->hs_halfbyte_en_ckln, timing->hs_prep_dly,
+	    timing->hs_prep_dly_ckln);
+
+
+	return 0;
+}
+
 void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id, u32 reg,
 				u32 bit_mask)
 {

drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +2 -0

index c56268c..a24ab80 100644
@@ -101,6 +101,8 @@ int msm_dsi_dphy_timing_calc(struct msm_dsi_dphy_timing *timing,
 			     struct msm_dsi_phy_clk_request *clk_req);
 int msm_dsi_dphy_timing_calc_v2(struct msm_dsi_dphy_timing *timing,
 				struct msm_dsi_phy_clk_request *clk_req);
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				struct msm_dsi_phy_clk_request *clk_req);
 void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id, u32 reg,
 				u32 bit_mask);
 int msm_dsi_phy_init_common(struct msm_dsi_phy *phy);

drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c +0 -28

index 0af951a..b3fffc8 100644
@@ -79,34 +79,6 @@ static void dsi_phy_hw_v3_0_lane_settings(struct msm_dsi_phy *phy)
 	dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 0x04);
 }
 
-static int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
-				       struct msm_dsi_phy_clk_request *clk_req)
-{
-	/*
-	 * TODO: These params need to be computed, they're currently hardcoded
-	 * for a 1440x2560@60Hz panel with a byteclk of 100.618 Mhz, and a
-	 * default escape clock of 19.2 Mhz.
-	 */
-
-	timing->hs_halfbyte_en = 0;
-	timing->clk_zero = 0x1c;
-	timing->clk_prepare = 0x07;
-	timing->clk_trail = 0x07;
-	timing->hs_exit = 0x23;
-	timing->hs_zero = 0x21;
-	timing->hs_prepare = 0x07;
-	timing->hs_trail = 0x07;
-	timing->hs_rqst = 0x05;
-	timing->ta_sure = 0x00;
-	timing->ta_go = 0x03;
-	timing->ta_get = 0x04;
-
-	timing->shared_timings.clk_pre = 0x2d;
-	timing->shared_timings.clk_post = 0x0d;
-
-	return 0;
-}
-
 static int dsi_10nm_phy_enable(struct msm_dsi_phy *phy, int src_pll_id,
 			       struct msm_dsi_phy_clk_request *clk_req)
 {

Signature · 2 lines

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

Quoted 3 lines · Abhinav Kumar

Currently the DSI PHY timings are hard-coded for a specific panel for the 10nm PHY.

Quoted 2 lines

Replace this with the auto PHY timing calculator which can calculate the PHY timings for any panel.

Any chance you could document what you're doing so anyone without documentation has a clue what's going on?

Sean

Quoted 192 lines

Changes in v2:
- None

Reviewed-by Archit Taneja <[email protected]>

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c      | 111 
+++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h      |   2 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c |  28 --------
 3 files changed, 113 insertions(+), 28 deletions(-)

drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +111 -0

index 8e9d5c2..5b42885 100644
@@ -265,6 +265,117 @@ int msm_dsi_dphy_timing_calc_v2(struct 
 msm_dsi_dphy_timing *timing,
 	return 0;
 }
 
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				       struct msm_dsi_phy_clk_request *clk_req)
+{
+	const unsigned long bit_rate = clk_req->bitclk_rate;
+	const unsigned long esc_rate = clk_req->escclk_rate;
+	s32 ui, ui_x8, lpx;
+	s32 tmax, tmin;
+	s32 pcnt0 = 50;
+	s32 pcnt1 = 50;
+	s32 pcnt2 = 10;
+	s32 pcnt3 = 30;
+	s32 pcnt4 = 10;
+	s32 pcnt5 = 2;
+	s32 coeff = 1000; /* Precision, should avoid overflow */
+	s32 hb_en, hb_en_ckln;
+	s32 temp;
+
+	if (!bit_rate || !esc_rate)
+		return -EINVAL;
+
+	timing->hs_halfbyte_en = 0;
+	hb_en = 0;
+	timing->hs_halfbyte_en_ckln = 0;
+	hb_en_ckln = 0;
+
+	ui = mult_frac(NSEC_PER_MSEC, coeff, bit_rate / 1000);
+	ui_x8 = ui << 3;
+	lpx = mult_frac(NSEC_PER_MSEC, coeff, esc_rate / 1000);
+
+	temp = S_DIV_ROUND_UP(38 * coeff, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (95 * coeff) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->clk_prepare = linear_inter(tmax, tmin, pcnt0, 0, false);
+
+
+	temp = 300 * coeff - (timing->clk_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = (tmin > 255) ? 511 : 255;
+	timing->clk_zero = linear_inter(tmax, tmin, pcnt5, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 3 * ui, ui_x8);
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp + 3 * ui) / ui_x8;
+	timing->clk_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = S_DIV_ROUND_UP(40 * coeff + 4 * ui, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (85 * coeff + 6 * ui) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->hs_prepare = linear_inter(tmax, tmin, pcnt1, 0, false);
+
+	temp = 145 * coeff + 10 * ui - (timing->hs_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_zero = linear_inter(tmax, tmin, pcnt4, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 4 * ui, ui_x8) - 1;
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp / ui_x8) - 1;
+	timing->hs_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = 50 * coeff + ((hb_en << 2) - 8) * ui;
+	timing->hs_rqst = S_DIV_ROUND_UP(temp, ui_x8);
+
+	tmin = DIV_ROUND_UP(100 * coeff, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_exit = linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 50 * coeff + ((hb_en_ckln << 2) - 8) * ui;
+	timing->hs_rqst_ckln = S_DIV_ROUND_UP(temp, ui_x8);
+
+	temp = 60 * coeff + 52 * ui - 43 * ui;
+	tmin = DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	timing->shared_timings.clk_post =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 8 * ui + (timing->clk_prepare << 3) * ui;
+	temp += (((timing->clk_zero + 3) << 3) + 11) * ui;
+	temp += hb_en_ckln ? (((timing->hs_rqst_ckln << 3) + 4) * ui) :
+				(((timing->hs_rqst_ckln << 3) + 8) * ui);
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	if (tmin > tmax) {
+		temp = linear_inter(tmax << 1, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre = temp >> 1;
+		timing->shared_timings.clk_pre_inc_by_2 = 1;
+	} else {
+		timing->shared_timings.clk_pre =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre_inc_by_2 = 0;
+	}
+
+	timing->ta_go = 3;
+	timing->ta_sure = 0;
+	timing->ta_get = 4;
+
+	DBG("%d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, 
 %d",
+	    timing->shared_timings.clk_pre, timing->shared_timings.clk_post,
+	    timing->shared_timings.clk_pre_inc_by_2, timing->clk_zero,
+	    timing->clk_trail, timing->clk_prepare, timing->hs_exit,
+	    timing->hs_zero, timing->hs_prepare, timing->hs_trail,
+	    timing->hs_rqst, timing->hs_rqst_ckln, timing->hs_halfbyte_en,
+	    timing->hs_halfbyte_en_ckln, timing->hs_prep_dly,
+	    timing->hs_prep_dly_ckln);
+
+
+	return 0;
+}
+
 void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id, u32 

reg,

drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +0 -0

 				u32 bit_mask)
 {

drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +2 -0

index c56268c..a24ab80 100644
@@ -101,6 +101,8 @@ int msm_dsi_dphy_timing_calc(struct 
 msm_dsi_dphy_timing *timing,
 			     struct msm_dsi_phy_clk_request *clk_req);
 int msm_dsi_dphy_timing_calc_v2(struct msm_dsi_dphy_timing *timing,
 				struct msm_dsi_phy_clk_request *clk_req);
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				struct msm_dsi_phy_clk_request *clk_req);
 void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id, u32 
 reg,

drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +0 -0

 				u32 bit_mask);
 int msm_dsi_phy_init_common(struct msm_dsi_phy *phy);

drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c +0 -5

index 0af951a..b3fffc8 100644
@@ -79,34 +79,6 @@ static void dsi_phy_hw_v3_0_lane_settings(struct 
 msm_dsi_phy *phy)
 	dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 0x04);
 }
 
-static int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing 
 *timing,
-				       struct msm_dsi_phy_clk_request *clk_req)
-{
-	/*
-	 * TODO: These params need to be computed, they're currently 

hardcoded

drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c +0 -23

-	 * for a 1440x2560@60Hz panel with a byteclk of 100.618 Mhz, and a
-	 * default escape clock of 19.2 Mhz.
-	 */
-
-	timing->hs_halfbyte_en = 0;
-	timing->clk_zero = 0x1c;
-	timing->clk_prepare = 0x07;
-	timing->clk_trail = 0x07;
-	timing->hs_exit = 0x23;
-	timing->hs_zero = 0x21;
-	timing->hs_prepare = 0x07;
-	timing->hs_trail = 0x07;
-	timing->hs_rqst = 0x05;
-	timing->ta_sure = 0x00;
-	timing->ta_go = 0x03;
-	timing->ta_get = 0x04;
-
-	timing->shared_timings.clk_pre = 0x2d;
-	timing->shared_timings.clk_post = 0x0d;
-
-	return 0;
-}
-

Quoted 8 lines

static int dsi_10nm_phy_enable(struct msm_dsi_phy *phy, int
src_pll_id,
struct msm_dsi_phy_clk_request *clk_req)
{
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Quoted 11 lines · Sean Paul

Quoted 5 lines · Abhinav Kumar

Currently the DSI PHY timings are hard-coded for a specific panel for the 10nm PHY.

Replace this with the auto PHY timing calculator which can calculate the PHY timings for any panel.

Any chance you could document what you're doing so anyone without documentation
has a clue what's going on?

Quoted 2 lines

Sean

I am afraid it will hard to document more about this function other than whats mentioned here.
Basically, we have an excel sheet which does this math to calculate the DSI timings.
This patch implements that excel sheet for SDM845 which uses 10nm PHY. We will not be able to explain the math in more detail.

Quoted 204 lines

Quoted 204 lines

Changes in v2:
- None

Reviewed-by Archit Taneja <[email protected]>

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c      | 111
+++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h      |   2 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c |  28 --------
 3 files changed, 113 insertions(+), 28 deletions(-)

drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +111 -0

index 8e9d5c2..5b42885 100644
@@ -265,6 +265,117 @@ int msm_dsi_dphy_timing_calc_v2(struct
 msm_dsi_dphy_timing *timing,
 	return 0;
 }
 
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				       struct msm_dsi_phy_clk_request *clk_req)
+{
+	const unsigned long bit_rate = clk_req->bitclk_rate;
+	const unsigned long esc_rate = clk_req->escclk_rate;
+	s32 ui, ui_x8, lpx;
+	s32 tmax, tmin;
+	s32 pcnt0 = 50;
+	s32 pcnt1 = 50;
+	s32 pcnt2 = 10;
+	s32 pcnt3 = 30;
+	s32 pcnt4 = 10;
+	s32 pcnt5 = 2;
+	s32 coeff = 1000; /* Precision, should avoid overflow */
+	s32 hb_en, hb_en_ckln;
+	s32 temp;
+
+	if (!bit_rate || !esc_rate)
+		return -EINVAL;
+
+	timing->hs_halfbyte_en = 0;
+	hb_en = 0;
+	timing->hs_halfbyte_en_ckln = 0;
+	hb_en_ckln = 0;
+
+	ui = mult_frac(NSEC_PER_MSEC, coeff, bit_rate / 1000);
+	ui_x8 = ui << 3;
+	lpx = mult_frac(NSEC_PER_MSEC, coeff, esc_rate / 1000);
+
+	temp = S_DIV_ROUND_UP(38 * coeff, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (95 * coeff) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->clk_prepare = linear_inter(tmax, tmin, pcnt0, 0, false);
+
+
+	temp = 300 * coeff - (timing->clk_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = (tmin > 255) ? 511 : 255;
+	timing->clk_zero = linear_inter(tmax, tmin, pcnt5, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 3 * ui, ui_x8);
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp + 3 * ui) / ui_x8;
+	timing->clk_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = S_DIV_ROUND_UP(40 * coeff + 4 * ui, ui_x8);
+	tmin = max_t(s32, temp, 0);
+	temp = (85 * coeff + 6 * ui) / ui_x8;
+	tmax = max_t(s32, temp, 0);
+	timing->hs_prepare = linear_inter(tmax, tmin, pcnt1, 0, false);
+
+	temp = 145 * coeff + 10 * ui - (timing->hs_prepare << 3) * ui;
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_zero = linear_inter(tmax, tmin, pcnt4, 0, false);
+
+	tmin = DIV_ROUND_UP(60 * coeff + 4 * ui, ui_x8) - 1;
+	temp = 105 * coeff + 12 * ui - 20 * coeff;
+	tmax = (temp / ui_x8) - 1;
+	timing->hs_trail = linear_inter(tmax, tmin, pcnt3, 0, false);
+
+	temp = 50 * coeff + ((hb_en << 2) - 8) * ui;
+	timing->hs_rqst = S_DIV_ROUND_UP(temp, ui_x8);
+
+	tmin = DIV_ROUND_UP(100 * coeff, ui_x8) - 1;
+	tmax = 255;
+	timing->hs_exit = linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 50 * coeff + ((hb_en_ckln << 2) - 8) * ui;
+	timing->hs_rqst_ckln = S_DIV_ROUND_UP(temp, ui_x8);
+
+	temp = 60 * coeff + 52 * ui - 43 * ui;
+	tmin = DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	timing->shared_timings.clk_post =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+
+	temp = 8 * ui + (timing->clk_prepare << 3) * ui;
+	temp += (((timing->clk_zero + 3) << 3) + 11) * ui;
+	temp += hb_en_ckln ? (((timing->hs_rqst_ckln << 3) + 4) * ui) :
+				(((timing->hs_rqst_ckln << 3) + 8) * ui);
+	tmin = S_DIV_ROUND_UP(temp, ui_x8) - 1;
+	tmax = 63;
+	if (tmin > tmax) {
+		temp = linear_inter(tmax << 1, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre = temp >> 1;
+		timing->shared_timings.clk_pre_inc_by_2 = 1;
+	} else {
+		timing->shared_timings.clk_pre =
+				linear_inter(tmax, tmin, pcnt2, 0, false);
+		timing->shared_timings.clk_pre_inc_by_2 = 0;
+	}
+
+	timing->ta_go = 3;
+	timing->ta_sure = 0;
+	timing->ta_get = 4;
+
+	DBG("%d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d,
 %d",
+	    timing->shared_timings.clk_pre, timing->shared_timings.clk_post,
+	    timing->shared_timings.clk_pre_inc_by_2, timing->clk_zero,
+	    timing->clk_trail, timing->clk_prepare, timing->hs_exit,
+	    timing->hs_zero, timing->hs_prepare, timing->hs_trail,
+	    timing->hs_rqst, timing->hs_rqst_ckln, timing->hs_halfbyte_en,
+	    timing->hs_halfbyte_en_ckln, timing->hs_prep_dly,
+	    timing->hs_prep_dly_ckln);
+
+
+	return 0;
+}
+
 void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id,

u32 reg,

drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +0 -0

 				u32 bit_mask)
 {

drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +2 -0

index c56268c..a24ab80 100644
@@ -101,6 +101,8 @@ int msm_dsi_dphy_timing_calc(struct
 msm_dsi_dphy_timing *timing,
 			     struct msm_dsi_phy_clk_request *clk_req);
 int msm_dsi_dphy_timing_calc_v2(struct msm_dsi_dphy_timing *timing,
 				struct msm_dsi_phy_clk_request *clk_req);
+int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,
+				struct msm_dsi_phy_clk_request *clk_req);
 void msm_dsi_phy_set_src_pll(struct msm_dsi_phy *phy, int pll_id,
 u32 reg,

drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +0 -0

 				u32 bit_mask);
 int msm_dsi_phy_init_common(struct msm_dsi_phy *phy);

drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c +0 -5

index 0af951a..b3fffc8 100644
@@ -79,34 +79,6 @@ static void dsi_phy_hw_v3_0_lane_settings(struct
 msm_dsi_phy *phy)
 	dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 0x04);
 }
 
-static int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing
 *timing,
-				       struct msm_dsi_phy_clk_request *clk_req)
-{
-	/*
-	 * TODO: These params need to be computed, they're currently

hardcoded

drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c +0 -23

-	 * for a 1440x2560@60Hz panel with a byteclk of 100.618 Mhz, and a
-	 * default escape clock of 19.2 Mhz.
-	 */
-
-	timing->hs_halfbyte_en = 0;
-	timing->clk_zero = 0x1c;
-	timing->clk_prepare = 0x07;
-	timing->clk_trail = 0x07;
-	timing->hs_exit = 0x23;
-	timing->hs_zero = 0x21;
-	timing->hs_prepare = 0x07;
-	timing->hs_trail = 0x07;
-	timing->hs_rqst = 0x05;
-	timing->ta_sure = 0x00;
-	timing->ta_go = 0x03;
-	timing->ta_get = 0x04;
-
-	timing->shared_timings.clk_pre = 0x2d;
-	timing->shared_timings.clk_post = 0x0d;
-
-	return 0;
-}
-
 static int dsi_10nm_phy_enable(struct msm_dsi_phy *phy, int

src_pll_id,
struct msm_dsi_phy_clk_request *clk_req)
{
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Changes in v2:
- Replace pr_err with dev_err
- Changed error message

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +10 -3

index 7a03a94..5b7b290 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int mode, struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+	struct device *dev = &msm_host->pdev->dev;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
 
 	reinit_completion(&msm_host->video_comp);
 
-	wait_for_completion_timeout(&msm_host->video_comp,
+	ret = wait_for_completion_timeout(&msm_host->video_comp,
 			msecs_to_jiffies(70));
 
+	if (ret <= 0)
+		dev_err(dev, "wait for video done timed out\n");
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }
 

@@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct msm_dsi_host *msm_host)
 	if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
 		return;
 
-	if (msm_host->power_on) {
+	if (msm_host->power_on && msm_host->enabled) {
 		dsi_wait4video_done(msm_host);
 		/* delay 4 ms to skip BLLP */
 		usleep_range(2000, 4000);

@@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct mipi_dsi_host *host)
 	 *	pm_runtime_put_autosuspend(&msm_host->pdev->dev);
 	 * }
 	 */
-
+	msm_host->enabled = true;
 	return 0;
 }
 

drivers/gpu/drm/msm/dsi/dsi_host.c +0 -0

 	return 0;
 }
 

Signature · 7 lines

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

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Quoted 70 lines · Abhinav Kumar

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Changes in v2:
- Replace pr_err with dev_err
- Changed error message

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +9 -2

index 7a03a94..5b7b290 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int mode, 
 struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+	struct device *dev = &msm_host->pdev->dev;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
 
 	reinit_completion(&msm_host->video_comp);
 
-	wait_for_completion_timeout(&msm_host->video_comp,
+	ret = wait_for_completion_timeout(&msm_host->video_comp,
 			msecs_to_jiffies(70));
 
+	if (ret <= 0)
+		dev_err(dev, "wait for video done timed out\n");
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }

@@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct 
 msm_dsi_host *msm_host)
 	if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
 		return;
 
-	if (msm_host->power_on) {
+	if (msm_host->power_on && msm_host->enabled) {
 		dsi_wait4video_done(msm_host);
 		/* delay 4 ms to skip BLLP */

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

 		usleep_range(2000, 4000);

@@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct mipi_dsi_host 
 *host)
 	 *	pm_runtime_put_autosuspend(&msm_host->pdev->dev);
 	 * }
 	 */
-
+	msm_host->enabled = true;
 	return 0;
 }

drivers/gpu/drm/msm/dsi/dsi_host.c int msm_dsi_host_disable(struct mipi_dsi_host · line 2230

Quoted 6 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

@@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct mipi_dsi_host 
 *host)
 	 * Reset to disable video engine so that we can send off cmd.
 	 */
 	dsi_sw_reset(msm_host);
-
+	msm_host->enabled = false;

This should go at the start of the function. Also, it's unclear from this patch, but I assume this is protected by a lock?

Sean

Quoted 5 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +0 -0

 	return 0;
 }

Quoted 8 lines

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

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Hi Sean

Thanks for the review.

Reply inline.

Quoted 81 lines · Sean Paul

Quoted 70 lines · Abhinav Kumar

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Changes in v2:
- Replace pr_err with dev_err
- Changed error message

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +9 -2

index 7a03a94..5b7b290 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int mode, 
 struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+	struct device *dev = &msm_host->pdev->dev;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
 
 	reinit_completion(&msm_host->video_comp);
 
-	wait_for_completion_timeout(&msm_host->video_comp,
+	ret = wait_for_completion_timeout(&msm_host->video_comp,
 			msecs_to_jiffies(70));
 
+	if (ret <= 0)
+		dev_err(dev, "wait for video done timed out\n");
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }

@@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct 
 msm_dsi_host *msm_host)
 	if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
 		return;
 
-	if (msm_host->power_on) {
+	if (msm_host->power_on && msm_host->enabled) {
 		dsi_wait4video_done(msm_host);
 		/* delay 4 ms to skip BLLP */

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

 		usleep_range(2000, 4000);

@@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct mipi_dsi_host 
 *host)
 	 *	pm_runtime_put_autosuspend(&msm_host->pdev->dev);
 	 * }
 	 */
-
+	msm_host->enabled = true;
 	return 0;
 }

drivers/gpu/drm/msm/dsi/dsi_host.c int msm_dsi_host_disable(struct mipi_dsi_host · line 2230

Quoted 6 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

@@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct mipi_dsi_host 
 *host)
 	 * Reset to disable video engine so that we can send off cmd.
 	 */
 	dsi_sw_reset(msm_host);
-
+	msm_host->enabled = false;

This should go at the start of the function. Also, it's unclear from this patch, but I assume this is protected by a lock?

Quoted 1 line

Sean

[Abhinav] Yes, will move this to the start.
No, there is no lock here but at this point doesnt need one. The reason is that, this variable will be written to and read by the same process
(suspend thread OR resume thread which sends the panel ON/OFF commands). If we decide to expose other interfaces to send commands like debugfs or sysfs and
introduce more threads, we will add the locking.

Quoted 9 lines

Quoted 9 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +0 -1

 	return 0;
 }
 
--

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

Quoted 4 lines

Quoted 4 lines

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 91 lines · On 2018-04-13 14:10, [email protected] wrote:

Hi Sean

Thanks for the review.

Reply inline.

Quoted 83 lines · Sean Paul

Quoted 71 lines · Abhinav Kumar

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Changes in v2:
- Replace pr_err with dev_err
- Changed error message

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +9 -2

index 7a03a94..5b7b290 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int
 mode, struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+	struct device *dev = &msm_host->pdev->dev;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
 
 	reinit_completion(&msm_host->video_comp);
 
-	wait_for_completion_timeout(&msm_host->video_comp,
+	ret = wait_for_completion_timeout(&msm_host->video_comp,
 			msecs_to_jiffies(70));
 
+	if (ret <= 0)
+		dev_err(dev, "wait for video done timed out\n");
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }

@@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct
 msm_dsi_host *msm_host)
 	if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
 		return;
 
-	if (msm_host->power_on) {
+	if (msm_host->power_on && msm_host->enabled) {
 		dsi_wait4video_done(msm_host);
 		/* delay 4 ms to skip BLLP */

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

 		usleep_range(2000, 4000);

@@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct
 mipi_dsi_host *host)
 	 *	pm_runtime_put_autosuspend(&msm_host->pdev->dev);
 	 * }
 	 */
-
+	msm_host->enabled = true;
 	return 0;
 }

drivers/gpu/drm/msm/dsi/dsi_host.c int msm_dsi_host_disable(struct · line 2230

Quoted 6 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

@@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct
 mipi_dsi_host *host)
 	 * Reset to disable video engine so that we can send off cmd.
 	 */
 	dsi_sw_reset(msm_host);
-
+	msm_host->enabled = false;

This should go at the start of the function. Also, it's unclear from this patch,
but I assume this is protected by a lock?

Quoted 1 line

Sean

Quoted 8 lines

[Abhinav] Yes, will move this to the start.
No, there is no lock here but at this point doesnt need one. The reason is that, this variable will be written to and read by the same process
(suspend thread OR resume thread which sends the panel ON/OFF commands). If we decide to expose other interfaces to send commands like debugfs or sysfs and
introduce more threads, we will add the locking.

[Abhinav] Correction to my prev comment, we do have the msm_host->cmd_mutex which will
ensure this entire process is protected. That should suffice.

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +0 -1

 	return 0;
 }
 
--

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

Quoted 4 lines

Quoted 4 lines

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 100 lines · On Fri, Apr 13, 2018 at 03:04:48PM -0700, [email protected] wrote:

Quoted 99 lines · On 2018-04-13 14:10, [email protected] wrote:

Hi Sean

Thanks for the review.

Reply inline.

Quoted 83 lines · Sean Paul

Quoted 71 lines · Abhinav Kumar

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Changes in v2:
- Replace pr_err with dev_err
- Changed error message

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +9 -2

index 7a03a94..5b7b290 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int
 mode, struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+	struct device *dev = &msm_host->pdev->dev;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
 
 	reinit_completion(&msm_host->video_comp);
 
-	wait_for_completion_timeout(&msm_host->video_comp,
+	ret = wait_for_completion_timeout(&msm_host->video_comp,
 			msecs_to_jiffies(70));
 
+	if (ret <= 0)
+		dev_err(dev, "wait for video done timed out\n");
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }

@@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct
 msm_dsi_host *msm_host)
 	if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
 		return;
 
-	if (msm_host->power_on) {
+	if (msm_host->power_on && msm_host->enabled) {
 		dsi_wait4video_done(msm_host);
 		/* delay 4 ms to skip BLLP */

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

 		usleep_range(2000, 4000);

@@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct
 mipi_dsi_host *host)
 	 *	pm_runtime_put_autosuspend(&msm_host->pdev->dev);
 	 * }
 	 */
-
+	msm_host->enabled = true;
 	return 0;
 }

drivers/gpu/drm/msm/dsi/dsi_host.c int msm_dsi_host_disable(struct · line 2230

Quoted 6 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

@@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct
 mipi_dsi_host *host)
 	 * Reset to disable video engine so that we can send off cmd.
 	 */
 	dsi_sw_reset(msm_host);
-
+	msm_host->enabled = false;

This should go at the start of the function. Also, it's unclear from this patch,
but I assume this is protected by a lock?

Quoted 1 line

Sean

[Abhinav] Yes, will move this to the start.
No, there is no lock here but at this point doesnt need one. The reason is that, this variable will be written to and read by the same process
(suspend thread OR resume thread which sends the panel ON/OFF commands). If we decide to expose other interfaces to send commands like debugfs or sysfs and
introduce more threads, we will add the locking.

Quoted 3 lines

[Abhinav] Correction to my prev comment, we do have the msm_host->cmd_mutex which will
ensure this entire process is protected. That should suffice.

Ok, thanks for confirming. Could you also please split this patch into the wait4video_done ret fix and the ->enabled addition? The 2 seem mostly unrelated.

Sean

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +0 -1

 	return 0;
 }
 
--

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

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Quoted 5 lines

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Sean

Thanks for reviewing.

Reply inline.

Quoted 111 lines · Sean Paul

Quoted 100 lines · On Fri, Apr 13, 2018 at 03:04:48PM -0700, [email protected] wrote:

Quoted 91 lines · On 2018-04-13 14:10, [email protected] wrote:

Hi Sean

Thanks for the review.

Reply inline.

Quoted 83 lines · Sean Paul

Quoted 71 lines · Abhinav Kumar

Make sure the video mode engine is on before waiting for the video done interrupt.

Otherwise it leads to silent timeouts increasing display turn ON time.

Changes in v2:
- Replace pr_err with dev_err
- Changed error message

Signed-off-by Abhinav Kumar <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +9 -2

index 7a03a94..5b7b290 100644
@@ -173,6 +173,7 @@ struct msm_dsi_host {
 
 	bool registered;
 	bool power_on;
+	bool enabled;
 	int irq;
 };
 

@@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int
 mode, struct msm_dsi_host *msm_host)
 
 static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
 {
+	u32 ret = 0;
+	struct device *dev = &msm_host->pdev->dev;
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
 
 	reinit_completion(&msm_host->video_comp);
 
-	wait_for_completion_timeout(&msm_host->video_comp,
+	ret = wait_for_completion_timeout(&msm_host->video_comp,
 			msecs_to_jiffies(70));
 
+	if (ret <= 0)
+		dev_err(dev, "wait for video done timed out\n");
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }

@@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct
 msm_dsi_host *msm_host)
 	if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
 		return;
 
-	if (msm_host->power_on) {
+	if (msm_host->power_on && msm_host->enabled) {
 		dsi_wait4video_done(msm_host);
 		/* delay 4 ms to skip BLLP */

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

 		usleep_range(2000, 4000);

@@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct
 mipi_dsi_host *host)
 	 *	pm_runtime_put_autosuspend(&msm_host->pdev->dev);
 	 * }
 	 */
-
+	msm_host->enabled = true;
 	return 0;
 }

drivers/gpu/drm/msm/dsi/dsi_host.c int msm_dsi_host_disable(struct · line 2230

Quoted 6 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

@@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct
 mipi_dsi_host *host)
 	 * Reset to disable video engine so that we can send off cmd.
 	 */
 	dsi_sw_reset(msm_host);
-
+	msm_host->enabled = false;

This should go at the start of the function. Also, it's unclear from this patch,
but I assume this is protected by a lock?

Quoted 1 line

Sean

Quoted 8 lines

[Abhinav] Yes, will move this to the start.
No, there is no lock here but at this point doesnt need one. The reason is that, this variable will be written to and read by the same process
(suspend thread OR resume thread which sends the panel ON/OFF commands). If we decide to expose other interfaces to send commands like debugfs or sysfs and
introduce more threads, we will add the locking.

Quoted 4 lines

[Abhinav] Correction to my prev comment, we do have the msm_host->cmd_mutex
which will
ensure this entire process is protected. That should suffice.

Ok, thanks for confirming. Could you also please split this patch into the
wait4video_done ret fix and the ->enabled addition? The 2 seem mostly unrelated.

Quoted 1 line

Sean

They are quite related actually. So we were not able to catch this earlier because the wait was silently timing out. Hence along with the fix I wanted to change the ret to add the error log because such condition should not happen with the ->enabled fix. To signify that I clubbed them together. I can split them, if this still feels unrelated.

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/msm/dsi/dsi_host.c +0 -1

 	return 0;
 }
 
--

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

Quoted 4 lines

Quoted 4 lines

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html