On Fri, 3 Jan 2014 17:46:40 -0200 Paulo Zanoni <przanoni@xxxxxxxxx> wrote: > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > The eDP code records a few timestamps containing the last time we took > some actions, because we need to wait before doing some other actions. > The problem is that if we store a timestamp when suspending and then > look at it when resuming, we'll ignore the unknown amount of time we > actually were suspended. > > This happens with the panel power cycle delay: it's 500ms on my > machine, and it's delaying the resume sequence by 200ms due to a > timestamp we recorded before suspending. This patch should solve this > problem by resetting the timestamps, creating encoder->resume() > callbacks. > > Q: Why don't we use DRM's connector->reset() callback? > A: Because it is also called then the driver is loaded, and it's on a point > where we have already used the panel, so we can't just reset any of the > timtestamps there. > > v2: - Fix the madatory jiffies/milliseconds bug. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 ++ > drivers/gpu/drm/i915/intel_ddi.c | 1 + > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > 5 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 61fb9fc..4aa42e5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -649,6 +649,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > mutex_lock(&dev->struct_mutex); > > + intel_encoder_resume(dev); > + > error = i915_gem_init_hw(dev); > mutex_unlock(&dev->struct_mutex); > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 1488b28..527e89c 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1606,6 +1606,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port) > intel_encoder->post_disable = intel_ddi_post_disable; > intel_encoder->get_hw_state = intel_ddi_get_hw_state; > intel_encoder->get_config = intel_ddi_get_config; > + intel_encoder->resume = intel_dp_resume; > > intel_dig_port->port = port; > intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) & > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4d1357a..869be78 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11096,6 +11096,17 @@ void i915_redisable_vga(struct drm_device *dev) > } > } > > +void intel_encoder_resume(struct drm_device *dev) > +{ > + struct intel_encoder *encoder; > + > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, > + base.head) { > + if (encoder->resume) > + encoder->resume(encoder); > + } > +} > + > static void intel_modeset_readout_hw_state(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2f82af4..dafb204 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3393,6 +3393,27 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect > } > } > > +void intel_dp_resume(struct intel_encoder *encoder) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > + unsigned long tmp_jiffies = jiffies; > + > + if (!is_edp(intel_dp)) > + return; > + > + /* > + * Reset everything, otherwise when suspend/resume gets very fast, we > + * delay the resume based on the values that were set when we were still > + * suspending. > + */ > + intel_dp->last_power_on = tmp_jiffies - > + msecs_to_jiffies(intel_dp->backlight_on_delay); > + intel_dp->last_power_cycle = tmp_jiffies - > + msecs_to_jiffies(intel_dp->panel_power_cycle_delay); > + intel_dp->last_backlight_off = tmp_jiffies - > + msecs_to_jiffies(intel_dp->backlight_off_delay); > +} > + > static void > intel_dp_init_panel_power_sequencer(struct drm_device *dev, > struct intel_dp *intel_dp, > @@ -3784,6 +3805,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > intel_encoder->post_disable = intel_post_disable_dp; > intel_encoder->get_hw_state = intel_dp_get_hw_state; > intel_encoder->get_config = intel_dp_get_config; > + intel_encoder->resume = intel_dp_resume; > if (IS_VALLEYVIEW(dev)) { > intel_encoder->pre_pll_enable = vlv_dp_pre_pll_enable; > intel_encoder->pre_enable = vlv_pre_enable_dp; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 92de688..8fb6dbe 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -149,6 +149,7 @@ struct intel_encoder { > * be set correctly before calling this function. */ > void (*get_config)(struct intel_encoder *, > struct intel_crtc_config *pipe_config); > + void (*resume)(struct intel_encoder *); > int crtc_mask; > enum hpd_pin hpd_pin; > }; > @@ -711,6 +712,7 @@ void hsw_enable_ips(struct intel_crtc *crtc); > void hsw_disable_ips(struct intel_crtc *crtc); > void intel_display_set_init_power(struct drm_device *dev, bool enable); > int valleyview_get_vco(struct drm_i915_private *dev_priv); > +void intel_encoder_resume(struct drm_device *dev); > > /* intel_dp.c */ > void intel_dp_init(struct drm_device *dev, int output_reg, enum port port); > @@ -734,6 +736,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync); > void intel_edp_psr_enable(struct intel_dp *intel_dp); > void intel_edp_psr_disable(struct intel_dp *intel_dp); > void intel_edp_psr_update(struct drm_device *dev); > +void intel_dp_resume(struct intel_encoder *encoder); > > > /* intel_dsi.c */ Looks good. I wonder if the resume call could be put into modeset_init_hw somehow, or maybe a new modeset_resume_hw routine or something, since the resume placement above the gem init seems odd, but that's not a blocker by any means. Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx