Hi All,
This is a resend of my 3th attempt to fix the pclk we calculate for DSI panels and the pclk which the GOP has configured, causing fastboot to not work.
As requested in the review of earlier versions, this version moves the overriding of the pclk out of intel_dsi_vbt.c and into vlv_dsi.c.
This series was first posted in December 2018, but has gotten 0 comments.
This resend is rebased on top of 4.12-rc1 and applies cleanly to the current drm-tip.
Regards,
Hans
The next patch in this series uses intel_fuzzy_clock_check from the vlv_dsi.c code.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5098228f1302..ceb78f44f087 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11942,7 +11942,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, return 0; }
-static bool intel_fuzzy_clock_check(int clock1, int clock2) +bool intel_fuzzy_clock_check(int clock1, int clock2) { int diff;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a38b9cff5cd0..e85cd377a652 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1742,6 +1742,7 @@ int vlv_force_pll_on(struct drm_i915_private *dev_priv, enum pipe pipe, const struct dpll *dpll); void vlv_force_pll_off(struct drm_i915_private *dev_priv, enum pipe pipe); int lpt_get_iclkip(struct drm_i915_private *dev_priv); +bool intel_fuzzy_clock_check(int clock1, int clock2);
/* modesetting asserts */ void assert_panel_unlocked(struct drm_i915_private *dev_priv,
This is a preparation patch for moving the calling of *_dphy_param_init() out of intel_dsi_vbt_init.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/intel_dsi_vbt.c | 77 +++++++++++++++------------- 1 file changed, 42 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c index 3074448446bc..3448e8d51057 100644 --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c @@ -532,6 +532,44 @@ void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec) msleep(msec); }
+static void intel_dsi_log_params(struct intel_dsi *intel_dsi) +{ + DRM_DEBUG_KMS("Pclk %d\n", intel_dsi->pclk); + DRM_DEBUG_KMS("Pixel overlap %d\n", intel_dsi->pixel_overlap); + DRM_DEBUG_KMS("Lane count %d\n", intel_dsi->lane_count); + DRM_DEBUG_KMS("DPHY param reg 0x%x\n", intel_dsi->dphy_reg); + DRM_DEBUG_KMS("Video mode format %s\n", + intel_dsi->video_mode_format == VIDEO_MODE_NON_BURST_WITH_SYNC_PULSE ? + "non-burst with sync pulse" : + intel_dsi->video_mode_format == VIDEO_MODE_NON_BURST_WITH_SYNC_EVENTS ? + "non-burst with sync events" : + intel_dsi->video_mode_format == VIDEO_MODE_BURST ? + "burst" : "<unknown>"); + DRM_DEBUG_KMS("Burst mode ratio %d\n", intel_dsi->burst_mode_ratio); + DRM_DEBUG_KMS("Reset timer %d\n", intel_dsi->rst_timer_val); + DRM_DEBUG_KMS("Eot %s\n", enableddisabled(intel_dsi->eotp_pkt)); + DRM_DEBUG_KMS("Clockstop %s\n", enableddisabled(!intel_dsi->clock_stop)); + DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "command" : "video"); + if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK) + DRM_DEBUG_KMS("Dual link: DSI_DUAL_LINK_FRONT_BACK\n"); + else if (intel_dsi->dual_link == DSI_DUAL_LINK_PIXEL_ALT) + DRM_DEBUG_KMS("Dual link: DSI_DUAL_LINK_PIXEL_ALT\n"); + else + DRM_DEBUG_KMS("Dual link: NONE\n"); + DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format); + DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div); + DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout); + DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val); + DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count); + DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count); + DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk); + DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer); + DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count); + DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count); + DRM_DEBUG_KMS("BTA %s\n", + enableddisabled(!(intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA))); +} + #define ICL_PREPARE_CNT_MAX 0x7 #define ICL_CLK_ZERO_CNT_MAX 0xf #define ICL_TRAIL_CNT_MAX 0x7 @@ -635,6 +673,8 @@ static void icl_dphy_param_init(struct intel_dsi *intel_dsi) HS_TRAIL(trail_cnt) | HS_EXIT_OVERRIDE | HS_EXIT(exit_zero_cnt)); + + intel_dsi_log_params(intel_dsi); }
static void vlv_dphy_param_init(struct intel_dsi *intel_dsi) @@ -794,6 +834,8 @@ static void vlv_dphy_param_init(struct intel_dsi *intel_dsi) DIV_ROUND_UP(2 * tlpx_ui + trail_cnt * 2 + 8, 8); intel_dsi->clk_hs_to_lp_count += extra_byte_count; + + intel_dsi_log_params(intel_dsi); }
bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) @@ -877,41 +919,6 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) else vlv_dphy_param_init(intel_dsi);
- DRM_DEBUG_KMS("Pclk %d\n", intel_dsi->pclk); - DRM_DEBUG_KMS("Pixel overlap %d\n", intel_dsi->pixel_overlap); - DRM_DEBUG_KMS("Lane count %d\n", intel_dsi->lane_count); - DRM_DEBUG_KMS("DPHY param reg 0x%x\n", intel_dsi->dphy_reg); - DRM_DEBUG_KMS("Video mode format %s\n", - intel_dsi->video_mode_format == VIDEO_MODE_NON_BURST_WITH_SYNC_PULSE ? - "non-burst with sync pulse" : - intel_dsi->video_mode_format == VIDEO_MODE_NON_BURST_WITH_SYNC_EVENTS ? - "non-burst with sync events" : - intel_dsi->video_mode_format == VIDEO_MODE_BURST ? - "burst" : "<unknown>"); - DRM_DEBUG_KMS("Burst mode ratio %d\n", intel_dsi->burst_mode_ratio); - DRM_DEBUG_KMS("Reset timer %d\n", intel_dsi->rst_timer_val); - DRM_DEBUG_KMS("Eot %s\n", enableddisabled(intel_dsi->eotp_pkt)); - DRM_DEBUG_KMS("Clockstop %s\n", enableddisabled(!intel_dsi->clock_stop)); - DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "command" : "video"); - if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK) - DRM_DEBUG_KMS("Dual link: DSI_DUAL_LINK_FRONT_BACK\n"); - else if (intel_dsi->dual_link == DSI_DUAL_LINK_PIXEL_ALT) - DRM_DEBUG_KMS("Dual link: DSI_DUAL_LINK_PIXEL_ALT\n"); - else - DRM_DEBUG_KMS("Dual link: NONE\n"); - DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format); - DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div); - DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout); - DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val); - DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count); - DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count); - DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk); - DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer); - DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count); - DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count); - DRM_DEBUG_KMS("BTA %s\n", - enableddisabled(!(intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA))); - /* delays in VBT are in unit of 100us, so need to convert * here in ms * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
On Fri, May 24, 2019 at 06:30:18PM +0200, Hans de Goede wrote:
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
The vlv/icl_dphy_param_init calls do various calculations to set dphy parameters based on the pclk.
Move the calling of vlv/icl_dphy_param_init to vlv_dsi_init to give vlv_dsi_init a chance to tweak the pclk before these calculations are done.
This also removes the single "if (IS_ICELAKE(dev_priv))" check from intel_dsi_vbt_init making it fully platform agnostic.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/icl_dsi.c | 1 + drivers/gpu/drm/i915/intel_dsi.h | 2 ++ drivers/gpu/drm/i915/intel_dsi_vbt.c | 9 ++------- drivers/gpu/drm/i915/vlv_dsi.c | 2 ++ 4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c index 9d962ea1e635..0f43ef07efec 100644 --- a/drivers/gpu/drm/i915/icl_dsi.c +++ b/drivers/gpu/drm/i915/icl_dsi.c @@ -1455,6 +1455,7 @@ void icl_dsi_init(struct drm_i915_private *dev_priv) goto err; }
+ icl_dphy_param_init(intel_dsi); return;
err: diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 705a609050c0..a58d3d988d9f 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -192,5 +192,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id); void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi, enum mipi_seq seq_id); void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec); +void icl_dphy_param_init(struct intel_dsi *intel_dsi); +void vlv_dphy_param_init(struct intel_dsi *intel_dsi);
#endif /* _INTEL_DSI_H */ diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c index 3448e8d51057..022bf59418df 100644 --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c @@ -578,7 +578,7 @@ static void intel_dsi_log_params(struct intel_dsi *intel_dsi) #define ICL_HS_ZERO_CNT_MAX 0xf #define ICL_EXIT_ZERO_CNT_MAX 0x7
-static void icl_dphy_param_init(struct intel_dsi *intel_dsi) +void icl_dphy_param_init(struct intel_dsi *intel_dsi) { struct drm_device *dev = intel_dsi->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); @@ -677,7 +677,7 @@ static void icl_dphy_param_init(struct intel_dsi *intel_dsi) intel_dsi_log_params(intel_dsi); }
-static void vlv_dphy_param_init(struct intel_dsi *intel_dsi) +void vlv_dphy_param_init(struct intel_dsi *intel_dsi) { struct drm_device *dev = intel_dsi->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); @@ -914,11 +914,6 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id)
intel_dsi->burst_mode_ratio = burst_mode_ratio;
- if (INTEL_GEN(dev_priv) >= 11) - icl_dphy_param_init(intel_dsi); - else - vlv_dphy_param_init(intel_dsi); - /* delays in VBT are in unit of 100us, so need to convert * here in ms * Delay (100us) * 100 /1000 = Delay / 10 (ms) */ diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index fce8b58f7f93..3329ccf3b346 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -1782,6 +1782,8 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv) goto err; }
+ vlv_dphy_param_init(intel_dsi); + /* * In case of BYT with CRC PMIC, we need to use GPIO for * Panel control.
Hi,
On 04-06-19 19:35, Ville Syrjälä wrote:
I was thinking the same thing when I was writing the patch, but I was not 100% sure. I'm glad that you think the same, I will post a new version with this fixed (both of them).
Regards,
Hans
The GOP sometimes initializes the pclk at a (slightly) different frequency then the pclk which we've calculated.
This commit makes the DSI code read-back the pclk set by the GOP and if that is within a reasonable margin of the calculated pclk, uses that instead.
This fixes the first modeset being a full modeset instead of a fast modeset on systems where the GOP pclk is different.
Changes in v2: -Use intel_encoder_current_mode() to get the pclk setup by the GOP
Changes in v3: -Back to the readback approach, skipping the dsi_pll.ctrl / .dev checks in intel_pipe_config_compare() when adjust is set leads to: [drm:pipe_config_err [i915]] *ERROR* mismatch in dsi_pll.ctrl (...) [drm:pipe_config_err [i915]] *ERROR* mismatch in dsi_pll.div (...) -Do the readback and pclk overriding from vlv_dsi_init(), rather then from intel_dsi_vbt_init() as the vbt code should not be touching the hw
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/vlv_dsi.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index 3329ccf3b346..49975dd84ff4 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -1701,7 +1701,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv) struct drm_encoder *encoder; struct intel_connector *intel_connector; struct drm_connector *connector; - struct drm_display_mode *fixed_mode; + struct drm_display_mode *current_mode, *fixed_mode; enum port port;
DRM_DEBUG_KMS("\n"); @@ -1745,6 +1745,9 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv) intel_connector->get_hw_state = intel_connector_get_hw_state;
intel_encoder->port = port; + intel_encoder->type = INTEL_OUTPUT_DSI; + intel_encoder->power_domain = POWER_DOMAIN_PORT_DSI; + intel_encoder->cloneable = 0;
/* * On BYT/CHV, pipe A maps to MIPI DSI port A, pipe B maps to MIPI DSI @@ -1782,6 +1785,20 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv) goto err; }
+ /* Use clock read-back from current hw-state for fastboot */ + current_mode = intel_encoder_current_mode(intel_encoder); + if (current_mode) { + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", + intel_dsi->pclk, current_mode->clock); + if (intel_fuzzy_clock_check(intel_dsi->pclk, + current_mode->clock)) { + DRM_DEBUG_KMS("Using GOP pclk\n"); + intel_dsi->pclk = current_mode->clock; + } + + kfree(current_mode); + } + vlv_dphy_param_init(intel_dsi);
/* @@ -1799,9 +1816,6 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv) } }
- intel_encoder->type = INTEL_OUTPUT_DSI; - intel_encoder->power_domain = POWER_DOMAIN_PORT_DSI; - intel_encoder->cloneable = 0; drm_connector_init(dev, connector, &intel_dsi_connector_funcs, DRM_MODE_CONNECTOR_DSI);
On Fri, May 24, 2019 at 06:30:20PM +0200, Hans de Goede wrote:
I wonder if we should be checking whether the mode is otherwise identical to whatever we got from VBT? Though I suppose that shouldn't really happen.
The whole dsi clock handling is a proper mess, but looks like ->pclk is supposed to be the burst clock so I think this should end up doing more or less the right thing because we seem to stuffing the DPLL readout into the mode->clock.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
dri-devel@lists.freedesktop.org