[PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers

[PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers

[Ramalingam C]

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

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

[Ramalingam C]

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(&current_config->name, \
 				    &pipe_config->name,\
@@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
 	PIPE_CONF_CHECK_I(has_dsi_encoder);
 
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
 
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
@@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
 
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
+#undef PIPE_CONF_CHECK_I_RANGE
 #undef PIPE_CONF_CHECK_P
 #undef PIPE_CONF_CHECK_I_ALT
 #undef PIPE_CONF_CHECK_FLAGS
-- 
1.7.9.5

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

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

[kbuild test robot]

[-- 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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/BXT: Get pipe conf from the port registers

[Patchwork]

== 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

Re: [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers

[Jani Nikula]

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

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

[Jani Nikula]

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(&current_config->name, \
>  				    &pipe_config->name,\
> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>  	PIPE_CONF_CHECK_I(has_dsi_encoder);
>  
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);

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

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

[Daniel Vetter]

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(&current_config->name, \
>  				    &pipe_config->name,\
> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>  	PIPE_CONF_CHECK_I(has_dsi_encoder);
>  
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
>  
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>  
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
> +#undef PIPE_CONF_CHECK_I_RANGE
>  #undef PIPE_CONF_CHECK_P
>  #undef PIPE_CONF_CHECK_I_ALT
>  #undef PIPE_CONF_CHECK_FLAGS
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

Re: [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers

[Ramalingam C]

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

[PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915

[Ramalingam C]

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

[PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers

[Ramalingam C]

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

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

[Ramalingam C]

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(&current_config->name, \
>>   				    &pipe_config->name,\
>> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>>   	PIPE_CONF_CHECK_I(has_dsi_encoder);
>>   
>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
>>   
>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
>> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>>   
>>   #undef PIPE_CONF_CHECK_X
>>   #undef PIPE_CONF_CHECK_I
>> +#undef PIPE_CONF_CHECK_I_RANGE
>>   #undef PIPE_CONF_CHECK_P
>>   #undef PIPE_CONF_CHECK_I_ALT
>>   #undef PIPE_CONF_CHECK_FLAGS
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Thanks,
--Ram

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

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

[Daniel Vetter]

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(&current_config->name, \
> >>  				    &pipe_config->name,\
> >>@@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
> >>  	PIPE_CONF_CHECK_I(has_dsi_encoder);
> >>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> >>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> >>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
> >>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
> >>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
> >>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
> >>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
> >>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
> >>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
> >>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
> >>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
> >>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
> >>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
> >>@@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
> >>  #undef PIPE_CONF_CHECK_X
> >>  #undef PIPE_CONF_CHECK_I
> >>+#undef PIPE_CONF_CHECK_I_RANGE
> >>  #undef PIPE_CONF_CHECK_P
> >>  #undef PIPE_CONF_CHECK_I_ALT
> >>  #undef PIPE_CONF_CHECK_FLAGS
> >>-- 
> >>1.7.9.5
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx@lists.freedesktop.org
> >>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Thanks,
> --Ram
> 

-- 
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

✗ Fi.CI.BAT: failure for series starting with [2/2] drm/i915/BXT: Get pipe conf from the port registers (rev3)

[Patchwork]

== 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

Re: [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers

[Ramalingam C]

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

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

[Ramalingam C]

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(&current_config->name, \
>>>>   				    &pipe_config->name,\
>>>> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>   	PIPE_CONF_CHECK_I(has_dsi_encoder);
>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
>>>> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>   #undef PIPE_CONF_CHECK_X
>>>>   #undef PIPE_CONF_CHECK_I
>>>> +#undef PIPE_CONF_CHECK_I_RANGE
>>>>   #undef PIPE_CONF_CHECK_P
>>>>   #undef PIPE_CONF_CHECK_I_ALT
>>>>   #undef PIPE_CONF_CHECK_FLAGS
>>>> -- 
>>>> 1.7.9.5
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Thanks,
>> --Ram
>>

-- 
Thanks,
--Ram

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

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

[Jani Nikula]

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(&current_config->name, \
>>>>>   				    &pipe_config->name,\
>>>>> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>   	PIPE_CONF_CHECK_I(has_dsi_encoder);
>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
>>>>> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>   #undef PIPE_CONF_CHECK_X
>>>>>   #undef PIPE_CONF_CHECK_I
>>>>> +#undef PIPE_CONF_CHECK_I_RANGE
>>>>>   #undef PIPE_CONF_CHECK_P
>>>>>   #undef PIPE_CONF_CHECK_I_ALT
>>>>>   #undef PIPE_CONF_CHECK_FLAGS
>>>>> -- 
>>>>> 1.7.9.5
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> -- 
>>> Thanks,
>>> --Ram
>>>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ramalingam C]

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(&current_config->name, \
>>>>>>    				    &pipe_config->name,\
>>>>>> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>>    	PIPE_CONF_CHECK_I(has_dsi_encoder);
>>>>>>    	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
>>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
>>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
>>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
>>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
>>>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
>>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
>>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
>>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
>>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
>>>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
>>>>>>    	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>>>>>>    	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
>>>>>> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>>>    #undef PIPE_CONF_CHECK_X
>>>>>>    #undef PIPE_CONF_CHECK_I
>>>>>> +#undef PIPE_CONF_CHECK_I_RANGE
>>>>>>    #undef PIPE_CONF_CHECK_P
>>>>>>    #undef PIPE_CONF_CHECK_I_ALT
>>>>>>    #undef PIPE_CONF_CHECK_FLAGS
>>>>>> -- 
>>>>>> 1.7.9.5
>>>>>>
>>>>>> _______________________________________________
>>>>>> Intel-gfx mailing list
>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> -- 
>>>> Thanks,
>>>> --Ram
>>>>

-- 
Thanks,
--Ram

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

Re: [PATCH 1/2] drm/i915: Sharing the pixel_format_from_vbt to whole i915

[Jani Nikula]

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

Re: [PATCH 2/2] drm/i915/BXT: Get pipe conf from the port registers

[Jani Nikula]

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

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

[Daniel Vetter]

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(&current_config->name, \
> >>>>>   				    &pipe_config->name,\
> >>>>> @@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
> >>>>>   	PIPE_CONF_CHECK_I(has_dsi_encoder);
> >>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> >>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> >>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
> >>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
> >>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
> >>>>> -	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
> >>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
> >>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
> >>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
> >>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
> >>>>> +	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
> >>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
> >>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
> >>>>> @@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
> >>>>>   #undef PIPE_CONF_CHECK_X
> >>>>>   #undef PIPE_CONF_CHECK_I
> >>>>> +#undef PIPE_CONF_CHECK_I_RANGE
> >>>>>   #undef PIPE_CONF_CHECK_P
> >>>>>   #undef PIPE_CONF_CHECK_I_ALT
> >>>>>   #undef PIPE_CONF_CHECK_FLAGS
> >>>>> -- 
> >>>>> 1.7.9.5
> >>>>>
> >>>>> _______________________________________________
> >>>>> Intel-gfx mailing list
> >>>>> Intel-gfx@lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>> -- 
> >>> Thanks,
> >>> --Ram
> >>>
> 
> -- 
> 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

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

[Daniel Vetter]

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(&current_config->name, \
> >>>>>>   				    &pipe_config->name,\
> >>>>>>@@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
> >>>>>>   	PIPE_CONF_CHECK_I(has_dsi_encoder);
> >>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
> >>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
> >>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
> >>>>>>@@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
> >>>>>>   #undef PIPE_CONF_CHECK_X
> >>>>>>   #undef PIPE_CONF_CHECK_I
> >>>>>>+#undef PIPE_CONF_CHECK_I_RANGE
> >>>>>>   #undef PIPE_CONF_CHECK_P
> >>>>>>   #undef PIPE_CONF_CHECK_I_ALT
> >>>>>>   #undef PIPE_CONF_CHECK_FLAGS
> >>>>>>-- 
> >>>>>>1.7.9.5
> >>>>>>
> >>>>>>_______________________________________________
> >>>>>>Intel-gfx mailing list
> >>>>>>Intel-gfx@lists.freedesktop.org
> >>>>>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>>-- 
> >>>>Thanks,
> >>>>--Ram
> >>>>
> 
> -- 
> Thanks,
> --Ram
> 

-- 
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

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

[Jani Nikula]

On Wed, 13 Apr 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 05, 2016 at 03:10:39PM +0530, Ramalingam C wrote:
>> 
>> On Tuesday 05 April 2016 02:00 PM, Jani Nikula wrote:
>> >On Mon, 04 Apr 2016, Ramalingam C <ramalingam.c@intel.com> wrote:
>> >>On Thursday 31 March 2016 12:34 AM, Daniel Vetter wrote:
>> >>>On Wed, Mar 30, 2016 at 07:49:40PM +0530, Ramalingam C wrote:
>> >>>>On Wednesday 30 March 2016 05:02 PM, Daniel Vetter wrote:
>> >>>>>On Tue, Mar 29, 2016 at 11:04:51PM +0530, Ramalingam C wrote:
>> >>>>>>At BXT DSI, PIPE registers are inactive. So we can't get the
>> >>>>>>PIPE's mode parameters from them. The possible option is
>> >>>>>>retriving them from the PORT registers. But mode timing
>> >>>>>>parameters are progammed to port registers interms of byteclocks.
>> >>>>>>
>> >>>>>>The formula used to convert the pixels interms of byteclk is
>> >>>>>>	DIV_ROUND_UP(DIV_ROUND_UP(pixels * bpp * burst_mode_ratio,
>> >>>>>>   						8 * 100), lane_count);
>> >>>>>>
>> >>>>>>So we retrieve them, interms of pixels as
>> >>>>>>	DIV_ROUND_UP((clk_hs * lane_count * 8 * 100),
>> >>>>>>					(bpp * burst_mode_ratio));
>> >>>>>>
>> >>>>>>Due to the multiple DIV_ROUND_UP in both formulas we get the worst
>> >>>>>>case delta in the retrieved PIPE's timing parameter as below
>> >>>>>>	DIV_ROUND_UP((8 * intel_dsi->lane_count * 100),
>> >>>>>>		(dsi_pixel_format_bpp(intel_dsi->pixel_format) *
>> >>>>>>			intel_dsi->burst_mode_ratio)))
>> >>>>>>
>> >>>>>>This converson of byteclk to pixel is required for hsync, hfp and hbp.
>> >>>>>>Which intern impacts horrizontal timing parameters. At worst case to
>> >>>>>>get htotal all there parameters are added with hactive.
>> >>>>>>Hence delta will be 3 times of above formula. Hence this value is
>> >>>>>>considered as tolerance for pipe_config comparison, in case of BXT DSI.
>> >>>>>>
>> >>>>>>Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> >>>>>This is the wrong way round imo, better would be to adjust the adjusted
>> >>>>>mode in the bxt dsi compute_config function to match the hw granularity.
>> >>>>>Stuff _really_ should match exactly, the fuzzy clock matching is mostly
>> >>>>>because our clock cod is a mess, and we can't/don't properly
>> >>>>>forward-compuate the actual clock timings we program into the hardware.
>> >>>>>-Daniel
>> >>>>Daniel, I got your point. But the problem will be that difficulty(even if
>> >>>>possible) in adjusting the adjusted mode parameters.
>> >>>>Reason is we are not programing the mode parameter as such. We will derive
>> >>>>the hfp, hsync and hbp from
>> >>>>hsync_start, hsync_end, hdisplay and htotal. These will be adjusted(divided
>> >>>>by 2) for dual link scenario.
>> >>>>And then resultant will go into the conversion as mentioned in the commit
>> >>>>message (two DIV_ROUND_UP onwards
>> >>>>and one DIV_ROUND_UP backwards). For this we have to make the parameter
>> >>>>divisible by three different factors.
>> >>>>So IMHO, even if this is possible, it will look more messy.
>> >>>>
>> >>>>Predicting the max error and tolerating it in pipe_config_compare will be
>> >>>>the straight forward and more reasonable.
>> >>>>Please let me know if i can go ahead in this approach.
>> >>>Yeah I discussed this some more with Jani on irc. I'd say we should store
>> >>>this adjusted horizontal timings (the ones fudged with burst_mode_ratio,
>> >>>lane_count, dual-link and all these things applied) into
>> >>>crtc_state->base.adjusted_mode. And then ofc also read those values out.
>> >>>
>> >>>The overall idea of the state verify/compare logic is that we start out
>> >>>with requested state from userspace, then derive the real hw state. And
>> >>>then compare that computed hw state with what's there already. Except for
>> >>>clocks, where there's special reasons, we never go back, since going back
>> >>>requires us to apply a range. This is the only way to guarnatee that "hw
>> >>>has the same exact mode programmed in both cases" iff "intel_crtc_state
>> >>>matches per intel_crtc_config_compare".
>> >>>
>> >>>state->adjusted_mode is never exposed to userspace, so there's no problem
>> >>>if it's has "strange" values. And we already have pipe_src_h/w to express
>> >>>the logical input rectangle.
>> >>>
>> >>>The idea is similar to how we set adjusted_mode.flags to what we actually
>> >>>program, instead of trying to make something up that's not perfectly
>> >>>accurate.
>> >>>-Daniel
>> >>Daniel,
>> >>
>> >>I have tested by adjusting the adjusted_mode in set_dsi_timings()
>> >>instead of intel_dsi_compute_config().
>> >>Reason is if we modify the adjusted mode at intel_dsi_compute_config()
>> >>itself, then modified value will
>> >>be taken as input for set_dsi_timings. Hence the get_config will deviate
>> >>further. I hope this should be fine with you and Jani.
>> >>
>> >>This will work out, if set_dsi_timings() is called after the
>> >>dsi_compute_config() on every suspend and resume or modeset.
>> >>I will verify this on Android once and update.
>> >>
>> >>Please share your view on this, so that can update the patch with
>> >>corresponding changes.
>> >I can't speak for Daniel, but I think his point was to update adjusted
>> >mode in ->compute_config() in a way that can be used directly in
>> >set_dsi_timings(). Then, it should be possible to read the timings from
>> >the hardware, and compare.
>> 
>> No, thats not possible jani. I think i didn't elaborate the problem
>> statement enough.
>> If you can read the programmed value from the hardware without any error,
>> then there is no need for this patch itself.
>> 
>> Even if we program the modified adjusted mode, timing parameters read from
>> get_config() will not be same as of modified adjusted mode.
>> 
>> In BXT DSI only available hw registers doesn't provide all timing parameters
>> in terms of pixels but txbyteclkhs.
>> adjusted mode has the parameters(start and end of hsync, htotal and hdisplay
>> and others) in terms of pixels.
>
> Then fix adjusted_mode to have the timings in terms of txbyteclkhs
> already. Problem solved.

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

BR,
Jani.


> -Daniel
>
>> So some conversion involved in programming few parameters (hfp, hsync and
>> hbp) and also in retrieving them.
>> 
>> As discussed above port registers expects hfp, hsync and hbp interms of
>> txbyteclkhs.
>> 
>> Sequence of programing (set_dsi_timings) the dsi port registers:
>> parameters from mode ---> (calc hfp, hsync and hbp) ---> (adjust for dual
>> link) ----> (conversion of Pixels to txbyteclkhs) ---> Program to Port
>> register
>> 
>> Sequence of get_config():
>> Read from port register ---> (conversion of txbyteclkhs to Pixels) --->
>> (adjust for dual link) ---> (recalculate the adjusted mode parameters from
>> hfp, hsync and hbp and other readings)
>> 
>> Here if we assume the input  to the set_dsi_timings is X(adjusted mode
>> parameter), output of get_config() will be  (X + delta1).
>> Here delta1 is error due to multiple DIV_ROUND_UP() in the conversion of
>> bytes <===> txbyteclkhs.
>> So as daniel says if you modify the adjusted_mode in compute_config()
>> itself, input to the set_dsi_timings() will become (X + delta1)
>> and the readings from the get_config() will become (X + delta1 + delta2)
>> 
>> And it wouldn't be appropriate to program the hw with modified adjusted
>> mode. This modification is just to match it with the pipe_config read from
>> hw.
>> Hence adjusted mode can be modified after the hw programming only, so the
>> place to do is end of set_dsi_timings().
>> 
>> Hope I explained the situation enough.
>> 
>> >
>> >BR,
>> >Jani.
>> >
>> >
>> >>>>>>---
>> >>>>>>Reviewed at https://lists.freedesktop.org/archives/intel-gfx/2016-March/089548.html
>> >>>>>>
>> >>>>>>   drivers/gpu/drm/i915/intel_display.c |   62 +++++++++++++++++++++++++++++++---
>> >>>>>>   1 file changed, 57 insertions(+), 5 deletions(-)
>> >>>>>>
>> >>>>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >>>>>>index c0627d6..282f036 100644
>> >>>>>>--- a/drivers/gpu/drm/i915/intel_display.c
>> >>>>>>+++ b/drivers/gpu/drm/i915/intel_display.c
>> >>>>>>@@ -12557,6 +12557,9 @@ intel_pipe_config_compare(struct drm_device *dev,
>> >>>>>>   			  bool adjust)
>> >>>>>>   {
>> >>>>>>   	bool ret = true;
>> >>>>>>+	struct intel_crtc *crtc = to_intel_crtc(current_config->base.crtc);
>> >>>>>>+	struct intel_encoder *intel_encoder;
>> >>>>>>+	struct intel_dsi *intel_dsi = NULL;
>> >>>>>>   #define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
>> >>>>>>   	do { \
>> >>>>>>@@ -12593,6 +12596,54 @@ intel_pipe_config_compare(struct drm_device *dev,
>> >>>>>>   		ret = false; \
>> >>>>>>   	}
>> >>>>>>+/*
>> >>>>>>+ * In case of BXT DSI, HW pipe_config will be retrieved from the port's timing
>> >>>>>>+ * configuration. This retrival includes some errors due to the DIV_ROUND_UP.
>> >>>>>>+ * So we are considering the max possible error at the comparison.
>> >>>>>>+ */
>> >>>>>>+/*
>> >>>>>>+ * htotal = hactive + hfp + hsync + hbp. Here last three lements might have
>> >>>>>>+ * the converson error, hence we consider the 3 times of error as tolerance.
>> >>>>>>+ */
>> >>>>>>+
>> >>>>>>+#define MAX_BXT_DSI_TIMING_RETRIVAL_ERR \
>> >>>>>>+		(intel_dsi == NULL ? 0 : \
>> >>>>>>+		DIV_ROUND_UP((3 * 8 * intel_dsi->lane_count * 100), \
>> >>>>>>+		(dsi_pixel_format_bpp(intel_dsi->pixel_format) * \
>> >>>>>>+			intel_dsi->burst_mode_ratio)))
>> >>>>>>+
>> >>>>>>+#define BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) { \
>> >>>>>>+	for_each_encoder_on_crtc(dev, &crtc->base, \
>> >>>>>>+					intel_encoder) { \
>> >>>>>>+		if (intel_encoder->type == INTEL_OUTPUT_DSI) { \
>> >>>>>>+			intel_dsi = enc_to_intel_dsi(&intel_encoder->base); \
>> >>>>>>+		} \
>> >>>>>>+	} \
>> >>>>>>+	if (!(current_config->name < pipe_config->name && \
>> >>>>>>+		current_config->name >= (pipe_config->name - \
>> >>>>>>+			MAX_BXT_DSI_TIMING_RETRIVAL_ERR))) { \
>> >>>>>>+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>> >>>>>>+		  "(expected %i, found %i(Err tolerance considered))\n", \
>> >>>>>>+		  current_config->name, \
>> >>>>>>+		  pipe_config->name); \
>> >>>>>>+		ret = false; \
>> >>>>>>+	} \
>> >>>>>>+}
>> >>>>>>+
>> >>>>>>+#define PIPE_CONF_CHECK_I_RANGE(name) { \
>> >>>>>>+	if (current_config->name != pipe_config->name) { \
>> >>>>>>+		if (IS_BROXTON(dev) && crtc->config->has_dsi_encoder) { \
>> >>>>>>+			BXT_DSI_PIPE_CONF_CHECK_I_RANGE(name) \
>> >>>>>>+		} else { \
>> >>>>>>+			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>> >>>>>>+			  "(expected %i, found %i)\n", \
>> >>>>>>+			  current_config->name, \
>> >>>>>>+			  pipe_config->name); \
>> >>>>>>+			ret = false; \
>> >>>>>>+		} \
>> >>>>>>+	} \
>> >>>>>>+}
>> >>>>>>+
>> >>>>>>   #define PIPE_CONF_CHECK_M_N(name) \
>> >>>>>>   	if (!intel_compare_link_m_n(&current_config->name, \
>> >>>>>>   				    &pipe_config->name,\
>> >>>>>>@@ -12697,11 +12748,11 @@ intel_pipe_config_compare(struct drm_device *dev,
>> >>>>>>   	PIPE_CONF_CHECK_I(has_dsi_encoder);
>> >>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
>> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
>> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_start);
>> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hblank_end);
>> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_start);
>> >>>>>>-	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hsync_end);
>> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_htotal);
>> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_start);
>> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hblank_end);
>> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_start);
>> >>>>>>+	PIPE_CONF_CHECK_I_RANGE(base.adjusted_mode.crtc_hsync_end);
>> >>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vdisplay);
>> >>>>>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vtotal);
>> >>>>>>@@ -12779,6 +12830,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>> >>>>>>   #undef PIPE_CONF_CHECK_X
>> >>>>>>   #undef PIPE_CONF_CHECK_I
>> >>>>>>+#undef PIPE_CONF_CHECK_I_RANGE
>> >>>>>>   #undef PIPE_CONF_CHECK_P
>> >>>>>>   #undef PIPE_CONF_CHECK_I_ALT
>> >>>>>>   #undef PIPE_CONF_CHECK_FLAGS
>> >>>>>>-- 
>> >>>>>>1.7.9.5
>> >>>>>>
>> >>>>>>_______________________________________________
>> >>>>>>Intel-gfx mailing list
>> >>>>>>Intel-gfx@lists.freedesktop.org
>> >>>>>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >>>>-- 
>> >>>>Thanks,
>> >>>>--Ram
>> >>>>
>> 
>> -- 
>> Thanks,
>> --Ram
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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

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

[Daniel Vetter]

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

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

[Ramalingam C]

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

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

[Daniel Vetter]

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

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

[Ramalingam C]

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

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

[Ramalingam C]

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