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/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