On Mon, 28 Mar 2016, Ramalingam C <ramalinga...@intel.com> wrote: > On Monday 21 March 2016 07:13 PM, Jani Nikula wrote: >> On Fri, 11 Mar 2016, Ramalingam C <ramalinga...@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-ramalinga...@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 <ramalinga...@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(¤t_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
