| Message ID | 20181017110943.30770-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/intel_dsi: Read back pclk set by GOP and use that as plck | expand |
On Wed, 17 Oct 2018, Hans de Goede <j.w.r.degoede@gmail.com> wrote: > On BYT and CHT 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. I assume we don't do the fast path because intel_pipe_config_compare() returns false due to crtc_clock and port_clock mismatch. The dmesg should tell you the reason with drm.debugs enabled. Now, the clock checks are already "fuzzy", and should account for slight variations. I think the goal was the same as here, plus IIUC we may lose some accuracy on the hardware roundtrip. Please try adding more tolerance in intel_fuzzy_clock_check() and see if that helps. The code looks like it allows 5% diff, but it's 2.5%. Regardless of whether we end up changing the tolerance or adjusting the state based on the hardware readout or what, I think doing the adjustment in intel_dsi_vbt.c init is the wrong place. We shouldn't have anything that depends on accessing the hardware there. I believe that's what Ville was trying to say in [1]. BR, Jani. [1] http://mid.mail-archive.com/20180706141652.GK5565@intel.com > > Changes in v2: > -Use intel_encoder_current_mode() to get the pclk setup by the GOP > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/i915/intel_dsi_vbt.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c > index ac83d6b89ae0..3387b187105c 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c > +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c > @@ -506,6 +506,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > struct mipi_config *mipi_config = dev_priv->vbt.dsi.config; > struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps; > struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode; > + struct drm_display_mode *curr; > u32 bpp; > u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui; > u32 ui_num, ui_den; > @@ -583,6 +584,23 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > } else > burst_mode_ratio = 100; > > + /* > + * On BYT / CRC the GOP sometimes picks a slightly different pclk, > + * read back the GOP configured pclk and prefer it over ours. > + */ > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > + curr = intel_encoder_current_mode(&intel_dsi->base); > + if (curr) { > + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", > + pclk, curr->clock); > + if (curr->clock >= (pclk * 9 / 10) && > + curr->clock <= (pclk * 11 / 10)) > + pclk = curr->clock; > + > + kfree(curr); > + } > + } > + > intel_dsi->burst_mode_ratio = burst_mode_ratio; > intel_dsi->pclk = pclk;
Hi, On 19-10-18 11:20, Jani Nikula wrote: > On Wed, 17 Oct 2018, Hans de Goede <j.w.r.degoede@gmail.com> wrote: >> On BYT and CHT 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. > > I assume we don't do the fast path because intel_pipe_config_compare() > returns false due to crtc_clock and port_clock mismatch. The dmesg > should tell you the reason with drm.debugs enabled. As I think I mentioned already last time (but that was a while ago, so I understand you cannot remember the details), the problem is these checks from intel_pipe_config_compare(): PIPE_CONF_CHECK_X(dsi_pll.ctrl); PIPE_CONF_CHECK_X(dsi_pll.div); > Now, the clock checks are already "fuzzy", and should account for slight > variations. I think the goal was the same as here, plus IIUC we may lose > some accuracy on the hardware roundtrip. The check I quoted above are basically doing a non-fuzzy port_clock check, we already do a fuzzy port_clock check, so maybe we should just drop them? > Please try adding more tolerance in intel_fuzzy_clock_check() and see if > that helps. The code looks like it allows 5% diff, but it's 2.5%. > > Regardless of whether we end up changing the tolerance or adjusting the > state based on the hardware readout or what, I think doing the > adjustment in intel_dsi_vbt.c init is the wrong place. We shouldn't have > anything that depends on accessing the hardware there. I believe that's > what Ville was trying to say in [1]. If you agree with dropping these 2 in essence non-fuzzy port-clock checks: PIPE_CONF_CHECK_X(dsi_pll.ctrl); PIPE_CONF_CHECK_X(dsi_pll.div); Then we won't need this state adjustment. But in case you do want to keep these, then were should the adjustment be done? Keep in mind that the pclk is initially calculated in intel_dsi_vbt.c and then immediately used to calculate a bunch of other settings such as intel_dsi->lp_byte_clk, prepare_cnt, etc. So there really is no other place were we can reasonably do this. Regards, Hans > > BR, > Jani. > > > [1] http://mid.mail-archive.com/20180706141652.GK5565@intel.com > > >> >> Changes in v2: >> -Use intel_encoder_current_mode() to get the pclk setup by the GOP >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/i915/intel_dsi_vbt.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c >> index ac83d6b89ae0..3387b187105c 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c >> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c >> @@ -506,6 +506,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >> struct mipi_config *mipi_config = dev_priv->vbt.dsi.config; >> struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps; >> struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode; >> + struct drm_display_mode *curr; >> u32 bpp; >> u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui; >> u32 ui_num, ui_den; >> @@ -583,6 +584,23 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >> } else >> burst_mode_ratio = 100; >> >> + /* >> + * On BYT / CRC the GOP sometimes picks a slightly different pclk, >> + * read back the GOP configured pclk and prefer it over ours. >> + */ >> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { >> + curr = intel_encoder_current_mode(&intel_dsi->base); >> + if (curr) { >> + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", >> + pclk, curr->clock); >> + if (curr->clock >= (pclk * 9 / 10) && >> + curr->clock <= (pclk * 11 / 10)) >> + pclk = curr->clock; >> + >> + kfree(curr); >> + } >> + } >> + >> intel_dsi->burst_mode_ratio = burst_mode_ratio; >> intel_dsi->pclk = pclk; >
On Fri, Oct 19, 2018 at 03:49:15PM +0200, Hans de Goede wrote: > Hi, > > On 19-10-18 11:20, Jani Nikula wrote: > > On Wed, 17 Oct 2018, Hans de Goede <j.w.r.degoede@gmail.com> wrote: > > > On BYT and CHT 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. > > > > I assume we don't do the fast path because intel_pipe_config_compare() > > returns false due to crtc_clock and port_clock mismatch. The dmesg > > should tell you the reason with drm.debugs enabled. > > As I think I mentioned already last time (but that was a while ago, > so I understand you cannot remember the details), the problem is these > checks from intel_pipe_config_compare(): > > PIPE_CONF_CHECK_X(dsi_pll.ctrl); > PIPE_CONF_CHECK_X(dsi_pll.div); > > > Now, the clock checks are already "fuzzy", and should account for slight > > variations. I think the goal was the same as here, plus IIUC we may lose > > some accuracy on the hardware roundtrip. > > The check I quoted above are basically doing a non-fuzzy port_clock check, > we already do a fuzzy port_clock check, so maybe we should just drop them? > > > Please try adding more tolerance in intel_fuzzy_clock_check() and see if > > that helps. The code looks like it allows 5% diff, but it's 2.5%. > > > > Regardless of whether we end up changing the tolerance or adjusting the > > state based on the hardware readout or what, I think doing the > > adjustment in intel_dsi_vbt.c init is the wrong place. We shouldn't have > > anything that depends on accessing the hardware there. I believe that's > > what Ville was trying to say in [1]. > > If you agree with dropping these 2 in essence non-fuzzy port-clock checks: > > PIPE_CONF_CHECK_X(dsi_pll.ctrl); > PIPE_CONF_CHECK_X(dsi_pll.div); > > Then we won't need this state adjustment. > > But in case you do want to keep these, then were should the adjustment be done? > > Keep in mind that the pclk is initially calculated in intel_dsi_vbt.c > and then immediately used to calculate a bunch of other settings such as > intel_dsi->lp_byte_clk, prepare_cnt, etc. So there really is no other > place were we can reasonably do this. We still want to do these checks when validating a modeset afterwards, but for the fuzzy case we need to cut them out indeed. Similar to what we do for the some of the other fastboot registers already with the if (!adjust) condition. I guess the trick here is making sure we pick the right set of registers ... Maybe we want to make all the pll low-level state like that (so both dsi_pll and dpll_hw_state)? Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c index ac83d6b89ae0..3387b187105c 100644 --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c @@ -506,6 +506,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) struct mipi_config *mipi_config = dev_priv->vbt.dsi.config; struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps; struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode; + struct drm_display_mode *curr; u32 bpp; u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui; u32 ui_num, ui_den; @@ -583,6 +584,23 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) } else burst_mode_ratio = 100; + /* + * On BYT / CRC the GOP sometimes picks a slightly different pclk, + * read back the GOP configured pclk and prefer it over ours. + */ + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { + curr = intel_encoder_current_mode(&intel_dsi->base); + if (curr) { + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", + pclk, curr->clock); + if (curr->clock >= (pclk * 9 / 10) && + curr->clock <= (pclk * 11 / 10)) + pclk = curr->clock; + + kfree(curr); + } + } + intel_dsi->burst_mode_ratio = burst_mode_ratio; intel_dsi->pclk = pclk;
On BYT and CHT 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 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/intel_dsi_vbt.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)