[PATCH] drm/i915: Suspend resume timing optimization.

[PATCH] drm/i915: Suspend resume timing optimization.

[abhay.kumar]

From: Abhay Kumar <abhay.kumar@intel.com>

Moving 250ms from T12 timing to suspend path so that
resume path will be faster.

Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 7f618cf..2679c9e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2389,6 +2389,12 @@ 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);
+
+		/* Give additional delay of 250 ms so that resume time will
+		   be faster and also meets T12 delay.
+		*/
+		wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
+                                      (intel_dp->panel_power_cycle_delay/2));
 	}
 
 	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
-- 
1.9.1

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

Re: [PATCH] drm/i915: Suspend resume timing optimization.

[Paulo Zanoni]

2015-12-07 18:28 GMT-02:00  <abhay.kumar@intel.com>:
> From: Abhay Kumar <abhay.kumar@intel.com>
>
> Moving 250ms from T12 timing to suspend path so that
> resume path will be faster.

Can you please elaborate more on your motivation for this patch? I'm a
little confused. You're trying to make resume faster by making suspend
slower? What are your main arguments for this?

>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 7f618cf..2679c9e 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2389,6 +2389,12 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)

Funcion intel_ddi_post_disable() doesn't only run on suspend
situations, yet your commit message suggests you're optimizing
suspend. Maybe this commit makes non-suspend modesets slower because
now we need to wait the panel power cycle earlier? Have you measured
the possible downsides?

>                 intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>                 intel_edp_panel_vdd_on(intel_dp);
>                 intel_edp_panel_off(intel_dp);
> +
> +               /* Give additional delay of 250 ms so that resume time will
> +                  be faster and also meets T12 delay.
> +               */

The comment says 250ms, but the code doesn't. Also, there's a missing
'*' char in the comment.

> +               wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
> +                                      (intel_dp->panel_power_cycle_delay/2));

Why wait half the panel power cycle? Why did you choose exactly this value?

Thanks,
Paulo

>         }
>
>         if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: Suspend resume timing optimization.

[Kumar, Abhay]

[-- Attachment #1.1.1: Type: text/plain, Size: 2991 bytes --]

This is for Chrome os perspective which is using upstream kernel.  We have S3 suspend and resume time limit of 1sec.



Currently suspend of i915 driver takes 280ms and resume takes 600ms. To make resume faster and also follow edp spec this patch will move

Half of t12 time from resume path to suspend path.



Will take care of comment in next patchset.

[cid:image001.png@01D130F9.DFFC0300]







-----Original Message-----
From: Paulo Zanoni [mailto:przanoni@gmail.com]
Sent: Monday, December 7, 2015 12:52 PM
To: Kumar, Abhay
Cc: Intel Graphics Development
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Suspend resume timing optimization.



2015-12-07 18:28 GMT-02:00  <abhay.kumar@intel.com<mailto:abhay.kumar@intel.com>>:

> From: Abhay Kumar <abhay.kumar@intel.com<mailto:abhay.kumar@intel.com>>

>

> Moving 250ms from T12 timing to suspend path so that resume path will

> be faster.



Can you please elaborate more on your motivation for this patch? I'm a little confused. You're trying to make resume faster by making suspend slower? What are your main arguments for this?



>

> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com<mailto:abhay.kumar@intel.com>>

> ---

>  drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++

>  1 file changed, 6 insertions(+)

>

> diff --git a/drivers/gpu/drm/i915/intel_ddi.c

> b/drivers/gpu/drm/i915/intel_ddi.c

> index 7f618cf..2679c9e 100644

> --- a/drivers/gpu/drm/i915/intel_ddi.c

> +++ b/drivers/gpu/drm/i915/intel_ddi.c

> @@ -2389,6 +2389,12 @@ static void intel_ddi_post_disable(struct

> intel_encoder *intel_encoder)



Funcion intel_ddi_post_disable() doesn't only run on suspend situations, yet your commit message suggests you're optimizing suspend. Maybe this commit makes non-suspend modesets slower because now we need to wait the panel power cycle earlier? Have you measured the possible downsides?



>                 intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);

>                 intel_edp_panel_vdd_on(intel_dp);

>                 intel_edp_panel_off(intel_dp);

> +

> +               /* Give additional delay of 250 ms so that resume time will

> +                  be faster and also meets T12 delay.

> +               */



The comment says 250ms, but the code doesn't. Also, there's a missing '*' char in the comment.



> +               wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,

> +

> + (intel_dp->panel_power_cycle_delay/2));



Why wait half the panel power cycle? Why did you choose exactly this value?



Thanks,

Paulo



>         }

>

>         if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))

> --

> 1.9.1

>

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org<mailto:Intel-gfx@lists.freedesktop.org>

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx







--

Paulo Zanoni

[-- Attachment #1.1.2: Type: text/html, Size: 9467 bytes --]

[-- Attachment #1.2: image001.png --]
[-- Type: image/png, Size: 53549 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Suspend resume timing optimization.

[Kumar, Shobhit]

On 12/08/2015 02:22 AM, Paulo Zanoni wrote:
> 2015-12-07 18:28 GMT-02:00  <abhay.kumar@intel.com>:
>> From: Abhay Kumar <abhay.kumar@intel.com>
>>
>> Moving 250ms from T12 timing to suspend path so that
>> resume path will be faster.
>
> Can you please elaborate more on your motivation for this patch? I'm a
> little confused. You're trying to make resume faster by making suspend
> slower? What are your main arguments for this?
>

Actually the display resume time is currently roughly around ~700ms for 
eDP. The panel power sequence as per spec needs to be minimum 510ms . So 
the t11_12 time is 600ms in our code. Question is how to optimize this. 
What Abhay has tried is to move the wait for panel_power_cycle_delay in 
the suspend path rather then in resume path when we check if panel has 
power. Since this is jiffied based we end up waiting while going down 
and by the time we come back jiffies have already expired, giving us 
optimization in resume path. Will this work. Is it violates the spec in 
any way ? Any other suggestions ?

>>
>> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 7f618cf..2679c9e 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2389,6 +2389,12 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>
> Funcion intel_ddi_post_disable() doesn't only run on suspend
> situations, yet your commit message suggests you're optimizing
> suspend. Maybe this commit makes non-suspend modesets slower because
> now we need to wait the panel power cycle earlier? Have you measured
> the possible downsides?

Yes this is a problem. I again looked at the spec and found that t7 can 
be 50, but VBT was default programming 200ms which is overriding our 
driver initialized value. Correcting VBT gives back 150ms here. But yes 
this will impact non-suspend modeset paths. Perhaps we should figure out 
a way to avoid this and do only when relevant. A flag based check would 
work ? I know it sounds hackish.

>
>>                  intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>                  intel_edp_panel_vdd_on(intel_dp);
>>                  intel_edp_panel_off(intel_dp);
>> +
>> +               /* Give additional delay of 250 ms so that resume time will
>> +                  be faster and also meets T12 delay.
>> +               */
>
> The comment says 250ms, but the code doesn't. Also, there's a missing
> '*' char in the comment.
>
>> +               wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
>> +                                      (intel_dp->panel_power_cycle_delay/2));
>
> Why wait half the panel power cycle? Why did you choose exactly this value?
>

Actually I would want to wait out full panel power cycle delay. Brings 
our resume time down to ~250ms. But ultimately if this is acceptable 
solution, will still depend on suspend KPIs and we might have to 
balance. But as far as I know suspend path has no strict KPI, so we 
might get away with this.

Regards
Shobhit

> Thanks,
> Paulo
>
>>          }
>>
>>          if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Suspend resume timing optimization.

[Ville Syrjälä]

On Wed, Dec 09, 2015 at 04:25:41PM +0530, Kumar, Shobhit wrote:
> On 12/08/2015 02:22 AM, Paulo Zanoni wrote:
> > 2015-12-07 18:28 GMT-02:00  <abhay.kumar@intel.com>:
> >> From: Abhay Kumar <abhay.kumar@intel.com>
> >>
> >> Moving 250ms from T12 timing to suspend path so that
> >> resume path will be faster.
> >
> > Can you please elaborate more on your motivation for this patch? I'm a
> > little confused. You're trying to make resume faster by making suspend
> > slower? What are your main arguments for this?
> >
> 
> Actually the display resume time is currently roughly around ~700ms for 
> eDP. The panel power sequence as per spec needs to be minimum 510ms . So 
> the t11_12 time is 600ms in our code. Question is how to optimize this. 
> What Abhay has tried is to move the wait for panel_power_cycle_delay in 
> the suspend path rather then in resume path when we check if panel has 
> power. Since this is jiffied based we end up waiting while going down 
> and by the time we come back jiffies have already expired, giving us 
> optimization in resume path. Will this work. Is it violates the spec in 
> any way ? Any other suggestions ?

I'm thinking this should work. However, it seems unlikely we would
really need any wait during suspend or resume since the panel will
likely be off for a longer period anyway while the machine is
suspended. So I wonder if we could look at the wall clock instead
during suspend? Eg. sample it during susend and resume,
and adjust last_power_cycle jiffies based on the difference.

> 
> >>
> >> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 7f618cf..2679c9e 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -2389,6 +2389,12 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> >
> > Funcion intel_ddi_post_disable() doesn't only run on suspend
> > situations, yet your commit message suggests you're optimizing
> > suspend. Maybe this commit makes non-suspend modesets slower because
> > now we need to wait the panel power cycle earlier? Have you measured
> > the possible downsides?
> 
> Yes this is a problem. I again looked at the spec and found that t7 can 
> be 50, but VBT was default programming 200ms which is overriding our 
> driver initialized value. Correcting VBT gives back 150ms here. But yes 
> this will impact non-suspend modeset paths. Perhaps we should figure out 
> a way to avoid this and do only when relevant. A flag based check would 
> work ? I know it sounds hackish.
> 
> >
> >>                  intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >>                  intel_edp_panel_vdd_on(intel_dp);
> >>                  intel_edp_panel_off(intel_dp);
> >> +
> >> +               /* Give additional delay of 250 ms so that resume time will
> >> +                  be faster and also meets T12 delay.
> >> +               */
> >
> > The comment says 250ms, but the code doesn't. Also, there's a missing
> > '*' char in the comment.
> >
> >> +               wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
> >> +                                      (intel_dp->panel_power_cycle_delay/2));
> >
> > Why wait half the panel power cycle? Why did you choose exactly this value?
> >
> 
> Actually I would want to wait out full panel power cycle delay. Brings 
> our resume time down to ~250ms. But ultimately if this is acceptable 
> solution, will still depend on suspend KPIs and we might have to 
> balance. But as far as I know suspend path has no strict KPI, so we 
> might get away with this.
> 
> Regards
> Shobhit
> 
> > Thanks,
> > Paulo
> >
> >>          }
> >>
> >>          if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Suspend resume timing optimization.

[Kumar, Shobhit]

On 12/09/2015 05:42 PM, Ville Syrjälä wrote:
> On Wed, Dec 09, 2015 at 04:25:41PM +0530, Kumar, Shobhit wrote:
>> On 12/08/2015 02:22 AM, Paulo Zanoni wrote:
>>> 2015-12-07 18:28 GMT-02:00  <abhay.kumar@intel.com>:
>>>> From: Abhay Kumar <abhay.kumar@intel.com>
>>>>
>>>> Moving 250ms from T12 timing to suspend path so that
>>>> resume path will be faster.
>>>
>>> Can you please elaborate more on your motivation for this patch? I'm a
>>> little confused. You're trying to make resume faster by making suspend
>>> slower? What are your main arguments for this?
>>>
>>
>> Actually the display resume time is currently roughly around ~700ms for
>> eDP. The panel power sequence as per spec needs to be minimum 510ms . So
>> the t11_12 time is 600ms in our code. Question is how to optimize this.
>> What Abhay has tried is to move the wait for panel_power_cycle_delay in
>> the suspend path rather then in resume path when we check if panel has
>> power. Since this is jiffied based we end up waiting while going down
>> and by the time we come back jiffies have already expired, giving us
>> optimization in resume path. Will this work. Is it violates the spec in
>> any way ? Any other suggestions ?
>
> I'm thinking this should work. However, it seems unlikely we would
> really need any wait during suspend or resume since the panel will
> likely be off for a longer period anyway while the machine is
> suspended. So I wonder if we could look at the wall clock instead
> during suspend? Eg. sample it during susend and resume,
> and adjust last_power_cycle jiffies based on the difference.

Yes, but infact I found another thing that in edp_panel_on we just 
blindly call this wait_for_panel_cycle. We can check there if any PPS is 
in progress at that time via PP_STAT, and if no PPS (mainly power down 
seq) we should just skip the wait. My understanding of the 
panel_power_cycle_delay is that if we power it off, we should not try to 
turn on within 500ms.

>
>>
>>>>
>>>> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 7f618cf..2679c9e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2389,6 +2389,12 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>>>
>>> Funcion intel_ddi_post_disable() doesn't only run on suspend
>>> situations, yet your commit message suggests you're optimizing
>>> suspend. Maybe this commit makes non-suspend modesets slower because
>>> now we need to wait the panel power cycle earlier? Have you measured
>>> the possible downsides?
>>
>> Yes this is a problem. I again looked at the spec and found that t7 can
>> be 50, but VBT was default programming 200ms which is overriding our
>> driver initialized value. Correcting VBT gives back 150ms here. But yes
>> this will impact non-suspend modeset paths. Perhaps we should figure out
>> a way to avoid this and do only when relevant. A flag based check would
>> work ? I know it sounds hackish.
>>
>>>
>>>>                   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>                   intel_edp_panel_vdd_on(intel_dp);
>>>>                   intel_edp_panel_off(intel_dp);
>>>> +
>>>> +               /* Give additional delay of 250 ms so that resume time will
>>>> +                  be faster and also meets T12 delay.
>>>> +               */
>>>
>>> The comment says 250ms, but the code doesn't. Also, there's a missing
>>> '*' char in the comment.
>>>
>>>> +               wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
>>>> +                                      (intel_dp->panel_power_cycle_delay/2));
>>>
>>> Why wait half the panel power cycle? Why did you choose exactly this value?
>>>
>>
>> Actually I would want to wait out full panel power cycle delay. Brings
>> our resume time down to ~250ms. But ultimately if this is acceptable
>> solution, will still depend on suspend KPIs and we might have to
>> balance. But as far as I know suspend path has no strict KPI, so we
>> might get away with this.
>>
>> Regards
>> Shobhit
>>
>>> Thanks,
>>> Paulo
>>>
>>>>           }
>>>>
>>>>           if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Shobhit Kumar]

During resume, while turning the EDP panel power on, we need not wait
blindly for panel_power_cycle_delay. Check if panel power down sequence
in progress and then only wait. This improves our resume time significantly.

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f335c92..10ec669 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
 	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
 }
 
+static bool edp_panel_off_seq(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	lockdep_assert_held(&dev_priv->pps_mutex);
+
+	if (IS_VALLEYVIEW(dev) &&
+	    intel_dp->pps_pipe == INVALID_PIPE)
+		return false;
+
+	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
+}
+
 static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
 		 port_name(dp_to_dig_port(intel_dp)->port)))
 		return;
 
-	wait_panel_power_cycle(intel_dp);
+	if (edp_panel_off_seq(intel_dp))
+		wait_panel_power_cycle(intel_dp);
 
 	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
 	pp = ironlake_get_pp_control(intel_dp);
-- 
2.4.3

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

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Kumar, Shobhit]

On 12/09/2015 06:51 PM, Shobhit Kumar wrote:
> During resume, while turning the EDP panel power on, we need not wait
> blindly for panel_power_cycle_delay. Check if panel power down sequence
> in progress and then only wait. This improves our resume time significantly.
>

With this in case of actual suspend and resume, by the time we resume 
panel power down sequence is really completed and we don't need to 
actually wait and I am getting ~240ms as resume time.

To test the other case where we enable panel power within 500ms cycle 
time, I tested using pm_test(devices) interface with a 5ms delay between 
device suspend and resume and the resume time is back to ~730ms which is 
what we would want in this case.

Regards
Shobhit

> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f335c92..10ec669 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>   	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>   }
>
> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	lockdep_assert_held(&dev_priv->pps_mutex);
> +
> +	if (IS_VALLEYVIEW(dev) &&
> +	    intel_dp->pps_pipe == INVALID_PIPE)
> +		return false;
> +
> +	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> +}
> +
>   static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>   {
>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>   		 port_name(dp_to_dig_port(intel_dp)->port)))
>   		return;
>
> -	wait_panel_power_cycle(intel_dp);
> +	if (edp_panel_off_seq(intel_dp))
> +		wait_panel_power_cycle(intel_dp);
>
>   	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>   	pp = ironlake_get_pp_control(intel_dp);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Ville Syrjälä]

On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> During resume, while turning the EDP panel power on, we need not wait
> blindly for panel_power_cycle_delay. Check if panel power down sequence
> in progress and then only wait. This improves our resume time significantly.
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f335c92..10ec669 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>  	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>  }
>  
> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	lockdep_assert_held(&dev_priv->pps_mutex);
> +
> +	if (IS_VALLEYVIEW(dev) &&
> +	    intel_dp->pps_pipe == INVALID_PIPE)
> +		return false;
> +
> +	return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> +}

This doens't make sense to me. The power down cycle may have
completed just before, and so this would claim we don't have to
wait for the power_cycle_delay.

> +
>  static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>  		 port_name(dp_to_dig_port(intel_dp)->port)))
>  		return;
>  
> -	wait_panel_power_cycle(intel_dp);
> +	if (edp_panel_off_seq(intel_dp))
> +		wait_panel_power_cycle(intel_dp);
>  
>  	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>  	pp = ironlake_get_pp_control(intel_dp);
> -- 
> 2.4.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Shobhit Kumar]

On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>> During resume, while turning the EDP panel power on, we need not wait
>> blindly for panel_power_cycle_delay. Check if panel power down sequence
>> in progress and then only wait. This improves our resume time significantly.
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index f335c92..10ec669 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>       return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>>  }
>>
>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>> +{
>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>> +
>> +     if (IS_VALLEYVIEW(dev) &&
>> +         intel_dp->pps_pipe == INVALID_PIPE)
>> +             return false;
>> +
>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>> +}
>
> This doens't make sense to me. The power down cycle may have
> completed just before, and so this would claim we don't have to
> wait for the power_cycle_delay.

Not sure I understand your concern correctly. You are right, power
down cycle may have completed just before and if it has then we don't
need to wait. But in case the power down cycle is in progress as per
internal state, then we need to wait for it to complete. This will
happen for example in non-suspend disable path and will be handled
correctly. In case of actual suspend/resume, this would have
successfully completed and will skip the wait as it is not needed
before enabling panel power.

>
>> +
>>  static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>  {
>>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>>                port_name(dp_to_dig_port(intel_dp)->port)))
>>               return;
>>
>> -     wait_panel_power_cycle(intel_dp);
>> +     if (edp_panel_off_seq(intel_dp))
>> +             wait_panel_power_cycle(intel_dp);
>>
>>       pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
>>       pp = ironlake_get_pp_control(intel_dp);
>> --
>> 2.4.3
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Chris Wilson]

On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> >> During resume, while turning the EDP panel power on, we need not wait
> >> blindly for panel_power_cycle_delay. Check if panel power down sequence
> >> in progress and then only wait. This improves our resume time significantly.
> >>
> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index f335c92..10ec669 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> >>       return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> >>  }
> >>
> >> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> >> +{
> >> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +     lockdep_assert_held(&dev_priv->pps_mutex);
> >> +
> >> +     if (IS_VALLEYVIEW(dev) &&
> >> +         intel_dp->pps_pipe == INVALID_PIPE)
> >> +             return false;
> >> +
> >> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> >> +}
> >
> > This doens't make sense to me. The power down cycle may have
> > completed just before, and so this would claim we don't have to
> > wait for the power_cycle_delay.
> 
> Not sure I understand your concern correctly. You are right, power
> down cycle may have completed just before and if it has then we don't
> need to wait. But in case the power down cycle is in progress as per
> internal state, then we need to wait for it to complete. This will
> happen for example in non-suspend disable path and will be handled
> correctly. In case of actual suspend/resume, this would have
> successfully completed and will skip the wait as it is not needed
> before enabling panel power.
> 
> >
> >> +
> >>  static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >>  {
> >>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
> >>                port_name(dp_to_dig_port(intel_dp)->port)))
> >>               return;
> >>
> >> -     wait_panel_power_cycle(intel_dp);
> >> +     if (edp_panel_off_seq(intel_dp))
> >> +             wait_panel_power_cycle(intel_dp);

Looking in from the side, I have no idea what this is meant to do. At
the very least you need your explanatory paragraph here which would
include what exactly you are waiting for at the start of edp_panel_on
(and please try and find a better name for edp_panel_off_seq()).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Shobhit Kumar]

On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>> >> During resume, while turning the EDP panel power on, we need not wait
>> >> blindly for panel_power_cycle_delay. Check if panel power down sequence
>> >> in progress and then only wait. This improves our resume time significantly.
>> >>
>> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>> >>  1 file changed, 16 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> >> index f335c92..10ec669 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dp.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> >> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>> >>       return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>> >>  }
>> >>
>> >> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>> >> +{
>> >> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> >> +
>> >> +     lockdep_assert_held(&dev_priv->pps_mutex);
>> >> +
>> >> +     if (IS_VALLEYVIEW(dev) &&
>> >> +         intel_dp->pps_pipe == INVALID_PIPE)
>> >> +             return false;
>> >> +
>> >> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>> >> +}
>> >
>> > This doens't make sense to me. The power down cycle may have
>> > completed just before, and so this would claim we don't have to
>> > wait for the power_cycle_delay.
>>
>> Not sure I understand your concern correctly. You are right, power
>> down cycle may have completed just before and if it has then we don't
>> need to wait. But in case the power down cycle is in progress as per
>> internal state, then we need to wait for it to complete. This will
>> happen for example in non-suspend disable path and will be handled
>> correctly. In case of actual suspend/resume, this would have
>> successfully completed and will skip the wait as it is not needed
>> before enabling panel power.
>>
>> >
>> >> +
>> >>  static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>> >>  {
>> >>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> >> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>> >>                port_name(dp_to_dig_port(intel_dp)->port)))
>> >>               return;
>> >>
>> >> -     wait_panel_power_cycle(intel_dp);
>> >> +     if (edp_panel_off_seq(intel_dp))
>> >> +             wait_panel_power_cycle(intel_dp);
>
> Looking in from the side, I have no idea what this is meant to do. At
> the very least you need your explanatory paragraph here which would
> include what exactly you are waiting for at the start of edp_panel_on
> (and please try and find a better name for edp_panel_off_seq()).

I will add a comment. Basically I am not additionally waiting, but
converting the wait which was already there to a conditional wait. The
edp_panel_off_seq, checks if panel power down sequence is in progress.
In that case we need to wait for the panel power cycle delay. If it is
not in that sequence, there is no need to wait. I will make an attempt
again on the naming in next patch update.

Regards
Shobhit

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Ville Syrjälä]

On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
> >> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> >> >> During resume, while turning the EDP panel power on, we need not wait
> >> >> blindly for panel_power_cycle_delay. Check if panel power down sequence
> >> >> in progress and then only wait. This improves our resume time significantly.
> >> >>
> >> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
> >> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> >> index f335c92..10ec669 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> >> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> >> >>       return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> >> >>  }
> >> >>
> >> >> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> >> >> +{
> >> >> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> >> +
> >> >> +     lockdep_assert_held(&dev_priv->pps_mutex);
> >> >> +
> >> >> +     if (IS_VALLEYVIEW(dev) &&
> >> >> +         intel_dp->pps_pipe == INVALID_PIPE)
> >> >> +             return false;
> >> >> +
> >> >> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> >> >> +}
> >> >
> >> > This doens't make sense to me. The power down cycle may have
> >> > completed just before, and so this would claim we don't have to
> >> > wait for the power_cycle_delay.
> >>
> >> Not sure I understand your concern correctly. You are right, power
> >> down cycle may have completed just before and if it has then we don't
> >> need to wait. But in case the power down cycle is in progress as per
> >> internal state, then we need to wait for it to complete. This will
> >> happen for example in non-suspend disable path and will be handled
> >> correctly. In case of actual suspend/resume, this would have
> >> successfully completed and will skip the wait as it is not needed
> >> before enabling panel power.
> >>
> >> >
> >> >> +
> >> >>  static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >> >>  {
> >> >>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> >> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
> >> >>                port_name(dp_to_dig_port(intel_dp)->port)))
> >> >>               return;
> >> >>
> >> >> -     wait_panel_power_cycle(intel_dp);
> >> >> +     if (edp_panel_off_seq(intel_dp))
> >> >> +             wait_panel_power_cycle(intel_dp);
> >
> > Looking in from the side, I have no idea what this is meant to do. At
> > the very least you need your explanatory paragraph here which would
> > include what exactly you are waiting for at the start of edp_panel_on
> > (and please try and find a better name for edp_panel_off_seq()).
> 
> I will add a comment. Basically I am not additionally waiting, but
> converting the wait which was already there to a conditional wait. The
> edp_panel_off_seq, checks if panel power down sequence is in progress.
> In that case we need to wait for the panel power cycle delay. If it is
> not in that sequence, there is no need to wait. I will make an attempt
> again on the naming in next patch update.

As far I remeber you need to wait for power_cycle_delay between power
down cycle and power up cycle. You're trying to throw that wait away
entirely, unless the function happens get called while the power down
cycle is still in progress. We should already optimize away redundant
waits by tracking the end of the power down cycle with the jiffies
tracking.

Actually looking at the code the power_cycle_delay gets counted from
the start of the last power down cycle, so supposedly it's always at
least as long as the power down cycle, and typically it's quite a bit
longer that that. But that doesn't change the fact that you can't
just skip it because the power down cycle delay happened to end
already.

So what we do now is:
1. initiate power down cycle
2. last_power_cycle=jiffies
3. wait for power down (I suppose this actually waits
   until the power down delay has passed since that's
   programmes into the PPS).
4. wait for power_cycle_delay from last_power_cycle
5. initiate power up cycle

I think with your patch step 4 would always be skipped since the
power down cycle has already ended, and then we fail to honor the
power cycle delay.

Actually the power_cycle delay also gets programmed into the PPS so I
supose it would enforce the wait anyway when you initiate the power
up cycle (unless the PPS got totally reset due to power wells etc.,
which does seem like a real concern. The even bigger concern is the
vdd force bit for which the PPS does no enforcement.

The power_down_delay handling seems a bit wonky. We only wait for it
when turning off the port. I guess I would need to go re-read the spec
to figure out what it's meant to protect anyway.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Kumar, Shobhit]

On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>> During resume, while turning the EDP panel power on, we need not wait
>>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
>>>>>> in progress and then only wait. This improves our resume time significantly.
>>>>>>
>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index f335c92..10ec669 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>>>>>        return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>>>>>>   }
>>>>>>
>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>> +{
>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>> +
>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>> +
>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>> +             return false;
>>>>>> +
>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>> +}
>>>>>
>>>>> This doens't make sense to me. The power down cycle may have
>>>>> completed just before, and so this would claim we don't have to
>>>>> wait for the power_cycle_delay.
>>>>
>>>> Not sure I understand your concern correctly. You are right, power
>>>> down cycle may have completed just before and if it has then we don't
>>>> need to wait. But in case the power down cycle is in progress as per
>>>> internal state, then we need to wait for it to complete. This will
>>>> happen for example in non-suspend disable path and will be handled
>>>> correctly. In case of actual suspend/resume, this would have
>>>> successfully completed and will skip the wait as it is not needed
>>>> before enabling panel power.
>>>>
>>>>>
>>>>>> +
>>>>>>   static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>   {
>>>>>>        struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>>>>>>                 port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>                return;
>>>>>>
>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>
>>> Looking in from the side, I have no idea what this is meant to do. At
>>> the very least you need your explanatory paragraph here which would
>>> include what exactly you are waiting for at the start of edp_panel_on
>>> (and please try and find a better name for edp_panel_off_seq()).
>>
>> I will add a comment. Basically I am not additionally waiting, but
>> converting the wait which was already there to a conditional wait. The
>> edp_panel_off_seq, checks if panel power down sequence is in progress.
>> In that case we need to wait for the panel power cycle delay. If it is
>> not in that sequence, there is no need to wait. I will make an attempt
>> again on the naming in next patch update.
>
> As far I remeber you need to wait for power_cycle_delay between power
> down cycle and power up cycle. You're trying to throw that wait away
> entirely, unless the function happens get called while the power down

Yes you are right and I realize I made a mistake in my patch which is 
not checking PP_CYCLE_DELAY_ACTIVE bit.

> cycle is still in progress. We should already optimize away redundant
> waits by tracking the end of the power down cycle with the jiffies
> tracking.
>
> Actually looking at the code the power_cycle_delay gets counted from
> the start of the last power down cycle, so supposedly it's always at
> least as long as the power down cycle, and typically it's quite a bit
> longer that that. But that doesn't change the fact that you can't
> just skip it because the power down cycle delay happened to end
> already.
>
> So what we do now is:
> 1. initiate power down cycle
> 2. last_power_cycle=jiffies
> 3. wait for power down (I suppose this actually waits
>     until the power down delay has passed since that's
>     programmes into the PPS).
> 4. wait for power_cycle_delay from last_power_cycle
> 5. initiate power up cycle
>
> I think with your patch step 4 would always be skipped since the
> power down cycle has already ended, and then we fail to honor the
> power cycle delay.

Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that 
check will take care of this scenario I guess ?

Regards
Shobhit

>
> Actually the power_cycle delay also gets programmed into the PPS so I
> supose it would enforce the wait anyway when you initiate the power
> up cycle (unless the PPS got totally reset due to power wells etc.,
> which does seem like a real concern. The even bigger concern is the
> vdd force bit for which the PPS does no enforcement.
>
> The power_down_delay handling seems a bit wonky. We only wait for it
> when turning off the port. I guess I would need to go re-read the spec
> to figure out what it's meant to protect anyway.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Daniel Vetter]

On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
> >On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
> >>On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >>>On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
> >>>>On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
> >>>><ville.syrjala@linux.intel.com> wrote:
> >>>>>On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> >>>>>>During resume, while turning the EDP panel power on, we need not wait
> >>>>>>blindly for panel_power_cycle_delay. Check if panel power down sequence
> >>>>>>in progress and then only wait. This improves our resume time significantly.
> >>>>>>
> >>>>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >>>>>>---
> >>>>>>  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
> >>>>>>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>index f335c92..10ec669 100644
> >>>>>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>@@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> >>>>>>       return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> >>>>>>  }
> >>>>>>
> >>>>>>+static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> >>>>>>+{
> >>>>>>+     struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>>+     struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>>>+
> >>>>>>+     lockdep_assert_held(&dev_priv->pps_mutex);
> >>>>>>+
> >>>>>>+     if (IS_VALLEYVIEW(dev) &&
> >>>>>>+         intel_dp->pps_pipe == INVALID_PIPE)
> >>>>>>+             return false;
> >>>>>>+
> >>>>>>+     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> >>>>>>+}
> >>>>>
> >>>>>This doens't make sense to me. The power down cycle may have
> >>>>>completed just before, and so this would claim we don't have to
> >>>>>wait for the power_cycle_delay.
> >>>>
> >>>>Not sure I understand your concern correctly. You are right, power
> >>>>down cycle may have completed just before and if it has then we don't
> >>>>need to wait. But in case the power down cycle is in progress as per
> >>>>internal state, then we need to wait for it to complete. This will
> >>>>happen for example in non-suspend disable path and will be handled
> >>>>correctly. In case of actual suspend/resume, this would have
> >>>>successfully completed and will skip the wait as it is not needed
> >>>>before enabling panel power.
> >>>>
> >>>>>
> >>>>>>+
> >>>>>>  static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >>>>>>  {
> >>>>>>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>>@@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
> >>>>>>                port_name(dp_to_dig_port(intel_dp)->port)))
> >>>>>>               return;
> >>>>>>
> >>>>>>-     wait_panel_power_cycle(intel_dp);
> >>>>>>+     if (edp_panel_off_seq(intel_dp))
> >>>>>>+             wait_panel_power_cycle(intel_dp);
> >>>
> >>>Looking in from the side, I have no idea what this is meant to do. At
> >>>the very least you need your explanatory paragraph here which would
> >>>include what exactly you are waiting for at the start of edp_panel_on
> >>>(and please try and find a better name for edp_panel_off_seq()).
> >>
> >>I will add a comment. Basically I am not additionally waiting, but
> >>converting the wait which was already there to a conditional wait. The
> >>edp_panel_off_seq, checks if panel power down sequence is in progress.
> >>In that case we need to wait for the panel power cycle delay. If it is
> >>not in that sequence, there is no need to wait. I will make an attempt
> >>again on the naming in next patch update.
> >
> >As far I remeber you need to wait for power_cycle_delay between power
> >down cycle and power up cycle. You're trying to throw that wait away
> >entirely, unless the function happens get called while the power down
> 
> Yes you are right and I realize I made a mistake in my patch which is not
> checking PP_CYCLE_DELAY_ACTIVE bit.
> 
> >cycle is still in progress. We should already optimize away redundant
> >waits by tracking the end of the power down cycle with the jiffies
> >tracking.
> >
> >Actually looking at the code the power_cycle_delay gets counted from
> >the start of the last power down cycle, so supposedly it's always at
> >least as long as the power down cycle, and typically it's quite a bit
> >longer that that. But that doesn't change the fact that you can't
> >just skip it because the power down cycle delay happened to end
> >already.
> >
> >So what we do now is:
> >1. initiate power down cycle
> >2. last_power_cycle=jiffies
> >3. wait for power down (I suppose this actually waits
> >    until the power down delay has passed since that's
> >    programmes into the PPS).
> >4. wait for power_cycle_delay from last_power_cycle
> >5. initiate power up cycle
> >
> >I think with your patch step 4 would always be skipped since the
> >power down cycle has already ended, and then we fail to honor the
> >power cycle delay.
> 
> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that check
> will take care of this scenario I guess ?

I think Ville's idea a bit earlier on this thread to adjust the jiffie
values for any wall clock time that passed while we're suspend is much
more solid than trying to hack around with magic checks for what the hw
pps is doing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Kumar, Shobhit]

On 12/10/2015 03:20 PM, Daniel Vetter wrote:
> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>>>> During resume, while turning the EDP panel power on, we need not wait
>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
>>>>>>>> in progress and then only wait. This improves our resume time significantly.
>>>>>>>>
>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> index f335c92..10ec669 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>>>>>>>        return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>>>> +{
>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>> +
>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>>>> +
>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>>>> +             return false;
>>>>>>>> +
>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>>>> +}
>>>>>>>
>>>>>>> This doens't make sense to me. The power down cycle may have
>>>>>>> completed just before, and so this would claim we don't have to
>>>>>>> wait for the power_cycle_delay.
>>>>>>
>>>>>> Not sure I understand your concern correctly. You are right, power
>>>>>> down cycle may have completed just before and if it has then we don't
>>>>>> need to wait. But in case the power down cycle is in progress as per
>>>>>> internal state, then we need to wait for it to complete. This will
>>>>>> happen for example in non-suspend disable path and will be handled
>>>>>> correctly. In case of actual suspend/resume, this would have
>>>>>> successfully completed and will skip the wait as it is not needed
>>>>>> before enabling panel power.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>>   static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>>>   {
>>>>>>>>        struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>>>>>>>>                 port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>>>                return;
>>>>>>>>
>>>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>>>
>>>>> Looking in from the side, I have no idea what this is meant to do. At
>>>>> the very least you need your explanatory paragraph here which would
>>>>> include what exactly you are waiting for at the start of edp_panel_on
>>>>> (and please try and find a better name for edp_panel_off_seq()).
>>>>
>>>> I will add a comment. Basically I am not additionally waiting, but
>>>> converting the wait which was already there to a conditional wait. The
>>>> edp_panel_off_seq, checks if panel power down sequence is in progress.
>>>> In that case we need to wait for the panel power cycle delay. If it is
>>>> not in that sequence, there is no need to wait. I will make an attempt
>>>> again on the naming in next patch update.
>>>
>>> As far I remeber you need to wait for power_cycle_delay between power
>>> down cycle and power up cycle. You're trying to throw that wait away
>>> entirely, unless the function happens get called while the power down
>>
>> Yes you are right and I realize I made a mistake in my patch which is not
>> checking PP_CYCLE_DELAY_ACTIVE bit.
>>
>>> cycle is still in progress. We should already optimize away redundant
>>> waits by tracking the end of the power down cycle with the jiffies
>>> tracking.
>>>
>>> Actually looking at the code the power_cycle_delay gets counted from
>>> the start of the last power down cycle, so supposedly it's always at
>>> least as long as the power down cycle, and typically it's quite a bit
>>> longer that that. But that doesn't change the fact that you can't
>>> just skip it because the power down cycle delay happened to end
>>> already.
>>>
>>> So what we do now is:
>>> 1. initiate power down cycle
>>> 2. last_power_cycle=jiffies
>>> 3. wait for power down (I suppose this actually waits
>>>     until the power down delay has passed since that's
>>>     programmes into the PPS).
>>> 4. wait for power_cycle_delay from last_power_cycle
>>> 5. initiate power up cycle
>>>
>>> I think with your patch step 4 would always be skipped since the
>>> power down cycle has already ended, and then we fail to honor the
>>> power cycle delay.
>>
>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that check
>> will take care of this scenario I guess ?
>
> I think Ville's idea a bit earlier on this thread to adjust the jiffie
> values for any wall clock time that passed while we're suspend is much
> more solid than trying to hack around with magic checks for what the hw
> pps is doing.

Do not completely agree. The HW PPS state machine is pretty fine and 
reliable and making our software to track that, based on PP status seems 
at least to me like a better idea. In my view it simplifies the whole 
thing and these status bits are very well document as well.

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

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Thulasimani, Sivakumar]

On 12/10/2015 3:20 PM, Daniel Vetter wrote:
> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>>>> During resume, while turning the EDP panel power on, we need not wait
>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
>>>>>>>> in progress and then only wait. This improves our resume time significantly.
>>>>>>>>
>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> index f335c92..10ec669 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>>>>>>>        return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>>>> +{
>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>> +
>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>>>> +
>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>>>> +             return false;
>>>>>>>> +
>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>>>> +}
>>>>>>> This doens't make sense to me. The power down cycle may have
>>>>>>> completed just before, and so this would claim we don't have to
>>>>>>> wait for the power_cycle_delay.
>>>>>> Not sure I understand your concern correctly. You are right, power
>>>>>> down cycle may have completed just before and if it has then we don't
>>>>>> need to wait. But in case the power down cycle is in progress as per
>>>>>> internal state, then we need to wait for it to complete. This will
>>>>>> happen for example in non-suspend disable path and will be handled
>>>>>> correctly. In case of actual suspend/resume, this would have
>>>>>> successfully completed and will skip the wait as it is not needed
>>>>>> before enabling panel power.
>>>>>>
>>>>>>>> +
>>>>>>>>   static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>>>   {
>>>>>>>>        struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>>>>>>>>                 port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>>>                return;
>>>>>>>>
>>>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>>> Looking in from the side, I have no idea what this is meant to do. At
>>>>> the very least you need your explanatory paragraph here which would
>>>>> include what exactly you are waiting for at the start of edp_panel_on
>>>>> (and please try and find a better name for edp_panel_off_seq()).
>>>> I will add a comment. Basically I am not additionally waiting, but
>>>> converting the wait which was already there to a conditional wait. The
>>>> edp_panel_off_seq, checks if panel power down sequence is in progress.
>>>> In that case we need to wait for the panel power cycle delay. If it is
>>>> not in that sequence, there is no need to wait. I will make an attempt
>>>> again on the naming in next patch update.
>>> As far I remeber you need to wait for power_cycle_delay between power
>>> down cycle and power up cycle. You're trying to throw that wait away
>>> entirely, unless the function happens get called while the power down
>> Yes you are right and I realize I made a mistake in my patch which is not
>> checking PP_CYCLE_DELAY_ACTIVE bit.
>>
>>> cycle is still in progress. We should already optimize away redundant
>>> waits by tracking the end of the power down cycle with the jiffies
>>> tracking.
>>>
>>> Actually looking at the code the power_cycle_delay gets counted from
>>> the start of the last power down cycle, so supposedly it's always at
>>> least as long as the power down cycle, and typically it's quite a bit
>>> longer that that. But that doesn't change the fact that you can't
>>> just skip it because the power down cycle delay happened to end
>>> already.
>>>
>>> So what we do now is:
>>> 1. initiate power down cycle
>>> 2. last_power_cycle=jiffies
>>> 3. wait for power down (I suppose this actually waits
>>>     until the power down delay has passed since that's
>>>     programmes into the PPS).
>>> 4. wait for power_cycle_delay from last_power_cycle
>>> 5. initiate power up cycle
>>>
>>> I think with your patch step 4 would always be skipped since the
>>> power down cycle has already ended, and then we fail to honor the
>>> power cycle delay.
>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that check
>> will take care of this scenario I guess ?
> I think Ville's idea a bit earlier on this thread to adjust the jiffie
> values for any wall clock time that passed while we're suspend is much
> more solid than trying to hack around with magic checks for what the hw
> pps is doing.
> -Daniel
I would prefer a lot of cleanup for eDP, especially the multiple VDD on/off
(which is a topic for different thread i guess :) ) with that said I 
would prefer
  relying on HW status for power down delay  and power cycle delay.
Wall clock based logic will do the same but tracking
the various combinations of "OFF" and "ON" will complicate the logic.
Also i would not call it magic checks if bspec doc explains which bit
corresponds to  which operation.
i have to look into the all scenarios to understand if the change above
is valid or not, but the relying on HW status register is correct.

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

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Ville Syrjälä]

On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
> > On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
> >> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
> >>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
> >>>> <ville.syrjala@linux.intel.com> wrote:
> >>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> >>>>>> During resume, while turning the EDP panel power on, we need not wait
> >>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
> >>>>>> in progress and then only wait. This improves our resume time significantly.
> >>>>>>
> >>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >>>>>> ---
> >>>>>>   drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
> >>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>> index f335c92..10ec669 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> >>>>>>        return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> >>>>>>   }
> >>>>>>
> >>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> >>>>>> +{
> >>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>>> +
> >>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
> >>>>>> +
> >>>>>> +     if (IS_VALLEYVIEW(dev) &&
> >>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
> >>>>>> +             return false;
> >>>>>> +
> >>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> >>>>>> +}
> >>>>>
> >>>>> This doens't make sense to me. The power down cycle may have
> >>>>> completed just before, and so this would claim we don't have to
> >>>>> wait for the power_cycle_delay.
> >>>>
> >>>> Not sure I understand your concern correctly. You are right, power
> >>>> down cycle may have completed just before and if it has then we don't
> >>>> need to wait. But in case the power down cycle is in progress as per
> >>>> internal state, then we need to wait for it to complete. This will
> >>>> happen for example in non-suspend disable path and will be handled
> >>>> correctly. In case of actual suspend/resume, this would have
> >>>> successfully completed and will skip the wait as it is not needed
> >>>> before enabling panel power.
> >>>>
> >>>>>
> >>>>>> +
> >>>>>>   static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >>>>>>   {
> >>>>>>        struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
> >>>>>>                 port_name(dp_to_dig_port(intel_dp)->port)))
> >>>>>>                return;
> >>>>>>
> >>>>>> -     wait_panel_power_cycle(intel_dp);
> >>>>>> +     if (edp_panel_off_seq(intel_dp))
> >>>>>> +             wait_panel_power_cycle(intel_dp);
> >>>
> >>> Looking in from the side, I have no idea what this is meant to do. At
> >>> the very least you need your explanatory paragraph here which would
> >>> include what exactly you are waiting for at the start of edp_panel_on
> >>> (and please try and find a better name for edp_panel_off_seq()).
> >>
> >> I will add a comment. Basically I am not additionally waiting, but
> >> converting the wait which was already there to a conditional wait. The
> >> edp_panel_off_seq, checks if panel power down sequence is in progress.
> >> In that case we need to wait for the panel power cycle delay. If it is
> >> not in that sequence, there is no need to wait. I will make an attempt
> >> again on the naming in next patch update.
> >
> > As far I remeber you need to wait for power_cycle_delay between power
> > down cycle and power up cycle. You're trying to throw that wait away
> > entirely, unless the function happens get called while the power down
> 
> Yes you are right and I realize I made a mistake in my patch which is 
> not checking PP_CYCLE_DELAY_ACTIVE bit.
> 
> > cycle is still in progress. We should already optimize away redundant
> > waits by tracking the end of the power down cycle with the jiffies
> > tracking.
> >
> > Actually looking at the code the power_cycle_delay gets counted from
> > the start of the last power down cycle, so supposedly it's always at
> > least as long as the power down cycle, and typically it's quite a bit
> > longer that that. But that doesn't change the fact that you can't
> > just skip it because the power down cycle delay happened to end
> > already.
> >
> > So what we do now is:
> > 1. initiate power down cycle
> > 2. last_power_cycle=jiffies
> > 3. wait for power down (I suppose this actually waits
> >     until the power down delay has passed since that's
> >     programmes into the PPS).
> > 4. wait for power_cycle_delay from last_power_cycle
> > 5. initiate power up cycle
> >
> > I think with your patch step 4 would always be skipped since the
> > power down cycle has already ended, and then we fail to honor the
> > power cycle delay.
> 
> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that 
> check will take care of this scenario I guess ?

Nope. The vdd force bit doesn't respect the PPS state machine, so we
must do the waits manually instead. And in theory your patch wouldn't
do anything anyway since the sleep already takes into account when the
power cycle delay started.

The fact that we use msleep() may actually make those sleeps somewhat
longer, and maybe we should also think about switching to usleep_range()
here.

> 
> Regards
> Shobhit
> 
> >
> > Actually the power_cycle delay also gets programmed into the PPS so I
> > supose it would enforce the wait anyway when you initiate the power
> > up cycle (unless the PPS got totally reset due to power wells etc.,
> > which does seem like a real concern. The even bigger concern is the
> > vdd force bit for which the PPS does no enforcement.
> >
> > The power_down_delay handling seems a bit wonky. We only wait for it
> > when turning off the port. I guess I would need to go re-read the spec
> > to figure out what it's meant to protect anyway.
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Ville Syrjälä]

On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
> > On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
> > > On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
> > >> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
> > >>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
> > >>>> <ville.syrjala@linux.intel.com> wrote:
> > >>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> > >>>>>> During resume, while turning the EDP panel power on, we need not wait
> > >>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
> > >>>>>> in progress and then only wait. This improves our resume time significantly.
> > >>>>>>
> > >>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > >>>>>> ---
> > >>>>>>   drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
> > >>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > >>>>>> index f335c92..10ec669 100644
> > >>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> > >>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> > >>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> > >>>>>>        return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> > >>>>>>   }
> > >>>>>>
> > >>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> > >>>>>> +{
> > >>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > >>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
> > >>>>>> +
> > >>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
> > >>>>>> +
> > >>>>>> +     if (IS_VALLEYVIEW(dev) &&
> > >>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
> > >>>>>> +             return false;
> > >>>>>> +
> > >>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> > >>>>>> +}
> > >>>>>
> > >>>>> This doens't make sense to me. The power down cycle may have
> > >>>>> completed just before, and so this would claim we don't have to
> > >>>>> wait for the power_cycle_delay.
> > >>>>
> > >>>> Not sure I understand your concern correctly. You are right, power
> > >>>> down cycle may have completed just before and if it has then we don't
> > >>>> need to wait. But in case the power down cycle is in progress as per
> > >>>> internal state, then we need to wait for it to complete. This will
> > >>>> happen for example in non-suspend disable path and will be handled
> > >>>> correctly. In case of actual suspend/resume, this would have
> > >>>> successfully completed and will skip the wait as it is not needed
> > >>>> before enabling panel power.
> > >>>>
> > >>>>>
> > >>>>>> +
> > >>>>>>   static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> > >>>>>>   {
> > >>>>>>        struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > >>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
> > >>>>>>                 port_name(dp_to_dig_port(intel_dp)->port)))
> > >>>>>>                return;
> > >>>>>>
> > >>>>>> -     wait_panel_power_cycle(intel_dp);
> > >>>>>> +     if (edp_panel_off_seq(intel_dp))
> > >>>>>> +             wait_panel_power_cycle(intel_dp);
> > >>>
> > >>> Looking in from the side, I have no idea what this is meant to do. At
> > >>> the very least you need your explanatory paragraph here which would
> > >>> include what exactly you are waiting for at the start of edp_panel_on
> > >>> (and please try and find a better name for edp_panel_off_seq()).
> > >>
> > >> I will add a comment. Basically I am not additionally waiting, but
> > >> converting the wait which was already there to a conditional wait. The
> > >> edp_panel_off_seq, checks if panel power down sequence is in progress.
> > >> In that case we need to wait for the panel power cycle delay. If it is
> > >> not in that sequence, there is no need to wait. I will make an attempt
> > >> again on the naming in next patch update.
> > >
> > > As far I remeber you need to wait for power_cycle_delay between power
> > > down cycle and power up cycle. You're trying to throw that wait away
> > > entirely, unless the function happens get called while the power down
> > 
> > Yes you are right and I realize I made a mistake in my patch which is 
> > not checking PP_CYCLE_DELAY_ACTIVE bit.
> > 
> > > cycle is still in progress. We should already optimize away redundant
> > > waits by tracking the end of the power down cycle with the jiffies
> > > tracking.
> > >
> > > Actually looking at the code the power_cycle_delay gets counted from
> > > the start of the last power down cycle, so supposedly it's always at
> > > least as long as the power down cycle, and typically it's quite a bit
> > > longer that that. But that doesn't change the fact that you can't
> > > just skip it because the power down cycle delay happened to end
> > > already.
> > >
> > > So what we do now is:
> > > 1. initiate power down cycle
> > > 2. last_power_cycle=jiffies
> > > 3. wait for power down (I suppose this actually waits
> > >     until the power down delay has passed since that's
> > >     programmes into the PPS).
> > > 4. wait for power_cycle_delay from last_power_cycle
> > > 5. initiate power up cycle
> > >
> > > I think with your patch step 4 would always be skipped since the
> > > power down cycle has already ended, and then we fail to honor the
> > > power cycle delay.
> > 
> > Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that 
> > check will take care of this scenario I guess ?
> 
> Nope. The vdd force bit doesn't respect the PPS state machine, so we
> must do the waits manually instead. And in theory your patch wouldn't
> do anything anyway since the sleep already takes into account when the
> power cycle delay started.
> 
> The fact that we use msleep() may actually make those sleeps somewhat
> longer, and maybe we should also think about switching to usleep_range()
> here.

Oh, and we may want to stop using the power sequencer forced delay,
at least for the power up delay since we could then speed up the power
up time by waiting for the long HPD instead. Clint even sent a patch for
that, but I don't think it actually change the PPS delays. But maybe
the PPS won't really kick in due to our use of the vdd force bit during
panel power up?

The bahaviour of the PPS delays vs. the vdd force bit isn't actually
documented, so it would be something that could be studied a bit by
banging on the hardware. Probably best done on a machine where there's
no local panel to avoid damaging it if things go bad.

> 
> > 
> > Regards
> > Shobhit
> > 
> > >
> > > Actually the power_cycle delay also gets programmed into the PPS so I
> > > supose it would enforce the wait anyway when you initiate the power
> > > up cycle (unless the PPS got totally reset due to power wells etc.,
> > > which does seem like a real concern. The even bigger concern is the
> > > vdd force bit for which the PPS does no enforcement.
> > >
> > > The power_down_delay handling seems a bit wonky. We only wait for it
> > > when turning off the port. I guess I would need to go re-read the spec
> > > to figure out what it's meant to protect anyway.
> > >
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Thulasimani, Sivakumar]

On 12/10/2015 7:08 PM, Ville Syrjälä wrote:
> On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote:
>> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
>>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
>>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>>>>> During resume, while turning the EDP panel power on, we need not wait
>>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
>>>>>>>>> in progress and then only wait. This improves our resume time significantly.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>>> ---
>>>>>>>>>    drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>>>>    1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>> index f335c92..10ec669 100644
>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>>>>>>>>         return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>>>>>>>>>    }
>>>>>>>>>
>>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>>>>> +{
>>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>>> +
>>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>>>>> +
>>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>>>>> +             return false;
>>>>>>>>> +
>>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>>>>> +}
>>>>>>>> This doens't make sense to me. The power down cycle may have
>>>>>>>> completed just before, and so this would claim we don't have to
>>>>>>>> wait for the power_cycle_delay.
>>>>>>> Not sure I understand your concern correctly. You are right, power
>>>>>>> down cycle may have completed just before and if it has then we don't
>>>>>>> need to wait. But in case the power down cycle is in progress as per
>>>>>>> internal state, then we need to wait for it to complete. This will
>>>>>>> happen for example in non-suspend disable path and will be handled
>>>>>>> correctly. In case of actual suspend/resume, this would have
>>>>>>> successfully completed and will skip the wait as it is not needed
>>>>>>> before enabling panel power.
>>>>>>>
>>>>>>>>> +
>>>>>>>>>    static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>>>>    {
>>>>>>>>>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>>>>>>>>>                  port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>>>>                 return;
>>>>>>>>>
>>>>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>>>> Looking in from the side, I have no idea what this is meant to do. At
>>>>>> the very least you need your explanatory paragraph here which would
>>>>>> include what exactly you are waiting for at the start of edp_panel_on
>>>>>> (and please try and find a better name for edp_panel_off_seq()).
>>>>> I will add a comment. Basically I am not additionally waiting, but
>>>>> converting the wait which was already there to a conditional wait. The
>>>>> edp_panel_off_seq, checks if panel power down sequence is in progress.
>>>>> In that case we need to wait for the panel power cycle delay. If it is
>>>>> not in that sequence, there is no need to wait. I will make an attempt
>>>>> again on the naming in next patch update.
>>>> As far I remeber you need to wait for power_cycle_delay between power
>>>> down cycle and power up cycle. You're trying to throw that wait away
>>>> entirely, unless the function happens get called while the power down
>>> Yes you are right and I realize I made a mistake in my patch which is
>>> not checking PP_CYCLE_DELAY_ACTIVE bit.
>>>
>>>> cycle is still in progress. We should already optimize away redundant
>>>> waits by tracking the end of the power down cycle with the jiffies
>>>> tracking.
>>>>
>>>> Actually looking at the code the power_cycle_delay gets counted from
>>>> the start of the last power down cycle, so supposedly it's always at
>>>> least as long as the power down cycle, and typically it's quite a bit
>>>> longer that that. But that doesn't change the fact that you can't
>>>> just skip it because the power down cycle delay happened to end
>>>> already.
>>>>
>>>> So what we do now is:
>>>> 1. initiate power down cycle
>>>> 2. last_power_cycle=jiffies
>>>> 3. wait for power down (I suppose this actually waits
>>>>      until the power down delay has passed since that's
>>>>      programmes into the PPS).
>>>> 4. wait for power_cycle_delay from last_power_cycle
>>>> 5. initiate power up cycle
>>>>
>>>> I think with your patch step 4 would always be skipped since the
>>>> power down cycle has already ended, and then we fail to honor the
>>>> power cycle delay.
>>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that
>>> check will take care of this scenario I guess ?
>> Nope. The vdd force bit doesn't respect the PPS state machine, so we
>> must do the waits manually instead. And in theory your patch wouldn't
>> do anything anyway since the sleep already takes into account when the
>> power cycle delay started.
>>
>> The fact that we use msleep() may actually make those sleeps somewhat
>> longer, and maybe we should also think about switching to usleep_range()
>> here.
> Oh, and we may want to stop using the power sequencer forced delay,
> at least for the power up delay since we could then speed up the power
> up time by waiting for the long HPD instead. Clint even sent a patch for
> that, but I don't think it actually change the PPS delays. But maybe
> the PPS won't really kick in due to our use of the vdd force bit during
> panel power up?
>
> The bahaviour of the PPS delays vs. the vdd force bit isn't actually
> documented, so it would be something that could be studied a bit by
> banging on the hardware. Probably best done on a machine where there's
> no local panel to avoid damaging it if things go bad.
i think i updated about using hpd/edid read instead of waiting entire T3 
delay for power on
during IRC chat sometime back to danvet. in short it does not work across
all panels, it causes blooming effect on some panels resulting in panel 
failing
to turn on, which will also affect panel life.

regards,
Sivakumar
>>> Regards
>>> Shobhit
>>>
>>>> Actually the power_cycle delay also gets programmed into the PPS so I
>>>> supose it would enforce the wait anyway when you initiate the power
>>>> up cycle (unless the PPS got totally reset due to power wells etc.,
>>>> which does seem like a real concern. The even bigger concern is the
>>>> vdd force bit for which the PPS does no enforcement.
>>>>
>>>> The power_down_delay handling seems a bit wonky. We only wait for it
>>>> when turning off the port. I guess I would need to go re-read the spec
>>>> to figure out what it's meant to protect anyway.
>>>>
>> -- 
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Ville Syrjälä]

On Thu, Dec 10, 2015 at 08:09:01PM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 12/10/2015 7:08 PM, Ville Syrjälä wrote:
> > On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote:
> >> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
> >>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
> >>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
> >>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
> >>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
> >>>>>>> <ville.syrjala@linux.intel.com> wrote:
> >>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> >>>>>>>>> During resume, while turning the EDP panel power on, we need not wait
> >>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
> >>>>>>>>> in progress and then only wait. This improves our resume time significantly.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >>>>>>>>> ---
> >>>>>>>>>    drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
> >>>>>>>>>    1 file changed, 16 insertions(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>>>> index f335c92..10ec669 100644
> >>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
> >>>>>>>>>         return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
> >>>>>>>>>    }
> >>>>>>>>>
> >>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> >>>>>>>>> +{
> >>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>>>>>> +
> >>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
> >>>>>>>>> +
> >>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
> >>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
> >>>>>>>>> +             return false;
> >>>>>>>>> +
> >>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
> >>>>>>>>> +}
> >>>>>>>> This doens't make sense to me. The power down cycle may have
> >>>>>>>> completed just before, and so this would claim we don't have to
> >>>>>>>> wait for the power_cycle_delay.
> >>>>>>> Not sure I understand your concern correctly. You are right, power
> >>>>>>> down cycle may have completed just before and if it has then we don't
> >>>>>>> need to wait. But in case the power down cycle is in progress as per
> >>>>>>> internal state, then we need to wait for it to complete. This will
> >>>>>>> happen for example in non-suspend disable path and will be handled
> >>>>>>> correctly. In case of actual suspend/resume, this would have
> >>>>>>> successfully completed and will skip the wait as it is not needed
> >>>>>>> before enabling panel power.
> >>>>>>>
> >>>>>>>>> +
> >>>>>>>>>    static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >>>>>>>>>    {
> >>>>>>>>>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
> >>>>>>>>>                  port_name(dp_to_dig_port(intel_dp)->port)))
> >>>>>>>>>                 return;
> >>>>>>>>>
> >>>>>>>>> -     wait_panel_power_cycle(intel_dp);
> >>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
> >>>>>>>>> +             wait_panel_power_cycle(intel_dp);
> >>>>>> Looking in from the side, I have no idea what this is meant to do. At
> >>>>>> the very least you need your explanatory paragraph here which would
> >>>>>> include what exactly you are waiting for at the start of edp_panel_on
> >>>>>> (and please try and find a better name for edp_panel_off_seq()).
> >>>>> I will add a comment. Basically I am not additionally waiting, but
> >>>>> converting the wait which was already there to a conditional wait. The
> >>>>> edp_panel_off_seq, checks if panel power down sequence is in progress.
> >>>>> In that case we need to wait for the panel power cycle delay. If it is
> >>>>> not in that sequence, there is no need to wait. I will make an attempt
> >>>>> again on the naming in next patch update.
> >>>> As far I remeber you need to wait for power_cycle_delay between power
> >>>> down cycle and power up cycle. You're trying to throw that wait away
> >>>> entirely, unless the function happens get called while the power down
> >>> Yes you are right and I realize I made a mistake in my patch which is
> >>> not checking PP_CYCLE_DELAY_ACTIVE bit.
> >>>
> >>>> cycle is still in progress. We should already optimize away redundant
> >>>> waits by tracking the end of the power down cycle with the jiffies
> >>>> tracking.
> >>>>
> >>>> Actually looking at the code the power_cycle_delay gets counted from
> >>>> the start of the last power down cycle, so supposedly it's always at
> >>>> least as long as the power down cycle, and typically it's quite a bit
> >>>> longer that that. But that doesn't change the fact that you can't
> >>>> just skip it because the power down cycle delay happened to end
> >>>> already.
> >>>>
> >>>> So what we do now is:
> >>>> 1. initiate power down cycle
> >>>> 2. last_power_cycle=jiffies
> >>>> 3. wait for power down (I suppose this actually waits
> >>>>      until the power down delay has passed since that's
> >>>>      programmes into the PPS).
> >>>> 4. wait for power_cycle_delay from last_power_cycle
> >>>> 5. initiate power up cycle
> >>>>
> >>>> I think with your patch step 4 would always be skipped since the
> >>>> power down cycle has already ended, and then we fail to honor the
> >>>> power cycle delay.
> >>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that
> >>> check will take care of this scenario I guess ?
> >> Nope. The vdd force bit doesn't respect the PPS state machine, so we
> >> must do the waits manually instead. And in theory your patch wouldn't
> >> do anything anyway since the sleep already takes into account when the
> >> power cycle delay started.
> >>
> >> The fact that we use msleep() may actually make those sleeps somewhat
> >> longer, and maybe we should also think about switching to usleep_range()
> >> here.
> > Oh, and we may want to stop using the power sequencer forced delay,
> > at least for the power up delay since we could then speed up the power
> > up time by waiting for the long HPD instead. Clint even sent a patch for
> > that, but I don't think it actually change the PPS delays. But maybe
> > the PPS won't really kick in due to our use of the vdd force bit during
> > panel power up?
> >
> > The bahaviour of the PPS delays vs. the vdd force bit isn't actually
> > documented, so it would be something that could be studied a bit by
> > banging on the hardware. Probably best done on a machine where there's
> > no local panel to avoid damaging it if things go bad.
> i think i updated about using hpd/edid read instead of waiting entire T3 
> delay for power on
> during IRC chat sometime back to danvet. in short it does not work across
> all panels, it causes blooming effect on some panels resulting in panel 
> failing
> to turn on, which will also affect panel life.

:( I was hoping we could use it for eDP, but I guess being an optimist
when it comes to hardware never pays off.

I wonder if we could at least start doing some of the AUX communication
sooner. That is, maybe we can do at least the DP_LINK_BW_SET and the
DP_DOWNSPREAD_CTRL DPCD writes after getting the HPD, and only then
wait for the end of the power on delay just before starting the
link training? And the same for any other AUX chatter when the panel
is otherwise powered off?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Thulasimani, Sivakumar]

On 12/10/2015 8:32 PM, Ville Syrjälä wrote:
> On Thu, Dec 10, 2015 at 08:09:01PM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 12/10/2015 7:08 PM, Ville Syrjälä wrote:
>>> On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote:
>>>> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
>>>>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
>>>>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>>>>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>>>>>>> During resume, while turning the EDP panel power on, we need not wait
>>>>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
>>>>>>>>>>> in progress and then only wait. This improves our resume time significantly.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>>     drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>>>>>>     1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>> index f335c92..10ec669 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>>>>>>>>>>          return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>>>>>>>>>>>     }
>>>>>>>>>>>
>>>>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>>>>>>> +{
>>>>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>>>>> +
>>>>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>>>>>>> +
>>>>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>>>>>>> +             return false;
>>>>>>>>>>> +
>>>>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>>>>>>> +}
>>>>>>>>>> This doens't make sense to me. The power down cycle may have
>>>>>>>>>> completed just before, and so this would claim we don't have to
>>>>>>>>>> wait for the power_cycle_delay.
>>>>>>>>> Not sure I understand your concern correctly. You are right, power
>>>>>>>>> down cycle may have completed just before and if it has then we don't
>>>>>>>>> need to wait. But in case the power down cycle is in progress as per
>>>>>>>>> internal state, then we need to wait for it to complete. This will
>>>>>>>>> happen for example in non-suspend disable path and will be handled
>>>>>>>>> correctly. In case of actual suspend/resume, this would have
>>>>>>>>> successfully completed and will skip the wait as it is not needed
>>>>>>>>> before enabling panel power.
>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>>     static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>>>>>>     {
>>>>>>>>>>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>>>>>>>>>>>                   port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>>>>>>                  return;
>>>>>>>>>>>
>>>>>>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>>>>>> Looking in from the side, I have no idea what this is meant to do. At
>>>>>>>> the very least you need your explanatory paragraph here which would
>>>>>>>> include what exactly you are waiting for at the start of edp_panel_on
>>>>>>>> (and please try and find a better name for edp_panel_off_seq()).
>>>>>>> I will add a comment. Basically I am not additionally waiting, but
>>>>>>> converting the wait which was already there to a conditional wait. The
>>>>>>> edp_panel_off_seq, checks if panel power down sequence is in progress.
>>>>>>> In that case we need to wait for the panel power cycle delay. If it is
>>>>>>> not in that sequence, there is no need to wait. I will make an attempt
>>>>>>> again on the naming in next patch update.
>>>>>> As far I remeber you need to wait for power_cycle_delay between power
>>>>>> down cycle and power up cycle. You're trying to throw that wait away
>>>>>> entirely, unless the function happens get called while the power down
>>>>> Yes you are right and I realize I made a mistake in my patch which is
>>>>> not checking PP_CYCLE_DELAY_ACTIVE bit.
>>>>>
>>>>>> cycle is still in progress. We should already optimize away redundant
>>>>>> waits by tracking the end of the power down cycle with the jiffies
>>>>>> tracking.
>>>>>>
>>>>>> Actually looking at the code the power_cycle_delay gets counted from
>>>>>> the start of the last power down cycle, so supposedly it's always at
>>>>>> least as long as the power down cycle, and typically it's quite a bit
>>>>>> longer that that. But that doesn't change the fact that you can't
>>>>>> just skip it because the power down cycle delay happened to end
>>>>>> already.
>>>>>>
>>>>>> So what we do now is:
>>>>>> 1. initiate power down cycle
>>>>>> 2. last_power_cycle=jiffies
>>>>>> 3. wait for power down (I suppose this actually waits
>>>>>>       until the power down delay has passed since that's
>>>>>>       programmes into the PPS).
>>>>>> 4. wait for power_cycle_delay from last_power_cycle
>>>>>> 5. initiate power up cycle
>>>>>>
>>>>>> I think with your patch step 4 would always be skipped since the
>>>>>> power down cycle has already ended, and then we fail to honor the
>>>>>> power cycle delay.
>>>>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that
>>>>> check will take care of this scenario I guess ?
>>>> Nope. The vdd force bit doesn't respect the PPS state machine, so we
>>>> must do the waits manually instead. And in theory your patch wouldn't
>>>> do anything anyway since the sleep already takes into account when the
>>>> power cycle delay started.
>>>>
>>>> The fact that we use msleep() may actually make those sleeps somewhat
>>>> longer, and maybe we should also think about switching to usleep_range()
>>>> here.
>>> Oh, and we may want to stop using the power sequencer forced delay,
>>> at least for the power up delay since we could then speed up the power
>>> up time by waiting for the long HPD instead. Clint even sent a patch for
>>> that, but I don't think it actually change the PPS delays. But maybe
>>> the PPS won't really kick in due to our use of the vdd force bit during
>>> panel power up?
>>>
>>> The bahaviour of the PPS delays vs. the vdd force bit isn't actually
>>> documented, so it would be something that could be studied a bit by
>>> banging on the hardware. Probably best done on a machine where there's
>>> no local panel to avoid damaging it if things go bad.
>> i think i updated about using hpd/edid read instead of waiting entire T3
>> delay for power on
>> during IRC chat sometime back to danvet. in short it does not work across
>> all panels, it causes blooming effect on some panels resulting in panel
>> failing
>> to turn on, which will also affect panel life.
> :( I was hoping we could use it for eDP, but I guess being an optimist
> when it comes to hardware never pays off.
>
> I wonder if we could at least start doing some of the AUX communication
> sooner. That is, maybe we can do at least the DP_LINK_BW_SET and the
> DP_DOWNSPREAD_CTRL DPCD writes after getting the HPD, and only then
> wait for the end of the power on delay just before starting the
> link training? And the same for any other AUX chatter when the panel
> is otherwise powered off?
>
:) two parts to optimization of such DPCD aux communications
1) they can change during modeset so we can never be sure if the value 
will be final
2) those are too small operations to help in any major way for power on.

Some more costly/time consuming operations we can optimize to "boot to 
edp" are
1) avoid VDD & PPS on/0ff. i.e  totally avoid doing any modeset op, 
since eDP usually
has one or two modes it is highly likely that we will apply the same mode
brought up by BIOS, so any effort on our side is just redoing stuff.
2) if the above is not possible, we can remove as much of VDD off/on
in our flow as possible.(each off will take the entire Power down delay time
and the next on will take power on delay time)
3) our dpcd read/write logic should be optimized, our whole Link training
on CHT is taking ~15ms, which should have been completed in under 10ms.

if we implement point 1 above for boot/s4 resume we can bring down modeset
time for edp alone scenario to few ms.
i don't want to keep talking but not doing anything, will finish my 
compliance
activities (including upstreaming) and then will try to target each of 
the above. :)

regards,
Sivakumar

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

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Kumar, Shobhit]

On 12/11/2015 04:55 PM, Thulasimani, Sivakumar wrote:
>
>
> On 12/10/2015 8:32 PM, Ville Syrjälä wrote:
>> On Thu, Dec 10, 2015 at 08:09:01PM +0530, Thulasimani, Sivakumar wrote:
>>>
>>> On 12/10/2015 7:08 PM, Ville Syrjälä wrote:
>>>> On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote:
>>>>> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
>>>>>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
>>>>>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>>>>>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson
>>>>>>>> <chris@chris-wilson.co.uk> wrote:
>>>>>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>>>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>>>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>>>>>>>> During resume, while turning the EDP panel power on, we need
>>>>>>>>>>>> not wait
>>>>>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power
>>>>>>>>>>>> down sequence
>>>>>>>>>>>> in progress and then only wait. This improves our resume
>>>>>>>>>>>> time significantly.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>     drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>>>>>>>     1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>> index f335c92..10ec669 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct
>>>>>>>>>>>> intel_dp *intel_dp)
>>>>>>>>>>>>          return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON)
>>>>>>>>>>>> != 0;
>>>>>>>>>>>>     }
>>>>>>>>>>>>
>>>>>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>>>>>> +
>>>>>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>>>>>>>> +
>>>>>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>>>>>>>> +             return false;
>>>>>>>>>>>> +
>>>>>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) &
>>>>>>>>>>>> PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>>>>>>>> +}
>>>>>>>>>>> This doens't make sense to me. The power down cycle may have
>>>>>>>>>>> completed just before, and so this would claim we don't have to
>>>>>>>>>>> wait for the power_cycle_delay.
>>>>>>>>>> Not sure I understand your concern correctly. You are right,
>>>>>>>>>> power
>>>>>>>>>> down cycle may have completed just before and if it has then
>>>>>>>>>> we don't
>>>>>>>>>> need to wait. But in case the power down cycle is in progress
>>>>>>>>>> as per
>>>>>>>>>> internal state, then we need to wait for it to complete. This
>>>>>>>>>> will
>>>>>>>>>> happen for example in non-suspend disable path and will be
>>>>>>>>>> handled
>>>>>>>>>> correctly. In case of actual suspend/resume, this would have
>>>>>>>>>> successfully completed and will skip the wait as it is not needed
>>>>>>>>>> before enabling panel power.
>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>>     static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>>>>>>>     {
>>>>>>>>>>>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct
>>>>>>>>>>>> intel_dp *intel_dp)
>>>>>>>>>>>>                   port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>>>>>>>                  return;
>>>>>>>>>>>>
>>>>>>>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>>>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>>>>>>> Looking in from the side, I have no idea what this is meant to
>>>>>>>>> do. At
>>>>>>>>> the very least you need your explanatory paragraph here which
>>>>>>>>> would
>>>>>>>>> include what exactly you are waiting for at the start of
>>>>>>>>> edp_panel_on
>>>>>>>>> (and please try and find a better name for edp_panel_off_seq()).
>>>>>>>> I will add a comment. Basically I am not additionally waiting, but
>>>>>>>> converting the wait which was already there to a conditional
>>>>>>>> wait. The
>>>>>>>> edp_panel_off_seq, checks if panel power down sequence is in
>>>>>>>> progress.
>>>>>>>> In that case we need to wait for the panel power cycle delay. If
>>>>>>>> it is
>>>>>>>> not in that sequence, there is no need to wait. I will make an
>>>>>>>> attempt
>>>>>>>> again on the naming in next patch update.
>>>>>>> As far I remeber you need to wait for power_cycle_delay between
>>>>>>> power
>>>>>>> down cycle and power up cycle. You're trying to throw that wait away
>>>>>>> entirely, unless the function happens get called while the power
>>>>>>> down
>>>>>> Yes you are right and I realize I made a mistake in my patch which is
>>>>>> not checking PP_CYCLE_DELAY_ACTIVE bit.
>>>>>>
>>>>>>> cycle is still in progress. We should already optimize away
>>>>>>> redundant
>>>>>>> waits by tracking the end of the power down cycle with the jiffies
>>>>>>> tracking.
>>>>>>>
>>>>>>> Actually looking at the code the power_cycle_delay gets counted from
>>>>>>> the start of the last power down cycle, so supposedly it's always at
>>>>>>> least as long as the power down cycle, and typically it's quite a
>>>>>>> bit
>>>>>>> longer that that. But that doesn't change the fact that you can't
>>>>>>> just skip it because the power down cycle delay happened to end
>>>>>>> already.
>>>>>>>
>>>>>>> So what we do now is:
>>>>>>> 1. initiate power down cycle
>>>>>>> 2. last_power_cycle=jiffies
>>>>>>> 3. wait for power down (I suppose this actually waits
>>>>>>>       until the power down delay has passed since that's
>>>>>>>       programmes into the PPS).
>>>>>>> 4. wait for power_cycle_delay from last_power_cycle
>>>>>>> 5. initiate power up cycle
>>>>>>>
>>>>>>> I think with your patch step 4 would always be skipped since the
>>>>>>> power down cycle has already ended, and then we fail to honor the
>>>>>>> power cycle delay.
>>>>>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding
>>>>>> that
>>>>>> check will take care of this scenario I guess ?
>>>>> Nope. The vdd force bit doesn't respect the PPS state machine, so we
>>>>> must do the waits manually instead. And in theory your patch wouldn't
>>>>> do anything anyway since the sleep already takes into account when the
>>>>> power cycle delay started.
>>>>>
>>>>> The fact that we use msleep() may actually make those sleeps somewhat
>>>>> longer, and maybe we should also think about switching to
>>>>> usleep_range()
>>>>> here.
>>>> Oh, and we may want to stop using the power sequencer forced delay,
>>>> at least for the power up delay since we could then speed up the power
>>>> up time by waiting for the long HPD instead. Clint even sent a patch
>>>> for
>>>> that, but I don't think it actually change the PPS delays. But maybe
>>>> the PPS won't really kick in due to our use of the vdd force bit during
>>>> panel power up?
>>>>
>>>> The bahaviour of the PPS delays vs. the vdd force bit isn't actually
>>>> documented, so it would be something that could be studied a bit by
>>>> banging on the hardware. Probably best done on a machine where there's
>>>> no local panel to avoid damaging it if things go bad.
>>> i think i updated about using hpd/edid read instead of waiting entire T3
>>> delay for power on
>>> during IRC chat sometime back to danvet. in short it does not work
>>> across
>>> all panels, it causes blooming effect on some panels resulting in panel
>>> failing
>>> to turn on, which will also affect panel life.
>> :( I was hoping we could use it for eDP, but I guess being an optimist
>> when it comes to hardware never pays off.
>>
>> I wonder if we could at least start doing some of the AUX communication
>> sooner. That is, maybe we can do at least the DP_LINK_BW_SET and the
>> DP_DOWNSPREAD_CTRL DPCD writes after getting the HPD, and only then
>> wait for the end of the power on delay just before starting the
>> link training? And the same for any other AUX chatter when the panel
>> is otherwise powered off?
>>
> :) two parts to optimization of such DPCD aux communications
> 1) they can change during modeset so we can never be sure if the value
> will be final
> 2) those are too small operations to help in any major way for power on.
>
> Some more costly/time consuming operations we can optimize to "boot to
> edp" are
> 1) avoid VDD & PPS on/0ff. i.e  totally avoid doing any modeset op,
> since eDP usually
> has one or two modes it is highly likely that we will apply the same mode
> brought up by BIOS, so any effort on our side is just redoing stuff.
> 2) if the above is not possible, we can remove as much of VDD off/on
> in our flow as possible.(each off will take the entire Power down delay
> time
> and the next on will take power on delay time)
> 3) our dpcd read/write logic should be optimized, our whole Link training
> on CHT is taking ~15ms, which should have been completed in under 10ms.
>
> if we implement point 1 above for boot/s4 resume we can bring down modeset
> time for edp alone scenario to few ms.
> i don't want to keep talking but not doing anything, will finish my
> compliance
> activities (including upstreaming) and then will try to target each of
> the above. :)

Thanks Siva for iterating few optimization points which I have been also 
thinking are needed. But lets take the over all optimization 
possibilities in another thread. My question to you guys in the current 
context is -

1. Shall we keep the panel_power_cycle_delay as is but adjusting jiffies 
based on wall clock as Ville suggested. This will ensure that we skip 
the wait in suspend/resume scenario, or

2. Have the HW PPS based tracking for delays and also take care that VDD 
force on is not called before panel power cycle is completely over. If 
all do not agree or see issues with this approach, we can keep this for 
later when we optimize vdd on/off sequences as suggested by siva and go 
for now with approach 1.

Regards
Shobhit

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

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Daniel Vetter]

On Fri, Dec 11, 2015 at 05:11:23PM +0530, Kumar, Shobhit wrote:
> On 12/11/2015 04:55 PM, Thulasimani, Sivakumar wrote:
> >
> >
> >On 12/10/2015 8:32 PM, Ville Syrjälä wrote:
> >>On Thu, Dec 10, 2015 at 08:09:01PM +0530, Thulasimani, Sivakumar wrote:
> >>>
> >>>On 12/10/2015 7:08 PM, Ville Syrjälä wrote:
> >>>>On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote:
> >>>>>On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
> >>>>>>On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
> >>>>>>>On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
> >>>>>>>>On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson
> >>>>>>>><chris@chris-wilson.co.uk> wrote:
> >>>>>>>>>On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
> >>>>>>>>>>On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
> >>>>>>>>>><ville.syrjala@linux.intel.com> wrote:
> >>>>>>>>>>>On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
> >>>>>>>>>>>>During resume, while turning the EDP panel power on, we need
> >>>>>>>>>>>>not wait
> >>>>>>>>>>>>blindly for panel_power_cycle_delay. Check if panel power
> >>>>>>>>>>>>down sequence
> >>>>>>>>>>>>in progress and then only wait. This improves our resume
> >>>>>>>>>>>>time significantly.
> >>>>>>>>>>>>
> >>>>>>>>>>>>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >>>>>>>>>>>>---
> >>>>>>>>>>>>    drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
> >>>>>>>>>>>>    1 file changed, 16 insertions(+), 1 deletion(-)
> >>>>>>>>>>>>
> >>>>>>>>>>>>diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>>>>>>>b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>>>>>>>index f335c92..10ec669 100644
> >>>>>>>>>>>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>>>>>>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>>>>>>>>>@@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct
> >>>>>>>>>>>>intel_dp *intel_dp)
> >>>>>>>>>>>>         return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON)
> >>>>>>>>>>>>!= 0;
> >>>>>>>>>>>>    }
> >>>>>>>>>>>>
> >>>>>>>>>>>>+static bool edp_panel_off_seq(struct intel_dp *intel_dp)
> >>>>>>>>>>>>+{
> >>>>>>>>>>>>+     struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>>>>>>>>+     struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>>>>>>>>>+
> >>>>>>>>>>>>+     lockdep_assert_held(&dev_priv->pps_mutex);
> >>>>>>>>>>>>+
> >>>>>>>>>>>>+     if (IS_VALLEYVIEW(dev) &&
> >>>>>>>>>>>>+         intel_dp->pps_pipe == INVALID_PIPE)
> >>>>>>>>>>>>+             return false;
> >>>>>>>>>>>>+
> >>>>>>>>>>>>+     return (I915_READ(_pp_stat_reg(intel_dp)) &
> >>>>>>>>>>>>PP_SEQUENCE_POWER_DOWN) != 0;
> >>>>>>>>>>>>+}
> >>>>>>>>>>>This doens't make sense to me. The power down cycle may have
> >>>>>>>>>>>completed just before, and so this would claim we don't have to
> >>>>>>>>>>>wait for the power_cycle_delay.
> >>>>>>>>>>Not sure I understand your concern correctly. You are right,
> >>>>>>>>>>power
> >>>>>>>>>>down cycle may have completed just before and if it has then
> >>>>>>>>>>we don't
> >>>>>>>>>>need to wait. But in case the power down cycle is in progress
> >>>>>>>>>>as per
> >>>>>>>>>>internal state, then we need to wait for it to complete. This
> >>>>>>>>>>will
> >>>>>>>>>>happen for example in non-suspend disable path and will be
> >>>>>>>>>>handled
> >>>>>>>>>>correctly. In case of actual suspend/resume, this would have
> >>>>>>>>>>successfully completed and will skip the wait as it is not needed
> >>>>>>>>>>before enabling panel power.
> >>>>>>>>>>
> >>>>>>>>>>>>+
> >>>>>>>>>>>>    static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >>>>>>>>>>>>    {
> >>>>>>>>>>>>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>>>>>>>>>@@ -2025,7 +2039,8 @@ static void edp_panel_on(struct
> >>>>>>>>>>>>intel_dp *intel_dp)
> >>>>>>>>>>>>                  port_name(dp_to_dig_port(intel_dp)->port)))
> >>>>>>>>>>>>                 return;
> >>>>>>>>>>>>
> >>>>>>>>>>>>-     wait_panel_power_cycle(intel_dp);
> >>>>>>>>>>>>+     if (edp_panel_off_seq(intel_dp))
> >>>>>>>>>>>>+             wait_panel_power_cycle(intel_dp);
> >>>>>>>>>Looking in from the side, I have no idea what this is meant to
> >>>>>>>>>do. At
> >>>>>>>>>the very least you need your explanatory paragraph here which
> >>>>>>>>>would
> >>>>>>>>>include what exactly you are waiting for at the start of
> >>>>>>>>>edp_panel_on
> >>>>>>>>>(and please try and find a better name for edp_panel_off_seq()).
> >>>>>>>>I will add a comment. Basically I am not additionally waiting, but
> >>>>>>>>converting the wait which was already there to a conditional
> >>>>>>>>wait. The
> >>>>>>>>edp_panel_off_seq, checks if panel power down sequence is in
> >>>>>>>>progress.
> >>>>>>>>In that case we need to wait for the panel power cycle delay. If
> >>>>>>>>it is
> >>>>>>>>not in that sequence, there is no need to wait. I will make an
> >>>>>>>>attempt
> >>>>>>>>again on the naming in next patch update.
> >>>>>>>As far I remeber you need to wait for power_cycle_delay between
> >>>>>>>power
> >>>>>>>down cycle and power up cycle. You're trying to throw that wait away
> >>>>>>>entirely, unless the function happens get called while the power
> >>>>>>>down
> >>>>>>Yes you are right and I realize I made a mistake in my patch which is
> >>>>>>not checking PP_CYCLE_DELAY_ACTIVE bit.
> >>>>>>
> >>>>>>>cycle is still in progress. We should already optimize away
> >>>>>>>redundant
> >>>>>>>waits by tracking the end of the power down cycle with the jiffies
> >>>>>>>tracking.
> >>>>>>>
> >>>>>>>Actually looking at the code the power_cycle_delay gets counted from
> >>>>>>>the start of the last power down cycle, so supposedly it's always at
> >>>>>>>least as long as the power down cycle, and typically it's quite a
> >>>>>>>bit
> >>>>>>>longer that that. But that doesn't change the fact that you can't
> >>>>>>>just skip it because the power down cycle delay happened to end
> >>>>>>>already.
> >>>>>>>
> >>>>>>>So what we do now is:
> >>>>>>>1. initiate power down cycle
> >>>>>>>2. last_power_cycle=jiffies
> >>>>>>>3. wait for power down (I suppose this actually waits
> >>>>>>>      until the power down delay has passed since that's
> >>>>>>>      programmes into the PPS).
> >>>>>>>4. wait for power_cycle_delay from last_power_cycle
> >>>>>>>5. initiate power up cycle
> >>>>>>>
> >>>>>>>I think with your patch step 4 would always be skipped since the
> >>>>>>>power down cycle has already ended, and then we fail to honor the
> >>>>>>>power cycle delay.
> >>>>>>Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding
> >>>>>>that
> >>>>>>check will take care of this scenario I guess ?
> >>>>>Nope. The vdd force bit doesn't respect the PPS state machine, so we
> >>>>>must do the waits manually instead. And in theory your patch wouldn't
> >>>>>do anything anyway since the sleep already takes into account when the
> >>>>>power cycle delay started.
> >>>>>
> >>>>>The fact that we use msleep() may actually make those sleeps somewhat
> >>>>>longer, and maybe we should also think about switching to
> >>>>>usleep_range()
> >>>>>here.
> >>>>Oh, and we may want to stop using the power sequencer forced delay,
> >>>>at least for the power up delay since we could then speed up the power
> >>>>up time by waiting for the long HPD instead. Clint even sent a patch
> >>>>for
> >>>>that, but I don't think it actually change the PPS delays. But maybe
> >>>>the PPS won't really kick in due to our use of the vdd force bit during
> >>>>panel power up?
> >>>>
> >>>>The bahaviour of the PPS delays vs. the vdd force bit isn't actually
> >>>>documented, so it would be something that could be studied a bit by
> >>>>banging on the hardware. Probably best done on a machine where there's
> >>>>no local panel to avoid damaging it if things go bad.
> >>>i think i updated about using hpd/edid read instead of waiting entire T3
> >>>delay for power on
> >>>during IRC chat sometime back to danvet. in short it does not work
> >>>across
> >>>all panels, it causes blooming effect on some panels resulting in panel
> >>>failing
> >>>to turn on, which will also affect panel life.
> >>:( I was hoping we could use it for eDP, but I guess being an optimist
> >>when it comes to hardware never pays off.
> >>
> >>I wonder if we could at least start doing some of the AUX communication
> >>sooner. That is, maybe we can do at least the DP_LINK_BW_SET and the
> >>DP_DOWNSPREAD_CTRL DPCD writes after getting the HPD, and only then
> >>wait for the end of the power on delay just before starting the
> >>link training? And the same for any other AUX chatter when the panel
> >>is otherwise powered off?
> >>
> >:) two parts to optimization of such DPCD aux communications
> >1) they can change during modeset so we can never be sure if the value
> >will be final
> >2) those are too small operations to help in any major way for power on.
> >
> >Some more costly/time consuming operations we can optimize to "boot to
> >edp" are
> >1) avoid VDD & PPS on/0ff. i.e  totally avoid doing any modeset op,
> >since eDP usually
> >has one or two modes it is highly likely that we will apply the same mode
> >brought up by BIOS, so any effort on our side is just redoing stuff.
> >2) if the above is not possible, we can remove as much of VDD off/on
> >in our flow as possible.(each off will take the entire Power down delay
> >time
> >and the next on will take power on delay time)
> >3) our dpcd read/write logic should be optimized, our whole Link training
> >on CHT is taking ~15ms, which should have been completed in under 10ms.
> >
> >if we implement point 1 above for boot/s4 resume we can bring down modeset
> >time for edp alone scenario to few ms.
> >i don't want to keep talking but not doing anything, will finish my
> >compliance
> >activities (including upstreaming) and then will try to target each of
> >the above. :)
> 
> Thanks Siva for iterating few optimization points which I have been also
> thinking are needed. But lets take the over all optimization possibilities
> in another thread. My question to you guys in the current context is -
> 
> 1. Shall we keep the panel_power_cycle_delay as is but adjusting jiffies
> based on wall clock as Ville suggested. This will ensure that we skip the
> wait in suspend/resume scenario, or
> 
> 2. Have the HW PPS based tracking for delays and also take care that VDD
> force on is not called before panel power cycle is completely over. If all
> do not agree or see issues with this approach, we can keep this for later
> when we optimize vdd on/off sequences as suggested by siva and go for now
> with approach 1.

Iirc on some platforms PPS resets incorrectly and we need the waits from
step 1 to avoid frying the panel. That's why we added them. So I think
adjusting jiffie by the wallclock time we've been suspended would be best.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Kumar, Shobhit]

On 12/11/2015 10:30 PM, Daniel Vetter wrote:
> On Fri, Dec 11, 2015 at 05:11:23PM +0530, Kumar, Shobhit wrote:
>> On 12/11/2015 04:55 PM, Thulasimani, Sivakumar wrote:
>>>
>>>
>>> On 12/10/2015 8:32 PM, Ville Syrjälä wrote:
>>>> On Thu, Dec 10, 2015 at 08:09:01PM +0530, Thulasimani, Sivakumar wrote:
>>>>>
>>>>> On 12/10/2015 7:08 PM, Ville Syrjälä wrote:
>>>>>> On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote:
>>>>>>> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
>>>>>>>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
>>>>>>>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>>>>>>>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson
>>>>>>>>>> <chris@chris-wilson.co.uk> wrote:
>>>>>>>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>>>>>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>>>>>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>>>>>>>>>> During resume, while turning the EDP panel power on, we need
>>>>>>>>>>>>>> not wait
>>>>>>>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power
>>>>>>>>>>>>>> down sequence
>>>>>>>>>>>>>> in progress and then only wait. This improves our resume
>>>>>>>>>>>>>> time significantly.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>     drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>>>>>>>>>     1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>>>> index f335c92..10ec669 100644
>>>>>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct
>>>>>>>>>>>>>> intel_dp *intel_dp)
>>>>>>>>>>>>>>          return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON)
>>>>>>>>>>>>>> != 0;
>>>>>>>>>>>>>>     }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>>>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>>>>>>>>>> +             return false;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) &
>>>>>>>>>>>>>> PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>> This doens't make sense to me. The power down cycle may have
>>>>>>>>>>>>> completed just before, and so this would claim we don't have to
>>>>>>>>>>>>> wait for the power_cycle_delay.
>>>>>>>>>>>> Not sure I understand your concern correctly. You are right,
>>>>>>>>>>>> power
>>>>>>>>>>>> down cycle may have completed just before and if it has then
>>>>>>>>>>>> we don't
>>>>>>>>>>>> need to wait. But in case the power down cycle is in progress
>>>>>>>>>>>> as per
>>>>>>>>>>>> internal state, then we need to wait for it to complete. This
>>>>>>>>>>>> will
>>>>>>>>>>>> happen for example in non-suspend disable path and will be
>>>>>>>>>>>> handled
>>>>>>>>>>>> correctly. In case of actual suspend/resume, this would have
>>>>>>>>>>>> successfully completed and will skip the wait as it is not needed
>>>>>>>>>>>> before enabling panel power.
>>>>>>>>>>>>
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>     static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>>>>>>>>>     {
>>>>>>>>>>>>>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct
>>>>>>>>>>>>>> intel_dp *intel_dp)
>>>>>>>>>>>>>>                   port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>>>>>>>>>                  return;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>>>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>>>>>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>>>>>>>>> Looking in from the side, I have no idea what this is meant to
>>>>>>>>>>> do. At
>>>>>>>>>>> the very least you need your explanatory paragraph here which
>>>>>>>>>>> would
>>>>>>>>>>> include what exactly you are waiting for at the start of
>>>>>>>>>>> edp_panel_on
>>>>>>>>>>> (and please try and find a better name for edp_panel_off_seq()).
>>>>>>>>>> I will add a comment. Basically I am not additionally waiting, but
>>>>>>>>>> converting the wait which was already there to a conditional
>>>>>>>>>> wait. The
>>>>>>>>>> edp_panel_off_seq, checks if panel power down sequence is in
>>>>>>>>>> progress.
>>>>>>>>>> In that case we need to wait for the panel power cycle delay. If
>>>>>>>>>> it is
>>>>>>>>>> not in that sequence, there is no need to wait. I will make an
>>>>>>>>>> attempt
>>>>>>>>>> again on the naming in next patch update.
>>>>>>>>> As far I remeber you need to wait for power_cycle_delay between
>>>>>>>>> power
>>>>>>>>> down cycle and power up cycle. You're trying to throw that wait away
>>>>>>>>> entirely, unless the function happens get called while the power
>>>>>>>>> down
>>>>>>>> Yes you are right and I realize I made a mistake in my patch which is
>>>>>>>> not checking PP_CYCLE_DELAY_ACTIVE bit.
>>>>>>>>
>>>>>>>>> cycle is still in progress. We should already optimize away
>>>>>>>>> redundant
>>>>>>>>> waits by tracking the end of the power down cycle with the jiffies
>>>>>>>>> tracking.
>>>>>>>>>
>>>>>>>>> Actually looking at the code the power_cycle_delay gets counted from
>>>>>>>>> the start of the last power down cycle, so supposedly it's always at
>>>>>>>>> least as long as the power down cycle, and typically it's quite a
>>>>>>>>> bit
>>>>>>>>> longer that that. But that doesn't change the fact that you can't
>>>>>>>>> just skip it because the power down cycle delay happened to end
>>>>>>>>> already.
>>>>>>>>>
>>>>>>>>> So what we do now is:
>>>>>>>>> 1. initiate power down cycle
>>>>>>>>> 2. last_power_cycle=jiffies
>>>>>>>>> 3. wait for power down (I suppose this actually waits
>>>>>>>>>       until the power down delay has passed since that's
>>>>>>>>>       programmes into the PPS).
>>>>>>>>> 4. wait for power_cycle_delay from last_power_cycle
>>>>>>>>> 5. initiate power up cycle
>>>>>>>>>
>>>>>>>>> I think with your patch step 4 would always be skipped since the
>>>>>>>>> power down cycle has already ended, and then we fail to honor the
>>>>>>>>> power cycle delay.
>>>>>>>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding
>>>>>>>> that
>>>>>>>> check will take care of this scenario I guess ?
>>>>>>> Nope. The vdd force bit doesn't respect the PPS state machine, so we
>>>>>>> must do the waits manually instead. And in theory your patch wouldn't
>>>>>>> do anything anyway since the sleep already takes into account when the
>>>>>>> power cycle delay started.
>>>>>>>
>>>>>>> The fact that we use msleep() may actually make those sleeps somewhat
>>>>>>> longer, and maybe we should also think about switching to
>>>>>>> usleep_range()
>>>>>>> here.
>>>>>> Oh, and we may want to stop using the power sequencer forced delay,
>>>>>> at least for the power up delay since we could then speed up the power
>>>>>> up time by waiting for the long HPD instead. Clint even sent a patch
>>>>>> for
>>>>>> that, but I don't think it actually change the PPS delays. But maybe
>>>>>> the PPS won't really kick in due to our use of the vdd force bit during
>>>>>> panel power up?
>>>>>>
>>>>>> The bahaviour of the PPS delays vs. the vdd force bit isn't actually
>>>>>> documented, so it would be something that could be studied a bit by
>>>>>> banging on the hardware. Probably best done on a machine where there's
>>>>>> no local panel to avoid damaging it if things go bad.
>>>>> i think i updated about using hpd/edid read instead of waiting entire T3
>>>>> delay for power on
>>>>> during IRC chat sometime back to danvet. in short it does not work
>>>>> across
>>>>> all panels, it causes blooming effect on some panels resulting in panel
>>>>> failing
>>>>> to turn on, which will also affect panel life.
>>>> :( I was hoping we could use it for eDP, but I guess being an optimist
>>>> when it comes to hardware never pays off.
>>>>
>>>> I wonder if we could at least start doing some of the AUX communication
>>>> sooner. That is, maybe we can do at least the DP_LINK_BW_SET and the
>>>> DP_DOWNSPREAD_CTRL DPCD writes after getting the HPD, and only then
>>>> wait for the end of the power on delay just before starting the
>>>> link training? And the same for any other AUX chatter when the panel
>>>> is otherwise powered off?
>>>>
>>> :) two parts to optimization of such DPCD aux communications
>>> 1) they can change during modeset so we can never be sure if the value
>>> will be final
>>> 2) those are too small operations to help in any major way for power on.
>>>
>>> Some more costly/time consuming operations we can optimize to "boot to
>>> edp" are
>>> 1) avoid VDD & PPS on/0ff. i.e  totally avoid doing any modeset op,
>>> since eDP usually
>>> has one or two modes it is highly likely that we will apply the same mode
>>> brought up by BIOS, so any effort on our side is just redoing stuff.
>>> 2) if the above is not possible, we can remove as much of VDD off/on
>>> in our flow as possible.(each off will take the entire Power down delay
>>> time
>>> and the next on will take power on delay time)
>>> 3) our dpcd read/write logic should be optimized, our whole Link training
>>> on CHT is taking ~15ms, which should have been completed in under 10ms.
>>>
>>> if we implement point 1 above for boot/s4 resume we can bring down modeset
>>> time for edp alone scenario to few ms.
>>> i don't want to keep talking but not doing anything, will finish my
>>> compliance
>>> activities (including upstreaming) and then will try to target each of
>>> the above. :)
>>
>> Thanks Siva for iterating few optimization points which I have been also
>> thinking are needed. But lets take the over all optimization possibilities
>> in another thread. My question to you guys in the current context is -
>>
>> 1. Shall we keep the panel_power_cycle_delay as is but adjusting jiffies
>> based on wall clock as Ville suggested. This will ensure that we skip the
>> wait in suspend/resume scenario, or
>>
>> 2. Have the HW PPS based tracking for delays and also take care that VDD
>> force on is not called before panel power cycle is completely over. If all
>> do not agree or see issues with this approach, we can keep this for later
>> when we optimize vdd on/off sequences as suggested by siva and go for now
>> with approach 1.
>
> Iirc on some platforms PPS resets incorrectly and we need the waits from
> step 1 to avoid frying the panel. That's why we added them. So I think
> adjusting jiffie by the wallclock time we've been suspended would be best.

Very well. Will get the updated patch out asap.

Regards
Shobhit

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

Re: [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence

[Kumar, Shobhit]

On 12/10/2015 06:45 PM, Ville Syrjälä wrote:
> On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote:
>> On 12/09/2015 09:35 PM, Ville Syrjälä wrote:
>>> On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote:
>>>> On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>> On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote:
>>>>>> On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä
>>>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>>>> On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote:
>>>>>>>> During resume, while turning the EDP panel power on, we need not wait
>>>>>>>> blindly for panel_power_cycle_delay. Check if panel power down sequence
>>>>>>>> in progress and then only wait. This improves our resume time significantly.
>>>>>>>>
>>>>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
>>>>>>>>    1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> index f335c92..10ec669 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>>> @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp)
>>>>>>>>         return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0;
>>>>>>>>    }
>>>>>>>>
>>>>>>>> +static bool edp_panel_off_seq(struct intel_dp *intel_dp)
>>>>>>>> +{
>>>>>>>> +     struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>> +
>>>>>>>> +     lockdep_assert_held(&dev_priv->pps_mutex);
>>>>>>>> +
>>>>>>>> +     if (IS_VALLEYVIEW(dev) &&
>>>>>>>> +         intel_dp->pps_pipe == INVALID_PIPE)
>>>>>>>> +             return false;
>>>>>>>> +
>>>>>>>> +     return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0;
>>>>>>>> +}
>>>>>>>
>>>>>>> This doens't make sense to me. The power down cycle may have
>>>>>>> completed just before, and so this would claim we don't have to
>>>>>>> wait for the power_cycle_delay.
>>>>>>
>>>>>> Not sure I understand your concern correctly. You are right, power
>>>>>> down cycle may have completed just before and if it has then we don't
>>>>>> need to wait. But in case the power down cycle is in progress as per
>>>>>> internal state, then we need to wait for it to complete. This will
>>>>>> happen for example in non-suspend disable path and will be handled
>>>>>> correctly. In case of actual suspend/resume, this would have
>>>>>> successfully completed and will skip the wait as it is not needed
>>>>>> before enabling panel power.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>>    static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>>>>>>>    {
>>>>>>>>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>>> @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp)
>>>>>>>>                  port_name(dp_to_dig_port(intel_dp)->port)))
>>>>>>>>                 return;
>>>>>>>>
>>>>>>>> -     wait_panel_power_cycle(intel_dp);
>>>>>>>> +     if (edp_panel_off_seq(intel_dp))
>>>>>>>> +             wait_panel_power_cycle(intel_dp);
>>>>>
>>>>> Looking in from the side, I have no idea what this is meant to do. At
>>>>> the very least you need your explanatory paragraph here which would
>>>>> include what exactly you are waiting for at the start of edp_panel_on
>>>>> (and please try and find a better name for edp_panel_off_seq()).
>>>>
>>>> I will add a comment. Basically I am not additionally waiting, but
>>>> converting the wait which was already there to a conditional wait. The
>>>> edp_panel_off_seq, checks if panel power down sequence is in progress.
>>>> In that case we need to wait for the panel power cycle delay. If it is
>>>> not in that sequence, there is no need to wait. I will make an attempt
>>>> again on the naming in next patch update.
>>>
>>> As far I remeber you need to wait for power_cycle_delay between power
>>> down cycle and power up cycle. You're trying to throw that wait away
>>> entirely, unless the function happens get called while the power down
>>
>> Yes you are right and I realize I made a mistake in my patch which is
>> not checking PP_CYCLE_DELAY_ACTIVE bit.
>>
>>> cycle is still in progress. We should already optimize away redundant
>>> waits by tracking the end of the power down cycle with the jiffies
>>> tracking.
>>>
>>> Actually looking at the code the power_cycle_delay gets counted from
>>> the start of the last power down cycle, so supposedly it's always at
>>> least as long as the power down cycle, and typically it's quite a bit
>>> longer that that. But that doesn't change the fact that you can't
>>> just skip it because the power down cycle delay happened to end
>>> already.
>>>
>>> So what we do now is:
>>> 1. initiate power down cycle
>>> 2. last_power_cycle=jiffies
>>> 3. wait for power down (I suppose this actually waits
>>>      until the power down delay has passed since that's
>>>      programmes into the PPS).
>>> 4. wait for power_cycle_delay from last_power_cycle
>>> 5. initiate power up cycle
>>>
>>> I think with your patch step 4 would always be skipped since the
>>> power down cycle has already ended, and then we fail to honor the
>>> power cycle delay.
>>
>> Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that
>> check will take care of this scenario I guess ?
>
> Nope. The vdd force bit doesn't respect the PPS state machine, so we
> must do the waits manually instead. And in theory your patch wouldn't
> do anything anyway since the sleep already takes into account when the
> power cycle delay started.

I think we don't even need to do force vdd on during suspend/resume. It 
should be done mainly in the beginning when detecting eDP/DPCD/Edid 
reads, which should be cached thereon.

Probably even before doing vdd on we should honor the t12. So its better 
to check the PP STATUS for cycle down and cycle delay active and wait 
out the delay even before we do VDD force on. I may be completely wrong 
in my understanding here.

>
> The fact that we use msleep() may actually make those sleeps somewhat
> longer, and maybe we should also think about switching to usleep_range()
> here.
>
>>
>> Regards
>> Shobhit
>>
>>>
>>> Actually the power_cycle delay also gets programmed into the PPS so I
>>> supose it would enforce the wait anyway when you initiate the power
>>> up cycle (unless the PPS got totally reset due to power wells etc.,
>>> which does seem like a real concern. The even bigger concern is the
>>> vdd force bit for which the PPS does no enforcement.
>>>
>>> The power_down_delay handling seems a bit wonky. We only wait for it
>>> when turning off the port. I guess I would need to go re-read the spec
>>> to figure out what it's meant to protect anyway.
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx