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

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

On Wed, 13 Apr 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 05, 2016 at 03:10:39PM +0530, Ramalingam C wrote:
>> 
>> On Tuesday 05 April 2016 02:00 PM, Jani Nikula wrote:
>> >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.
>> 
>> No, thats not possible jani. I think i didn't elaborate the problem
>> statement enough.
>> If you can read the programmed value from the hardware without any error,
>> then there is no need for this patch itself.
>> 
>> Even if we program the modified adjusted mode, timing parameters read from
>> get_config() will not be same as of modified adjusted mode.
>> 
>> In BXT DSI only available hw registers doesn't provide all timing parameters
>> in terms of pixels but txbyteclkhs.
>> adjusted mode has the parameters(start and end of hsync, htotal and hdisplay
>> and others) in terms of pixels.
>
> Then fix adjusted_mode to have the timings in terms of txbyteclkhs
> already. Problem solved.

I let Ville convince me there would be problems with that. Ville, care
to fill in the details?

BR,
Jani.


> -Daniel
>
>> So some conversion involved in programming few parameters (hfp, hsync and
>> hbp) and also in retrieving them.
>> 
>> As discussed above port registers expects hfp, hsync and hbp interms of
>> txbyteclkhs.
>> 
>> Sequence of programing (set_dsi_timings) the dsi port registers:
>> parameters from mode ---> (calc hfp, hsync and hbp) ---> (adjust for dual
>> link) ----> (conversion of Pixels to txbyteclkhs) ---> Program to Port
>> register
>> 
>> Sequence of get_config():
>> Read from port register ---> (conversion of txbyteclkhs to Pixels) --->
>> (adjust for dual link) ---> (recalculate the adjusted mode parameters from
>> hfp, hsync and hbp and other readings)
>> 
>> Here if we assume the input  to the set_dsi_timings is X(adjusted mode
>> parameter), output of get_config() will be  (X + delta1).
>> Here delta1 is error due to multiple DIV_ROUND_UP() in the conversion of
>> bytes <===> txbyteclkhs.
>> So as daniel says if you modify the adjusted_mode in compute_config()
>> itself, input to the set_dsi_timings() will become (X + delta1)
>> and the readings from the get_config() will become (X + delta1 + delta2)
>> 
>> And it wouldn't be appropriate to program the hw with modified adjusted
>> mode. This modification is just to match it with the pipe_config read from
>> hw.
>> Hence adjusted mode can be modified after the hw programming only, so the
>> place to do is end of set_dsi_timings().
>> 
>> Hope I explained the situation enough.
>> 
>> >
>> >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
>> >>>>
>> 
>> -- 
>> 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