| Message ID | 1389989862-1904-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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. But you don't explain why this is safe. The code nerfs the timeouts so that they are ignored, yet the delays are independent. Should this be based on realtime rather than jiffies? -Chris
2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>: > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> 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. > > But you don't explain why this is safe. The code nerfs the timeouts so > that they are ignored, yet the delays are independent. Should this be > based on realtime rather than jiffies? I'm not sure I understand your question. What's the problem you see exactly? Thanks, Paulo > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote: > 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>: > > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> 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. > > > > But you don't explain why this is safe. The code nerfs the timeouts so > > that they are ignored, yet the delays are independent. Should this be > > based on realtime rather than jiffies? > > I'm not sure I understand your question. What's the problem you see exactly? Given the fast suspend & resume, we will not have waited the required panel off time before poking it again etc. What makes that safe? -Chris
On Fri, Jan 17, 2014 at 10:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote: >> 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>: >> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote: >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> >> >> 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. >> > >> > But you don't explain why this is safe. The code nerfs the timeouts so >> > that they are ignored, yet the delays are independent. Should this be >> > based on realtime rather than jiffies? >> >> I'm not sure I understand your question. What's the problem you see exactly? > > Given the fast suspend & resume, we will not have waited the required > panel off time before poking it again etc. What makes that safe? Even worse the kernel might abort the suspend due to some issue and we'll immediately resume. Also, and immediate thaw operation after freezing is how hibernate works. Iirc the hw always enforces the full power off delay after a power reset for exactly this reason (at least on current platforms afaik). But with the minimal delays in patch 6 that won't help any more, either. I fear we need a bit more smarts here using a realtime clock source to figure out whether actually sufficient time elapsed :( -Daniel
2014/1/17 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Jan 17, 2014 at 10:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote: >>> 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>: >>> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote: >>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> >> >>> >> 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. >>> > >>> > But you don't explain why this is safe. The code nerfs the timeouts so >>> > that they are ignored, yet the delays are independent. Should this be >>> > based on realtime rather than jiffies? >>> >>> I'm not sure I understand your question. What's the problem you see exactly? >> >> Given the fast suspend & resume, we will not have waited the required >> panel off time before poking it again etc. What makes that safe? > > Even worse the kernel might abort the suspend due to some issue and > we'll immediately resume. Also, and immediate thaw operation after > freezing is how hibernate works. Iirc the hw always enforces the full > power off delay after a power reset for exactly this reason (at least > on current platforms afaik). Oh, now I get it. My bad. A brief experiment here shows that do_gettimeofday or current_kernel_time can probably be used to fix the problem. But since that requires properly getting all the timestamps in the correct units, maybe rewriting some functions, I'll wait a few days before I come back to this problem. > But with the minimal delays in patch 6 > that won't help any more, either. Patch 6 should be independent of this. This patch is just to save us some time in the resume cases, but patch 6 is to correct the amount of time we wait while we disable the panel. I don't see a reason to block patch 6 on this one. Thanks for the reviews, Paulo > > I fear we need a bit more smarts here using a realtime clock source to > figure out whether actually sufficient time elapsed :( > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jan 20, 2014 at 01:47:51PM -0200, Paulo Zanoni wrote: > 2014/1/17 Daniel Vetter <daniel@ffwll.ch>: > > On Fri, Jan 17, 2014 at 10:21 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> On Fri, Jan 17, 2014 at 07:11:14PM -0200, Paulo Zanoni wrote: > >>> 2014/1/17 Chris Wilson <chris@chris-wilson.co.uk>: > >>> > On Fri, Jan 17, 2014 at 06:17:42PM -0200, Paulo Zanoni wrote: > >>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>> >> > >>> >> 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. > >>> > > >>> > But you don't explain why this is safe. The code nerfs the timeouts so > >>> > that they are ignored, yet the delays are independent. Should this be > >>> > based on realtime rather than jiffies? > >>> > >>> I'm not sure I understand your question. What's the problem you see exactly? > >> > >> Given the fast suspend & resume, we will not have waited the required > >> panel off time before poking it again etc. What makes that safe? > > > > Even worse the kernel might abort the suspend due to some issue and > > we'll immediately resume. Also, and immediate thaw operation after > > freezing is how hibernate works. Iirc the hw always enforces the full > > power off delay after a power reset for exactly this reason (at least > > on current platforms afaik). > > Oh, now I get it. My bad. A brief experiment here shows that > do_gettimeofday or current_kernel_time can probably be used to fix the > problem. But since that requires properly getting all the timestamps > in the correct units, maybe rewriting some functions, I'll wait a few > days before I come back to this problem. A simpler solution might be to grab timestamps in our resume code both right at the end of our freeze function and at the beginning of thaw. Then the driver code could convert this once into dev->jiffies_elapsed_in_suspend or something and the dp->reset function could simply use that to adjust the timeout values a bit (instead of completely resetting everything). The tricky bit should only be how to get the off-by-one stuff right: For timeouts/waits we need to round up (+1), for this elapsed time we need to round down one jiffy i.e. -1, but not less than 0 ofc ;-) So I think you should be able to fix up your patch with very little work (and no changes to the logic you've already created&tested). > > But with the minimal delays in patch 6 > > that won't help any more, either. > > Patch 6 should be independent of this. This patch is just to save us > some time in the resume cases, but patch 6 is to correct the amount of > time we wait while we disable the panel. I don't see a reason to block > patch 6 on this one. Hm yeah, with the new logic (but without the reset on resume) we should still be save. I'll merge it, together with my prep patch to move the reset callbacks around a bit. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 55c43cf..001fb06 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3302,12 +3302,34 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) kfree(intel_dig_port); } +void intel_dp_reset(struct drm_connector *connector) +{ + struct intel_dp *intel_dp = intel_attached_dp(connector); + 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 const struct drm_connector_funcs intel_dp_connector_funcs = { .dpms = intel_connector_dpms, .detect = intel_dp_detect, .fill_modes = drm_helper_probe_single_connector_modes, .set_property = intel_dp_set_property, .destroy = intel_dp_connector_destroy, + .reset = intel_dp_reset, }; static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = {