| Message ID | 1387470584-1662-3-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 19 Dec 2013 14:29:40 -0200 Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > The eDP spec defines some points where after you do action A, you have > to wait some time before action B. The thing is that in our driver > action B does not happen exactly after action A, but we still use > msleep() calls directly. What this patch does is that we record the > timestamp of when action A happened, then, just before action B, we > look at how much time has passed and only sleep the remaining amount > needed. > > With this change, I am able to save about 5-20ms (out of the total > 200ms) of the backlight_off delay and completely skip the 1ms > backlight_on delay. The 600ms vdd_off delay doesn't happen during > normal usage anymore due to a previous patch. > > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and > move it to intel_display.c > - Fix the msleep call: diff is in jiffies > v3: - Use "tmp_jiffies" so we don't need to worry about the value of > "jiffies" advancing while we're doing the math. > v4: - Rename function again. > - Move function to i915_drv.h. > - Store last_power_cycle at edp_panel_off too. > - Use msecs_to_jiffies_timeout, then replace the msleep with an > open-coded version that avoids the extra +1 jiffy. > - Try to add units to every variable name so we don't confuse > jiffies with milliseconds. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 29 +++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++++--- > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > 3 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index cc8afff..7e9b436 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) > return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); > } > > +/* > + * If you need to wait X milliseconds between events A and B, but event B > + * doesn't happen exactly after event A, you record the timestamp (jiffies) of > + * when event A happened, then just before event B you call this function and > + * pass the timestamp as the first argument, and X as the second argument. > + */ > +static inline void > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > +{ > + unsigned long target_jiffies, tmp_jiffies; > + unsigned int remaining_ms; > + > + /* > + * Don't re-read the value of "jiffies" every time since it may change > + * behind our back and break the math. > + */ > + tmp_jiffies = jiffies; > + target_jiffies = timestamp_jiffies + > + msecs_to_jiffies_timeout(to_wait_ms); > + > + if (time_after(target_jiffies, tmp_jiffies)) { > + remaining_ms = jiffies_to_msecs((long)target_jiffies - > + (long)tmp_jiffies); > + while (remaining_ms) > + remaining_ms = > + schedule_timeout_uninterruptible(remaining_ms); > + } > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 9d96447..2f82af4 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) > static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp) > { > DRM_DEBUG_KMS("Wait for panel power cycle\n"); > + > + /* When we disable the VDD override bit last we have to do the manual > + * wait. */ > + wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, > + intel_dp->panel_power_cycle_delay); > + > ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); > } > > +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp) > +{ > + wait_remaining_ms_from_jiffies(intel_dp->last_power_on, > + intel_dp->backlight_on_delay); > +} > + > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp) > +{ > + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off, > + intel_dp->backlight_off_delay); > +} > > /* Read the current pp_control value, unlocking the register if it > * is locked > @@ -1147,7 +1164,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp) > I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); > > if ((pp & POWER_TARGET_ON) == 0) > - msleep(intel_dp->panel_power_cycle_delay); > + intel_dp->last_power_cycle = jiffies; > > intel_runtime_pm_put(dev_priv); > } > @@ -1222,6 +1239,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp) > POSTING_READ(pp_ctrl_reg); > > ironlake_wait_panel_on(intel_dp); > + intel_dp->last_power_on = jiffies; > > if (IS_GEN5(dev)) { > pp |= PANEL_POWER_RESET; /* restore panel reset bit */ > @@ -1242,6 +1260,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > + ironlake_edp_wait_backlight_off(intel_dp); > + > pp = ironlake_get_pp_control(intel_dp); > /* We need to switch off panel power _and_ force vdd, for otherwise some > * panels get very unhappy and cease to work. */ > @@ -1252,6 +1272,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > > + intel_dp->last_power_cycle = jiffies; > ironlake_wait_panel_off(intel_dp); > } > > @@ -1273,7 +1294,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) > * link. So delay a bit to make sure the image is solid before > * allowing it to appear. > */ > - msleep(intel_dp->backlight_on_delay); > + ironlake_wait_backlight_on(intel_dp); > pp = ironlake_get_pp_control(intel_dp); > pp |= EDP_BLC_ENABLE; > > @@ -1305,7 +1326,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) > > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > - msleep(intel_dp->backlight_off_delay); > + intel_dp->last_backlight_off = jiffies; > } > > static void ironlake_edp_pll_on(struct intel_dp *intel_dp) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 46aea6c..92de688 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -485,6 +485,9 @@ struct intel_dp { > int backlight_off_delay; > struct delayed_work panel_vdd_work; > bool want_panel_vdd; > + unsigned long last_power_cycle; > + unsigned long last_power_on; > + unsigned long last_backlight_off; > bool psr_setup_done; > struct intel_connector *attached_connector; > }; We could bikeshed the name all day, but this looks like a great improvement. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > The eDP spec defines some points where after you do action A, you have > to wait some time before action B. The thing is that in our driver > action B does not happen exactly after action A, but we still use > msleep() calls directly. What this patch does is that we record the > timestamp of when action A happened, then, just before action B, we > look at how much time has passed and only sleep the remaining amount > needed. > > With this change, I am able to save about 5-20ms (out of the total > 200ms) of the backlight_off delay and completely skip the 1ms > backlight_on delay. The 600ms vdd_off delay doesn't happen during > normal usage anymore due to a previous patch. > > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and > move it to intel_display.c > - Fix the msleep call: diff is in jiffies > v3: - Use "tmp_jiffies" so we don't need to worry about the value of > "jiffies" advancing while we're doing the math. > v4: - Rename function again. > - Move function to i915_drv.h. > - Store last_power_cycle at edp_panel_off too. > - Use msecs_to_jiffies_timeout, then replace the msleep with an > open-coded version that avoids the extra +1 jiffy. > - Try to add units to every variable name so we don't confuse > jiffies with milliseconds. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 29 +++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++++--- > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > 3 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index cc8afff..7e9b436 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) > return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); > } > > +/* > + * If you need to wait X milliseconds between events A and B, but event B > + * doesn't happen exactly after event A, you record the timestamp (jiffies) of > + * when event A happened, then just before event B you call this function and > + * pass the timestamp as the first argument, and X as the second argument. > + */ > +static inline void > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > +{ > + unsigned long target_jiffies, tmp_jiffies; > + unsigned int remaining_ms; > + > + /* > + * Don't re-read the value of "jiffies" every time since it may change > + * behind our back and break the math. > + */ > + tmp_jiffies = jiffies; > + target_jiffies = timestamp_jiffies + > + msecs_to_jiffies_timeout(to_wait_ms); Is it possible this gets called before the last_* timestamp fields are initialized, resulting to timestamp_jiffies being 0? Do we need to special case that? If not, perhaps add a WARN_ON(timestamp_jiffies == 0) so we maintain this precondition. > + > + if (time_after(target_jiffies, tmp_jiffies)) { > + remaining_ms = jiffies_to_msecs((long)target_jiffies - > + (long)tmp_jiffies); > + while (remaining_ms) > + remaining_ms = > + schedule_timeout_uninterruptible(remaining_ms); schedule_timeout_uninterruptible takes jiffies, not ms. I think you should do the measurements again after fixing this. > + } > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 9d96447..2f82af4 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) > static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp) > { > DRM_DEBUG_KMS("Wait for panel power cycle\n"); > + > + /* When we disable the VDD override bit last we have to do the manual > + * wait. */ > + wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, > + intel_dp->panel_power_cycle_delay); > + > ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); > } > > +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp) > +{ > + wait_remaining_ms_from_jiffies(intel_dp->last_power_on, > + intel_dp->backlight_on_delay); > +} > + > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp) > +{ > + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off, > + intel_dp->backlight_off_delay); > +} > > /* Read the current pp_control value, unlocking the register if it > * is locked > @@ -1147,7 +1164,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp) > I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); > > if ((pp & POWER_TARGET_ON) == 0) > - msleep(intel_dp->panel_power_cycle_delay); > + intel_dp->last_power_cycle = jiffies; > > intel_runtime_pm_put(dev_priv); > } > @@ -1222,6 +1239,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp) > POSTING_READ(pp_ctrl_reg); > > ironlake_wait_panel_on(intel_dp); > + intel_dp->last_power_on = jiffies; > > if (IS_GEN5(dev)) { > pp |= PANEL_POWER_RESET; /* restore panel reset bit */ > @@ -1242,6 +1260,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > + ironlake_edp_wait_backlight_off(intel_dp); > + > pp = ironlake_get_pp_control(intel_dp); > /* We need to switch off panel power _and_ force vdd, for otherwise some > * panels get very unhappy and cease to work. */ > @@ -1252,6 +1272,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > > + intel_dp->last_power_cycle = jiffies; > ironlake_wait_panel_off(intel_dp); > } > > @@ -1273,7 +1294,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) > * link. So delay a bit to make sure the image is solid before > * allowing it to appear. > */ > - msleep(intel_dp->backlight_on_delay); > + ironlake_wait_backlight_on(intel_dp); > pp = ironlake_get_pp_control(intel_dp); > pp |= EDP_BLC_ENABLE; > > @@ -1305,7 +1326,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) > > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > - msleep(intel_dp->backlight_off_delay); > + intel_dp->last_backlight_off = jiffies; > } > > static void ironlake_edp_pll_on(struct intel_dp *intel_dp) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 46aea6c..92de688 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -485,6 +485,9 @@ struct intel_dp { > int backlight_off_delay; > struct delayed_work panel_vdd_work; > bool want_panel_vdd; > + unsigned long last_power_cycle; > + unsigned long last_power_on; > + unsigned long last_backlight_off; The delays and the timestamps could be grouped into an unnamed struct within intel_dp, but this can be left as a follow-up patch. BR, Jani. > bool psr_setup_done; > struct intel_connector *attached_connector; > }; > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Dec 20, 2013 at 11:30:15AM +0200, Jani Nikula wrote: > On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > The eDP spec defines some points where after you do action A, you have > > to wait some time before action B. The thing is that in our driver > > action B does not happen exactly after action A, but we still use > > msleep() calls directly. What this patch does is that we record the > > timestamp of when action A happened, then, just before action B, we > > look at how much time has passed and only sleep the remaining amount > > needed. > > > > With this change, I am able to save about 5-20ms (out of the total > > 200ms) of the backlight_off delay and completely skip the 1ms > > backlight_on delay. The 600ms vdd_off delay doesn't happen during > > normal usage anymore due to a previous patch. > > > > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and > > move it to intel_display.c > > - Fix the msleep call: diff is in jiffies > > v3: - Use "tmp_jiffies" so we don't need to worry about the value of > > "jiffies" advancing while we're doing the math. > > v4: - Rename function again. > > - Move function to i915_drv.h. > > - Store last_power_cycle at edp_panel_off too. > > - Use msecs_to_jiffies_timeout, then replace the msleep with an > > open-coded version that avoids the extra +1 jiffy. > > - Try to add units to every variable name so we don't confuse > > jiffies with milliseconds. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 29 +++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++++--- > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > 3 files changed, 56 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index cc8afff..7e9b436 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) > > return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); > > } > > > > +/* > > + * If you need to wait X milliseconds between events A and B, but event B > > + * doesn't happen exactly after event A, you record the timestamp (jiffies) of > > + * when event A happened, then just before event B you call this function and > > + * pass the timestamp as the first argument, and X as the second argument. > > + */ > > +static inline void > > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > > +{ > > + unsigned long target_jiffies, tmp_jiffies; > > + unsigned int remaining_ms; > > + > > + /* > > + * Don't re-read the value of "jiffies" every time since it may change > > + * behind our back and break the math. > > + */ > > + tmp_jiffies = jiffies; > > + target_jiffies = timestamp_jiffies + > > + msecs_to_jiffies_timeout(to_wait_ms); > > Is it possible this gets called before the last_* timestamp fields are > initialized, resulting to timestamp_jiffies being 0? Do we need to > special case that? If not, perhaps add a WARN_ON(timestamp_jiffies == 0) > so we maintain this precondition. afaik jiffies are allowed to wrap around to 0 ...
2013/12/20 Jani Nikula <jani.nikula@linux.intel.com>: > On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> The eDP spec defines some points where after you do action A, you have >> to wait some time before action B. The thing is that in our driver >> action B does not happen exactly after action A, but we still use >> msleep() calls directly. What this patch does is that we record the >> timestamp of when action A happened, then, just before action B, we >> look at how much time has passed and only sleep the remaining amount >> needed. >> >> With this change, I am able to save about 5-20ms (out of the total >> 200ms) of the backlight_off delay and completely skip the 1ms >> backlight_on delay. The 600ms vdd_off delay doesn't happen during >> normal usage anymore due to a previous patch. >> >> v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and >> move it to intel_display.c >> - Fix the msleep call: diff is in jiffies >> v3: - Use "tmp_jiffies" so we don't need to worry about the value of >> "jiffies" advancing while we're doing the math. >> v4: - Rename function again. >> - Move function to i915_drv.h. >> - Store last_power_cycle at edp_panel_off too. >> - Use msecs_to_jiffies_timeout, then replace the msleep with an >> open-coded version that avoids the extra +1 jiffy. >> - Try to add units to every variable name so we don't confuse >> jiffies with milliseconds. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 29 +++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++++--- >> drivers/gpu/drm/i915/intel_drv.h | 3 +++ >> 3 files changed, 56 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index cc8afff..7e9b436 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) >> return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); >> } >> >> +/* >> + * If you need to wait X milliseconds between events A and B, but event B >> + * doesn't happen exactly after event A, you record the timestamp (jiffies) of >> + * when event A happened, then just before event B you call this function and >> + * pass the timestamp as the first argument, and X as the second argument. >> + */ >> +static inline void >> +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) >> +{ >> + unsigned long target_jiffies, tmp_jiffies; >> + unsigned int remaining_ms; >> + >> + /* >> + * Don't re-read the value of "jiffies" every time since it may change >> + * behind our back and break the math. >> + */ >> + tmp_jiffies = jiffies; >> + target_jiffies = timestamp_jiffies + >> + msecs_to_jiffies_timeout(to_wait_ms); > > Is it possible this gets called before the last_* timestamp fields are > initialized, resulting to timestamp_jiffies being 0? Do we need to > special case that? If not, perhaps add a WARN_ON(timestamp_jiffies == 0) > so we maintain this precondition. The jiffies variable keeps running forever, it can be 0 at any time. In the worst case we will wait more than we should, and this case is really rare, so it's fine IMHO. And when booting, jiffies usually has a super-high value. > >> + >> + if (time_after(target_jiffies, tmp_jiffies)) { >> + remaining_ms = jiffies_to_msecs((long)target_jiffies - >> + (long)tmp_jiffies); >> + while (remaining_ms) >> + remaining_ms = >> + schedule_timeout_uninterruptible(remaining_ms); > > schedule_timeout_uninterruptible takes jiffies, not ms. I think you > should do the measurements again after fixing this. Thanks for catching it! I have CONFIG_HZ=1000, so I guess 1ms == 1 jiffy here. > >> + } >> +} >> + >> #endif >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 9d96447..2f82af4 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) >> static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp) >> { >> DRM_DEBUG_KMS("Wait for panel power cycle\n"); >> + >> + /* When we disable the VDD override bit last we have to do the manual >> + * wait. */ >> + wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, >> + intel_dp->panel_power_cycle_delay); >> + >> ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); >> } >> >> +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp) >> +{ >> + wait_remaining_ms_from_jiffies(intel_dp->last_power_on, >> + intel_dp->backlight_on_delay); >> +} >> + >> +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp) >> +{ >> + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off, >> + intel_dp->backlight_off_delay); >> +} >> >> /* Read the current pp_control value, unlocking the register if it >> * is locked >> @@ -1147,7 +1164,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp) >> I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); >> >> if ((pp & POWER_TARGET_ON) == 0) >> - msleep(intel_dp->panel_power_cycle_delay); >> + intel_dp->last_power_cycle = jiffies; >> >> intel_runtime_pm_put(dev_priv); >> } >> @@ -1222,6 +1239,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp) >> POSTING_READ(pp_ctrl_reg); >> >> ironlake_wait_panel_on(intel_dp); >> + intel_dp->last_power_on = jiffies; >> >> if (IS_GEN5(dev)) { >> pp |= PANEL_POWER_RESET; /* restore panel reset bit */ >> @@ -1242,6 +1260,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) >> >> DRM_DEBUG_KMS("Turn eDP power off\n"); >> >> + ironlake_edp_wait_backlight_off(intel_dp); >> + >> pp = ironlake_get_pp_control(intel_dp); >> /* We need to switch off panel power _and_ force vdd, for otherwise some >> * panels get very unhappy and cease to work. */ >> @@ -1252,6 +1272,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) >> I915_WRITE(pp_ctrl_reg, pp); >> POSTING_READ(pp_ctrl_reg); >> >> + intel_dp->last_power_cycle = jiffies; >> ironlake_wait_panel_off(intel_dp); >> } >> >> @@ -1273,7 +1294,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) >> * link. So delay a bit to make sure the image is solid before >> * allowing it to appear. >> */ >> - msleep(intel_dp->backlight_on_delay); >> + ironlake_wait_backlight_on(intel_dp); >> pp = ironlake_get_pp_control(intel_dp); >> pp |= EDP_BLC_ENABLE; >> >> @@ -1305,7 +1326,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) >> >> I915_WRITE(pp_ctrl_reg, pp); >> POSTING_READ(pp_ctrl_reg); >> - msleep(intel_dp->backlight_off_delay); >> + intel_dp->last_backlight_off = jiffies; >> } >> >> static void ironlake_edp_pll_on(struct intel_dp *intel_dp) >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 46aea6c..92de688 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -485,6 +485,9 @@ struct intel_dp { >> int backlight_off_delay; >> struct delayed_work panel_vdd_work; >> bool want_panel_vdd; >> + unsigned long last_power_cycle; >> + unsigned long last_power_on; >> + unsigned long last_backlight_off; > > The delays and the timestamps could be grouped into an unnamed struct > within intel_dp, but this can be left as a follow-up patch. > > BR, > Jani. > >> bool psr_setup_done; >> struct intel_connector *attached_connector; >> }; >> -- >> 1.8.3.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > The eDP spec defines some points where after you do action A, you have > to wait some time before action B. The thing is that in our driver > action B does not happen exactly after action A, but we still use > msleep() calls directly. What this patch does is that we record the > timestamp of when action A happened, then, just before action B, we > look at how much time has passed and only sleep the remaining amount > needed. > > With this change, I am able to save about 5-20ms (out of the total > 200ms) of the backlight_off delay and completely skip the 1ms > backlight_on delay. The 600ms vdd_off delay doesn't happen during > normal usage anymore due to a previous patch. > > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and > move it to intel_display.c > - Fix the msleep call: diff is in jiffies > v3: - Use "tmp_jiffies" so we don't need to worry about the value of > "jiffies" advancing while we're doing the math. > v4: - Rename function again. > - Move function to i915_drv.h. > - Store last_power_cycle at edp_panel_off too. > - Use msecs_to_jiffies_timeout, then replace the msleep with an > open-coded version that avoids the extra +1 jiffy. > - Try to add units to every variable name so we don't confuse > jiffies with milliseconds. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 29 +++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++++--- > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > 3 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index cc8afff..7e9b436 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) > return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); > } > > +/* > + * If you need to wait X milliseconds between events A and B, but event B > + * doesn't happen exactly after event A, you record the timestamp (jiffies) of > + * when event A happened, then just before event B you call this function and > + * pass the timestamp as the first argument, and X as the second argument. > + */ > +static inline void > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > +{ > + unsigned long target_jiffies, tmp_jiffies; > + unsigned int remaining_ms; > + > + /* > + * Don't re-read the value of "jiffies" every time since it may change > + * behind our back and break the math. > + */ > + tmp_jiffies = jiffies; > + target_jiffies = timestamp_jiffies + > + msecs_to_jiffies_timeout(to_wait_ms); > + > + if (time_after(target_jiffies, tmp_jiffies)) { > + remaining_ms = jiffies_to_msecs((long)target_jiffies - > + (long)tmp_jiffies); > + while (remaining_ms) > + remaining_ms = > + schedule_timeout_uninterruptible(remaining_ms); > + } > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 9d96447..2f82af4 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) > static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp) > { > DRM_DEBUG_KMS("Wait for panel power cycle\n"); > + > + /* When we disable the VDD override bit last we have to do the manual > + * wait. */ > + wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, > + intel_dp->panel_power_cycle_delay); > + > ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); > } > > +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp) > +{ > + wait_remaining_ms_from_jiffies(intel_dp->last_power_on, > + intel_dp->backlight_on_delay); > +} > + > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp) > +{ > + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off, > + intel_dp->backlight_off_delay); > +} I think the naming here is a bit unfortunate. You call wait_backlight_on() *before* you enable backlight, but wait_backlight_off() *after* you disable backlight. The wait_panel_{on,off} functions wait for some event to happen. I think ironlake_wait_backlight_on() *sounds* like something to call *after* you've enabled backlight. So instead, I think the function should be called something like wait_power_on()... but then it gets confusing with the wait_panel_on(). Ugh. I don't know. I'll just say Reviewed-by: Jani Nikula <jani.nikula@intel.com> because I think the patch looks good otherwise, and maybe someone will come up with the Perfect Naming Scheme and send follow-up patches we can bikeshed until the end of time... > > /* Read the current pp_control value, unlocking the register if it > * is locked > @@ -1147,7 +1164,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp) > I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); > > if ((pp & POWER_TARGET_ON) == 0) > - msleep(intel_dp->panel_power_cycle_delay); > + intel_dp->last_power_cycle = jiffies; > > intel_runtime_pm_put(dev_priv); > } > @@ -1222,6 +1239,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp) > POSTING_READ(pp_ctrl_reg); > > ironlake_wait_panel_on(intel_dp); > + intel_dp->last_power_on = jiffies; > > if (IS_GEN5(dev)) { > pp |= PANEL_POWER_RESET; /* restore panel reset bit */ > @@ -1242,6 +1260,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > + ironlake_edp_wait_backlight_off(intel_dp); > + > pp = ironlake_get_pp_control(intel_dp); > /* We need to switch off panel power _and_ force vdd, for otherwise some > * panels get very unhappy and cease to work. */ > @@ -1252,6 +1272,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > > + intel_dp->last_power_cycle = jiffies; > ironlake_wait_panel_off(intel_dp); > } > > @@ -1273,7 +1294,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) > * link. So delay a bit to make sure the image is solid before > * allowing it to appear. > */ > - msleep(intel_dp->backlight_on_delay); > + ironlake_wait_backlight_on(intel_dp); > pp = ironlake_get_pp_control(intel_dp); > pp |= EDP_BLC_ENABLE; > > @@ -1305,7 +1326,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) > > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > - msleep(intel_dp->backlight_off_delay); > + intel_dp->last_backlight_off = jiffies; > } > > static void ironlake_edp_pll_on(struct intel_dp *intel_dp) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 46aea6c..92de688 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -485,6 +485,9 @@ struct intel_dp { > int backlight_off_delay; > struct delayed_work panel_vdd_work; > bool want_panel_vdd; > + unsigned long last_power_cycle; > + unsigned long last_power_on; > + unsigned long last_backlight_off; > bool psr_setup_done; > struct intel_connector *attached_connector; > }; > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 17, 2014 at 03:09:58PM +0200, Jani Nikula wrote: > On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > The eDP spec defines some points where after you do action A, you have > > to wait some time before action B. The thing is that in our driver > > action B does not happen exactly after action A, but we still use > > msleep() calls directly. What this patch does is that we record the > > timestamp of when action A happened, then, just before action B, we > > look at how much time has passed and only sleep the remaining amount > > needed. > > > > With this change, I am able to save about 5-20ms (out of the total > > 200ms) of the backlight_off delay and completely skip the 1ms > > backlight_on delay. The 600ms vdd_off delay doesn't happen during > > normal usage anymore due to a previous patch. > > > > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and > > move it to intel_display.c > > - Fix the msleep call: diff is in jiffies > > v3: - Use "tmp_jiffies" so we don't need to worry about the value of > > "jiffies" advancing while we're doing the math. > > v4: - Rename function again. > > - Move function to i915_drv.h. > > - Store last_power_cycle at edp_panel_off too. > > - Use msecs_to_jiffies_timeout, then replace the msleep with an > > open-coded version that avoids the extra +1 jiffy. > > - Try to add units to every variable name so we don't confuse > > jiffies with milliseconds. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 29 +++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++++--- > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > 3 files changed, 56 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index cc8afff..7e9b436 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) > > return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); > > } > > > > +/* > > + * If you need to wait X milliseconds between events A and B, but event B > > + * doesn't happen exactly after event A, you record the timestamp (jiffies) of > > + * when event A happened, then just before event B you call this function and > > + * pass the timestamp as the first argument, and X as the second argument. > > + */ > > +static inline void > > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > > +{ > > + unsigned long target_jiffies, tmp_jiffies; > > + unsigned int remaining_ms; > > + > > + /* > > + * Don't re-read the value of "jiffies" every time since it may change > > + * behind our back and break the math. > > + */ > > + tmp_jiffies = jiffies; > > + target_jiffies = timestamp_jiffies + > > + msecs_to_jiffies_timeout(to_wait_ms); > > + > > + if (time_after(target_jiffies, tmp_jiffies)) { > > + remaining_ms = jiffies_to_msecs((long)target_jiffies - > > + (long)tmp_jiffies); > > + while (remaining_ms) > > + remaining_ms = > > + schedule_timeout_uninterruptible(remaining_ms); > > + } > > +} > > + > > #endif > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 9d96447..2f82af4 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) > > static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp) > > { > > DRM_DEBUG_KMS("Wait for panel power cycle\n"); > > + > > + /* When we disable the VDD override bit last we have to do the manual > > + * wait. */ > > + wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, > > + intel_dp->panel_power_cycle_delay); > > + > > ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); > > } > > > > +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp) > > +{ > > + wait_remaining_ms_from_jiffies(intel_dp->last_power_on, > > + intel_dp->backlight_on_delay); > > +} > > + > > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp) > > +{ > > + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off, > > + intel_dp->backlight_off_delay); > > +} > > I think the naming here is a bit unfortunate. You call wait_backlight_on() > *before* you enable backlight, but wait_backlight_off() *after* you > disable backlight. > > The wait_panel_{on,off} functions wait for some event to happen. > > I think ironlake_wait_backlight_on() *sounds* like something to call > *after* you've enabled backlight. So instead, I think the function > should be called something like wait_power_on()... but then it gets > confusing with the wait_panel_on(). Ugh. > > I don't know. I'll just say > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > because I think the patch looks good otherwise, and maybe someone will > come up with the Perfect Naming Scheme and send follow-up patches we can > bikeshed until the end of time... I think a bikeshed patch on top to drop the ironlake_ prefixes would be good, since this isn't anything hw specific at all. I'll do that when applying. Otherwise no Clever Ideas for the actual names atm. -Daniel > > > > > /* Read the current pp_control value, unlocking the register if it > > * is locked > > @@ -1147,7 +1164,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp) > > I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); > > > > if ((pp & POWER_TARGET_ON) == 0) > > - msleep(intel_dp->panel_power_cycle_delay); > > + intel_dp->last_power_cycle = jiffies; > > > > intel_runtime_pm_put(dev_priv); > > } > > @@ -1222,6 +1239,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp) > > POSTING_READ(pp_ctrl_reg); > > > > ironlake_wait_panel_on(intel_dp); > > + intel_dp->last_power_on = jiffies; > > > > if (IS_GEN5(dev)) { > > pp |= PANEL_POWER_RESET; /* restore panel reset bit */ > > @@ -1242,6 +1260,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > > > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > > > + ironlake_edp_wait_backlight_off(intel_dp); > > + > > pp = ironlake_get_pp_control(intel_dp); > > /* We need to switch off panel power _and_ force vdd, for otherwise some > > * panels get very unhappy and cease to work. */ > > @@ -1252,6 +1272,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) > > I915_WRITE(pp_ctrl_reg, pp); > > POSTING_READ(pp_ctrl_reg); > > > > + intel_dp->last_power_cycle = jiffies; > > ironlake_wait_panel_off(intel_dp); > > } > > > > @@ -1273,7 +1294,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) > > * link. So delay a bit to make sure the image is solid before > > * allowing it to appear. > > */ > > - msleep(intel_dp->backlight_on_delay); > > + ironlake_wait_backlight_on(intel_dp); > > pp = ironlake_get_pp_control(intel_dp); > > pp |= EDP_BLC_ENABLE; > > > > @@ -1305,7 +1326,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) > > > > I915_WRITE(pp_ctrl_reg, pp); > > POSTING_READ(pp_ctrl_reg); > > - msleep(intel_dp->backlight_off_delay); > > + intel_dp->last_backlight_off = jiffies; > > } > > > > static void ironlake_edp_pll_on(struct intel_dp *intel_dp) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 46aea6c..92de688 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -485,6 +485,9 @@ struct intel_dp { > > int backlight_off_delay; > > struct delayed_work panel_vdd_work; > > bool want_panel_vdd; > > + unsigned long last_power_cycle; > > + unsigned long last_power_on; > > + unsigned long last_backlight_off; > > bool psr_setup_done; > > struct intel_connector *attached_connector; > > }; > > -- > > 1.8.3.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 17, 2014 at 02:29:51PM +0100, Daniel Vetter wrote: > On Fri, Jan 17, 2014 at 03:09:58PM +0200, Jani Nikula wrote: > > On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote: > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > The eDP spec defines some points where after you do action A, you have > > > to wait some time before action B. The thing is that in our driver > > > action B does not happen exactly after action A, but we still use > > > msleep() calls directly. What this patch does is that we record the > > > timestamp of when action A happened, then, just before action B, we > > > look at how much time has passed and only sleep the remaining amount > > > needed. > > > > > > With this change, I am able to save about 5-20ms (out of the total > > > 200ms) of the backlight_off delay and completely skip the 1ms > > > backlight_on delay. The 600ms vdd_off delay doesn't happen during > > > normal usage anymore due to a previous patch. > > > > > > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and > > > move it to intel_display.c > > > - Fix the msleep call: diff is in jiffies > > > v3: - Use "tmp_jiffies" so we don't need to worry about the value of > > > "jiffies" advancing while we're doing the math. > > > v4: - Rename function again. > > > - Move function to i915_drv.h. > > > - Store last_power_cycle at edp_panel_off too. > > > - Use msecs_to_jiffies_timeout, then replace the msleep with an > > > open-coded version that avoids the extra +1 jiffy. > > > - Try to add units to every variable name so we don't confuse > > > jiffies with milliseconds. > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 29 +++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++++--- > > > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > > > 3 files changed, 56 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index cc8afff..7e9b436 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) > > > return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); > > > } > > > > > > +/* > > > + * If you need to wait X milliseconds between events A and B, but event B > > > + * doesn't happen exactly after event A, you record the timestamp (jiffies) of > > > + * when event A happened, then just before event B you call this function and > > > + * pass the timestamp as the first argument, and X as the second argument. > > > + */ > > > +static inline void > > > +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > > > +{ > > > + unsigned long target_jiffies, tmp_jiffies; > > > + unsigned int remaining_ms; > > > + > > > + /* > > > + * Don't re-read the value of "jiffies" every time since it may change > > > + * behind our back and break the math. > > > + */ > > > + tmp_jiffies = jiffies; > > > + target_jiffies = timestamp_jiffies + > > > + msecs_to_jiffies_timeout(to_wait_ms); > > > + > > > + if (time_after(target_jiffies, tmp_jiffies)) { > > > + remaining_ms = jiffies_to_msecs((long)target_jiffies - > > > + (long)tmp_jiffies); > > > + while (remaining_ms) > > > + remaining_ms = > > > + schedule_timeout_uninterruptible(remaining_ms); > > > + } > > > +} > > > + > > > #endif > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 9d96447..2f82af4 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) > > > static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp) > > > { > > > DRM_DEBUG_KMS("Wait for panel power cycle\n"); > > > + > > > + /* When we disable the VDD override bit last we have to do the manual > > > + * wait. */ > > > + wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, > > > + intel_dp->panel_power_cycle_delay); > > > + > > > ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); > > > } > > > > > > +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp) > > > +{ > > > + wait_remaining_ms_from_jiffies(intel_dp->last_power_on, > > > + intel_dp->backlight_on_delay); > > > +} > > > + > > > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp) > > > +{ > > > + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off, > > > + intel_dp->backlight_off_delay); > > > +} > > > > I think the naming here is a bit unfortunate. You call wait_backlight_on() > > *before* you enable backlight, but wait_backlight_off() *after* you > > disable backlight. > > > > The wait_panel_{on,off} functions wait for some event to happen. > > > > I think ironlake_wait_backlight_on() *sounds* like something to call > > *after* you've enabled backlight. So instead, I think the function > > should be called something like wait_power_on()... but then it gets > > confusing with the wait_panel_on(). Ugh. > > > > I don't know. I'll just say > > > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > > > because I think the patch looks good otherwise, and maybe someone will > > come up with the Perfect Naming Scheme and send follow-up patches we can > > bikeshed until the end of time... > > I think a bikeshed patch on top to drop the ironlake_ prefixes would be > good, since this isn't anything hw specific at all. I'll do that when > applying. Otherwise no Clever Ideas for the actual names atm. I've merged the first two patches for now. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index cc8afff..7e9b436 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value) return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1); } +/* + * If you need to wait X milliseconds between events A and B, but event B + * doesn't happen exactly after event A, you record the timestamp (jiffies) of + * when event A happened, then just before event B you call this function and + * pass the timestamp as the first argument, and X as the second argument. + */ +static inline void +wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) +{ + unsigned long target_jiffies, tmp_jiffies; + unsigned int remaining_ms; + + /* + * Don't re-read the value of "jiffies" every time since it may change + * behind our back and break the math. + */ + tmp_jiffies = jiffies; + target_jiffies = timestamp_jiffies + + msecs_to_jiffies_timeout(to_wait_ms); + + if (time_after(target_jiffies, tmp_jiffies)) { + remaining_ms = jiffies_to_msecs((long)target_jiffies - + (long)tmp_jiffies); + while (remaining_ms) + remaining_ms = + schedule_timeout_uninterruptible(remaining_ms); + } +} + #endif diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 9d96447..2f82af4 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp) static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp) { DRM_DEBUG_KMS("Wait for panel power cycle\n"); + + /* When we disable the VDD override bit last we have to do the manual + * wait. */ + wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, + intel_dp->panel_power_cycle_delay); + ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); } +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp) +{ + wait_remaining_ms_from_jiffies(intel_dp->last_power_on, + intel_dp->backlight_on_delay); +} + +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp) +{ + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off, + intel_dp->backlight_off_delay); +} /* Read the current pp_control value, unlocking the register if it * is locked @@ -1147,7 +1164,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp) I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); if ((pp & POWER_TARGET_ON) == 0) - msleep(intel_dp->panel_power_cycle_delay); + intel_dp->last_power_cycle = jiffies; intel_runtime_pm_put(dev_priv); } @@ -1222,6 +1239,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp) POSTING_READ(pp_ctrl_reg); ironlake_wait_panel_on(intel_dp); + intel_dp->last_power_on = jiffies; if (IS_GEN5(dev)) { pp |= PANEL_POWER_RESET; /* restore panel reset bit */ @@ -1242,6 +1260,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) DRM_DEBUG_KMS("Turn eDP power off\n"); + ironlake_edp_wait_backlight_off(intel_dp); + pp = ironlake_get_pp_control(intel_dp); /* We need to switch off panel power _and_ force vdd, for otherwise some * panels get very unhappy and cease to work. */ @@ -1252,6 +1272,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp) I915_WRITE(pp_ctrl_reg, pp); POSTING_READ(pp_ctrl_reg); + intel_dp->last_power_cycle = jiffies; ironlake_wait_panel_off(intel_dp); } @@ -1273,7 +1294,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) * link. So delay a bit to make sure the image is solid before * allowing it to appear. */ - msleep(intel_dp->backlight_on_delay); + ironlake_wait_backlight_on(intel_dp); pp = ironlake_get_pp_control(intel_dp); pp |= EDP_BLC_ENABLE; @@ -1305,7 +1326,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) I915_WRITE(pp_ctrl_reg, pp); POSTING_READ(pp_ctrl_reg); - msleep(intel_dp->backlight_off_delay); + intel_dp->last_backlight_off = jiffies; } static void ironlake_edp_pll_on(struct intel_dp *intel_dp) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 46aea6c..92de688 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -485,6 +485,9 @@ struct intel_dp { int backlight_off_delay; struct delayed_work panel_vdd_work; bool want_panel_vdd; + unsigned long last_power_cycle; + unsigned long last_power_on; + unsigned long last_backlight_off; bool psr_setup_done; struct intel_connector *attached_connector; };