[PATCH 1/2] drm/i915/BXT: Retrieving the horizontal timing for DSI

[PATCH 1/2] drm/i915/BXT: Retrieving the horizontal timing for DSI

[Ramalingam C]

Retriving the horizontal timings from the port registers as
part of get_config()

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c |   44 ++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 9ff6435..e0259e6 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -46,6 +46,14 @@ static const struct {
 	},
 };
 
+/* return pixels equvalent to txbyteclkhs */
+static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
+			u16 burst_mode_ratio)
+{
+	return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100),
+						(bpp * burst_mode_ratio));
+}
+
 enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt)
 {
 	/* It just so happens the VBT matches register contents. */
@@ -772,9 +780,10 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
 	struct drm_display_mode *adjusted_mode =
 					&pipe_config->base.adjusted_mode;
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	unsigned int lane_count = intel_dsi->lane_count;
 	unsigned int bpp, fmt;
 	enum port port;
-	u16 vfp, vsync, vbp;
+	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
 
 	/*
 	 * Atleast one port is active as encoder->get_config called only if
@@ -799,22 +808,43 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
 	adjusted_mode->crtc_vtotal =
 				I915_READ(BXT_MIPI_TRANS_VTOTAL(port));
 
+	hactive = adjusted_mode->crtc_hdisplay;
+	hfp = I915_READ(MIPI_HFP_COUNT(port));
+
 	/*
-	 * TODO: Retrieve hfp, hsync and hbp. Adjust them for dual link and
-	 * calculate hsync_start, hsync_end, htotal and hblank_end
+	 * Meaningful for video mode non-burst sync pulse mode only,
+	 * can be zero for non-burst sync events and burst modes
 	 */
+	hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port));
+	hbp = I915_READ(MIPI_HBP_COUNT(port));
+
+	/* harizontal values are in terms of high speed byte clock */
+	hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+	hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+	hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+
+	if (intel_dsi->dual_link) {
+		hfp *= 2;
+		hsync *= 2;
+		hbp *= 2;
+	}
 
 	/* vertical values are in terms of lines */
 	vfp = I915_READ(MIPI_VFP_COUNT(port));
 	vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port));
 	vbp = I915_READ(MIPI_VBP_COUNT(port));
 
+	adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp;
+	adjusted_mode->crtc_hsync_start = hfp + adjusted_mode->crtc_hdisplay;
+	adjusted_mode->crtc_hsync_end = hsync + adjusted_mode->crtc_hsync_start;
 	adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay;
+	adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal;
 
-	adjusted_mode->crtc_vsync_start =
-				vfp + adjusted_mode->crtc_vdisplay;
-	adjusted_mode->crtc_vsync_end =
-				vsync + adjusted_mode->crtc_vsync_start;
+	adjusted_mode->crtc_vsync_start = vfp + adjusted_mode->crtc_vdisplay;
+	adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
 	adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
 	adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
 }
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH 2/2] drm/i915/bxt: Adjusting the error in horizontal timings retrieval

[Ramalingam C]

In BXT DSI there is no regs programmed with few horizontal timings
in Pixels but txbyteclkhs.. So retrieval process adds some
ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.

Actually here for the given adjusted_mode, we are calculating the
value programmed to the port and then back to the horizontal timing
param in pixels. This is the expected value at the end of get_config,
including roundup errors. And if that is same as retrieved value
from port, then retrieved (HW state) adjusted_mode's horizontal
timings are corrected to match with SW state to nullify the errors.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c |   97 ++++++++++++++++++++++++++++++++++----
 1 file changed, 88 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index e0259e6..8a1b872 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -46,6 +46,14 @@ static const struct {
 	},
 };
 
+/* return pixels in terms of txbyteclkhs */
+static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
+		       u16 burst_mode_ratio)
+{
+	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
+					 8 * 100), lane_count);
+}
+
 /* return pixels equvalent to txbyteclkhs */
 static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
 			u16 burst_mode_ratio)
@@ -779,11 +787,19 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *adjusted_mode =
 					&pipe_config->base.adjusted_mode;
+	struct drm_display_mode *adjusted_mode_sw;
+	struct intel_crtc *intel_crtc;
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	unsigned int lane_count = intel_dsi->lane_count;
 	unsigned int bpp, fmt;
 	enum port port;
 	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
+	u16 hfp_sw, hsync_sw, hbp_sw;
+	u16 crtc_htotal_sw, crtc_hsync_start_sw, crtc_hsync_end_sw,
+				crtc_hblank_start_sw, crtc_hblank_end_sw;
+
+	intel_crtc = to_intel_crtc(encoder->base.crtc);
+	adjusted_mode_sw = &intel_crtc->config->base.adjusted_mode;
 
 	/*
 	 * Atleast one port is active as encoder->get_config called only if
@@ -847,8 +863,79 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
 	adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
 	adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
 	adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
-}
 
+	/*
+	 * In BXT DSI there is no regs programmed with few horizontal timings
+	 * in Pixels but txbyteclkhs.. So retrieval process adds some
+	 * ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
+	 * Actually here for the given adjusted_mode, we are calculating the
+	 * value programmed to the port and then back to the horizontal timing
+	 * param in pixels. This is the expected value, including roundup errors
+	 * And if that is same as retrieved value from port, then
+	 * (HW state) adjusted_mode's horizontal timings are corrected to
+	 * match with SW state to nullify the errors.
+	 */
+	/* Calculating the value programmed to the Port register */
+	hfp_sw = adjusted_mode_sw->crtc_hsync_start -
+					adjusted_mode_sw->crtc_hdisplay;
+	hsync_sw = adjusted_mode_sw->crtc_hsync_end -
+					adjusted_mode_sw->crtc_hsync_start;
+	hbp_sw = adjusted_mode_sw->crtc_htotal -
+					adjusted_mode_sw->crtc_hsync_end;
+
+	if (intel_dsi->dual_link) {
+		hfp_sw /= 2;
+		hsync_sw /= 2;
+		hbp_sw /= 2;
+	}
+
+	hfp_sw = txbyteclkhs(hfp_sw, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+	hsync_sw = txbyteclkhs(hsync_sw, bpp, lane_count,
+			    intel_dsi->burst_mode_ratio);
+	hbp_sw = txbyteclkhs(hbp_sw, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+
+	/* Reverse calculating the adjusted mode parameters from port reg vals*/
+	hfp_sw = pixels_from_txbyteclkhs(hfp_sw, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+	hsync_sw = pixels_from_txbyteclkhs(hsync_sw, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+	hbp_sw = pixels_from_txbyteclkhs(hbp_sw, bpp, lane_count,
+						intel_dsi->burst_mode_ratio);
+
+	if (intel_dsi->dual_link) {
+		hfp_sw *= 2;
+		hsync_sw *= 2;
+		hbp_sw *= 2;
+	}
+
+	crtc_htotal_sw = adjusted_mode_sw->crtc_hdisplay + hfp_sw +
+							hsync_sw + hbp_sw;
+	crtc_hsync_start_sw = hfp_sw + adjusted_mode_sw->crtc_hdisplay;
+	crtc_hsync_end_sw = hsync_sw + crtc_hsync_start_sw;
+	crtc_hblank_start_sw = adjusted_mode_sw->crtc_hdisplay;
+	crtc_hblank_end_sw = crtc_htotal_sw;
+
+	if (adjusted_mode->crtc_htotal == crtc_htotal_sw)
+		adjusted_mode->crtc_htotal = adjusted_mode_sw->crtc_htotal;
+
+	if (adjusted_mode->crtc_hsync_start == crtc_hsync_start_sw)
+		adjusted_mode->crtc_hsync_start =
+					adjusted_mode_sw->crtc_hsync_start;
+
+	if (adjusted_mode->crtc_hsync_end == crtc_hsync_end_sw)
+		adjusted_mode->crtc_hsync_end =
+					adjusted_mode_sw->crtc_hsync_end;
+
+	if (adjusted_mode->crtc_hblank_start == crtc_hblank_start_sw)
+		adjusted_mode->crtc_hblank_start =
+					adjusted_mode_sw->crtc_hblank_start;
+
+	if (adjusted_mode->crtc_hblank_end == crtc_hblank_end_sw)
+		adjusted_mode->crtc_hblank_end =
+					adjusted_mode_sw->crtc_hblank_end;
+}
 
 static void intel_dsi_get_config(struct intel_encoder *encoder,
 				 struct intel_crtc_state *pipe_config)
@@ -917,14 +1004,6 @@ static u16 txclkesc(u32 divider, unsigned int us)
 	}
 }
 
-/* return pixels in terms of txbyteclkhs */
-static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
-		       u16 burst_mode_ratio)
-{
-	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
-					 8 * 100), lane_count);
-}
-
 static void set_dsi_timings(struct drm_encoder *encoder,
 			    const struct drm_display_mode *adjusted_mode)
 {
-- 
1.7.9.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 2/2] drm/i915/bxt: Adjusting the error in horizontal timings retrieval

[Daniel Vetter]

On Tue, Apr 19, 2016 at 01:48:14PM +0530, Ramalingam C wrote:
> In BXT DSI there is no regs programmed with few horizontal timings
> in Pixels but txbyteclkhs.. So retrieval process adds some
> ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
> 
> Actually here for the given adjusted_mode, we are calculating the
> value programmed to the port and then back to the horizontal timing
> param in pixels. This is the expected value at the end of get_config,
> including roundup errors. And if that is same as retrieved value
> from port, then retrieved (HW state) adjusted_mode's horizontal
> timings are corrected to match with SW state to nullify the errors.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>

I didn't check the details, but I think this is the fix we want. Still
needs in-depth review, and some testing on a few different dsi bxt
machines just to make sure I think, but on both patches:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/intel_dsi.c |   97 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 88 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index e0259e6..8a1b872 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -46,6 +46,14 @@ static const struct {
>  	},
>  };
>  
> +/* return pixels in terms of txbyteclkhs */
> +static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
> +		       u16 burst_mode_ratio)
> +{
> +	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
> +					 8 * 100), lane_count);
> +}
> +
>  /* return pixels equvalent to txbyteclkhs */
>  static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
>  			u16 burst_mode_ratio)
> @@ -779,11 +787,19 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *adjusted_mode =
>  					&pipe_config->base.adjusted_mode;
> +	struct drm_display_mode *adjusted_mode_sw;
> +	struct intel_crtc *intel_crtc;
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	unsigned int lane_count = intel_dsi->lane_count;
>  	unsigned int bpp, fmt;
>  	enum port port;
>  	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
> +	u16 hfp_sw, hsync_sw, hbp_sw;
> +	u16 crtc_htotal_sw, crtc_hsync_start_sw, crtc_hsync_end_sw,
> +				crtc_hblank_start_sw, crtc_hblank_end_sw;
> +
> +	intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	adjusted_mode_sw = &intel_crtc->config->base.adjusted_mode;
>  
>  	/*
>  	 * Atleast one port is active as encoder->get_config called only if
> @@ -847,8 +863,79 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>  	adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
>  	adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
>  	adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
> -}
>  
> +	/*
> +	 * In BXT DSI there is no regs programmed with few horizontal timings
> +	 * in Pixels but txbyteclkhs.. So retrieval process adds some
> +	 * ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
> +	 * Actually here for the given adjusted_mode, we are calculating the
> +	 * value programmed to the port and then back to the horizontal timing
> +	 * param in pixels. This is the expected value, including roundup errors
> +	 * And if that is same as retrieved value from port, then
> +	 * (HW state) adjusted_mode's horizontal timings are corrected to
> +	 * match with SW state to nullify the errors.
> +	 */
> +	/* Calculating the value programmed to the Port register */
> +	hfp_sw = adjusted_mode_sw->crtc_hsync_start -
> +					adjusted_mode_sw->crtc_hdisplay;
> +	hsync_sw = adjusted_mode_sw->crtc_hsync_end -
> +					adjusted_mode_sw->crtc_hsync_start;
> +	hbp_sw = adjusted_mode_sw->crtc_htotal -
> +					adjusted_mode_sw->crtc_hsync_end;
> +
> +	if (intel_dsi->dual_link) {
> +		hfp_sw /= 2;
> +		hsync_sw /= 2;
> +		hbp_sw /= 2;
> +	}
> +
> +	hfp_sw = txbyteclkhs(hfp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hsync_sw = txbyteclkhs(hsync_sw, bpp, lane_count,
> +			    intel_dsi->burst_mode_ratio);
> +	hbp_sw = txbyteclkhs(hbp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +
> +	/* Reverse calculating the adjusted mode parameters from port reg vals*/
> +	hfp_sw = pixels_from_txbyteclkhs(hfp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hsync_sw = pixels_from_txbyteclkhs(hsync_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hbp_sw = pixels_from_txbyteclkhs(hbp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +
> +	if (intel_dsi->dual_link) {
> +		hfp_sw *= 2;
> +		hsync_sw *= 2;
> +		hbp_sw *= 2;
> +	}
> +
> +	crtc_htotal_sw = adjusted_mode_sw->crtc_hdisplay + hfp_sw +
> +							hsync_sw + hbp_sw;
> +	crtc_hsync_start_sw = hfp_sw + adjusted_mode_sw->crtc_hdisplay;
> +	crtc_hsync_end_sw = hsync_sw + crtc_hsync_start_sw;
> +	crtc_hblank_start_sw = adjusted_mode_sw->crtc_hdisplay;
> +	crtc_hblank_end_sw = crtc_htotal_sw;
> +
> +	if (adjusted_mode->crtc_htotal == crtc_htotal_sw)
> +		adjusted_mode->crtc_htotal = adjusted_mode_sw->crtc_htotal;
> +
> +	if (adjusted_mode->crtc_hsync_start == crtc_hsync_start_sw)
> +		adjusted_mode->crtc_hsync_start =
> +					adjusted_mode_sw->crtc_hsync_start;
> +
> +	if (adjusted_mode->crtc_hsync_end == crtc_hsync_end_sw)
> +		adjusted_mode->crtc_hsync_end =
> +					adjusted_mode_sw->crtc_hsync_end;
> +
> +	if (adjusted_mode->crtc_hblank_start == crtc_hblank_start_sw)
> +		adjusted_mode->crtc_hblank_start =
> +					adjusted_mode_sw->crtc_hblank_start;
> +
> +	if (adjusted_mode->crtc_hblank_end == crtc_hblank_end_sw)
> +		adjusted_mode->crtc_hblank_end =
> +					adjusted_mode_sw->crtc_hblank_end;
> +}
>  
>  static void intel_dsi_get_config(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *pipe_config)
> @@ -917,14 +1004,6 @@ static u16 txclkesc(u32 divider, unsigned int us)
>  	}
>  }
>  
> -/* return pixels in terms of txbyteclkhs */
> -static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
> -		       u16 burst_mode_ratio)
> -{
> -	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
> -					 8 * 100), lane_count);
> -}
> -
>  static void set_dsi_timings(struct drm_encoder *encoder,
>  			    const struct drm_display_mode *adjusted_mode)
>  {
> -- 
> 1.7.9.5

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 2/2] drm/i915/bxt: Adjusting the error in horizontal timings retrieval

[Ramalingam C]

On Wednesday 20 April 2016 06:56 PM, Daniel Vetter wrote:
> On Tue, Apr 19, 2016 at 01:48:14PM +0530, Ramalingam C wrote:
>> In BXT DSI there is no regs programmed with few horizontal timings
>> in Pixels but txbyteclkhs.. So retrieval process adds some
>> ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
>>
>> Actually here for the given adjusted_mode, we are calculating the
>> value programmed to the port and then back to the horizontal timing
>> param in pixels. This is the expected value at the end of get_config,
>> including roundup errors. And if that is same as retrieved value
>> from port, then retrieved (HW state) adjusted_mode's horizontal
>> timings are corrected to match with SW state to nullify the errors.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> I didn't check the details, but I think this is the fix we want. Still
> needs in-depth review, and some testing on a few different dsi bxt
> machines just to make sure I think, but on both patches:
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Daniel and Jani,

I have tested this on BXT-T B1 board with Tianma panel. This is working 
fine w.r.t PIPE_CONFIG COMPARE.
Not sure whether i can give tested-by for my own patch. :)

--Ram
>
> Cheers, Daniel
>> ---
>>   drivers/gpu/drm/i915/intel_dsi.c |   97 ++++++++++++++++++++++++++++++++++----
>>   1 file changed, 88 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index e0259e6..8a1b872 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -46,6 +46,14 @@ static const struct {
>>   	},
>>   };
>>   
>> +/* return pixels in terms of txbyteclkhs */
>> +static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
>> +		       u16 burst_mode_ratio)
>> +{
>> +	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>> +					 8 * 100), lane_count);
>> +}
>> +
>>   /* return pixels equvalent to txbyteclkhs */
>>   static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
>>   			u16 burst_mode_ratio)
>> @@ -779,11 +787,19 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	struct drm_display_mode *adjusted_mode =
>>   					&pipe_config->base.adjusted_mode;
>> +	struct drm_display_mode *adjusted_mode_sw;
>> +	struct intel_crtc *intel_crtc;
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	unsigned int lane_count = intel_dsi->lane_count;
>>   	unsigned int bpp, fmt;
>>   	enum port port;
>>   	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
>> +	u16 hfp_sw, hsync_sw, hbp_sw;
>> +	u16 crtc_htotal_sw, crtc_hsync_start_sw, crtc_hsync_end_sw,
>> +				crtc_hblank_start_sw, crtc_hblank_end_sw;
>> +
>> +	intel_crtc = to_intel_crtc(encoder->base.crtc);
>> +	adjusted_mode_sw = &intel_crtc->config->base.adjusted_mode;
>>   
>>   	/*
>>   	 * Atleast one port is active as encoder->get_config called only if
>> @@ -847,8 +863,79 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>>   	adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
>>   	adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
>>   	adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
>> -}
>>   
>> +	/*
>> +	 * In BXT DSI there is no regs programmed with few horizontal timings
>> +	 * in Pixels but txbyteclkhs.. So retrieval process adds some
>> +	 * ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
>> +	 * Actually here for the given adjusted_mode, we are calculating the
>> +	 * value programmed to the port and then back to the horizontal timing
>> +	 * param in pixels. This is the expected value, including roundup errors
>> +	 * And if that is same as retrieved value from port, then
>> +	 * (HW state) adjusted_mode's horizontal timings are corrected to
>> +	 * match with SW state to nullify the errors.
>> +	 */
>> +	/* Calculating the value programmed to the Port register */
>> +	hfp_sw = adjusted_mode_sw->crtc_hsync_start -
>> +					adjusted_mode_sw->crtc_hdisplay;
>> +	hsync_sw = adjusted_mode_sw->crtc_hsync_end -
>> +					adjusted_mode_sw->crtc_hsync_start;
>> +	hbp_sw = adjusted_mode_sw->crtc_htotal -
>> +					adjusted_mode_sw->crtc_hsync_end;
>> +
>> +	if (intel_dsi->dual_link) {
>> +		hfp_sw /= 2;
>> +		hsync_sw /= 2;
>> +		hbp_sw /= 2;
>> +	}
>> +
>> +	hfp_sw = txbyteclkhs(hfp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +	hsync_sw = txbyteclkhs(hsync_sw, bpp, lane_count,
>> +			    intel_dsi->burst_mode_ratio);
>> +	hbp_sw = txbyteclkhs(hbp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +
>> +	/* Reverse calculating the adjusted mode parameters from port reg vals*/
>> +	hfp_sw = pixels_from_txbyteclkhs(hfp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +	hsync_sw = pixels_from_txbyteclkhs(hsync_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +	hbp_sw = pixels_from_txbyteclkhs(hbp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +
>> +	if (intel_dsi->dual_link) {
>> +		hfp_sw *= 2;
>> +		hsync_sw *= 2;
>> +		hbp_sw *= 2;
>> +	}
>> +
>> +	crtc_htotal_sw = adjusted_mode_sw->crtc_hdisplay + hfp_sw +
>> +							hsync_sw + hbp_sw;
>> +	crtc_hsync_start_sw = hfp_sw + adjusted_mode_sw->crtc_hdisplay;
>> +	crtc_hsync_end_sw = hsync_sw + crtc_hsync_start_sw;
>> +	crtc_hblank_start_sw = adjusted_mode_sw->crtc_hdisplay;
>> +	crtc_hblank_end_sw = crtc_htotal_sw;
>> +
>> +	if (adjusted_mode->crtc_htotal == crtc_htotal_sw)
>> +		adjusted_mode->crtc_htotal = adjusted_mode_sw->crtc_htotal;
>> +
>> +	if (adjusted_mode->crtc_hsync_start == crtc_hsync_start_sw)
>> +		adjusted_mode->crtc_hsync_start =
>> +					adjusted_mode_sw->crtc_hsync_start;
>> +
>> +	if (adjusted_mode->crtc_hsync_end == crtc_hsync_end_sw)
>> +		adjusted_mode->crtc_hsync_end =
>> +					adjusted_mode_sw->crtc_hsync_end;
>> +
>> +	if (adjusted_mode->crtc_hblank_start == crtc_hblank_start_sw)
>> +		adjusted_mode->crtc_hblank_start =
>> +					adjusted_mode_sw->crtc_hblank_start;
>> +
>> +	if (adjusted_mode->crtc_hblank_end == crtc_hblank_end_sw)
>> +		adjusted_mode->crtc_hblank_end =
>> +					adjusted_mode_sw->crtc_hblank_end;
>> +}
>>   
>>   static void intel_dsi_get_config(struct intel_encoder *encoder,
>>   				 struct intel_crtc_state *pipe_config)
>> @@ -917,14 +1004,6 @@ static u16 txclkesc(u32 divider, unsigned int us)
>>   	}
>>   }
>>   
>> -/* return pixels in terms of txbyteclkhs */
>> -static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
>> -		       u16 burst_mode_ratio)
>> -{
>> -	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>> -					 8 * 100), lane_count);
>> -}
>> -
>>   static void set_dsi_timings(struct drm_encoder *encoder,
>>   			    const struct drm_display_mode *adjusted_mode)
>>   {
>> -- 
>> 1.7.9.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/BXT: Retrieving the horizontal timing for DSI

[Imre Deak]

On ti, 2016-04-19 at 13:48 +0530, Ramalingam C wrote:
> Retriving the horizontal timings from the port registers as
> part of get_config()
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>

I saw the crash below with current -nightly on a BXT-M board and Jani
pointed me to these two patches which get rid of it, so
Acked-by: Imre Deak <imre.deak@intel.com>

[   56.916557] divide error: 0000 [#1] PREEMPT SMP 
[   56.921741] Modules linked in: i915(+) drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm intel_gtt agpgart cf
g80211 rfkill binfmt_misc ax88179_178a kvm_intel kvm irqbypass crc32c_intel efivars tpm_tis tpm fuse
[   56.944106] CPU: 3 PID: 1097 Comm: modprobe Not tainted 4.6.0-rc4+ #433
[   56.951501] Hardware name: Intel Corp. Broxton M/RVP, BIOS BXT1RVPA.X64.0131.B30.1604142217 04/14/2016
[   56.961908] task: ffff88007a854d00 ti: ffff88007aea0000 task.ti: ffff88007aea0000
[   56.970273] RIP: 0010:[<ffffffffa01235b2>]  [<ffffffffa01235b2>] drm_mode_hsync+0x22/0x40 [drm]
[   56.980043] RSP: 0018:ffff88007aea3788  EFLAGS: 00010206
[   56.985982] RAX: 000000000788b600 RBX: ffff880073c22108 RCX: 0000000000000000
[   56.993957] RDX: 0000000000000000 RSI: ffff88007ab06800 RDI: ffff880073c22108
[   57.001935] RBP: ffff88007aea3788 R08: 0000000000000001 R09: ffff880073c221e8
[   57.009903] R10: ffff880073c22108 R11: 0000000000000001 R12: ffff88007a300000
[   57.017872] R13: ffff880073c22000 R14: ffff880175f78000 R15: ffff880175f78798
[   57.025849] FS:  00007f105d3e6700(0000) GS:ffff88017fd80000(0000) knlGS:0000000000000000
[   57.034894] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   57.041317] CR2: 00007f4d485101d0 CR3: 000000007a820000 CR4: 00000000003406e0
[   57.049292] Stack:
[   57.051539]  ffff88007aea37a0 ffffffffa043b632 ffff880175f787c8 ffff88007aea3810
[   57.059825]  ffffffffa043d59e ffff880175f787b0 ffff88007ab68c00 ffff88007aea37f0
[   57.068128]  ffff880073c221e8 ffff880073c22108 ffff880175f78780 ffff880100000000
[   57.076430] Call Trace:
[   57.079254]  [<ffffffffa043b632>] intel_mode_from_pipe_config+0x82/0xb0 [i915]
[   57.087405]  [<ffffffffa043d59e>] intel_modeset_setup_hw_state+0x55e/0xd60 [i915]
[   57.095847]  [<ffffffffa043ff94>] intel_modeset_init+0x8e4/0x1630 [i915]
[   57.103415]  [<ffffffffa047bcf0>] i915_driver_load+0xbe0/0x1980 [i915]
[   57.110745]  [<ffffffffa0116c19>] drm_dev_register+0xa9/0xc0 [drm]
[   57.117681]  [<ffffffffa011921d>] drm_get_pci_dev+0x8d/0x1e0 [drm]
[   57.124600]  [<ffffffff8195f942>] ? _raw_spin_unlock_irqrestore+0x42/0x70
[   57.132253]  [<ffffffffa03b0384>] i915_pci_probe+0x34/0x50 [i915]
[   57.139070]  [<ffffffff8149c375>] local_pci_probe+0x45/0xa0
[   57.145303]  [<ffffffff8149d300>] ? pci_match_device+0xe0/0x110
[   57.151924]  [<ffffffff8149d6cb>] pci_device_probe+0xdb/0x130
[   57.158355]  [<ffffffff81579b93>] driver_probe_device+0x223/0x440
[   57.165169]  [<ffffffff81579e85>] __driver_attach+0xd5/0x100
[   57.171500]  [<ffffffff81579db0>] ? driver_probe_device+0x440/0x440
[   57.178510]  [<ffffffff81577736>] bus_for_each_dev+0x66/0xa0
[   57.184841]  [<ffffffff815793de>] driver_attach+0x1e/0x20
[   57.190881]  [<ffffffff81578d6e>] bus_add_driver+0x1ee/0x280
[   57.197212]  [<ffffffff8157abc0>] driver_register+0x60/0xe0
[   57.203447]  [<ffffffff8149bc50>] __pci_register_driver+0x60/0x70
[   57.210285]  [<ffffffffa0119450>] drm_pci_init+0xe0/0x110 [drm]
[   57.216911]  [<ffffffff810dcd8d>] ? trace_hardirqs_on+0xd/0x10
[   57.223434]  [<ffffffffa023a000>] ? 0xffffffffa023a000
[   57.229237]  [<ffffffffa023a092>] i915_init+0x92/0x99 [i915]
[   57.235570]  [<ffffffff810003db>] do_one_initcall+0xab/0x1d0
[   57.241900]  [<ffffffff810f9eef>] ? rcu_read_lock_sched_held+0x7f/0x90
[   57.249205]  [<ffffffff81204f18>] ? kmem_cache_alloc_trace+0x248/0x2b0
[   57.256509]  [<ffffffff811a5eee>] ? do_init_module+0x27/0x1d9
[   57.262934]  [<ffffffff811a5f26>] do_init_module+0x5f/0x1d9
[   57.269167]  [<ffffffff8112392f>] load_module+0x20ef/0x27b0
[   57.275401]  [<ffffffff8111f8e0>] ? store_uevent+0x40/0x40
[   57.281541]  [<ffffffff81124243>] SYSC_finit_module+0xc3/0xf0
[   57.287969]  [<ffffffff8112428e>] SyS_finit_module+0xe/0x10
[   57.294203]  [<ffffffff81960069>] entry_SYSCALL_64_fastpath+0x1c/0xac
[   57.301406] Code: ff 5d c3 66 0f 1f 44 00 00 0f 1f 44 00 00 8b 87 d8 00 00 00 55 48 89 e5 85 c0 75 22 8b 4f 68 85 c9 78 1b 69 47 58 e8 03 00 00 99 <f7> f9 b9 d3 4d 62 10 05 f4 01 00 00 f7 e1 89 d0 c1 e8 06 5d c3 
[   57.322964] RIP  [<ffffffffa01235b2>] drm_mode_hsync+0x22/0x40 [drm]
[   57.330103]  RSP <ffff88007aea3788>
[   57.334276] ---[ end trace d414224cb2e2a4cf ]---
[   57.339861] modprobe (1097) used greatest stack depth: 12048 bytes left

> ---
>  drivers/gpu/drm/i915/intel_dsi.c |   44 ++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 9ff6435..e0259e6 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -46,6 +46,14 @@ static const struct {
>  	},
>  };
>  
> +/* return pixels equvalent to txbyteclkhs */
> +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
> +			u16 burst_mode_ratio)
> +{
> +	return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100),
> +						(bpp * burst_mode_ratio));
> +}
> +
>  enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt)
>  {
>  	/* It just so happens the VBT matches register contents. */
> @@ -772,9 +780,10 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>  	struct drm_display_mode *adjusted_mode =
>  					&pipe_config->base.adjusted_mode;
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	unsigned int lane_count = intel_dsi->lane_count;
>  	unsigned int bpp, fmt;
>  	enum port port;
> -	u16 vfp, vsync, vbp;
> +	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
>  
>  	/*
>  	 * Atleast one port is active as encoder->get_config called only if
> @@ -799,22 +808,43 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>  	adjusted_mode->crtc_vtotal =
>  				I915_READ(BXT_MIPI_TRANS_VTOTAL(port));
>  
> +	hactive = adjusted_mode->crtc_hdisplay;
> +	hfp = I915_READ(MIPI_HFP_COUNT(port));
> +
>  	/*
> -	 * TODO: Retrieve hfp, hsync and hbp. Adjust them for dual link and
> -	 * calculate hsync_start, hsync_end, htotal and hblank_end
> +	 * Meaningful for video mode non-burst sync pulse mode only,
> +	 * can be zero for non-burst sync events and burst modes
>  	 */
> +	hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port));
> +	hbp = I915_READ(MIPI_HBP_COUNT(port));
> +
> +	/* harizontal values are in terms of high speed byte clock */
> +	hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +
> +	if (intel_dsi->dual_link) {
> +		hfp *= 2;
> +		hsync *= 2;
> +		hbp *= 2;
> +	}
>  
>  	/* vertical values are in terms of lines */
>  	vfp = I915_READ(MIPI_VFP_COUNT(port));
>  	vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port));
>  	vbp = I915_READ(MIPI_VBP_COUNT(port));
>  
> +	adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp;
> +	adjusted_mode->crtc_hsync_start = hfp + adjusted_mode->crtc_hdisplay;
> +	adjusted_mode->crtc_hsync_end = hsync + adjusted_mode->crtc_hsync_start;
>  	adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay;
> +	adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal;
>  
> -	adjusted_mode->crtc_vsync_start =
> -				vfp + adjusted_mode->crtc_vdisplay;
> -	adjusted_mode->crtc_vsync_end =
> -				vsync + adjusted_mode->crtc_vsync_start;
> +	adjusted_mode->crtc_vsync_start = vfp + adjusted_mode->crtc_vdisplay;
> +	adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
>  	adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
>  	adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 2/2] drm/i915/bxt: Adjusting the error in horizontal timings retrieval

[Ville Syrjälä]

On Tue, Apr 19, 2016 at 01:48:14PM +0530, Ramalingam C wrote:
> In BXT DSI there is no regs programmed with few horizontal timings
> in Pixels but txbyteclkhs.. So retrieval process adds some
> ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
> 
> Actually here for the given adjusted_mode, we are calculating the
> value programmed to the port and then back to the horizontal timing
> param in pixels. This is the expected value at the end of get_config,
> including roundup errors. And if that is same as retrieved value
> from port, then retrieved (HW state) adjusted_mode's horizontal
> timings are corrected to match with SW state to nullify the errors.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c |   97 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 88 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index e0259e6..8a1b872 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -46,6 +46,14 @@ static const struct {
>  	},
>  };
>  
> +/* return pixels in terms of txbyteclkhs */
> +static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
> +		       u16 burst_mode_ratio)
> +{
> +	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
> +					 8 * 100), lane_count);
> +}
> +
>  /* return pixels equvalent to txbyteclkhs */
>  static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
>  			u16 burst_mode_ratio)
> @@ -779,11 +787,19 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *adjusted_mode =
>  					&pipe_config->base.adjusted_mode;
> +	struct drm_display_mode *adjusted_mode_sw;
> +	struct intel_crtc *intel_crtc;
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	unsigned int lane_count = intel_dsi->lane_count;
>  	unsigned int bpp, fmt;
>  	enum port port;
>  	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
> +	u16 hfp_sw, hsync_sw, hbp_sw;
> +	u16 crtc_htotal_sw, crtc_hsync_start_sw, crtc_hsync_end_sw,
> +				crtc_hblank_start_sw, crtc_hblank_end_sw;
> +
> +	intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	adjusted_mode_sw = &intel_crtc->config->base.adjusted_mode;
>
 
>  	/*
>  	 * Atleast one port is active as encoder->get_config called only if
> @@ -847,8 +863,79 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>  	adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
>  	adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
>  	adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
> -}
>  
> +	/*
> +	 * In BXT DSI there is no regs programmed with few horizontal timings
> +	 * in Pixels but txbyteclkhs.. So retrieval process adds some
> +	 * ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
> +	 * Actually here for the given adjusted_mode, we are calculating the
> +	 * value programmed to the port and then back to the horizontal timing
> +	 * param in pixels. This is the expected value, including roundup errors
> +	 * And if that is same as retrieved value from port, then
> +	 * (HW state) adjusted_mode's horizontal timings are corrected to
> +	 * match with SW state to nullify the errors.
> +	 */
> +	/* Calculating the value programmed to the Port register */
> +	hfp_sw = adjusted_mode_sw->crtc_hsync_start -
> +					adjusted_mode_sw->crtc_hdisplay;
> +	hsync_sw = adjusted_mode_sw->crtc_hsync_end -
> +					adjusted_mode_sw->crtc_hsync_start;
> +	hbp_sw = adjusted_mode_sw->crtc_htotal -
> +					adjusted_mode_sw->crtc_hsync_end;

So during the initial state readout we get passed crtc->config as
pipe_config, and so we'll end up comparing the thing with itself. That
should be fine. A bit of extra care is needed to make sure we don't use
anything from crtc->config before we've filled it out with something,
but looks like the code does things in the right order (given the
previous patch which fills out all the htimings with something).

I think the biggest issue with this patch is seeing the forest from the
trees. Some refactoring would be good so that we'd have some kind of
htimings_{to,from}_txbyteclkhs() functions instead of duplicating the
same code multiple times.

So while it does look quite strange to be using crtc->config in the
.get_config() I think it should work out.

Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	if (intel_dsi->dual_link) {
> +		hfp_sw /= 2;
> +		hsync_sw /= 2;
> +		hbp_sw /= 2;
> +	}
> +
> +	hfp_sw = txbyteclkhs(hfp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hsync_sw = txbyteclkhs(hsync_sw, bpp, lane_count,
> +			    intel_dsi->burst_mode_ratio);
> +	hbp_sw = txbyteclkhs(hbp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +
> +	/* Reverse calculating the adjusted mode parameters from port reg vals*/
> +	hfp_sw = pixels_from_txbyteclkhs(hfp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hsync_sw = pixels_from_txbyteclkhs(hsync_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +	hbp_sw = pixels_from_txbyteclkhs(hbp_sw, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +
> +	if (intel_dsi->dual_link) {
> +		hfp_sw *= 2;
> +		hsync_sw *= 2;
> +		hbp_sw *= 2;
> +	}
> +
> +	crtc_htotal_sw = adjusted_mode_sw->crtc_hdisplay + hfp_sw +
> +							hsync_sw + hbp_sw;
> +	crtc_hsync_start_sw = hfp_sw + adjusted_mode_sw->crtc_hdisplay;
> +	crtc_hsync_end_sw = hsync_sw + crtc_hsync_start_sw;
> +	crtc_hblank_start_sw = adjusted_mode_sw->crtc_hdisplay;
> +	crtc_hblank_end_sw = crtc_htotal_sw;
> +
> +	if (adjusted_mode->crtc_htotal == crtc_htotal_sw)
> +		adjusted_mode->crtc_htotal = adjusted_mode_sw->crtc_htotal;
> +
> +	if (adjusted_mode->crtc_hsync_start == crtc_hsync_start_sw)
> +		adjusted_mode->crtc_hsync_start =
> +					adjusted_mode_sw->crtc_hsync_start;
> +
> +	if (adjusted_mode->crtc_hsync_end == crtc_hsync_end_sw)
> +		adjusted_mode->crtc_hsync_end =
> +					adjusted_mode_sw->crtc_hsync_end;
> +
> +	if (adjusted_mode->crtc_hblank_start == crtc_hblank_start_sw)
> +		adjusted_mode->crtc_hblank_start =
> +					adjusted_mode_sw->crtc_hblank_start;
> +
> +	if (adjusted_mode->crtc_hblank_end == crtc_hblank_end_sw)
> +		adjusted_mode->crtc_hblank_end =
> +					adjusted_mode_sw->crtc_hblank_end;
> +}
>  
>  static void intel_dsi_get_config(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *pipe_config)
> @@ -917,14 +1004,6 @@ static u16 txclkesc(u32 divider, unsigned int us)
>  	}
>  }
>  
> -/* return pixels in terms of txbyteclkhs */
> -static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
> -		       u16 burst_mode_ratio)
> -{
> -	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
> -					 8 * 100), lane_count);
> -}
> -
>  static void set_dsi_timings(struct drm_encoder *encoder,
>  			    const struct drm_display_mode *adjusted_mode)
>  {
> -- 
> 1.7.9.5
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 2/2] drm/i915/bxt: Adjusting the error in horizontal timings retrieval

[Jani Nikula]

On Wed, 27 Apr 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Apr 19, 2016 at 01:48:14PM +0530, Ramalingam C wrote:
>> In BXT DSI there is no regs programmed with few horizontal timings
>> in Pixels but txbyteclkhs.. So retrieval process adds some
>> ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
>> 
>> Actually here for the given adjusted_mode, we are calculating the
>> value programmed to the port and then back to the horizontal timing
>> param in pixels. This is the expected value at the end of get_config,
>> including roundup errors. And if that is same as retrieved value
>> from port, then retrieved (HW state) adjusted_mode's horizontal
>> timings are corrected to match with SW state to nullify the errors.
>> 
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c |   97 ++++++++++++++++++++++++++++++++++----
>>  1 file changed, 88 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index e0259e6..8a1b872 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -46,6 +46,14 @@ static const struct {
>>  	},
>>  };
>>  
>> +/* return pixels in terms of txbyteclkhs */
>> +static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
>> +		       u16 burst_mode_ratio)
>> +{
>> +	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>> +					 8 * 100), lane_count);
>> +}
>> +
>>  /* return pixels equvalent to txbyteclkhs */
>>  static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
>>  			u16 burst_mode_ratio)
>> @@ -779,11 +787,19 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_display_mode *adjusted_mode =
>>  					&pipe_config->base.adjusted_mode;
>> +	struct drm_display_mode *adjusted_mode_sw;
>> +	struct intel_crtc *intel_crtc;
>>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>  	unsigned int lane_count = intel_dsi->lane_count;
>>  	unsigned int bpp, fmt;
>>  	enum port port;
>>  	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
>> +	u16 hfp_sw, hsync_sw, hbp_sw;
>> +	u16 crtc_htotal_sw, crtc_hsync_start_sw, crtc_hsync_end_sw,
>> +				crtc_hblank_start_sw, crtc_hblank_end_sw;
>> +
>> +	intel_crtc = to_intel_crtc(encoder->base.crtc);
>> +	adjusted_mode_sw = &intel_crtc->config->base.adjusted_mode;
>>
>  
>>  	/*
>>  	 * Atleast one port is active as encoder->get_config called only if
>> @@ -847,8 +863,79 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
>>  	adjusted_mode->crtc_vsync_end = vsync + adjusted_mode->crtc_vsync_start;
>>  	adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
>>  	adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
>> -}
>>  
>> +	/*
>> +	 * In BXT DSI there is no regs programmed with few horizontal timings
>> +	 * in Pixels but txbyteclkhs.. So retrieval process adds some
>> +	 * ROUND_UP ERRORS in the process of PIXELS<==>txbyteclkhs.
>> +	 * Actually here for the given adjusted_mode, we are calculating the
>> +	 * value programmed to the port and then back to the horizontal timing
>> +	 * param in pixels. This is the expected value, including roundup errors
>> +	 * And if that is same as retrieved value from port, then
>> +	 * (HW state) adjusted_mode's horizontal timings are corrected to
>> +	 * match with SW state to nullify the errors.
>> +	 */
>> +	/* Calculating the value programmed to the Port register */
>> +	hfp_sw = adjusted_mode_sw->crtc_hsync_start -
>> +					adjusted_mode_sw->crtc_hdisplay;
>> +	hsync_sw = adjusted_mode_sw->crtc_hsync_end -
>> +					adjusted_mode_sw->crtc_hsync_start;
>> +	hbp_sw = adjusted_mode_sw->crtc_htotal -
>> +					adjusted_mode_sw->crtc_hsync_end;
>
> So during the initial state readout we get passed crtc->config as
> pipe_config, and so we'll end up comparing the thing with itself. That
> should be fine. A bit of extra care is needed to make sure we don't use
> anything from crtc->config before we've filled it out with something,
> but looks like the code does things in the right order (given the
> previous patch which fills out all the htimings with something).
>
> I think the biggest issue with this patch is seeing the forest from the
> trees. Some refactoring would be good so that we'd have some kind of
> htimings_{to,from}_txbyteclkhs() functions instead of duplicating the
> same code multiple times.

Agreed. However, I opted to push them as-is as they fix the boot hang,
and leave the refactoring as follow-up.

BR,
Jani.


>
> So while it does look quite strange to be using crtc->config in the
> .get_config() I think it should work out.
>
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> +
>> +	if (intel_dsi->dual_link) {
>> +		hfp_sw /= 2;
>> +		hsync_sw /= 2;
>> +		hbp_sw /= 2;
>> +	}
>> +
>> +	hfp_sw = txbyteclkhs(hfp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +	hsync_sw = txbyteclkhs(hsync_sw, bpp, lane_count,
>> +			    intel_dsi->burst_mode_ratio);
>> +	hbp_sw = txbyteclkhs(hbp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +
>> +	/* Reverse calculating the adjusted mode parameters from port reg vals*/
>> +	hfp_sw = pixels_from_txbyteclkhs(hfp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +	hsync_sw = pixels_from_txbyteclkhs(hsync_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +	hbp_sw = pixels_from_txbyteclkhs(hbp_sw, bpp, lane_count,
>> +						intel_dsi->burst_mode_ratio);
>> +
>> +	if (intel_dsi->dual_link) {
>> +		hfp_sw *= 2;
>> +		hsync_sw *= 2;
>> +		hbp_sw *= 2;
>> +	}
>> +
>> +	crtc_htotal_sw = adjusted_mode_sw->crtc_hdisplay + hfp_sw +
>> +							hsync_sw + hbp_sw;
>> +	crtc_hsync_start_sw = hfp_sw + adjusted_mode_sw->crtc_hdisplay;
>> +	crtc_hsync_end_sw = hsync_sw + crtc_hsync_start_sw;
>> +	crtc_hblank_start_sw = adjusted_mode_sw->crtc_hdisplay;
>> +	crtc_hblank_end_sw = crtc_htotal_sw;
>> +
>> +	if (adjusted_mode->crtc_htotal == crtc_htotal_sw)
>> +		adjusted_mode->crtc_htotal = adjusted_mode_sw->crtc_htotal;
>> +
>> +	if (adjusted_mode->crtc_hsync_start == crtc_hsync_start_sw)
>> +		adjusted_mode->crtc_hsync_start =
>> +					adjusted_mode_sw->crtc_hsync_start;
>> +
>> +	if (adjusted_mode->crtc_hsync_end == crtc_hsync_end_sw)
>> +		adjusted_mode->crtc_hsync_end =
>> +					adjusted_mode_sw->crtc_hsync_end;
>> +
>> +	if (adjusted_mode->crtc_hblank_start == crtc_hblank_start_sw)
>> +		adjusted_mode->crtc_hblank_start =
>> +					adjusted_mode_sw->crtc_hblank_start;
>> +
>> +	if (adjusted_mode->crtc_hblank_end == crtc_hblank_end_sw)
>> +		adjusted_mode->crtc_hblank_end =
>> +					adjusted_mode_sw->crtc_hblank_end;
>> +}
>>  
>>  static void intel_dsi_get_config(struct intel_encoder *encoder,
>>  				 struct intel_crtc_state *pipe_config)
>> @@ -917,14 +1004,6 @@ static u16 txclkesc(u32 divider, unsigned int us)
>>  	}
>>  }
>>  
>> -/* return pixels in terms of txbyteclkhs */
>> -static u16 txbyteclkhs(u16 pixels, int bpp, int lane_count,
>> -		       u16 burst_mode_ratio)
>> -{
>> -	return DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>> -					 8 * 100), lane_count);
>> -}
>> -
>>  static void set_dsi_timings(struct drm_encoder *encoder,
>>  			    const struct drm_display_mode *adjusted_mode)
>>  {
>> -- 
>> 1.7.9.5
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx