Re: [PATCH] drm/i915: edp resume/On time optimization.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.kumar@xxxxxxxxx wrote:
> >> From: Abhay Kumar <abhay.kumar@xxxxxxxxx>
> >>
> >> 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.syrjala@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Abhay Kumar <abhay.kumar@xxxxxxxxx>
> >> ---
> >>   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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux