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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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