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

["Kumar, Shobhit" <shobhit.kumar@linux.intel.com>]

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