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

["Kumar, Abhay" <abhay.kumar@intel.com>]

On 1/7/2016 10:15 AM, Ville Syrjälä wrote:
> On Mon, Dec 21, 2015 at 05:18:52PM -0800, abhay.kumar@intel.com wrote:
>> From: Abhay Kumar <abhay.kumar@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.syrjala@linux.intel.com>
>> Signed-off-by: Abhay Kumar <abhay.kumar@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.
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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx