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 <ch...@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.syrj...@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.ku...@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
