* [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers @ 2016-03-29 17:34 Ramalingam C 2016-03-29 17:34 ` [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison Ramalingam C ` (3 more replies) 0 siblings, 4 replies; 29+ messages in thread From: Ramalingam C @ 2016-03-29 17:34 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula 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. The required changes are added for BXT in intel_dsi_get_config (encoder->get_config). Signed-off-by: Ramalingam C <ramalingam.c@intel.com> Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- Previously reviewed at https://patchwork.freedesktop.org/patch/75301/ drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++ drivers/gpu/drm/i915/intel_dsi.c | 104 ++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c839ce9..da3cdef 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8210,6 +8210,7 @@ enum skl_disp_power_wells { #define BXT_PIPE_SELECT_SHIFT 7 #define BXT_PIPE_SELECT_MASK (7 << 7) #define BXT_PIPE_SELECT(pipe) ((pipe) << 7) +#define BXT_PORT_TO_PIPE(ctrl) ((ctrl & BXT_PIPE_SELECT_MASK) >> 7) #define _MIPIA_DATA_ADDRESS (dev_priv->mipi_mmio_base + 0xb108) #define _MIPIC_DATA_ADDRESS (dev_priv->mipi_mmio_base + 0xb908) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 29aa64b..c0627d6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9941,11 +9941,40 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc, } } +struct intel_encoder *bxt_get_dsi_encoder_for_crtc(struct intel_crtc *crtc, + struct intel_crtc_state *pipe_config) +{ + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_encoder *intel_encoder; + struct intel_dsi *intel_dsi; + enum port port; + u32 tmp; + + for_each_intel_encoder(dev, intel_encoder) { + if (intel_encoder->type == INTEL_OUTPUT_DSI) { + intel_dsi = enc_to_intel_dsi(&intel_encoder->base); + for_each_dsi_port(port, intel_dsi->ports) { + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & + DPI_ENABLE)) + break; + + tmp = I915_READ(MIPI_CTRL(port)); + if ((tmp & BXT_PIPE_SELECT_MASK) == + BXT_PIPE_SELECT(crtc->pipe)) + return intel_encoder; + } + } + } + return NULL; +} + static bool haswell_get_pipe_config(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_encoder *intel_encoder, *attached_encoder = NULL; enum intel_display_power_domain power_domain; unsigned long power_domain_mask; bool active; @@ -9965,6 +9994,21 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, WARN_ON(active && pipe_config->has_dsi_encoder); if (pipe_config->has_dsi_encoder) active = true; + + for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder) + attached_encoder = intel_encoder; + + /* + * attached_encoder will be NULL, if there is no modeset from + * the kernel bootup. + */ + if (!attached_encoder && pipe_config->has_dsi_encoder) + attached_encoder = + bxt_get_dsi_encoder_for_crtc(crtc, pipe_config); + + if (attached_encoder && attached_encoder->get_config) + attached_encoder->get_config(attached_encoder, + pipe_config); } if (!active) diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 0de74e1..69a801e 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -740,14 +740,118 @@ out_put_power: return active; } +/* return pixels equvalent to txbyteclkhs */ +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count, + u16 burst_mode_ratio) +{ + return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100), + (bpp * burst_mode_ratio)); +} + +static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder, + struct intel_crtc_state *pipe_config) +{ + struct drm_device *dev = encoder->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_display_mode *adjusted_mode = + &pipe_config->base.adjusted_mode; + struct intel_dsi *intel_dsi = + enc_to_intel_dsi(&encoder->base); + unsigned int bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); + unsigned int lane_count = intel_dsi->lane_count; + enum port port; + enum pipe dsi_pipe; + u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp; + uint32_t tmp; + + for_each_dsi_port(port, intel_dsi->ports) { + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE)) + continue; + + /* In terms of pixels */ + adjusted_mode->crtc_hdisplay = + I915_READ(BXT_MIPI_TRANS_HACTIVE(port)); + adjusted_mode->crtc_vdisplay = + I915_READ(BXT_MIPI_TRANS_VACTIVE(port)); + adjusted_mode->crtc_vtotal = + I915_READ(BXT_MIPI_TRANS_VTOTAL(port)); + + hactive = adjusted_mode->crtc_hdisplay; + hfp = I915_READ(MIPI_HFP_COUNT(port)); + + /* + * meaningful for video mode non-burst sync pulse mode only, + * can be zero for non-burst sync events and burst modes + */ + hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port)); + hbp = I915_READ(MIPI_HBP_COUNT(port)); + + /* horizontal values are in terms of high speed byte clock */ + hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count, + intel_dsi->burst_mode_ratio); + hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count, + intel_dsi->burst_mode_ratio); + hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count, + intel_dsi->burst_mode_ratio); + + if (intel_dsi->dual_link) { + hfp *= 2; + hsync *= 2; + hbp *= 2; + } + + /* vertical values are in terms of lines */ + vfp = I915_READ(MIPI_VFP_COUNT(port)); + vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port)); + vbp = I915_READ(MIPI_VBP_COUNT(port)); + + adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp; + adjusted_mode->crtc_hsync_start = + hfp + adjusted_mode->crtc_hdisplay; + adjusted_mode->crtc_hsync_end = + hsync + adjusted_mode->crtc_hsync_start; + adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay; + adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal; + + adjusted_mode->crtc_vsync_start = + vfp + adjusted_mode->crtc_vdisplay; + adjusted_mode->crtc_vsync_end = + vsync + adjusted_mode->crtc_vsync_start; + adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay; + adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; + + pipe_config->pipe_bpp = + mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); + + dsi_pipe = BXT_PORT_TO_PIPE(I915_READ(MIPI_CTRL(port))); + if (dsi_pipe > PIPE_C) { + DRM_ERROR("Invalid PIPE configured\n"); + break; + } + + tmp = I915_READ(PIPESRC(dsi_pipe)); + pipe_config->pipe_src_h = (tmp & 0xffff) + 1; + pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1; + + pipe_config->base.mode.vdisplay = pipe_config->pipe_src_h; + pipe_config->base.mode.hdisplay = pipe_config->pipe_src_w; + break; + } +} + + static void intel_dsi_get_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config) { + struct drm_device *dev = encoder->base.dev; u32 pclk; DRM_DEBUG_KMS("\n"); pipe_config->has_dsi_encoder = true; + if (IS_BROXTON(dev)) + bxt_dsi_get_pipe_config(encoder, pipe_config); + /* * DPLL_MD is not used in case of DSI, reading will get some default value * set dpll_md = 0 -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-03-29 17:34 [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C @ 2016-03-29 17:34 ` Ramalingam C 2016-03-29 18:28 ` kbuild test robot ` (2 more replies) 2016-03-30 6:14 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/BXT: Get pipe conf from the port registers Patchwork ` (2 subsequent siblings) 3 siblings, 3 replies; 29+ messages in thread From: Ramalingam C @ 2016-03-29 17:34 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula 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> --- 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(¤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); 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-03-29 17:34 ` [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison Ramalingam C @ 2016-03-29 18:28 ` kbuild test robot 2016-03-30 11:03 ` Jani Nikula 2016-03-30 11:32 ` Daniel Vetter 2 siblings, 0 replies; 29+ messages in thread From: kbuild test robot @ 2016-03-29 18:28 UTC (permalink / raw) To: Ramalingam C; +Cc: jani.nikula, intel-gfx, kbuild-all [-- Attachment #1: Type: text/plain, Size: 2429 bytes --] Hi Ramalingam, [auto build test ERROR on drm-intel/for-linux-next] [cannot apply to v4.6-rc1 next-20160329] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Ramalingam-C/drm-i915-BXT-Get-pipe-conf-from-the-port-registers/20160330-014708 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: i386-randconfig-s1-201613 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): In file included from include/linux/list.h:8:0, from include/linux/dmi.h:4, from drivers/gpu/drm/i915/intel_display.c:27: drivers/gpu/drm/i915/intel_display.c: In function 'intel_pipe_config_compare': >> drivers/gpu/drm/i915/intel_display.c:12612:4: error: implicit declaration of function 'dsi_pixel_format_bpp' [-Werror=implicit-function-declaration] (dsi_pixel_format_bpp(intel_dsi->pixel_format) * \ ^ include/linux/kernel.h:67:36: note: in definition of macro 'DIV_ROUND_UP' #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d)) ^ drivers/gpu/drm/i915/intel_display.c:12624:4: note: in expansion of macro 'MAX_BXT_DSI_TIMING_RETRIVAL_ERR' MAX_BXT_DSI_TIMING_RETRIVAL_ERR))) { \ ^ drivers/gpu/drm/i915/intel_display.c:12636:4: note: in expansion of macro 'BXT_DSI_PIPE_CONF_CHECK_I_RANGE' BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) \ ^ drivers/gpu/drm/i915/intel_display.c:12751:2: note: in expansion of macro 'PIPE_CONF_CHECK_I_RANGE' PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal); ^ cc1: some warnings being treated as errors vim +/dsi_pixel_format_bpp +12612 drivers/gpu/drm/i915/intel_display.c 12606 * the converson error, hence we consider the 3 times of error as tolerance. 12607 */ 12608 12609 #define MAX_BXT_DSI_TIMING_RETRIVAL_ERR \ 12610 (intel_dsi == NULL ? 0 : \ 12611 DIV_ROUND_UP((3 * 8 * intel_dsi->lane_count * 100), \ 12612 (dsi_pixel_format_bpp(intel_dsi->pixel_format) * \ 12613 intel_dsi->burst_mode_ratio))) 12614 12615 #define BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) { \ --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 27155 bytes --] [-- Attachment #3: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-03-29 17:34 ` [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison Ramalingam C 2016-03-29 18:28 ` kbuild test robot @ 2016-03-30 11:03 ` Jani Nikula 2016-03-30 11:32 ` Daniel Vetter 2 siblings, 0 replies; 29+ messages in thread From: Jani Nikula @ 2016-03-30 11:03 UTC (permalink / raw) To: Ramalingam C, intel-gfx On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c@intel.com> 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> > --- > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-03-29 17:34 ` [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison Ramalingam C 2016-03-29 18:28 ` kbuild test robot 2016-03-30 11:03 ` Jani Nikula @ 2016-03-30 11:32 ` Daniel Vetter 2016-03-30 14:19 ` Ramalingam C 2 siblings, 1 reply; 29+ messages in thread From: Daniel Vetter @ 2016-03-30 11:32 UTC (permalink / raw) To: Ramalingam C; +Cc: jani.nikula, intel-gfx 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 > --- > 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(¤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); > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-03-30 11:32 ` Daniel Vetter @ 2016-03-30 14:19 ` Ramalingam C 2016-03-30 19:04 ` Daniel Vetter 0 siblings, 1 reply; 29+ messages in thread From: Ramalingam C @ 2016-03-30 14:19 UTC (permalink / raw) To: Daniel Vetter; +Cc: jani.nikula, intel-gfx 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. > >> --- >> 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(¤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); >> >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-03-30 14:19 ` Ramalingam C @ 2016-03-30 19:04 ` Daniel Vetter 2016-04-04 15:43 ` Ramalingam C 0 siblings, 1 reply; 29+ messages in thread From: Daniel Vetter @ 2016-03-30 19:04 UTC (permalink / raw) To: Ramalingam C; +Cc: jani.nikula, intel-gfx 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 > > > >>--- > >>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(¤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); > >> 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-03-30 19:04 ` Daniel Vetter @ 2016-04-04 15:43 ` Ramalingam C 2016-04-05 8:30 ` Jani Nikula 0 siblings, 1 reply; 29+ messages in thread From: Ramalingam C @ 2016-04-04 15:43 UTC (permalink / raw) To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx 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. > >>>> --- >>>> 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(¤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); >>>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-04 15:43 ` Ramalingam C @ 2016-04-05 8:30 ` Jani Nikula 2016-04-05 9:40 ` Ramalingam C 2016-04-13 10:05 ` Daniel Vetter 0 siblings, 2 replies; 29+ messages in thread From: Jani Nikula @ 2016-04-05 8:30 UTC (permalink / raw) To: Ramalingam C, Daniel Vetter; +Cc: intel-gfx 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(¤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); >>>>> 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-05 8:30 ` Jani Nikula @ 2016-04-05 9:40 ` Ramalingam C 2016-04-13 10:06 ` Daniel Vetter 2016-04-13 10:05 ` Daniel Vetter 1 sibling, 1 reply; 29+ messages in thread From: Ramalingam C @ 2016-04-05 9:40 UTC (permalink / raw) To: Jani Nikula, Daniel Vetter; +Cc: intel-gfx 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. 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(¤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); >>>>>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-05 9:40 ` Ramalingam C @ 2016-04-13 10:06 ` Daniel Vetter 2016-04-13 11:06 ` Jani Nikula 0 siblings, 1 reply; 29+ messages in thread From: Daniel Vetter @ 2016-04-13 10:06 UTC (permalink / raw) To: Ramalingam C; +Cc: Jani Nikula, intel-gfx 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. -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(¤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); > >>>>>> 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-13 10:06 ` Daniel Vetter @ 2016-04-13 11:06 ` Jani Nikula 2016-04-13 11:48 ` Daniel Vetter 0 siblings, 1 reply; 29+ messages in thread From: Jani Nikula @ 2016-04-13 11:06 UTC (permalink / raw) To: Daniel Vetter, Ramalingam C; +Cc: intel-gfx, Syrjala, Ville 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(¤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); >> >>>>>> 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-13 11:06 ` Jani Nikula @ 2016-04-13 11:48 ` Daniel Vetter 2016-04-13 11:57 ` Daniel Vetter 0 siblings, 1 reply; 29+ messages in thread From: Daniel Vetter @ 2016-04-13 11:48 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx, Syrjala, Ville On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >> 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? If we change them too hard the accurate vblank timestamp stuff will be upset. But then we only need to adjust horizontal timings for dsi, whereas on gen5+ the vblank ts code uses the line counter (i.e. vertical timings) only. If it's just that it should work, and I don't think we have any other users of the adjusted_mode. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-13 11:48 ` Daniel Vetter @ 2016-04-13 11:57 ` Daniel Vetter 2016-04-13 13:04 ` Ramalingam C 0 siblings, 1 reply; 29+ messages in thread From: Daniel Vetter @ 2016-04-13 11:57 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx, Syrjala, Ville On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >>> 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? > > If we change them too hard the accurate vblank timestamp stuff will be > upset. But then we only need to adjust horizontal timings for dsi, > whereas on gen5+ the vblank ts code uses the line counter (i.e. > vertical timings) only. > > If it's just that it should work, and I don't think we have any other > users of the adjusted_mode. Ok, I was wrong and we obviously need the right dotclock to compute linedur_ns correctly in drm_calc_timestamping_constants(). So either we adjust the dotclock of adjusted_mode too (imo makes most sense), or we need yet another mode somewhere and use that for dsi cross checking (real ugly imo). More I missed? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-13 11:57 ` Daniel Vetter @ 2016-04-13 13:04 ` Ramalingam C 2016-04-13 14:46 ` Daniel Vetter 0 siblings, 1 reply; 29+ messages in thread From: Ramalingam C @ 2016-04-13 13:04 UTC (permalink / raw) To: Daniel Vetter, Jani Nikula; +Cc: intel-gfx, Syrjala, Ville On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: > On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >>>> 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? >> If we change them too hard the accurate vblank timestamp stuff will be >> upset. But then we only need to adjust horizontal timings for dsi, >> whereas on gen5+ the vblank ts code uses the line counter (i.e. >> vertical timings) only. >> >> If it's just that it should work, and I don't think we have any other >> users of the adjusted_mode. > Ok, I was wrong and we obviously need the right dotclock to compute > linedur_ns correctly in drm_calc_timestamping_constants(). So either > we adjust the dotclock of adjusted_mode too (imo makes most sense), or > we need yet another mode somewhere and use that for dsi cross checking > (real ugly imo). More I missed? Another point to be considered: And we program hsync, hfp and hbp in terms of txbyteclkhs to port register, which are not part of adjusted_mode. So for BXT DSI, we have to store them interms of txbyteclkhs and compare with hsync, hfp and hbp read from HW?? > -Daniel -- Thanks, --Ram _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-13 13:04 ` Ramalingam C @ 2016-04-13 14:46 ` Daniel Vetter 2016-04-15 10:57 ` Ramalingam C 0 siblings, 1 reply; 29+ messages in thread From: Daniel Vetter @ 2016-04-13 14:46 UTC (permalink / raw) To: Ramalingam C; +Cc: Jani Nikula, intel-gfx, Syrjala, Ville On Wed, Apr 13, 2016 at 06:34:25PM +0530, Ramalingam C wrote: > > On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: > >On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: > >>>>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? > >>If we change them too hard the accurate vblank timestamp stuff will be > >>upset. But then we only need to adjust horizontal timings for dsi, > >>whereas on gen5+ the vblank ts code uses the line counter (i.e. > >>vertical timings) only. > >> > >>If it's just that it should work, and I don't think we have any other > >>users of the adjusted_mode. > >Ok, I was wrong and we obviously need the right dotclock to compute > >linedur_ns correctly in drm_calc_timestamping_constants(). So either > >we adjust the dotclock of adjusted_mode too (imo makes most sense), or > >we need yet another mode somewhere and use that for dsi cross checking > >(real ugly imo). More I missed? > Another point to be considered: > And we program hsync, hfp and hbp in terms of txbyteclkhs to port register, > which are not part of adjusted_mode. > So for BXT DSI, we have to store them interms of txbyteclkhs and compare > with hsync, hfp and hbp read from HW?? Yeah that's my idea. Plus we should probably store txbyteclkhs somewhere, too. Or at least we need to adjust the clock in adjusted_mode to match txbyteclkhs, otherwise the vblank ts code goes off the rails. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-13 14:46 ` Daniel Vetter @ 2016-04-15 10:57 ` Ramalingam C 2016-04-19 10:30 ` Ramalingam C 0 siblings, 1 reply; 29+ messages in thread From: Ramalingam C @ 2016-04-15 10:57 UTC (permalink / raw) To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx, Syrjala, Ville On Wednesday 13 April 2016 08:16 PM, Daniel Vetter wrote: > On Wed, Apr 13, 2016 at 06:34:25PM +0530, Ramalingam C wrote: >> On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: >>> On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula <jani.nikula@intel.com> wrote: >>>>>> 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? >>>> If we change them too hard the accurate vblank timestamp stuff will be >>>> upset. But then we only need to adjust horizontal timings for dsi, >>>> whereas on gen5+ the vblank ts code uses the line counter (i.e. >>>> vertical timings) only. >>>> >>>> If it's just that it should work, and I don't think we have any other >>>> users of the adjusted_mode. >>> Ok, I was wrong and we obviously need the right dotclock to compute >>> linedur_ns correctly in drm_calc_timestamping_constants(). So either >>> we adjust the dotclock of adjusted_mode too (imo makes most sense), or >>> we need yet another mode somewhere and use that for dsi cross checking >>> (real ugly imo). More I missed? >> Another point to be considered: >> And we program hsync, hfp and hbp in terms of txbyteclkhs to port register, >> which are not part of adjusted_mode. >> So for BXT DSI, we have to store them interms of txbyteclkhs and compare >> with hsync, hfp and hbp read from HW?? > Yeah that's my idea. Plus we should probably store txbyteclkhs somewhere, > too. Or at least we need to adjust the clock in adjusted_mode to match > txbyteclkhs, otherwise the vblank ts code goes off the rails. Ok. I will post a RFC for this. But for that we need to finalize few points. 1. We need to add three more variables in pipe_config for caching(SW and HW State) the hfp, hsync and hbp. 2. On BXT DSI, We will compare the hdisplay, hfp, hsync and hbp only on pipe_config_compare not all horizontal timing param of adjusted mode. 3. So we dont need to recalculate all horizontal timing parameters of adjusted_mode from port register at get_config(). Should we fill them too (No harm Though)? Please clarify if we are fine with above points. -Ram > -Daniel -- Thanks, --Ram _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-15 10:57 ` Ramalingam C @ 2016-04-19 10:30 ` Ramalingam C 0 siblings, 0 replies; 29+ messages in thread From: Ramalingam C @ 2016-04-19 10:30 UTC (permalink / raw) To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx, Syrjala, Ville On Friday 15 April 2016 04:27 PM, Ramalingam C wrote: > > On Wednesday 13 April 2016 08:16 PM, Daniel Vetter wrote: >> On Wed, Apr 13, 2016 at 06:34:25PM +0530, Ramalingam C wrote: >>> On Wednesday 13 April 2016 05:27 PM, Daniel Vetter wrote: >>>> On Wed, Apr 13, 2016 at 1:48 PM, Daniel Vetter <daniel@ffwll.ch> >>>> wrote: >>>>> On Wed, Apr 13, 2016 at 1:06 PM, Jani Nikula >>>>> <jani.nikula@intel.com> wrote: >>>>>>> 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? >>>>> If we change them too hard the accurate vblank timestamp stuff >>>>> will be >>>>> upset. But then we only need to adjust horizontal timings for dsi, >>>>> whereas on gen5+ the vblank ts code uses the line counter (i.e. >>>>> vertical timings) only. >>>>> >>>>> If it's just that it should work, and I don't think we have any other >>>>> users of the adjusted_mode. >>>> Ok, I was wrong and we obviously need the right dotclock to compute >>>> linedur_ns correctly in drm_calc_timestamping_constants(). So either >>>> we adjust the dotclock of adjusted_mode too (imo makes most sense), or >>>> we need yet another mode somewhere and use that for dsi cross checking >>>> (real ugly imo). More I missed? >>> Another point to be considered: >>> And we program hsync, hfp and hbp in terms of txbyteclkhs to port >>> register, >>> which are not part of adjusted_mode. >>> So for BXT DSI, we have to store them interms of txbyteclkhs and >>> compare >>> with hsync, hfp and hbp read from HW?? >> Yeah that's my idea. Plus we should probably store txbyteclkhs >> somewhere, >> too. Or at least we need to adjust the clock in adjusted_mode to match >> txbyteclkhs, otherwise the vblank ts code goes off the rails. > Ok. I will post a RFC for this. But for that we need to finalize few > points. > > 1. We need to add three more variables in pipe_config for caching(SW > and HW State) the hfp, hsync and hbp. > 2. On BXT DSI, We will compare the hdisplay, hfp, hsync and hbp only > on pipe_config_compare not all horizontal timing param of adjusted mode. > 3. So we dont need to recalculate all horizontal timing parameters of > adjusted_mode from port register at get_config(). Should we fill them > too (No harm Though)? > > Please clarify if we are fine with above points. Hi, Based on the IRC discussion, we decided to nullify the round_up error in the dsi_get_config() itself. I have taken the AR. And the RFC is submitted for review at https://lists.freedesktop.org/archives/intel-gfx/2016-April/092946.html https://lists.freedesktop.org/archives/intel-gfx/2016-April/092947.html Please review the same. Thanks --Ram > > -Ram >> -Daniel > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison 2016-04-05 8:30 ` Jani Nikula 2016-04-05 9:40 ` Ramalingam C @ 2016-04-13 10:05 ` Daniel Vetter 1 sibling, 0 replies; 29+ messages in thread From: Daniel Vetter @ 2016-04-13 10:05 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Tue, Apr 05, 2016 at 11:30:19AM +0300, 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. Yup, that's the idea. And the idea furthermore is to store _exactly_ the timings we store in the dsi controller (including clock fudging and whatever) and _not_ try to compute things backwards. The idea behind the hw state checker is that you take the requested mode and compute forward until you are at the values you can program into the hw. Currently what we have in ->adjusted_mode is not actually what we program into the hw, so not yet good enough. Then the hw state verifier only compares things 1:1. Clocks are a bit an exception, for special reasons. But for anything else we shouldn't ever try to reconstruct the original requested mode from userspace, due to aliasing issues and rounding problems. This means that for dsi ->adjusted_mode will have even less to do with the requested mode from userspace, but that's no issue. We only use that internally to track the actual hw state. -Daniel > > 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(¤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); > >>>>> 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/BXT: Get pipe conf from the port registers 2016-03-29 17:34 [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C 2016-03-29 17:34 ` [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison Ramalingam C @ 2016-03-30 6:14 ` Patchwork 2016-03-30 10:43 ` [PATCH 1/2] " Jani Nikula 2016-03-31 12:51 ` ✗ Fi.CI.BAT: failure for series starting with [2/2] drm/i915/BXT: Get pipe conf from the port registers (rev3) Patchwork 3 siblings, 0 replies; 29+ messages in thread From: Patchwork @ 2016-03-30 6:14 UTC (permalink / raw) To: Ramalingam C; +Cc: intel-gfx == Series Details == Series: series starting with [1/2] drm/i915/BXT: Get pipe conf from the port registers URL : https://patchwork.freedesktop.org/series/5006/ State : failure == Summary == LD drivers/video/built-in.o CC drivers/usb/host/xhci-trace.o CC [M] drivers/net/ethernet/intel/e1000e/nvm.o CC [M] drivers/net/ethernet/intel/e1000e/phy.o CC drivers/usb/host/xhci-pci.o CC [M] drivers/net/ethernet/intel/e1000e/param.o CC [M] drivers/net/ethernet/intel/e1000e/ethtool.o CC [M] drivers/net/ethernet/intel/e1000e/netdev.o LD [M] drivers/net/ethernet/intel/e1000/e1000.o CC [M] drivers/net/ethernet/intel/e1000e/ptp.o LD [M] drivers/net/ethernet/intel/igbvf/igbvf.o LD drivers/usb/host/xhci-hcd.o LD drivers/usb/host/built-in.o LD drivers/usb/built-in.o LD [M] drivers/net/ethernet/intel/igb/igb.o cc1: some warnings being treated as errors scripts/Makefile.build:291: recipe for target 'drivers/gpu/drm/i915/intel_display.o' failed make[4]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1 scripts/Makefile.build:440: recipe for target 'drivers/gpu/drm/i915' failed make[3]: *** [drivers/gpu/drm/i915] Error 2 scripts/Makefile.build:440: recipe for target 'drivers/gpu/drm' failed make[2]: *** [drivers/gpu/drm] Error 2 scripts/Makefile.build:440: recipe for target 'drivers/gpu' failed make[1]: *** [drivers/gpu] Error 2 make[1]: *** Waiting for unfinished jobs.... LD [M] drivers/net/ethernet/intel/e1000e/e1000e.o LD drivers/net/ethernet/built-in.o LD drivers/net/built-in.o Makefile:962: recipe for target 'drivers' failed make: *** [drivers] Error 2 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers 2016-03-29 17:34 [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C 2016-03-29 17:34 ` [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison Ramalingam C 2016-03-30 6:14 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/BXT: Get pipe conf from the port registers Patchwork @ 2016-03-30 10:43 ` Jani Nikula 2016-03-30 13:28 ` Ramalingam C 2016-03-31 12:51 ` ✗ Fi.CI.BAT: failure for series starting with [2/2] drm/i915/BXT: Get pipe conf from the port registers (rev3) Patchwork 3 siblings, 1 reply; 29+ messages in thread From: Jani Nikula @ 2016-03-30 10:43 UTC (permalink / raw) To: Ramalingam C, intel-gfx On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c@intel.com> 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. > > The required changes are added for BXT in intel_dsi_get_config > (encoder->get_config). > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > Previously reviewed at https://patchwork.freedesktop.org/patch/75301/ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++ > drivers/gpu/drm/i915/intel_dsi.c | 104 ++++++++++++++++++++++++++++++++++ > 3 files changed, 149 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c839ce9..da3cdef 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -8210,6 +8210,7 @@ enum skl_disp_power_wells { > #define BXT_PIPE_SELECT_SHIFT 7 > #define BXT_PIPE_SELECT_MASK (7 << 7) > #define BXT_PIPE_SELECT(pipe) ((pipe) << 7) > +#define BXT_PORT_TO_PIPE(ctrl) ((ctrl & BXT_PIPE_SELECT_MASK) >> 7) > > #define _MIPIA_DATA_ADDRESS (dev_priv->mipi_mmio_base + 0xb108) > #define _MIPIC_DATA_ADDRESS (dev_priv->mipi_mmio_base + 0xb908) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 29aa64b..c0627d6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9941,11 +9941,40 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc, > } > } > > +struct intel_encoder *bxt_get_dsi_encoder_for_crtc(struct intel_crtc *crtc, > + struct intel_crtc_state *pipe_config) > +{ > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_encoder *intel_encoder; > + struct intel_dsi *intel_dsi; > + enum port port; > + u32 tmp; > + > + for_each_intel_encoder(dev, intel_encoder) { > + if (intel_encoder->type == INTEL_OUTPUT_DSI) { > + intel_dsi = enc_to_intel_dsi(&intel_encoder->base); > + for_each_dsi_port(port, intel_dsi->ports) { > + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & > + DPI_ENABLE)) > + break; > + > + tmp = I915_READ(MIPI_CTRL(port)); > + if ((tmp & BXT_PIPE_SELECT_MASK) == > + BXT_PIPE_SELECT(crtc->pipe)) > + return intel_encoder; > + } > + } > + } > + return NULL; > +} > + > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config) > { > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_encoder *intel_encoder, *attached_encoder = NULL; > enum intel_display_power_domain power_domain; > unsigned long power_domain_mask; > bool active; > @@ -9965,6 +9994,21 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > WARN_ON(active && pipe_config->has_dsi_encoder); > if (pipe_config->has_dsi_encoder) > active = true; > + > + for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder) > + attached_encoder = intel_encoder; > + > + /* > + * attached_encoder will be NULL, if there is no modeset from > + * the kernel bootup. > + */ > + if (!attached_encoder && pipe_config->has_dsi_encoder) > + attached_encoder = > + bxt_get_dsi_encoder_for_crtc(crtc, pipe_config); > + > + if (attached_encoder && attached_encoder->get_config) > + attached_encoder->get_config(attached_encoder, > + pipe_config); No, you must not add a new call to the encoder->get_config() hook. haswell_get_pipe_config() is called through the dev_priv->display.get_pipe_config() function pointer. This happens in check_crtc_state() and intel_modeset_readout_hw_state(). In both places, encoder->get_config() is called afterwards, if encoder->get_hw_state() returns true for the encoder. The infrastructure is there, you only need to update DSI ->get_config(). > } > > if (!active) > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 0de74e1..69a801e 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -740,14 +740,118 @@ out_put_power: > return active; > } > > +/* return pixels equvalent to txbyteclkhs */ > +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count, > + u16 burst_mode_ratio) > +{ > + return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100), > + (bpp * burst_mode_ratio)); > +} > + > +static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder, > + struct intel_crtc_state *pipe_config) > +{ > + struct drm_device *dev = encoder->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_display_mode *adjusted_mode = > + &pipe_config->base.adjusted_mode; > + struct intel_dsi *intel_dsi = > + enc_to_intel_dsi(&encoder->base); > + unsigned int bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); > + unsigned int lane_count = intel_dsi->lane_count; > + enum port port; > + enum pipe dsi_pipe; > + u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp; > + uint32_t tmp; > + > + for_each_dsi_port(port, intel_dsi->ports) { > + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE)) > + continue; Please use the loop just for figuring out the port to use for reading the registers. After that, you can do the rest at a lower indentation level, and it's clearer the rest only gets done once, for one port. > + > + /* In terms of pixels */ > + adjusted_mode->crtc_hdisplay = > + I915_READ(BXT_MIPI_TRANS_HACTIVE(port)); > + adjusted_mode->crtc_vdisplay = > + I915_READ(BXT_MIPI_TRANS_VACTIVE(port)); > + adjusted_mode->crtc_vtotal = > + I915_READ(BXT_MIPI_TRANS_VTOTAL(port)); > + > + hactive = adjusted_mode->crtc_hdisplay; > + hfp = I915_READ(MIPI_HFP_COUNT(port)); > + > + /* > + * meaningful for video mode non-burst sync pulse mode only, > + * can be zero for non-burst sync events and burst modes > + */ > + hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port)); > + hbp = I915_READ(MIPI_HBP_COUNT(port)); > + > + /* horizontal values are in terms of high speed byte clock */ > + hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + > + if (intel_dsi->dual_link) { > + hfp *= 2; > + hsync *= 2; > + hbp *= 2; > + } > + > + /* vertical values are in terms of lines */ > + vfp = I915_READ(MIPI_VFP_COUNT(port)); > + vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port)); > + vbp = I915_READ(MIPI_VBP_COUNT(port)); > + > + adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp; > + adjusted_mode->crtc_hsync_start = > + hfp + adjusted_mode->crtc_hdisplay; > + adjusted_mode->crtc_hsync_end = > + hsync + adjusted_mode->crtc_hsync_start; > + adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay; > + adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal; > + > + adjusted_mode->crtc_vsync_start = > + vfp + adjusted_mode->crtc_vdisplay; > + adjusted_mode->crtc_vsync_end = > + vsync + adjusted_mode->crtc_vsync_start; > + adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay; > + adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; > + > + pipe_config->pipe_bpp = > + mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); I guess this should be read from the register. > + > + dsi_pipe = BXT_PORT_TO_PIPE(I915_READ(MIPI_CTRL(port))); > + if (dsi_pipe > PIPE_C) { > + DRM_ERROR("Invalid PIPE configured\n"); > + break; > + } > + > + tmp = I915_READ(PIPESRC(dsi_pipe)); > + pipe_config->pipe_src_h = (tmp & 0xffff) + 1; > + pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1; > + > + pipe_config->base.mode.vdisplay = pipe_config->pipe_src_h; > + pipe_config->base.mode.hdisplay = pipe_config->pipe_src_w; This part is already covered by intel_get_pipe_src_size() for DSI. BR, Jani. > + break; > + } > +} > + > + > static void intel_dsi_get_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config) > { > + struct drm_device *dev = encoder->base.dev; > u32 pclk; > DRM_DEBUG_KMS("\n"); > > pipe_config->has_dsi_encoder = true; > > + if (IS_BROXTON(dev)) > + bxt_dsi_get_pipe_config(encoder, pipe_config); > + > /* > * DPLL_MD is not used in case of DSI, reading will get some default value > * set dpll_md = 0 -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers 2016-03-30 10:43 ` [PATCH 1/2] " Jani Nikula @ 2016-03-30 13:28 ` Ramalingam C 2016-03-30 13:53 ` [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Ramalingam C 0 siblings, 1 reply; 29+ messages in thread From: Ramalingam C @ 2016-03-30 13:28 UTC (permalink / raw) To: Jani Nikula, intel-gfx Jani, Thanks for the review comments. Addressing them in the next patch ver. On Wednesday 30 March 2016 04:13 PM, Jani Nikula wrote: > On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c@intel.com> 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. >> >> The required changes are added for BXT in intel_dsi_get_config >> (encoder->get_config). >> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> --- >> Previously reviewed at https://patchwork.freedesktop.org/patch/75301/ >> >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++ >> drivers/gpu/drm/i915/intel_dsi.c | 104 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 149 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index c839ce9..da3cdef 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -8210,6 +8210,7 @@ enum skl_disp_power_wells { >> #define BXT_PIPE_SELECT_SHIFT 7 >> #define BXT_PIPE_SELECT_MASK (7 << 7) >> #define BXT_PIPE_SELECT(pipe) ((pipe) << 7) >> +#define BXT_PORT_TO_PIPE(ctrl) ((ctrl & BXT_PIPE_SELECT_MASK) >> 7) >> >> #define _MIPIA_DATA_ADDRESS (dev_priv->mipi_mmio_base + 0xb108) >> #define _MIPIC_DATA_ADDRESS (dev_priv->mipi_mmio_base + 0xb908) >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 29aa64b..c0627d6 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9941,11 +9941,40 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc, >> } >> } >> >> +struct intel_encoder *bxt_get_dsi_encoder_for_crtc(struct intel_crtc *crtc, >> + struct intel_crtc_state *pipe_config) >> +{ >> + struct drm_device *dev = crtc->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_encoder *intel_encoder; >> + struct intel_dsi *intel_dsi; >> + enum port port; >> + u32 tmp; >> + >> + for_each_intel_encoder(dev, intel_encoder) { >> + if (intel_encoder->type == INTEL_OUTPUT_DSI) { >> + intel_dsi = enc_to_intel_dsi(&intel_encoder->base); >> + for_each_dsi_port(port, intel_dsi->ports) { >> + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & >> + DPI_ENABLE)) >> + break; >> + >> + tmp = I915_READ(MIPI_CTRL(port)); >> + if ((tmp & BXT_PIPE_SELECT_MASK) == >> + BXT_PIPE_SELECT(crtc->pipe)) >> + return intel_encoder; >> + } >> + } >> + } >> + return NULL; >> +} >> + >> static bool haswell_get_pipe_config(struct intel_crtc *crtc, >> struct intel_crtc_state *pipe_config) >> { >> struct drm_device *dev = crtc->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_encoder *intel_encoder, *attached_encoder = NULL; >> enum intel_display_power_domain power_domain; >> unsigned long power_domain_mask; >> bool active; >> @@ -9965,6 +9994,21 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, >> WARN_ON(active && pipe_config->has_dsi_encoder); >> if (pipe_config->has_dsi_encoder) >> active = true; >> + >> + for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder) >> + attached_encoder = intel_encoder; >> + >> + /* >> + * attached_encoder will be NULL, if there is no modeset from >> + * the kernel bootup. >> + */ >> + if (!attached_encoder && pipe_config->has_dsi_encoder) >> + attached_encoder = >> + bxt_get_dsi_encoder_for_crtc(crtc, pipe_config); >> + >> + if (attached_encoder && attached_encoder->get_config) >> + attached_encoder->get_config(attached_encoder, >> + pipe_config); > No, you must not add a new call to the encoder->get_config() hook. > > haswell_get_pipe_config() is called through the > dev_priv->display.get_pipe_config() function pointer. This happens in > check_crtc_state() and intel_modeset_readout_hw_state(). In both places, > encoder->get_config() is called afterwards, if encoder->get_hw_state() > returns true for the encoder. > > The infrastructure is there, you only need to update DSI ->get_config(). Thanks for catching such a redundant lines. dropping them. > >> } >> >> if (!active) >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index 0de74e1..69a801e 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -740,14 +740,118 @@ out_put_power: >> return active; >> } >> >> +/* return pixels equvalent to txbyteclkhs */ >> +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count, >> + u16 burst_mode_ratio) >> +{ >> + return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100), >> + (bpp * burst_mode_ratio)); >> +} >> + >> +static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder, >> + struct intel_crtc_state *pipe_config) >> +{ >> + struct drm_device *dev = encoder->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct drm_display_mode *adjusted_mode = >> + &pipe_config->base.adjusted_mode; >> + struct intel_dsi *intel_dsi = >> + enc_to_intel_dsi(&encoder->base); >> + unsigned int bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); >> + unsigned int lane_count = intel_dsi->lane_count; >> + enum port port; >> + enum pipe dsi_pipe; >> + u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp; >> + uint32_t tmp; >> + >> + for_each_dsi_port(port, intel_dsi->ports) { >> + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE)) >> + continue; > Please use the loop just for figuring out the port to use for reading > the registers. After that, you can do the rest at a lower indentation > level, and it's clearer the rest only gets done once, for one port. I take this suggestion for reducing the indentation level. including it next patch ver. > >> + >> + /* In terms of pixels */ >> + adjusted_mode->crtc_hdisplay = >> + I915_READ(BXT_MIPI_TRANS_HACTIVE(port)); >> + adjusted_mode->crtc_vdisplay = >> + I915_READ(BXT_MIPI_TRANS_VACTIVE(port)); >> + adjusted_mode->crtc_vtotal = >> + I915_READ(BXT_MIPI_TRANS_VTOTAL(port)); >> + >> + hactive = adjusted_mode->crtc_hdisplay; >> + hfp = I915_READ(MIPI_HFP_COUNT(port)); >> + >> + /* >> + * meaningful for video mode non-burst sync pulse mode only, >> + * can be zero for non-burst sync events and burst modes >> + */ >> + hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port)); >> + hbp = I915_READ(MIPI_HBP_COUNT(port)); >> + >> + /* horizontal values are in terms of high speed byte clock */ >> + hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count, >> + intel_dsi->burst_mode_ratio); >> + hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count, >> + intel_dsi->burst_mode_ratio); >> + hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count, >> + intel_dsi->burst_mode_ratio); >> + >> + if (intel_dsi->dual_link) { >> + hfp *= 2; >> + hsync *= 2; >> + hbp *= 2; >> + } >> + >> + /* vertical values are in terms of lines */ >> + vfp = I915_READ(MIPI_VFP_COUNT(port)); >> + vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port)); >> + vbp = I915_READ(MIPI_VBP_COUNT(port)); >> + >> + adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp; >> + adjusted_mode->crtc_hsync_start = >> + hfp + adjusted_mode->crtc_hdisplay; >> + adjusted_mode->crtc_hsync_end = >> + hsync + adjusted_mode->crtc_hsync_start; >> + adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay; >> + adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal; >> + >> + adjusted_mode->crtc_vsync_start = >> + vfp + adjusted_mode->crtc_vdisplay; >> + adjusted_mode->crtc_vsync_end = >> + vsync + adjusted_mode->crtc_vsync_start; >> + adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay; >> + adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; >> + >> + pipe_config->pipe_bpp = >> + mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); > I guess this should be read from the register. Can be read from the port register. Will do it. > >> + >> + dsi_pipe = BXT_PORT_TO_PIPE(I915_READ(MIPI_CTRL(port))); >> + if (dsi_pipe > PIPE_C) { >> + DRM_ERROR("Invalid PIPE configured\n"); >> + break; >> + } >> + >> + tmp = I915_READ(PIPESRC(dsi_pipe)); >> + pipe_config->pipe_src_h = (tmp & 0xffff) + 1; >> + pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1; >> + >> + pipe_config->base.mode.vdisplay = pipe_config->pipe_src_h; >> + pipe_config->base.mode.hdisplay = pipe_config->pipe_src_w; > This part is already covered by intel_get_pipe_src_size() for DSI. Dropping those lines from get_config. > > > BR, > Jani. > > >> + break; >> + } >> +} >> + >> + >> static void intel_dsi_get_config(struct intel_encoder *encoder, >> struct intel_crtc_state *pipe_config) >> { >> + struct drm_device *dev = encoder->base.dev; >> u32 pclk; >> DRM_DEBUG_KMS("\n"); >> >> pipe_config->has_dsi_encoder = true; >> >> + if (IS_BROXTON(dev)) >> + bxt_dsi_get_pipe_config(encoder, pipe_config); >> + >> /* >> * DPLL_MD is not used in case of DSI, reading will get some default value >> * set dpll_md = 0 -- Thanks, --Ram _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 2016-03-30 13:28 ` Ramalingam C @ 2016-03-30 13:53 ` Ramalingam C 2016-03-30 13:53 ` [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C 2016-04-06 11:37 ` [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Jani Nikula 0 siblings, 2 replies; 29+ messages in thread From: Ramalingam C @ 2016-03-30 13:53 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula Shared the function pixel_format_from_vbt for whole display module. Function declaration is added to intel_dsi.h. Signed-off-by: Ramalingam C <ramalingam.c@intel.com> --- drivers/gpu/drm/i915/intel_dsi.h | 1 + drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index ec58ead..9612916 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -126,6 +126,7 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) return container_of(encoder, struct intel_dsi, base.base); } +enum mipi_dsi_pixel_format pixel_format_from_vbt(u32 fmt); bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv); extern void intel_enable_dsi_pll(struct intel_encoder *encoder); extern void intel_disable_dsi_pll(struct intel_encoder *encoder); diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c index 8302a97..d78d59c 100644 --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c @@ -413,7 +413,7 @@ static const struct drm_panel_funcs vbt_panel_funcs = { }; /* XXX: This should be done when parsing the VBT in intel_bios.c */ -static enum mipi_dsi_pixel_format pixel_format_from_vbt(u32 fmt) +enum mipi_dsi_pixel_format pixel_format_from_vbt(u32 fmt) { /* It just so happens the VBT matches register contents. */ switch (fmt) { -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers 2016-03-30 13:53 ` [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Ramalingam C @ 2016-03-30 13:53 ` Ramalingam C 2016-04-04 9:18 ` Ramalingam C 2016-04-06 11:45 ` Jani Nikula 2016-04-06 11:37 ` [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Jani Nikula 1 sibling, 2 replies; 29+ messages in thread From: Ramalingam C @ 2016-03-30 13:53 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula 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. The required changes are added for BXT in intel_dsi_get_config (encoder->get_config). v2: Addressed the Jani's comments -removed the redundant call to encoder->get_config -read bpp from port register -removed retrival of src_size from encoder->get_config Signed-off-by: Ramalingam C <ramalingam.c@intel.com> Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- Previously reviewed at https://patchwork.freedesktop.org/patch/75301/ drivers/gpu/drm/i915/intel_dsi.c | 99 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 0de74e1..2117187 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -46,6 +46,11 @@ static const struct { }, }; +enum mipi_dsi_pixel_format reg_to_pixel_format(u32 fmt) +{ + return pixel_format_from_vbt(fmt); +} + static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port) { struct drm_encoder *encoder = &intel_dsi->base.base; @@ -740,14 +745,108 @@ out_put_power: return active; } +/* return pixels equvalent to txbyteclkhs */ +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count, + u16 burst_mode_ratio) +{ + return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100), + (bpp * burst_mode_ratio)); +} + +static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder, + struct intel_crtc_state *pipe_config) +{ + struct drm_device *dev = encoder->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_display_mode *adjusted_mode = + &pipe_config->base.adjusted_mode; + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); + unsigned int lane_count = intel_dsi->lane_count; + unsigned int bpp, fmt; + enum port port; + u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp; + + /* + * Atleast one port is active as encoder->get_config called only if + * encoder->get_hw_state() returns true. + */ + for_each_dsi_port(port, intel_dsi->ports) { + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE)) + continue; + break; + } + + fmt = I915_READ(MIPI_DSI_FUNC_PRG(port)) & VID_MODE_FORMAT_MASK; + pipe_config->pipe_bpp = reg_to_pixel_format(fmt); + + bpp = mipi_dsi_pixel_format_to_bpp(pipe_config->pipe_bpp); + + /* In terms of pixels */ + adjusted_mode->crtc_hdisplay = + I915_READ(BXT_MIPI_TRANS_HACTIVE(port)); + adjusted_mode->crtc_vdisplay = + I915_READ(BXT_MIPI_TRANS_VACTIVE(port)); + adjusted_mode->crtc_vtotal = + I915_READ(BXT_MIPI_TRANS_VTOTAL(port)); + + hactive = adjusted_mode->crtc_hdisplay; + hfp = I915_READ(MIPI_HFP_COUNT(port)); + + /* + * meaningful for video mode non-burst sync pulse mode only, + * can be zero for non-burst sync events and burst modes + */ + hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port)); + hbp = I915_READ(MIPI_HBP_COUNT(port)); + + /* horizontal values are in terms of high speed byte clock */ + hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count, + intel_dsi->burst_mode_ratio); + hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count, + intel_dsi->burst_mode_ratio); + hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count, + intel_dsi->burst_mode_ratio); + + if (intel_dsi->dual_link) { + hfp *= 2; + hsync *= 2; + hbp *= 2; + } + + /* vertical values are in terms of lines */ + vfp = I915_READ(MIPI_VFP_COUNT(port)); + vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port)); + vbp = I915_READ(MIPI_VBP_COUNT(port)); + + adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp; + adjusted_mode->crtc_hsync_start = + hfp + adjusted_mode->crtc_hdisplay; + adjusted_mode->crtc_hsync_end = + hsync + adjusted_mode->crtc_hsync_start; + adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay; + adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal; + + adjusted_mode->crtc_vsync_start = + vfp + adjusted_mode->crtc_vdisplay; + adjusted_mode->crtc_vsync_end = + vsync + adjusted_mode->crtc_vsync_start; + adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay; + adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; +} + + static void intel_dsi_get_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config) { + struct drm_device *dev = encoder->base.dev; u32 pclk; DRM_DEBUG_KMS("\n"); pipe_config->has_dsi_encoder = true; + if (IS_BROXTON(dev)) + bxt_dsi_get_pipe_config(encoder, pipe_config); + /* * DPLL_MD is not used in case of DSI, reading will get some default value * set dpll_md = 0 -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers 2016-03-30 13:53 ` [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C @ 2016-04-04 9:18 ` Ramalingam C 2016-04-06 11:45 ` Jani Nikula 1 sibling, 0 replies; 29+ messages in thread From: Ramalingam C @ 2016-04-04 9:18 UTC (permalink / raw) To: intel-gfx, jani.nikula, daniel@ffwll.ch >> Daniel Vetter Jani/Daniel, I am working on implementing the pipe_config compare as suggested by daniel at https://lists.freedesktop.org/archives/intel-gfx/2016-March/091148.html But I think this patch need not wait for that change. Either way this patch is required. We can continue review on this and proceed further. On Wednesday 30 March 2016 07:23 PM, 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. > > The required changes are added for BXT in intel_dsi_get_config > (encoder->get_config). > > v2: Addressed the Jani's comments > -removed the redundant call to encoder->get_config > -read bpp from port register > -removed retrival of src_size from encoder->get_config > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > Previously reviewed at https://patchwork.freedesktop.org/patch/75301/ > > drivers/gpu/drm/i915/intel_dsi.c | 99 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 0de74e1..2117187 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -46,6 +46,11 @@ static const struct { > }, > }; > > +enum mipi_dsi_pixel_format reg_to_pixel_format(u32 fmt) > +{ > + return pixel_format_from_vbt(fmt); > +} > + > static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port) > { > struct drm_encoder *encoder = &intel_dsi->base.base; > @@ -740,14 +745,108 @@ out_put_power: > return active; > } > > +/* return pixels equvalent to txbyteclkhs */ > +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count, > + u16 burst_mode_ratio) > +{ > + return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100), > + (bpp * burst_mode_ratio)); > +} > + > +static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder, > + struct intel_crtc_state *pipe_config) > +{ > + struct drm_device *dev = encoder->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_display_mode *adjusted_mode = > + &pipe_config->base.adjusted_mode; > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + unsigned int lane_count = intel_dsi->lane_count; > + unsigned int bpp, fmt; > + enum port port; > + u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp; > + > + /* > + * Atleast one port is active as encoder->get_config called only if > + * encoder->get_hw_state() returns true. > + */ > + for_each_dsi_port(port, intel_dsi->ports) { > + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE)) > + continue; > + break; > + } > + > + fmt = I915_READ(MIPI_DSI_FUNC_PRG(port)) & VID_MODE_FORMAT_MASK; > + pipe_config->pipe_bpp = reg_to_pixel_format(fmt); > + > + bpp = mipi_dsi_pixel_format_to_bpp(pipe_config->pipe_bpp); > + > + /* In terms of pixels */ > + adjusted_mode->crtc_hdisplay = > + I915_READ(BXT_MIPI_TRANS_HACTIVE(port)); > + adjusted_mode->crtc_vdisplay = > + I915_READ(BXT_MIPI_TRANS_VACTIVE(port)); > + adjusted_mode->crtc_vtotal = > + I915_READ(BXT_MIPI_TRANS_VTOTAL(port)); > + > + hactive = adjusted_mode->crtc_hdisplay; > + hfp = I915_READ(MIPI_HFP_COUNT(port)); > + > + /* > + * meaningful for video mode non-burst sync pulse mode only, > + * can be zero for non-burst sync events and burst modes > + */ > + hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port)); > + hbp = I915_READ(MIPI_HBP_COUNT(port)); > + > + /* horizontal values are in terms of high speed byte clock */ > + hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + > + if (intel_dsi->dual_link) { > + hfp *= 2; > + hsync *= 2; > + hbp *= 2; > + } > + > + /* vertical values are in terms of lines */ > + vfp = I915_READ(MIPI_VFP_COUNT(port)); > + vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port)); > + vbp = I915_READ(MIPI_VBP_COUNT(port)); > + > + adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp; > + adjusted_mode->crtc_hsync_start = > + hfp + adjusted_mode->crtc_hdisplay; > + adjusted_mode->crtc_hsync_end = > + hsync + adjusted_mode->crtc_hsync_start; > + adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay; > + adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal; > + > + adjusted_mode->crtc_vsync_start = > + vfp + adjusted_mode->crtc_vdisplay; > + adjusted_mode->crtc_vsync_end = > + vsync + adjusted_mode->crtc_vsync_start; > + adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay; > + adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; > +} > + > + > static void intel_dsi_get_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config) > { > + struct drm_device *dev = encoder->base.dev; > u32 pclk; > DRM_DEBUG_KMS("\n"); > > pipe_config->has_dsi_encoder = true; > > + if (IS_BROXTON(dev)) > + bxt_dsi_get_pipe_config(encoder, pipe_config); > + > /* > * DPLL_MD is not used in case of DSI, reading will get some default value > * set dpll_md = 0 -- Thanks, --Ram _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers 2016-03-30 13:53 ` [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C 2016-04-04 9:18 ` Ramalingam C @ 2016-04-06 11:45 ` Jani Nikula 1 sibling, 0 replies; 29+ messages in thread From: Jani Nikula @ 2016-04-06 11:45 UTC (permalink / raw) To: Ramalingam C, intel-gfx On Wed, 30 Mar 2016, Ramalingam C <ramalingam.c@intel.com> 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. > > The required changes are added for BXT in intel_dsi_get_config > (encoder->get_config). > > v2: Addressed the Jani's comments > -removed the redundant call to encoder->get_config > -read bpp from port register > -removed retrival of src_size from encoder->get_config > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > Previously reviewed at https://patchwork.freedesktop.org/patch/75301/ > > drivers/gpu/drm/i915/intel_dsi.c | 99 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 0de74e1..2117187 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -46,6 +46,11 @@ static const struct { > }, > }; > > +enum mipi_dsi_pixel_format reg_to_pixel_format(u32 fmt) > +{ > + return pixel_format_from_vbt(fmt); > +} > + See reply to preceding patch. > static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port) > { > struct drm_encoder *encoder = &intel_dsi->base.base; > @@ -740,14 +745,108 @@ out_put_power: > return active; > } > > +/* return pixels equvalent to txbyteclkhs */ > +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count, > + u16 burst_mode_ratio) > +{ > + return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100), > + (bpp * burst_mode_ratio)); > +} > + > +static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder, > + struct intel_crtc_state *pipe_config) > +{ > + struct drm_device *dev = encoder->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_display_mode *adjusted_mode = > + &pipe_config->base.adjusted_mode; > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + unsigned int lane_count = intel_dsi->lane_count; > + unsigned int bpp, fmt; > + enum port port; > + u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp; > + > + /* > + * Atleast one port is active as encoder->get_config called only if > + * encoder->get_hw_state() returns true. > + */ > + for_each_dsi_port(port, intel_dsi->ports) { > + if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE)) > + continue; > + break; In other words, if (I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE) break; > + } > + > + fmt = I915_READ(MIPI_DSI_FUNC_PRG(port)) & VID_MODE_FORMAT_MASK; > + pipe_config->pipe_bpp = reg_to_pixel_format(fmt); > + > + bpp = mipi_dsi_pixel_format_to_bpp(pipe_config->pipe_bpp); > + > + /* In terms of pixels */ > + adjusted_mode->crtc_hdisplay = > + I915_READ(BXT_MIPI_TRANS_HACTIVE(port)); > + adjusted_mode->crtc_vdisplay = > + I915_READ(BXT_MIPI_TRANS_VACTIVE(port)); > + adjusted_mode->crtc_vtotal = > + I915_READ(BXT_MIPI_TRANS_VTOTAL(port)); > + > + hactive = adjusted_mode->crtc_hdisplay; > + hfp = I915_READ(MIPI_HFP_COUNT(port)); > + > + /* > + * meaningful for video mode non-burst sync pulse mode only, > + * can be zero for non-burst sync events and burst modes > + */ > + hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port)); > + hbp = I915_READ(MIPI_HBP_COUNT(port)); > + > + /* horizontal values are in terms of high speed byte clock */ > + hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count, > + intel_dsi->burst_mode_ratio); > + > + if (intel_dsi->dual_link) { > + hfp *= 2; > + hsync *= 2; > + hbp *= 2; > + } > + > + /* vertical values are in terms of lines */ > + vfp = I915_READ(MIPI_VFP_COUNT(port)); > + vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port)); > + vbp = I915_READ(MIPI_VBP_COUNT(port)); > + > + adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp; > + adjusted_mode->crtc_hsync_start = > + hfp + adjusted_mode->crtc_hdisplay; > + adjusted_mode->crtc_hsync_end = > + hsync + adjusted_mode->crtc_hsync_start; > + adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay; > + adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal; > + > + adjusted_mode->crtc_vsync_start = > + vfp + adjusted_mode->crtc_vdisplay; > + adjusted_mode->crtc_vsync_end = > + vsync + adjusted_mode->crtc_vsync_start; > + adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay; > + adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal; As I suggested on IRC, to make progress please keep everything that we can calculate accurately (and hence get no warnings about the diff for them) and leave out all that is not accurate. We need to get back to them too, but currently there seems to be disagreement on what is the best/feasible/possible option. BR, Jani. > +} > + > + > static void intel_dsi_get_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config) > { > + struct drm_device *dev = encoder->base.dev; > u32 pclk; > DRM_DEBUG_KMS("\n"); > > pipe_config->has_dsi_encoder = true; > > + if (IS_BROXTON(dev)) > + bxt_dsi_get_pipe_config(encoder, pipe_config); > + > /* > * DPLL_MD is not used in case of DSI, reading will get some default value > * set dpll_md = 0 -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 2016-03-30 13:53 ` [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Ramalingam C 2016-03-30 13:53 ` [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C @ 2016-04-06 11:37 ` Jani Nikula 1 sibling, 0 replies; 29+ messages in thread From: Jani Nikula @ 2016-04-06 11:37 UTC (permalink / raw) To: Ramalingam C, intel-gfx On Wed, 30 Mar 2016, Ramalingam C <ramalingam.c@intel.com> wrote: > Shared the function pixel_format_from_vbt for whole display module. > Function declaration is added to intel_dsi.h. > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/intel_dsi.h | 1 + > drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index ec58ead..9612916 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -126,6 +126,7 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) > return container_of(encoder, struct intel_dsi, base.base); > } > > +enum mipi_dsi_pixel_format pixel_format_from_vbt(u32 fmt); > bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv); > extern void intel_enable_dsi_pll(struct intel_encoder *encoder); > extern void intel_disable_dsi_pll(struct intel_encoder *encoder); > diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > index 8302a97..d78d59c 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > @@ -413,7 +413,7 @@ static const struct drm_panel_funcs vbt_panel_funcs = { > }; > > /* XXX: This should be done when parsing the VBT in intel_bios.c */ > -static enum mipi_dsi_pixel_format pixel_format_from_vbt(u32 fmt) > +enum mipi_dsi_pixel_format pixel_format_from_vbt(u32 fmt) I think this one ends up being nicer if you move the whole function to intel_dsi.c and name it according to being a function to convert the pixel format from the *register* data, not vbt. BR, Jani. > { > /* It just so happens the VBT matches register contents. */ > switch (fmt) { -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [2/2] drm/i915/BXT: Get pipe conf from the port registers (rev3) 2016-03-29 17:34 [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C ` (2 preceding siblings ...) 2016-03-30 10:43 ` [PATCH 1/2] " Jani Nikula @ 2016-03-31 12:51 ` Patchwork 3 siblings, 0 replies; 29+ messages in thread From: Patchwork @ 2016-03-31 12:51 UTC (permalink / raw) To: Ramalingam C; +Cc: intel-gfx == Series Details == Series: series starting with [2/2] drm/i915/BXT: Get pipe conf from the port registers (rev3) URL : https://patchwork.freedesktop.org/series/5006/ State : failure == Summary == CC [M] drivers/net/ethernet/intel/igb/e1000_mac.o CC [M] drivers/net/ethernet/intel/e1000e/netdev.o CC [M] drivers/net/ethernet/intel/igb/e1000_nvm.o CC [M] drivers/net/ethernet/intel/e1000e/ptp.o CC [M] drivers/net/ethernet/intel/igb/e1000_phy.o CC [M] drivers/net/ethernet/intel/igb/e1000_mbx.o CC [M] drivers/net/ethernet/intel/igb/e1000_i210.o CC [M] drivers/net/ethernet/intel/igb/igb_ptp.o CC [M] drivers/net/ethernet/intel/igb/igb_hwmon.o LD drivers/usb/host/xhci-hcd.o LD drivers/usb/host/built-in.o LD drivers/usb/built-in.o LD [M] drivers/net/ethernet/intel/e1000/e1000.o LD [M] drivers/net/ethernet/intel/igbvf/igbvf.o cc1: some warnings being treated as errors scripts/Makefile.build:291: recipe for target 'drivers/gpu/drm/i915/intel_display.o' failed make[4]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1 scripts/Makefile.build:440: recipe for target 'drivers/gpu/drm/i915' failed make[3]: *** [drivers/gpu/drm/i915] Error 2 scripts/Makefile.build:440: recipe for target 'drivers/gpu/drm' failed make[2]: *** [drivers/gpu/drm] Error 2 scripts/Makefile.build:440: recipe for target 'drivers/gpu' failed make[1]: *** [drivers/gpu] Error 2 make[1]: *** Waiting for unfinished jobs.... LD [M] drivers/net/ethernet/intel/igb/igb.o LD [M] drivers/net/ethernet/intel/e1000e/e1000e.o LD drivers/net/ethernet/built-in.o LD drivers/net/built-in.o Makefile:962: recipe for target 'drivers' failed make: *** [drivers] Error 2 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 @ 2016-04-07 9:06 Ramalingam C 0 siblings, 0 replies; 29+ messages in thread From: Ramalingam C @ 2016-04-07 9:06 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula Shared the function pixel_format_from_vbt for whole display module. Function declaration is added to intel_dsi.h. V2: Moved the function to intel_dsi.c and renamed as per the purpose of the function. Suggested by Jani. Signed-off-by: Ramalingam C <ramalingam.c@intel.com> Previously reviewed at https://lists.freedesktop.org/archives/intel-gfx/2016-April/091736.html --- drivers/gpu/drm/i915/intel_dsi.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/intel_dsi.h | 1 + drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 23 +++-------------------- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 0de74e1..a0f374f 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -46,6 +46,24 @@ static const struct { }, }; +enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt) +{ + /* It just so happens the VBT matches register contents. */ + switch (fmt) { + case VID_MODE_FORMAT_RGB888: + return MIPI_DSI_FMT_RGB888; + case VID_MODE_FORMAT_RGB666: + return MIPI_DSI_FMT_RGB666; + case VID_MODE_FORMAT_RGB666_PACKED: + return MIPI_DSI_FMT_RGB666_PACKED; + case VID_MODE_FORMAT_RGB565: + return MIPI_DSI_FMT_RGB565; + default: + MISSING_CASE(fmt); + return MIPI_DSI_FMT_RGB666; + } +} + static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port) { struct drm_encoder *encoder = &intel_dsi->base.base; diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index ec58ead..dabde19 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -134,5 +134,6 @@ extern void intel_dsi_reset_clocks(struct intel_encoder *encoder, enum port port); struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id); +enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt); #endif /* _INTEL_DSI_H */ diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c index 8302a97..ca1b016 100644 --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c @@ -412,25 +412,6 @@ static const struct drm_panel_funcs vbt_panel_funcs = { .get_modes = vbt_panel_get_modes, }; -/* XXX: This should be done when parsing the VBT in intel_bios.c */ -static enum mipi_dsi_pixel_format pixel_format_from_vbt(u32 fmt) -{ - /* It just so happens the VBT matches register contents. */ - switch (fmt) { - case VID_MODE_FORMAT_RGB888: - return MIPI_DSI_FMT_RGB888; - case VID_MODE_FORMAT_RGB666: - return MIPI_DSI_FMT_RGB666; - case VID_MODE_FORMAT_RGB666_PACKED: - return MIPI_DSI_FMT_RGB666_PACKED; - case VID_MODE_FORMAT_RGB565: - return MIPI_DSI_FMT_RGB565; - default: - MISSING_CASE(fmt); - return MIPI_DSI_FMT_RGB666; - } -} - struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id) { struct drm_device *dev = intel_dsi->base.base.dev; @@ -455,7 +436,9 @@ struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id) intel_dsi->eotp_pkt = mipi_config->eot_pkt_disabled ? 0 : 1; intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0; intel_dsi->lane_count = mipi_config->lane_cnt + 1; - intel_dsi->pixel_format = pixel_format_from_vbt(mipi_config->videomode_color_format << 7); + intel_dsi->pixel_format = + pixel_format_from_register_bits( + mipi_config->videomode_color_format << 7); bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format); intel_dsi->dual_link = mipi_config->dual_link; -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-04-19 10:39 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-29 17:34 [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C 2016-03-29 17:34 ` [PATCH 2/2] drm/i915/BXT: Tolerance at BXT DSI pipe_config comparison Ramalingam C 2016-03-29 18:28 ` kbuild test robot 2016-03-30 11:03 ` Jani Nikula 2016-03-30 11:32 ` Daniel Vetter 2016-03-30 14:19 ` Ramalingam C 2016-03-30 19:04 ` Daniel Vetter 2016-04-04 15:43 ` Ramalingam C 2016-04-05 8:30 ` Jani Nikula 2016-04-05 9:40 ` Ramalingam C 2016-04-13 10:06 ` Daniel Vetter 2016-04-13 11:06 ` Jani Nikula 2016-04-13 11:48 ` Daniel Vetter 2016-04-13 11:57 ` Daniel Vetter 2016-04-13 13:04 ` Ramalingam C 2016-04-13 14:46 ` Daniel Vetter 2016-04-15 10:57 ` Ramalingam C 2016-04-19 10:30 ` Ramalingam C 2016-04-13 10:05 ` Daniel Vetter 2016-03-30 6:14 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/BXT: Get pipe conf from the port registers Patchwork 2016-03-30 10:43 ` [PATCH 1/2] " Jani Nikula 2016-03-30 13:28 ` Ramalingam C 2016-03-30 13:53 ` [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Ramalingam C 2016-03-30 13:53 ` [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers Ramalingam C 2016-04-04 9:18 ` Ramalingam C 2016-04-06 11:45 ` Jani Nikula 2016-04-06 11:37 ` [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Jani Nikula 2016-03-31 12:51 ` ✗ Fi.CI.BAT: failure for series starting with [2/2] drm/i915/BXT: Get pipe conf from the port registers (rev3) Patchwork 2016-04-07 9:06 [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915 Ramalingam C
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.