[Intel-gfx] [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison

[Jani Nikula]

On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c at intel.com> wrote:
> At BXT DSI, PIPE registers are inactive. So we can't get the
> PIPE's mode parameters from them. The possible option is
> retriving them from the PORT registers. But mode timing
> parameters are progammed to port registers interms of byteclocks.
>
> The formula used to convert the pixels interms of byteclk is
> 	DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>  						8 * 100), lane_count);
>
> So we retrieve them, interms of pixels as
> 	DIV_ROUND_UP((clk_hs * lane_count * 8 * 100),
> 					(bpp * burst_mode_ratio));
>
> Due to the multiple DIV_ROUND_UP in both formulas we get the worst
> case delta in the retrieved PIPE's timing parameter as below
> 	DIV_ROUND_UP((8 * intel_dsi->lane_count * 100),
> 		(dsi_pixel_format_bpp(intel_dsi->pixel_format) *
> 			intel_dsi->burst_mode_ratio)))
>
> This converson of byteclk to pixel is required for hsync, hfp and hbp.
> Which intern impacts horrizontal timing parameters. At worst case to
> get htotal all there parameters are added with hactive.
> Hence delta will be 3 times of above formula. Hence this value is
> considered as tolerance for pipe_config comparison, in case of BXT DSI.
>
> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> ---
> Reviewed at https://lists.freedesktop.org/archives/intel-gfx/2016-March/089548.html
>
>  drivers/gpu/drm/i915/intel_display.c |   62 +++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c0627d6..282f036 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12557,6 +12557,9 @@ intel_pipe_config_compare(struct drm_device *dev,
>  			  bool adjust)
>  {
>  	bool ret = true;
> +	struct intel_crtc *crtc = to_intel_crtc(current_config->base.crtc);
> +	struct intel_encoder *intel_encoder;
> +	struct intel_dsi *intel_dsi = NULL;
>  
>  #define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
>  	do { \
> @@ -12593,6 +12596,54 @@ intel_pipe_config_compare(struct drm_device *dev,
>  		ret = false; \
>  	}
>  
> +/*
> + * In case of BXT DSI, HW pipe_config will be retrieved from the port's timing
> + * configuration. This retrival includes some errors due to the DIV_ROUND_UP.
> + * So we are considering the max possible error at the comparison.
> + */
> +/*
> + * htotal = hactive + hfp + hsync + hbp. Here last three lements might have
> + * the converson error, hence we consider the 3 times of error as tolerance.
> + */
> +
> +#define MAX_BXT_DSI_TIMING_RETRIVAL_ERR \
> +		(intel_dsi == NULL ? 0 : \
> +		DIV_ROUND_UP((3 * 8 * intel_dsi->lane_count * 100), \
> +		(dsi_pixel_format_bpp(intel_dsi->pixel_format) * \
> +			intel_dsi->burst_mode_ratio)))
> +
> +#define BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) { \
> +	for_each_encoder_on_crtc(dev, &crtc->base, \
> +					intel_encoder) { \
> +		if (intel_encoder->type == INTEL_OUTPUT_DSI) { \
> +			intel_dsi = enc_to_intel_dsi(&intel_encoder->base); \
> +		} \
> +	} \
> +	if (!(current_config->name < pipe_config->name && \
> +		current_config->name >= (pipe_config->name - \
> +			MAX_BXT_DSI_TIMING_RETRIVAL_ERR))) { \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> +		  "(expected %i, found %i(Err tolerance considered))\n", \
> +		  current_config->name, \
> +		  pipe_config->name); \
> +		ret = false; \
> +	} \
> +}
> +
> +#define PIPE_CONF_CHECK_I_RANGE(name) { \
> +	if (current_config->name != pipe_config->name) { \
> +		if (IS_BROXTON(dev) && crtc->config->has_dsi_encoder) { \

Please drop the platform and encoder type checks here, and move them to
a higher level.

> +			BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) \
> +		} else { \
> +			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> +			  "(expected %i, found %i)\n", \
> +			  current_config->name, \
> +			  pipe_config->name); \
> +			ret = false; \
> +		} \
> +	} \
> +}

I think you should model this after PIPE_CONF_CHECK_CLOCK_FUZZY. Please
add a function to do the check, similar to intel_fuzzy_clock_check(),
and this will be much easier to read and understand.

Maybe call the macro PIPE_CONF_CHECK_DSI_TIMING_FUZZY or something,
since I guess it has to be encoder specific.

> +
>  #define PIPE_CONF_CHECK_M_N(name) \
>  	if (!intel_compare_link_m_n(&current_config->name, \
>  				    &pipe_config->name,\
> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>  	PIPE_CONF_CHECK_I(has_dsi_encoder);
>  
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);

Please add the platform and encoder type checks at this level for
clarity (and as I suggested to drop the checks from the macro itself).

	if (IS_BROXTON(dev_priv) && crtc->config->has_dsi_encoder) {
        	...
        } else {
        	...
        }

BR,
Jani.



>  
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>  
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
> +#undef PIPE_CONF_CHECK_I_RANGE
>  #undef PIPE_CONF_CHECK_P
>  #undef PIPE_CONF_CHECK_I_ALT
>  #undef PIPE_CONF_CHECK_FLAGS

-- 
Jani Nikula, Intel Open Source Technology Center