2013/12/20 Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>:
> On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
