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

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

On Mon, 04 Apr 2016, Ramalingam C <ramalingam.c@intel.com> wrote:
> On Thursday 31 March 2016 12:34 AM, Daniel Vetter wrote:
>> On Wed, Mar 30, 2016 at 07:49:40PM +0530, Ramalingam C wrote:
>>> On Wednesday 30 March 2016 05:02 PM, Daniel Vetter wrote:
>>>> On Tue, Mar 29, 2016 at 11:04:51PM +0530, Ramalingam C 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@intel.com>
>>>> This is the wrong way round imo, better would be to adjust the adjusted
>>>> mode in the bxt dsi compute_config function to match the hw granularity.
>>>> Stuff _really_ should match exactly, the fuzzy clock matching is mostly
>>>> because our clock cod is a mess, and we can't/don't properly
>>>> forward-compuate the actual clock timings we program into the hardware.
>>>> -Daniel
>>> Daniel, I got your point. But the problem will be that difficulty(even if
>>> possible) in adjusting the adjusted mode parameters.
>>> Reason is we are not programing the mode parameter as such. We will derive
>>> the hfp, hsync and hbp from
>>> hsync_start, hsync_end, hdisplay and htotal. These will be adjusted(divided
>>> by 2) for dual link scenario.
>>> And then resultant will go into the conversion as mentioned in the commit
>>> message (two DIV_ROUND_UP onwards
>>> and one DIV_ROUND_UP backwards). For this we have to make the parameter
>>> divisible by three different factors.
>>> So IMHO, even if this is possible, it will look more messy.
>>>
>>> Predicting the max error and tolerating it in pipe_config_compare will be
>>> the straight forward and more reasonable.
>>> Please let me know if i can go ahead in this approach.
>> Yeah I discussed this some more with Jani on irc. I'd say we should store
>> this adjusted horizontal timings (the ones fudged with burst_mode_ratio,
>> lane_count, dual-link and all these things applied) into
>> crtc_state->base.adjusted_mode. And then ofc also read those values out.
>>
>> The overall idea of the state verify/compare logic is that we start out
>> with requested state from userspace, then derive the real hw state. And
>> then compare that computed hw state with what's there already. Except for
>> clocks, where there's special reasons, we never go back, since going back
>> requires us to apply a range. This is the only way to guarnatee that "hw
>> has the same exact mode programmed in both cases" iff "intel_crtc_state
>> matches per intel_crtc_config_compare".
>>
>> state->adjusted_mode is never exposed to userspace, so there's no problem
>> if it's has "strange" values. And we already have pipe_src_h/w to express
>> the logical input rectangle.
>>
>> The idea is similar to how we set adjusted_mode.flags to what we actually
>> program, instead of trying to make something up that's not perfectly
>> accurate.
>> -Daniel
> Daniel,
>
> I have tested by adjusting the adjusted_mode in set_dsi_timings() 
> instead of intel_dsi_compute_config().
> Reason is if we modify the adjusted mode at intel_dsi_compute_config() 
> itself, then modified value will
> be taken as input for set_dsi_timings. Hence the get_config will deviate 
> further. I hope this should be fine with you and Jani.
>
> This will work out, if set_dsi_timings() is called after the 
> dsi_compute_config() on every suspend and resume or modeset.
> I will verify this on Android once and update.
>
> Please share your view on this, so that can update the patch with 
> corresponding changes.

I can't speak for Daniel, but I think his point was to update adjusted
mode in ->compute_config() in a way that can be used directly in
set_dsi_timings(). Then, it should be possible to read the timings from
the hardware, and compare.

BR,
Jani.


>>
>>>>> ---
>>>>> 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) { \
>>>>> +			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,\
>>>>> @@ -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);
>>>>>   	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
>>>>> -- 
>>>>> 1.7.9.5
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> -- 
>>> Thanks,
>>> --Ram
>>>

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