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

[Ramalingam C <ramalingam.c@xxxxxxxxx>]

On Tuesday 29 March 2016 01:08 PM, Jani Nikula wrote:
> On Mon, 28 Mar 2016, Ramalingam C <ramalingam.c@xxxxxxxxx> wrote:
> > On Monday 21 March 2016 07:13 PM, Jani Nikula wrote:
> > > On Fri, 11 Mar 2016, Ramalingam C <ramalingam.c@xxxxxxxxx> 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@xxxxxxxxx
> > 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.
Please find the change at 
https://lists.freedesktop.org/archives/intel-gfx/2016-March/090999.html

--Ram
> > --Ram
> >
> > > > Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > > >
> > > > 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

--
Thanks,
--Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx