2014/1/15 Daniel Vetter <daniel@xxxxxxxx>:
> On Wed, Jan 15, 2014 at 10:21:56AM -0800, Jesse Barnes wrote:
>> 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.
>
> Sorry I've missed this thus far, but we have such a callback already. It's
> the helper->reset thing and e.g. already used by the vga code for the
> hotplug setup. Adding a new resume callback for encoders shouldn't be
> necessary I think.
I remember you said this on IRC, then I tried it and failed. Please
check the commit message :)
> -Daniel
>
>>
>> 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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
