[DPU PATCH 1/2] drm/msm/dsi: check video mode engine status before waiting

[DPU PATCH 1/2] drm/msm/dsi: check video mode engine status before waiting

[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 <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 7a03a94..24766c3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -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);
 
 	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");
+
 	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
 }
 
@@ -1001,7 +1007,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 +2209,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;
 }
 
@@ -2219,7 +2225,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;
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[DPU PATCH 2/2] drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY

[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.

Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 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(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 8e9d5c2..5b42885 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -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)
 {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index c56268c..a24ab80 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -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);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 0af951a..b3fffc8 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -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)
 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [DPU PATCH 2/2] drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY

[Archit Taneja]

On Saturday 07 April 2018 01:20 PM, Abhinav Kumar wrote:
> 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.

Reviewed-by: Archit Taneja <architt@codeaurora.org>

> 
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>   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(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 8e9d5c2..5b42885 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -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)
>   {
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> index c56268c..a24ab80 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> @@ -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);
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> index 0af951a..b3fffc8 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> @@ -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)
>   {
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [DPU PATCH 1/2] drm/msm/dsi: check video mode engine status before waiting

[Jordan Crouse]

On Sat, Apr 07, 2018 at 12:50:04AM -0700, Abhinav Kumar wrote:
> 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 <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 7a03a94..24766c3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -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);
>  
>  	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
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [DPU PATCH 1/2] drm/msm/dsi: check video mode engine status before waiting

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-04-09 08:28, Jordan Crouse wrote:
> On Sat, Apr 07, 2018 at 12:50:04AM -0700, Abhinav Kumar wrote:
>> 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 <abhinavk@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 7a03a94..24766c3 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -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);
>> 
>>  	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
[Abhinav] With the rest of this change, this error should not happen.
This will print whenever we need to call this for waiting for each panel 
command.
The number of panel commands can vary with panel from < 10 to 100s at 
times.
But since we do not expect this to happen at all, at this point i dont 
think we need
to rate limit this.

Yes, will replace the pr_err with dev_err in V2.
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[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 <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 7a03a94..5b7b290 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -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;
 }
 
@@ -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;
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[DPU PATCH v2 2/2] drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY

[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.

Changes in v2:
- None

Reviewed-by: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 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(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 8e9d5c2..5b42885 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -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)
 {
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index c56268c..a24ab80 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -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);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 0af951a..b3fffc8 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -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)
 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[Sean Paul]

On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
> 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 <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 7a03a94..5b7b290 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -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;
>  }
>  
> @@ -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


>  	return 0;
>  }
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 2/2] drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY

[Sean Paul]

On Tue, Apr 10, 2018 at 06:54:07PM -0700, Abhinav Kumar wrote:
> 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?

Sean

> 
> Changes in v2:
> - None
> 
> Reviewed-by: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 8e9d5c2..5b42885 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -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)
>  {
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> index c56268c..a24ab80 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> @@ -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);
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> index 0af951a..b3fffc8 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> @@ -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)
>  {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 2/2] drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-04-13 13:29, Sean Paul wrote:
> On Tue, Apr 10, 2018 at 06:54:07PM -0700, Abhinav Kumar wrote:
>> 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?
> 
> 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.
>> 
>> Changes in v2:
>> - None
>> 
>> Reviewed-by: Archit Taneja <architt@codeaurora.org>
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>  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(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c 
>> b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
>> index 8e9d5c2..5b42885 100644
>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
>> @@ -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)
>>  {
>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h 
>> b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
>> index c56268c..a24ab80 100644
>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
>> @@ -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);
>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c 
>> b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
>> index 0af951a..b3fffc8 100644
>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
>> @@ -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)
>>  {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[abhinavk]

Hi Sean

Thanks for the review.

Reply inline.

On 2018-04-13 13:26, Sean Paul wrote:
> On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
>> 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 <abhinavk@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 7a03a94..5b7b290 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -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;
>>  }
>> 
>> @@ -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
[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.
> 
> 
>>  	return 0;
>>  }
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-04-13 14:10, abhinavk@codeaurora.org wrote:
> Hi Sean
> 
> Thanks for the review.
> 
> Reply inline.
> 
> On 2018-04-13 13:26, Sean Paul wrote:
>> On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
>>> 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 <abhinavk@codeaurora.org>
>>> ---
>>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 7a03a94..5b7b290 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -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;
>>>  }
>>> 
>>> @@ -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
> [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.
>> 
>> 
>>>  	return 0;
>>>  }
>>> 
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>> 
>>> _______________________________________________
>>> Freedreno mailing list
>>> Freedreno@lists.freedesktop.org
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 2/2] drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY

[Sean Paul]

On Fri, Apr 13, 2018 at 01:52:17PM -0700, abhinavk@codeaurora.org wrote:
> On 2018-04-13 13:29, Sean Paul wrote:
> > On Tue, Apr 10, 2018 at 06:54:07PM -0700, Abhinav Kumar wrote:
> > > 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?
> > 
> > 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.

Ahh, ok :(

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> > > 
> > > Changes in v2:
> > > - None
> > > 
> > > Reviewed-by: Archit Taneja <architt@codeaurora.org>
> > > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> > > ---
> > >  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(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > index 8e9d5c2..5b42885 100644
> > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > > @@ -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)
> > >  {
> > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > index c56268c..a24ab80 100644
> > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > @@ -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);
> > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > index 0af951a..b3fffc8 100644
> > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > @@ -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)
> > >  {
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project
> > > 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[Sean Paul]

On Fri, Apr 13, 2018 at 03:04:48PM -0700, abhinavk@codeaurora.org wrote:
> On 2018-04-13 14:10, abhinavk@codeaurora.org wrote:
> > Hi Sean
> > 
> > Thanks for the review.
> > 
> > Reply inline.
> > 
> > On 2018-04-13 13:26, Sean Paul wrote:
> > > On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
> > > > 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 <abhinavk@codeaurora.org>
> > > > ---
> > > >  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > index 7a03a94..5b7b290 100644
> > > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > @@ -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;
> > > >  }
> > > > 
> > > > @@ -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
> > [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.

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

> > > 
> > > 
> > > >  	return 0;
> > > >  }
> > > > 
> > > > --
> > > > The Qualcomm Innovation Center, Inc. is a member of the Code
> > > > Aurora Forum,
> > > > a Linux Foundation Collaborative Project
> > > > 
> > > > _______________________________________________
> > > > Freedreno mailing list
> > > > Freedreno@lists.freedesktop.org
> > > > 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ]

Hi Sean

Thanks for reviewing.

Reply inline.

On 2018-04-16 10:07, Sean Paul wrote:
> On Fri, Apr 13, 2018 at 03:04:48PM -0700, abhinavk@codeaurora.org 
> wrote:
>> On 2018-04-13 14:10, abhinavk@codeaurora.org wrote:
>> > Hi Sean
>> >
>> > Thanks for the review.
>> >
>> > Reply inline.
>> >
>> > On 2018-04-13 13:26, Sean Paul wrote:
>> > > On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
>> > > > 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 <abhinavk@codeaurora.org>
>> > > > ---
>> > > >  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
>> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > > > b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > > > index 7a03a94..5b7b290 100644
>> > > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> > > > @@ -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;
>> > > >  }
>> > > >
>> > > > @@ -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
>> > [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.
> 
> 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

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.

>> > >
>> > >
>> > > >  	return 0;
>> > > >  }
>> > > >
>> > > > --
>> > > > The Qualcomm Innovation Center, Inc. is a member of the Code
>> > > > Aurora Forum,
>> > > > a Linux Foundation Collaborative Project
>> > > >
>> > > > _______________________________________________
>> > > > Freedreno mailing list
>> > > > Freedreno@lists.freedesktop.org
>> > > > 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 majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[Sean Paul]

On Mon, Apr 16, 2018 at 10:44:57AM -0700, abhinavk@codeaurora.org wrote:
> Hi Sean
> 
> Thanks for reviewing.
> 
> Reply inline.
> 
> On 2018-04-16 10:07, Sean Paul wrote:
> > On Fri, Apr 13, 2018 at 03:04:48PM -0700, abhinavk@codeaurora.org wrote:
> > > On 2018-04-13 14:10, abhinavk@codeaurora.org wrote:
> > > > Hi Sean
> > > >
> > > > Thanks for the review.
> > > >
> > > > Reply inline.
> > > >
> > > > On 2018-04-13 13:26, Sean Paul wrote:
> > > > > On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
> > > > > > 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 <abhinavk@codeaurora.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
> > > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > > > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > > > index 7a03a94..5b7b290 100644
> > > > > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > > > > @@ -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;
> > > > > >  }
> > > > > >
> > > > > > @@ -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
> > > > [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.
> > 
> > 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
> 
> 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.

Thanks for the explanation, they're unrelated in the sense that you're fixing 2
things in this patch. The first fix is to not fail silently, and the second fix
is to ensure the engine is on before waiting for the interrupt. So IMO, this is
2 patches.

Sean

> 
> > > > >
> > > > >
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > --
> > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code
> > > > > > Aurora Forum,
> > > > > > a Linux Foundation Collaborative Project
> > > > > >
> > > > > > _______________________________________________
> > > > > > Freedreno mailing list
> > > > > > Freedreno@lists.freedesktop.org
> > > > > > 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 majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno