On Thu, Jan 07, 2016 at 06:27:24PM -0800, Kumar, Abhay wrote:
>
>
> On 1/7/2016 10:15 AM, Ville Syrjälä wrote:
> > On Mon, Dec 21, 2015 at 05:18:52PM -0800, abhay.ku...@intel.com wrote:
> >> From: Abhay Kumar <abhay.ku...@intel.com>
> >>
> >> Make resume/on codepath not to wait for panel_power_cycle_delay(t11_t12)
> >> if this time is already spent in suspend/poweron time.
> >>
> >> v2: Use CLOCK_BOOTTIME and remove jiffies for panel power cycle
> >> delay calculation(Ville).
> > The approach seems reasonable enough to me. There are a few issues with
> > the patch though, see below.
> >
> >> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >> Signed-off-by: Abhay Kumar <abhay.ku...@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_ddi.c | 3 +++
> >> drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++--------
> >> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> >> 3 files changed, 18 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >> b/drivers/gpu/drm/i915/intel_ddi.c
> >> index e6408e5..480697d 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -2395,6 +2395,9 @@ static void intel_ddi_post_disable(struct
> >> intel_encoder *intel_encoder)
> >> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >> intel_edp_panel_vdd_on(intel_dp);
> >> intel_edp_panel_off(intel_dp);
> >> +
> >> + /* storing panel power off time */
> > The comment seems rather pointless.
> >
> >> + intel_dp->panel_power_off_time =
> >> ktime_get_with_offset(TK_OFFS_BOOT);
> > There appears to be a wrapper for this: ktime_get_boottime().
> >
> > Not sure why you're adding this here anyway. Should be enough to just
> > replace the places where we currently sample jiffies to sample the boot
> > clock AFAICS.
> >
> >> }
> >>
> >> if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >> b/drivers/gpu/drm/i915/intel_dp.c
> >> index 796e3d3..c813605 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -38,7 +38,6 @@
> >> #include "intel_drv.h"
> >> #include <drm/i915_drm.h>
> >> #include "i915_drv.h"
> >> -
> > Spurious change.
> >
> >> #define DP_LINK_CHECK_TIMEOUT (10 * 1000)
> >>
> >> /* Compliance test status bits */
> >> @@ -1812,13 +1811,22 @@ static void wait_panel_off(struct intel_dp
> >> *intel_dp)
> >>
> >> static void wait_panel_power_cycle(struct intel_dp *intel_dp)
> >> {
> >> + ktime_t panel_power_on_time;
> >> + u32 panel_power_off_duration;
> >> +
> >> 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);
> >> + /* take the diffrence of currrent time and panel power off time
> >> + and then make panel wait for t11_t12 if needed */
> > Indent fail. Also the comment isn't in proper format.
> >
> >> + panel_power_on_time = ktime_get_with_offset(TK_OFFS_BOOT);
> >> + panel_power_off_duration = (panel_power_on_time.tv64 -
> >> intel_dp->panel_power_off_time.tv64);
> >> + panel_power_off_duration = panel_power_off_duration / 1000000;
> > ktime_ms_delta() perhaps?
> sure.
> >
> >>
> >> + /* When we disable the VDD override bit last we have to do the manual
> >> + * wait */
> > This comment formatting is a bit wonky too. Maybe polish it up while at
> > it.
> >
> >> + if (panel_power_off_duration < intel_dp->panel_power_cycle_delay)
> >> + wait_remaining_ms_from_jiffies(jiffies,
> >> + (intel_dp->panel_power_cycle_delay -
> >> panel_power_off_duration));
> >> wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
> >> }
> >>
> >> @@ -1969,7 +1977,7 @@ static void edp_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)
> >> - intel_dp->last_power_cycle = jiffies;
> >> + intel_dp->panel_power_off_time =
> >> ktime_get_with_offset(TK_OFFS_BOOT);
> >>
> >> power_domain = intel_display_port_aux_power_domain(intel_encoder);
> >> intel_display_power_put(dev_priv, power_domain);
> >> @@ -2118,7 +2126,6 @@ static void 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;
> > Removing this doens't seem correct. Instead we should sample the clock
> > here as well.
> Do we really need "last_power_cycle" ? As the ktime_get_bootime() will
> always calculate the boot time in fresh boot from zero and we only need
> to track the delta time when we go to suspend and resume.
We need to track if from when the last power cycle was initiated.
> this is the reason i removed last_power_cycle sampling and also
> initialization. Please let me know if this is ok and make sense?
>
> >
> >> wait_panel_off(intel_dp);
> >>
> >> /* We got a reference when we enabled the VDD. */
> >> @@ -5122,7 +5129,6 @@ intel_dp_add_properties(struct intel_dp *intel_dp,
> >> struct drm_connector *connect
> >>
> >> static void intel_dp_init_panel_power_timestamps(struct intel_dp
> >> *intel_dp)
> >> {
> >> - intel_dp->last_power_cycle = jiffies;
> > and I suppose here too.
> >
> >> intel_dp->last_power_on = jiffies;
> >> intel_dp->last_backlight_off = jiffies;
> >> }
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index d523ebb..84ad134 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -765,9 +765,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;
> >> + ktime_t panel_power_off_time;
> >>
> >> struct notifier_block edp_notifier;
> >>
> >> --
> >> 1.9.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
