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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >>>>>>>>>>>>---
> >>>>>>>>>>>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
