Re: [PATCH] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 28 Mar 2016, Ramalingam C <ramalingam.c@intel.com> wrote:
> On Monday 21 March 2016 07:13 PM, Jani Nikula wrote:
>> On Fri, 11 Mar 2016, Ramalingam C <ramalingam.c@intel.com> wrote:
>>> At BXT DSI, PIPE registers are inactive. So we can 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.
>> I think you should take the encoder specific callback parts from [1],
>> and instead of adding a new ->get_pipe_config hook, do them in
>> ->get_hw_state, and add the rebased work from this patch on top.
>>
>> BR,
>> Jani.
>>
>> [1] http://patchwork.freedesktop.org/patch/msgid/1456771760-18823-1-git-send-email-ramalingam.c@intel.com
>
> Jani,
>
> At present, encoder->get_hw_state is used for the getting the encoder's 
> activeness(dsi port is active or not) along with the pipe it is 
> connected to it.
> By definition of that encoder function declaration, I dont think we 
> should use it for retrieving the pipe timing parameters from port.
>
> I feel adding a new encoder function is making more sense, for handling 
> this special case of retrieving the timing parameters
> from port register for BXT.
>
> But If we dont want to use new encoder function I feel 
> encoder->get_config is the option. As this is supposed to get called after
> display->get_pipe_config and also this function fills the 
> pipe_config->base.adjusted_mode.crtc_clock already.
>
> Please tell me if you agree with the separate encoder function.

Please use encoder->get_config().

BR,
Jani.


>
> --Ram
>
>>
>>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>
>>> This change is required for
>>> https://lists.freedesktop.org/archives/intel-gfx/2016-February/088653.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 7068dc1..46ce662 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -12653,6 +12653,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 { \
>>> @@ -12680,6 +12683,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) { \
>>> +			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; \
>>> +		} \
>>> +	} \
>>> +}
>>> +
>>>   #define PIPE_CONF_CHECK_M_N(name) \
>>>   	if (!intel_compare_link_m_n(&current_config->name, \
>>>   				    &pipe_config->name,\
>>> @@ -12784,11 +12835,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);
>>>   
>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
>>> @@ -12866,6 +12917,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_I_ALT
>>>   #undef PIPE_CONF_CHECK_FLAGS
>>>   #undef PIPE_CONF_CHECK_CLOCK_FUZZY

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