On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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(¤t_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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
